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

Painless: Only allow Painless type names to be the same as the equivalent Java class. #27264

Merged
merged 12 commits into from Dec 12, 2017

Conversation

@jdconrad
Copy link
Contributor

jdconrad commented Nov 3, 2017

Updated the WhitelistLoader to reflect the changes along with current whitelist resource files.

When defining a Painless struct, the line will now be class java.lang.String { meaning java.lang.String will be available as Painless type.

If we want the short name of any class the line will now be class java.lang.String import { meaning both java.lang.String and String will be available as a Painless type.

This simplifies Painless whitelists in a significant way moving forward as we can now infer a Painless type name from a Java Class object. Also enforces a 1 to 1 Painless type to Java class naturally.

jdconrad added 2 commits Nov 3, 2017
equivalent Java class or the imported version of the Java class.
jdconrad added 3 commits Nov 3, 2017
@rjernst

This comment has been minimized.

Copy link
Member

rjernst commented Nov 9, 2017

One high level suggestion: I think most of the time we will use the simple class names. So instead of needing to add import for every class, why not flip it and have a special identifier for those we want to only allow with the fully qualified name? There are very few of these (only classes that are "internal" that must be whitelisted so their methods are available, but constructing them is not expected). I would be very explicit about this and replace import in your syntax with only_fqn (only full qualified name).

Copy link
Member

rjernst left a comment

I left some minor suggestions.

String painlessTypeName = whitelistStruct.javaClassName.replace('$', '.');
String importedPainlessTypeName = painlessTypeName;

if (painlessTypeName.matches("^[_a-zA-Z][._a-zA-Z0-9]*") == false) {

This comment has been minimized.

Copy link
@rjernst

rjernst Nov 9, 2017

Member

Maybe move this out to a private static constant so the regex is not recompiled for every struct in the whitelist?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Nov 21, 2017

Author Contributor

Done.

}
} else if (existingStruct.clazz.equals(javaClass) == false) {
throw new IllegalArgumentException("imported name [" + painlessTypeName + "] is used to " +
"illegally represent multiple java classes [" + whitelistStruct.javaClassName + "] " +

This comment has been minimized.

Copy link
@rjernst

rjernst Nov 9, 2017

Member

nit: I would reword this, as "illegally" sounds odd. Perhaps something like:

Simple type name [Foo] found for types [my.Foo] and [other.Foo]. At least one must require fully qualified name using "only_fqn"

This comment has been minimized.

Copy link
@jdconrad

jdconrad Nov 21, 2017

Author Contributor

Changed.

@@ -43,18 +43,29 @@
* and field. Most validation will be done at a later point after all white-lists have been gathered and their
* merging takes place.
*
* A painless type name is considered to one of the following:

This comment has been minimized.

Copy link
@rjernst

rjernst Nov 9, 2017

Member

is considered to one of -> is one of

This comment has been minimized.

Copy link
@rjernst

rjernst Nov 9, 2017

Member

Also, maybe make it clear this is explained so that these types can be used in method signatures. The class portion of the whitelist syntax must contain the fully qualified name.

This comment has been minimized.

Copy link
@jdconrad

jdconrad Nov 21, 2017

Author Contributor

Done.

@jdconrad jdconrad added the v6.3.0 label Nov 20, 2017
jdconrad added 2 commits Nov 20, 2017
@jdconrad

This comment has been minimized.

Copy link
Contributor Author

jdconrad commented Nov 21, 2017

@rjernst Changed import to only_fqn and responded to the rest of the PR comments. Would you please take another look? Thanks!

Copy link
Member

rjernst left a comment

This looks good I just have one last outstanding question and a couple other minor comments.


/**
* The entire API for Painless. Also used as a whitelist for checking for legal
* methods and fields during at both compile-time and runtime.
*/
public final class Definition {

private static final Pattern TYPE_NAME_PATTERN = Pattern.compile("^[_a-zA-Z][._a-zA-Z0-9]*$");

This comment has been minimized.

Copy link
@rjernst

rjernst Nov 27, 2017

Member

A type name can start with an underscore?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Nov 28, 2017

Author Contributor

Yes. This matches the ANTLR grammar. This can probably be changed given that we only allow Java-style names now, but I don't want to add more complications to grammar ID's right now since the same tokens are used for method/field names.

@@ -577,7 +583,13 @@ private Definition(List<Whitelist> whitelists) {

// goes through each Painless struct and determines the inheritance list,
// and then adds all inherited types to the Painless struct's whitelist
for (Struct painlessStruct : structsMap.values()) {
for (String painlessStructName : structsMap.keySet()) {

This comment has been minimized.

Copy link
@rjernst

rjernst Nov 27, 2017

Member

What's the reason for moving to key iteration followed by lookup and filtering? Iterating values + filtering should be faster (not that performance matters here really, but...).

This comment has been minimized.

Copy link
@jdconrad

jdconrad Dec 11, 2017

Author Contributor

Was for more explicit naming, but changed to use entryset to get the values from.


if (existingStruct == null) {
structsMap.put(importedPainlessTypeName, structsMap.get(painlessTypeName));
Type painlessType = simpleTypesMap.remove(painlessTypeName);

This comment has been minimized.

Copy link
@rjernst

rjernst Nov 27, 2017

Member

I'm a little confused by this block. Why is the type removed only to be added back?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Dec 11, 2017

Author Contributor

For the doc generator to work simple types names must not contain multiple entries of the same type with multiple names, so this removes the fully-qualified name in favor the short name for now to be consistent with what already exists. This definitely needs to be cleaned up but not in this PR.

@jdconrad jdconrad added v6.2.0 and removed v6.3.0 labels Nov 28, 2017
jdconrad added 2 commits Dec 11, 2017
@jdconrad

This comment has been minimized.

Copy link
Contributor Author

jdconrad commented Dec 11, 2017

@rjernst Responded to all PR comments.

@jdconrad

This comment has been minimized.

Copy link
Contributor Author

jdconrad commented Dec 11, 2017

@rjernst Fixed the inconsistency we chatted about.

Copy link
Member

rjernst left a comment

LGTM, just one last suggestion

simpleTypesMap.get(importedPainlessTypeName).clazz == javaClass ||
whitelistStruct.onlyFQNJavaClassName == false && (simpleTypesMap.containsKey(importedPainlessTypeName) == false ||
simpleTypesMap.get(importedPainlessTypeName).clazz != javaClass)) {
throw new IllegalArgumentException("inconsistent only_fqn parameters specified for type [" + painlessTypeName + "]");

This comment has been minimized.

Copy link
@rjernst

rjernst Dec 11, 2017

Member

This message might be confusing since this could happen across whitelists in different plugins right? Maybe expand it a little, or at least say "found" instead of "specified"?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Dec 11, 2017

Author Contributor

Changed.

@jdconrad

This comment has been minimized.

Copy link
Contributor Author

jdconrad commented Dec 11, 2017

@elasticmachine Please test this.

@jdconrad

This comment has been minimized.

Copy link
Contributor Author

jdconrad commented Dec 11, 2017

@elasticmachine test this please

@jdconrad jdconrad merged commit 8188d9f into elastic:master Dec 12, 2017
2 checks passed
2 checks passed
CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details
@jdconrad

This comment has been minimized.

Copy link
Contributor Author

jdconrad commented Dec 12, 2017

@rjernst Thanks for the review!

@jdconrad jdconrad removed the v6.1.0 label Dec 12, 2017
jdconrad added a commit that referenced this pull request Dec 13, 2017
…lent Java class. (#27264)

Also adds a parameter called only_fqn to the whitelist to enforce that a painless type must be specified as the fully-qualifed java class name.
@colings86 colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.