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

Add explicit plugin method for MappedActionFilter #108515

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented May 10, 2024

In preparation for #108210, this commit adds a separate method to gather MappedActionFilter instances. For now this remains compatible with the existing getActionFilters by allowing MappedActionFilter to exist in both places.

In preparation for elastic#108210, this commit adds a separate method to gather
MappedActionFilter instances. For now this remains compatible with the
existing getActionFilters by allowing MappedActionFilter to exist in
both places.
@rjernst rjernst added :Core/Infra/Core Core issues without another label >refactoring labels May 10, 2024
@rjernst rjernst requested a review from a team as a code owner May 10, 2024 15:40
@elasticsearchmachine elasticsearchmachine added Team:Core/Infra Meta label for core/infra team v8.15.0 labels May 10, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@rjernst rjernst requested a review from pgomulka May 13, 2024 22:00
Copy link
Contributor

@pgomulka pgomulka left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

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

One small nit/suggestion and one Q but LGTM

@@ -807,6 +807,7 @@ private static ActionFilters setupActionFilters(List<ActionPlugin> actionPlugins
finalFilters.add(filter);
}
}
mappedFilters.addAll(plugin.getMappedActionFilters());
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: maybe add a TODO/comment to remove the instanceof above?

Copy link
Contributor

Choose a reason for hiding this comment

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

(Never mind, I just saw #108210)

/**
* Action filters applying to a single action added by this plugin.
*/
default Collection<MappedActionFilter> getMappedActionFilters() {
Copy link
Contributor

Choose a reason for hiding this comment

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

A Q: plugins should migrate to this method to return a MappedActionFilter.
Would it be possible then to remove the ability to return a MappedActionFilter from getActionFilters()? I think it will be OK for modules and for serverless, but what about 3rd party? We do not guarantee any bwc there, correct? (They work only with the ES version they were compiled with, to the patch?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Plugins have zero backcompat guarantees (at least these classic plugins). After converting serverless to use MappedActionFilter I will followup with making MappedActionFilter no longer inherit from ActionFilter, so it will not be possible to return via getActionFilters().

@rjernst rjernst added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 14, 2024
@elasticsearchmachine elasticsearchmachine merged commit 4c58522 into elastic:main May 16, 2024
15 checks passed
@rjernst rjernst deleted the action_filters/separate_mapped_filters branch May 16, 2024 00:42
parkertimmins pushed a commit to parkertimmins/elasticsearch that referenced this pull request May 17, 2024
In preparation for elastic#108210, this commit adds a separate method to gather
MappedActionFilter instances. For now this remains compatible with the
existing getActionFilters by allowing MappedActionFilter to exist in
both places.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Core/Infra/Core Core issues without another label >refactoring Team:Core/Infra Meta label for core/infra team v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants