Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify CompositeRolesStore#getRoleDescriptors #103126

Conversation

DaveCTurner
Copy link
Contributor

This method has only one production caller, which asserts that the
response always has length 1 and (if assertions are disabled) discards
all but the first set of role descriptors in the response. We can push
that behaviour down into the implementation simplifying things quite
considerably and eliminating the allocation and copying needed to
support other sizes of response.

This method has only one production caller, which asserts that the
response always has length 1 and (if assertions are disabled) discards
all but the first set of role descriptors in the response. We can push
that behaviour down into the implementation simplifying things quite
considerably and eliminating the allocation and copying needed to
support other sizes of response.
@DaveCTurner DaveCTurner added >non-issue :Security/Security Security issues without another label v8.13.0 labels Dec 7, 2023
@DaveCTurner DaveCTurner requested a review from ywangd December 7, 2023 14:01
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Dec 7, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@DaveCTurner
Copy link
Contributor Author

NB this refactoring is only valid if we never complete the GroupedActionListener with a null, since GroupedActionListener only returns a list of the non-null responses. But AFAICT we always complete it with org.elasticsearch.xpack.core.security.authz.store.RolesRetrievalResult#roleDescriptors and there's no way for that field to be null.

Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DaveCTurner DaveCTurner added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Dec 8, 2023
@elasticsearchmachine elasticsearchmachine merged commit 5cf8b94 into elastic:main Dec 8, 2023
@DaveCTurner DaveCTurner deleted the 2023/12/07/getRoleDescriptorsList-singleton branch December 8, 2023 12:56
@DaveCTurner DaveCTurner restored the 2023/12/07/getRoleDescriptorsList-singleton branch June 17, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >non-issue :Security/Security Security issues without another label Team:Security Meta label for security team v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants