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.

Sign up for GitHub

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

Painless: Add Static Methods Shortcut #33440

Merged
merged 7 commits into from
Sep 8, 2018
Merged

Painless: Add Static Methods Shortcut #33440

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
46bf087
Painless: Add Static Methods Shortcut
jdconrad Sep 5, 2018
d69b10f
Make static final.
jdconrad Sep 5, 2018
ae83563
Merge branch 'master' into static2
jdconrad Sep 5, 2018
579c9a6
Merge branch 'master' into static2
jdconrad Sep 6, 2018
ab4c525
Merge branch 'master' into static2
jdconrad Sep 6, 2018
0d0017f
Merge branch 'master' into static2
jdconrad Sep 7, 2018
f4b5072
Reponse to PR comments.
jdconrad Sep 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion modules/lang-painless/spi/src/main/java/org/elasticsearch/painless/spi/Whitelist.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,12 +61,19 @@ public final class Whitelist {
/** 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;
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change this in a separate PR as we discussed.


/** The {@link List} of all the whitelisted Painless bindings. */
public final List<WhitelistBinding> whitelistBindings;

/** Standard constructor. All values must be not {@code null}. */
public Whitelist(ClassLoader classLoader, List<WhitelistClass> whitelistClasses, List<WhitelistBinding> whitelistBindings) {
public Whitelist(ClassLoader classLoader, List<WhitelistClass> whitelistClasses,
List<WhitelistMethod> whitelistStatics, List<WhitelistBinding> whitelistBindings) {

this.classLoader = Objects.requireNonNull(classLoader);
this.whitelistClasses = Collections.unmodifiableList(Objects.requireNonNull(whitelistClasses));
this.whitelistStatics = Collections.unmodifiableList(Objects.requireNonNull(whitelistStatics));
this.whitelistBindings = Collections.unmodifiableList(Objects.requireNonNull(whitelistBindings));
}
}
18 changes: 11 additions & 7 deletions modules/lang-painless/spi/src/main/java/org/elasticsearch/painless/spi/WhitelistLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ public final class WhitelistLoader {
*/
public static Whitelist loadFromResourceFiles(Class<?> resource, String... filepaths) {
List<WhitelistClass> whitelistClasses = new ArrayList<>();
List<WhitelistMethod> whitelistStatics = new ArrayList<>();
List<WhitelistBinding> whitelistBindings = new ArrayList<>();

// Execute a single pass through the whitelist text files. This will gather all the
Expand Down Expand Up @@ -230,7 +231,7 @@ public static Whitelist loadFromResourceFiles(Class<?> resource, String... filep
parseType = null;

// Handle static definition types.
// Expects the following format: ID ID '(' ( ID ( ',' ID )* )? ')' 'bound_to' ID '\n'
// Expects the following format: ID ID '(' ( ID ( ',' ID )* )? ')' ( 'from' | 'bound_to' ) ID '\n'
} else if ("static".equals(parseType)) {
// Mark the origin of this parsable object.
String origin = "[" + filepath + "]:[" + number + "]";
Expand Down Expand Up @@ -286,15 +287,18 @@ public static Whitelist loadFromResourceFiles(Class<?> resource, String... filep
throw new IllegalArgumentException("invalid static definition: unexpected format [" + line + "]");
}

// Check the static type is valid.
if ("bound_to".equals(staticType) == false) {
// Add a static method or binding depending on the static type.
if ("from".equals(staticType)) {
whitelistStatics.add(new WhitelistMethod(origin, targetJavaClassName,
methodName, returnCanonicalTypeName, Arrays.asList(canonicalTypeNameParameters)));
} else if ("bound_to".equals(staticType)) {
whitelistBindings.add(new WhitelistBinding(origin, targetJavaClassName,
methodName, returnCanonicalTypeName, Arrays.asList(canonicalTypeNameParameters)));
} else {
throw new IllegalArgumentException(
"invalid static definition: unexpected static type [" + staticType + "] [" + line + "]");
}

whitelistBindings.add(new WhitelistBinding(origin, targetJavaClassName,
methodName, returnCanonicalTypeName, Arrays.asList(canonicalTypeNameParameters)));

// Handle class definition types.
} else if ("class".equals(parseType)) {
// Mark the origin of this parsable object.
Expand Down Expand Up @@ -393,7 +397,7 @@ public static Whitelist loadFromResourceFiles(Class<?> resource, String... filep

ClassLoader loader = AccessController.doPrivileged((PrivilegedAction<ClassLoader>)resource::getClassLoader);

return new Whitelist(loader, whitelistClasses, whitelistBindings);
return new Whitelist(loader, whitelistClasses, whitelistStatics, whitelistBindings);
}

private WhitelistLoader() {}
Expand Down
26 changes: 16 additions & 10 deletions modules/lang-painless/src/main/java/org/elasticsearch/painless/FeatureTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,21 @@

/** Currently just a dummy class for testing a few features not yet exposed by whitelist! */
public class FeatureTest {
/** static method that returns true */
public static boolean overloadedStatic() {
return true;
}

/** static method that returns what you ask it */
public static boolean overloadedStatic(boolean whatToReturn) {
return whatToReturn;
}

/** static method only whitelisted as a static */
public static float staticAddFloatsTest(float x, float y) {
return x + y;
}

private int x;
private int y;
public int z;
Expand Down Expand Up @@ -58,21 +73,12 @@ public void setY(int y) {
this.y = y;
}

/** static method that returns true */
public static boolean overloadedStatic() {
return true;
}

/** static method that returns what you ask it */
public static boolean overloadedStatic(boolean whatToReturn) {
return whatToReturn;
}

/** method taking two functions! */
public Object twoFunctionsOfX(Function<Object,Object> f, Function<Object,Object> g) {
return f.apply(g.apply(x));
}

/** method to take in a list */
public void listInput(List<Object> list) {

}
Expand Down
15 changes: 15 additions & 0 deletions modules/lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,23 @@ public final class PainlessLookup {
private final Map<String, Class<?>> canonicalClassNamesToClasses;
private final Map<Class<?>, PainlessClass> classesToPainlessClasses;

private final Map<String, PainlessMethod> painlessMethodKeysToPainlessStatics;
private final Map<String, PainlessBinding> painlessMethodKeysToPainlessBindings;

PainlessLookup(Map<String, Class<?>> canonicalClassNamesToClasses, Map<Class<?>, PainlessClass> classesToPainlessClasses,
Map<String, PainlessMethod> painlessMethodKeysToPainlessStatics,
Map<String, PainlessBinding> painlessMethodKeysToPainlessBindings) {

Objects.requireNonNull(canonicalClassNamesToClasses);
Objects.requireNonNull(classesToPainlessClasses);

Objects.requireNonNull(painlessMethodKeysToPainlessStatics);
Objects.requireNonNull(painlessMethodKeysToPainlessBindings);

this.canonicalClassNamesToClasses = Collections.unmodifiableMap(canonicalClassNamesToClasses);
this.classesToPainlessClasses = Collections.unmodifiableMap(classesToPainlessClasses);

this.painlessMethodKeysToPainlessStatics = Collections.unmodifiableMap(painlessMethodKeysToPainlessStatics);
this.painlessMethodKeysToPainlessBindings = Collections.unmodifiableMap(painlessMethodKeysToPainlessBindings);
}

Expand Down Expand Up @@ -167,6 +174,14 @@ public PainlessField lookupPainlessField(Class<?> targetClass, boolean isStatic,
return painlessField;
}

public PainlessMethod lookupPainlessStatic(String methodName, int arity) {
Objects.requireNonNull(methodName);

String painlessMethodKey = buildPainlessMethodKey(methodName, arity);

return painlessMethodKeysToPainlessStatics.get(painlessMethodKey);
}

public PainlessBinding lookupPainlessBinding(String methodName, int arity) {
Objects.requireNonNull(methodName);

Expand Down
164 changes: 161 additions & 3 deletions .../lang-painless/src/main/java/org/elasticsearch/painless/lookup/PainlessLookupBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,14 @@ public static PainlessLookup buildFromWhitelists(List<Whitelist> whitelists) {
}
}

for (WhitelistMethod whitelistStatic : whitelist.whitelistStatics) {
origin = whitelistStatic.origin;
painlessLookupBuilder.addPainlessStatic(
whitelist.classLoader, whitelistStatic.augmentedCanonicalClassName,
whitelistStatic.methodName, whitelistStatic.returnCanonicalTypeName,
whitelistStatic.canonicalTypeNameParameters);
}

for (WhitelistBinding whitelistBinding : whitelist.whitelistBindings) {
origin = whitelistBinding.origin;
painlessLookupBuilder.addPainlessBinding(
Expand All @@ -261,12 +269,14 @@ public static PainlessLookup buildFromWhitelists(List<Whitelist> whitelists) {
private final Map<String, Class<?>> canonicalClassNamesToClasses;
private final Map<Class<?>, PainlessClassBuilder> classesToPainlessClassBuilders;

private final Map<String, PainlessMethod> painlessMethodKeysToPainlessStatics;
private final Map<String, PainlessBinding> painlessMethodKeysToPainlessBindings;

public PainlessLookupBuilder() {
canonicalClassNamesToClasses = new HashMap<>();
classesToPainlessClassBuilders = new HashMap<>();

painlessMethodKeysToPainlessStatics = new HashMap<>();
painlessMethodKeysToPainlessBindings = new HashMap<>();
}

Expand Down Expand Up @@ -513,8 +523,9 @@ public void addPainlessMethod(ClassLoader classLoader, String targetCanonicalCla
addPainlessMethod(targetClass, augmentedClass, methodName, returnType, typeParameters);
}

public void addPainlessMethod(Class<?> targetClass, Class<?> augmentedClass, String methodName,
Class<?> returnType, List<Class<?>> typeParameters) {
public void addPainlessMethod(Class<?> targetClass, Class<?> augmentedClass,
String methodName, Class<?> returnType, List<Class<?>> typeParameters) {

Objects.requireNonNull(targetClass);
Objects.requireNonNull(methodName);
Objects.requireNonNull(returnType);
Expand Down Expand Up @@ -788,6 +799,147 @@ public void addPainlessField(Class<?> targetClass, String fieldName, Class<?> ty
}
}

public void addPainlessStatic(ClassLoader classLoader, String targetCanonicalClassName,
String methodName, String returnCanonicalTypeName, List<String> canonicalTypeNameParameters) {

Objects.requireNonNull(classLoader);
Objects.requireNonNull(targetCanonicalClassName);
Objects.requireNonNull(methodName);
Objects.requireNonNull(returnCanonicalTypeName);
Objects.requireNonNull(canonicalTypeNameParameters);

Class<?> targetClass = canonicalClassNamesToClasses.get(targetCanonicalClassName);

if (targetClass == null) {
throw new IllegalArgumentException("target class [" + targetCanonicalClassName + "] not found for static " +
"[[" + targetCanonicalClassName + "], [" + methodName + "], " + canonicalTypeNameParameters + "]");
}

List<Class<?>> typeParameters = new ArrayList<>(canonicalTypeNameParameters.size());

for (String canonicalTypeNameParameter : canonicalTypeNameParameters) {
Class<?> typeParameter = canonicalTypeNameToType(canonicalTypeNameParameter);

if (typeParameter == null) {
throw new IllegalArgumentException("type parameter [" + canonicalTypeNameParameter + "] not found for static " +
"[[" + targetCanonicalClassName + "], [" + methodName + "], " + canonicalTypeNameParameters + "]");
}

typeParameters.add(typeParameter);
}

Class<?> returnType = canonicalTypeNameToType(returnCanonicalTypeName);

if (returnType == null) {
throw new IllegalArgumentException("return type [" + returnCanonicalTypeName + "] not found for static " +
"[[" + targetCanonicalClassName + "], [" + methodName + "], " + canonicalTypeNameParameters + "]");
}

addPainlessStatic(targetClass, methodName, returnType, typeParameters);
}

public void addPainlessStatic(Class<?> targetClass, String methodName, Class<?> returnType, List<Class<?>> typeParameters) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be public? It's only called above right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Objects.requireNonNull(targetClass);
Objects.requireNonNull(methodName);
Objects.requireNonNull(returnType);
Objects.requireNonNull(typeParameters);

if (targetClass == def.class) {
throw new IllegalArgumentException("cannot add static from reserved class [" + DEF_CLASS_NAME + "]");
}

String targetCanonicalClassName = typeToCanonicalTypeName(targetClass);

if (METHOD_NAME_PATTERN.matcher(methodName).matches() == false) {
throw new IllegalArgumentException(
"invalid method name [" + methodName + "] for static target class [" + targetCanonicalClassName + "].");
}

PainlessClassBuilder painlessClassBuilder = classesToPainlessClassBuilders.get(targetClass);

if (painlessClassBuilder == null) {
throw new IllegalArgumentException("target class [" + targetCanonicalClassName + "] not found for static " +
"[[" + targetCanonicalClassName + "], [" + methodName + "], " + typesToCanonicalTypeNames(typeParameters) + "]");
}

int typeParametersSize = typeParameters.size();
List<Class<?>> javaTypeParameters = new ArrayList<>(typeParametersSize);

for (Class<?> typeParameter : typeParameters) {
if (isValidType(typeParameter) == false) {
throw new IllegalArgumentException("type parameter [" + typeToCanonicalTypeName(typeParameter) + "] " +
"not found for static [[" + targetCanonicalClassName + "], [" + methodName + "], " +
typesToCanonicalTypeNames(typeParameters) + "]");
}

javaTypeParameters.add(typeToJavaType(typeParameter));
}

if (isValidType(returnType) == false) {
throw new IllegalArgumentException("return type [" + typeToCanonicalTypeName(returnType) + "] not found for static " +
"[[" + targetCanonicalClassName + "], [" + methodName + "], " + typesToCanonicalTypeNames(typeParameters) + "]");
}

Method javaMethod;

try {
javaMethod = targetClass.getMethod(methodName, javaTypeParameters.toArray(new Class<?>[typeParametersSize]));
} catch (NoSuchMethodException nsme) {
throw new IllegalArgumentException("static method reflection object [[" + targetCanonicalClassName + "], " +
"[" + methodName + "], " + typesToCanonicalTypeNames(typeParameters) + "] not found", nsme);
}

if (javaMethod.getReturnType() != typeToJavaType(returnType)) {
throw new IllegalArgumentException("return type [" + typeToCanonicalTypeName(javaMethod.getReturnType()) + "] " +
"does not match the specified returned type [" + typeToCanonicalTypeName(returnType) + "] " +
"for static [[" + targetClass.getCanonicalName() + "], [" + methodName + "], " +
typesToCanonicalTypeNames(typeParameters) + "]");
}

if (Modifier.isStatic(javaMethod.getModifiers()) == false) {
throw new IllegalArgumentException("return type [" + typeToCanonicalTypeName(javaMethod.getReturnType()) + "] " +
"does not match the specified returned type [" + typeToCanonicalTypeName(returnType) + "] " +
"for static [[" + targetClass.getCanonicalName() + "], [" + methodName + "], " +
typesToCanonicalTypeNames(typeParameters) + "]");
}

String painlessMethodKey = buildPainlessMethodKey(methodName, typeParametersSize);

if (painlessMethodKeysToPainlessBindings.containsKey(painlessMethodKey)) {
throw new IllegalArgumentException("static and binding cannot have the same name [" + methodName + "]");
}

PainlessMethod painlessStatic = painlessMethodKeysToPainlessStatics.get(painlessMethodKey);

if (painlessStatic == null) {
MethodHandle methodHandle;

try {
methodHandle = MethodHandles.publicLookup().in(targetClass).unreflect(javaMethod);
} catch (IllegalAccessException iae) {
throw new IllegalArgumentException("static method handle [[" + targetClass.getCanonicalName() + "], " +
"[" + methodName + "], " + typesToCanonicalTypeNames(typeParameters) + "] not found", iae);
}

MethodType methodType = methodHandle.type();

painlessStatic = painlessMethodCache.computeIfAbsent(
new PainlessMethodCacheKey(targetClass, methodName, returnType, typeParameters),
key -> new PainlessMethod(javaMethod, targetClass, returnType, typeParameters, methodHandle, methodType));

painlessMethodKeysToPainlessStatics.put(painlessMethodKey, painlessStatic);
} else if (painlessStatic.returnType == returnType && painlessStatic.typeParameters.equals(typeParameters) == false) {
throw new IllegalArgumentException("cannot have statics " +
"[[" + targetCanonicalClassName + "], [" + methodName + "], " +
"[" + typeToCanonicalTypeName(returnType) + "], " +
typesToCanonicalTypeNames(typeParameters) + "] and " +
"[[" + targetCanonicalClassName + "], [" + methodName + "], " +
"[" + typeToCanonicalTypeName(painlessStatic.returnType) + "], " +
typesToCanonicalTypeNames(painlessStatic.typeParameters) + "] " +
"with the same arity and different return type or type parameters");
}
}

public void addPainlessBinding(ClassLoader classLoader, String targetJavaClassName,
String methodName, String returnCanonicalTypeName, List<String> canonicalTypeNameParameters) {

Expand Down Expand Up @@ -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 + "]");
Copy link
Member

Choose a reason for hiding this comment

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

static -> static methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

PainlessBinding painlessBinding = painlessMethodKeysToPainlessBindings.get(painlessMethodKey);

if (painlessBinding == null) {
Expand Down Expand Up @@ -976,7 +1133,8 @@ public PainlessLookup build() {
classesToPainlessClasses.put(painlessClassBuilderEntry.getKey(), painlessClassBuilderEntry.getValue().build());
}

return new PainlessLookup(canonicalClassNamesToClasses, classesToPainlessClasses, painlessMethodKeysToPainlessBindings);
return new PainlessLookup(canonicalClassNamesToClasses, classesToPainlessClasses,
painlessMethodKeysToPainlessStatics, painlessMethodKeysToPainlessBindings);
}

private void copyPainlessClassMembers() {
Expand Down
Loading