Skip to content

add test ensuring PainlessPlugin#baseWhiteList() remains stable-API #107119

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yaauie
Copy link
Member

@yaauie yaauie commented Apr 4, 2024

A recent refactor in #106913 introduced PainlessPlugin#baseWhiteList() and removed PainlessPlugin.BASE_WHITELISTS, which caused builds of Logstash's Elastic integration plugin to break when run against Elasticsearch main (elastic/logstash-filter-elastic_integration#135).

Since we need an API-stable way of getting the plugin's default whitelists, here we add a test to document its use and ensure it remains API-stable. We also backport this test to the 8.13 series along with an implementation that falls through to the existing static field in #107120 so that our builds will succeed between now and when 8.14.0 is released.


Perhaps worth noting, the method introduced in #106913 in returns List<Whitelist>, but is named in a way that mismatches in two ways:

  • the method name is singular, even though it provides a List
  • the method name has superfluous capitalization WhiteList while the contents are a Whitelist

We may want to remedy this mismatch before marking the method as API-stable.

@yaauie yaauie requested a review from a team as a code owner April 4, 2024 19:09
@elasticsearchmachine elasticsearchmachine added needs:triage Requires assignment of a team area label v8.14.0 external-contributor Pull request authored by a developer outside the Elasticsearch team labels Apr 4, 2024
@yaauie yaauie added the :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache label Apr 4, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Apr 4, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Apr 4, 2024
@yaauie yaauie added the needs:triage Requires assignment of a team area label label Apr 4, 2024
@elasticsearchmachine elasticsearchmachine removed the needs:triage Requires assignment of a team area label label Apr 4, 2024
@yaauie yaauie force-pushed the painless-plugin-public-api branch from 03a9300 to 94f2a57 Compare April 4, 2024 20:59
@dakrone
Copy link
Member

dakrone commented Apr 4, 2024

@yaauie I thought when we ran into this last time we agreed that for things where we required a non-breaking stable API, we'd put that into the Logstash plugin layer, similar to what we did in #105163?

@yaauie yaauie force-pushed the painless-plugin-public-api branch from 94f2a57 to 9eadb2e Compare April 4, 2024 21:35
@yaauie
Copy link
Member Author

yaauie commented Apr 4, 2024

@dakrone the existing LogstashInternalBridge is a part of org.elasticsearch.ingest, which is not dependent on org.elasticsearch.painless and therefore can't reference bits that we need.

@yaauie yaauie force-pushed the painless-plugin-public-api branch from 9eadb2e to 6f5789a Compare April 5, 2024 06:08
Copy link
Contributor

@williamrandolph williamrandolph left a comment

Choose a reason for hiding this comment

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

Looks good to me.

I'd also support changing PainlessPlugin#baseWhiteList to PainlessPlugin#baseWhitelists or PainlessPlugin#getBaseWhitelists. How complicated will that be for the downstream dependency?

@yaauie
Copy link
Member Author

yaauie commented Apr 8, 2024

I'd also support changing PainlessPlugin#baseWhiteList to PainlessPlugin#baseWhitelists or PainlessPlugin#getBaseWhitelists. How complicated will that be for the downstream dependency?

If it happens before we rely on it, easy peasy (needs to be reflected in 8.13 series too), but if it happens after then things get messy.

@rjernst
Copy link
Member

rjernst commented Apr 8, 2024

While I understand the intention, I do not think adding a test in this way will prevent such a future breakage. We use code refactoring tools to make such changes, where it is likely we won't even notice the test.

Ultimately no Java code in Elasticsearch is stable (with the exception of the stable plugin api). Trying to force some stability on small parts of our internal APIs is likely to continue to cause friction.

If Logstash needs a stable way to access some internals, I suggest another gradle project within server that has the appropriate dependencies, and presents its "stable api". This would be very similar to the existing LogstashInternalBridge (I would even move that bridge to the new project. By keeping it in the Elasticsearch codebase, such future refactorings will be automatically handled if done through IntelliJ, or the use site will be necessarily adjusted to keep the ES build working. The only caveat is this should not be published to maven; we don't want to give the perception that such an api is stable for uses outside of Logstash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Scripting Scripting abstractions, Painless, and Mustache external-contributor Pull request authored by a developer outside the Elasticsearch team >non-issue Team:Core/Infra Meta label for core/infra team v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants