Painless: Add Static Methods Shortcut #33440
Conversation
jdconrad
added
>enhancement
:Core/Infra/Scripting
|
Pinging @elastic/es-core-infra |
|
@elasticmachine test this please |
| @@ -61,12 +61,19 @@ | |||
| /** The {@link List} of all the whitelisted Painless classes. */ | |||
| public final List<WhitelistClass> whitelistClasses; | |||
|
|
|||
| /** The {@link List} of all the whitelisted static Painless methods. */ | |||
| public final List<WhitelistMethod> whitelistStatics; | |||
There was a problem hiding this comment.
Nit: all these names start with "whitelist" but isn't that implied since they are in the whitelist class? Also, bindings are also in the static section, so statics seems to broad, unless this is staticMethods and bindings below are staticBindings?
There was a problem hiding this comment.
Will change this in a separate PR as we discussed.
| addPainlessStatic(targetClass, methodName, returnType, typeParameters); | ||
| } | ||
|
|
||
| public void addPainlessStatic(Class<?> targetClass, String methodName, Class<?> returnType, List<Class<?>> typeParameters) { |
There was a problem hiding this comment.
Does this need to be public? It's only called above right?
There was a problem hiding this comment.
I expect this set of methods to be used for loading defaults and I want them to remain public to indicate they can be used to build a custom lookup for either tests or possibly other sources of loading.
| @@ -937,6 +1089,11 @@ public void addPainlessBinding(Class<?> targetClass, String methodName, Class<?> | |||
| } | |||
|
|
|||
| String painlessMethodKey = buildPainlessMethodKey(methodName, constructorTypeParametersSize + methodTypeParametersSize); | |||
|
|
|||
| if (painlessMethodKeysToPainlessStatics.containsKey(painlessMethodKey)) { | |||
| throw new IllegalArgumentException("binding and static cannot have the same name [" + methodName + "]"); | |||
There was a problem hiding this comment.
Renamed to imported methods for all of these related to this feature. Will rename bindings to classbindings in the instancebindings PR or a separate PR.
| @@ -177,5 +177,6 @@ class org.elasticsearch.painless.FeatureTest no_import { | |||
|
|
|||
| # for testing | |||
| static { | |||
| float staticAddFloatsTest(float, float) from org.elasticsearch.painless.FeatureTest | |||
There was a problem hiding this comment.
(For a followup): can we move these test specific cases out of the base whitelist, since we have whitelist spi (I know we didn't have that when FeatureTest was added above)?
There was a problem hiding this comment.
@rjernst Yes, in fact given that we can do custom whitelists now, is there any reason not to move FeatureTest and BindingTest to the test framework and use a custom whitelist with those during the unit tests?
There was a problem hiding this comment.
What do you mean by test framework? These are just part of painless tests right?
There was a problem hiding this comment.
My understanding was that since FeatureTest and BindingTest are part src rather than test because we whitelist them in the base whitelists so they need to be distributed as part of src. Is it correct that if we have a custom whitelist for them and add that for unit tests they could be moved to test?
There was a problem hiding this comment.
I don't think they need to be part of src, or in the base whitelist. They can be exposed to tests through the compiler/contexts created in ScriptTestCase.
|
@rjernst Thanks for the review! Will commit as soon as CI passes. |
Allow static methods to be imported in Painless and called using just the method name.
* master: CORE: Make Pattern Exclusion Work with Aliases (elastic#33518) Reverse logic for CCR license checks (elastic#33549) Add latch countdown on failure in CCR license tests (elastic#33548) HLRC: Add put stored script support to high-level rest client (elastic#31323) Create temporary directory if needed in CCR test Add license checks for auto-follow implementation (elastic#33496) Bootstrap a new history_uuid when force allocating a stale primary (elastic#33432) INGEST: Remove Outdated TODOs (elastic#33458) Logging: Clean up skipping test Logging: Skip test if it'd fail CRUD: AwaitsFix entire wait_for_refresh close test Painless: Add Imported Static Method (elastic#33440)
Static methods in Painless can be whitelisted using the static section with the same format as Painless bindings except using the 'from' keyword instead of the 'bound_to' keyword. The following format is used:
and will be called as