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 script type and script name to error messages #11449

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@jdconrad
Copy link
Contributor

jdconrad commented Jun 1, 2015

Modified ScriptEngineService to pass in a CompiledScript object
to several methods with newly added name and type member variables.
This can in turn be used to give better scripting error messages
with the type of script used and the name of the script.

Required slight modifications to the caching mechanism.

Note that this does not enforce good behavior in that plugins will
have to write exceptions that also output the name of the script
in order to be effective. There was no way to wrap the script
methods in a try/catch block properly further up the chain because
many have script-like objects passed back that can be run at a
later time.

fixes #6653

}
return result.bytes();
} catch (Exception e) {
throw new ScriptException("Error with query template [" + template.name() + "]: " + e.getMessage(), e);
}

This comment has been minimized.

Copy link
@rmuir

rmuir Jun 1, 2015

Contributor

Can we simplify this to use try-with-resources? close() should call flush() anyway, and then this way no exceptions are ever hidden, instead they are chained with suppression: e.g.

try (UTF8StreamWriter writer = ...) {
  execute();
} catch (Exception e) {
  logger.error(xxx);
  throw new ScriptException(....)
}

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jun 2, 2015

Author Contributor

Changed.

@@ -109,7 +115,7 @@ public void setNextVar(String name, Object value) {
if (value instanceof Number) {
specialValue.setValue(((Number)value).doubleValue());
} else {
throw new ExpressionScriptExecutionException("Cannot use expression with text variable");
throw new ExpressionScriptExecutionException("Error with " + compiledScript.type().name() + " script [" + compiledScript.name() + "]: Cannot use expression with text variable");

This comment has been minimized.

Copy link
@s1monw

s1monw Jun 2, 2015

Contributor

@jdconrad would it make sense to have a toString methdo on CompiledScript that includes name and type?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jun 2, 2015

Author Contributor

Yes, thanks for the suggestion. I will make the change.

* @param lang The language of the script to be executed.
* @param compiled The compiled script Object that is executable.
*/
public CompiledScript(String lang, Object compiled) {
public CompiledScript(ScriptService.ScriptType type, String name, String lang, Object compiled) {

This comment has been minimized.

Copy link
@javanna

javanna Jun 2, 2015

Member

Would it make sense to move ScriptType out of ScriptService at some point (doesn't need to be in this PR necessarily)

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jun 2, 2015

Author Contributor

I think it probably does. I will opt to leave this for another PR. We may want to look at lot of the smaller classes in ScriptService as well. Indexed script for instance is only used in a single place as far as I can tell, so maybe we convert that to a method to get the indexed script name out of the url?

@@ -263,10 +263,12 @@ public CompiledScript compileInternal(Script script) {
return compiled;
}

String name = "";

This comment has been minimized.

Copy link
@javanna

javanna Jun 2, 2015

Member

can the name end up being empty?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jun 2, 2015

Author Contributor

For this and the comment below about null -- this was two different lines of thinking, and I forgot to change the latter. I was thinking for inline scripts I would leave the string empty. Given Simon's suggestion of a toString method, I'm going to go back to null.

try {
compiled = new CompiledScript(script.getType(), name, lang, scriptEngineService.compile(code));
} catch (RuntimeException re) {
throw new RuntimeException("Compilation error with " + script.getType().name()

This comment has been minimized.

Copy link
@javanna

javanna Jun 2, 2015

Member

ScriptType has a nice toString, not sure you should call name() to print it out

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jun 2, 2015

Author Contributor

Oops. I missed that. This is a better way to do this, and I will make the change.

// only the code of the script. Two indexed scripts may also have the same code, but different
// names. If this is case, the name and type parameters must be modified accordingly.
// Then cache the most recent version used.
if (compiled.type() != script.getType() || name != null && !name.equals(compiled.name())) {

This comment has been minimized.

Copy link
@javanna

javanna Jun 2, 2015

Member

can the name ever be null here?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jun 2, 2015

Author Contributor

See the comment above about this issue.

This comment has been minimized.

Copy link
@rmuir

rmuir Jun 5, 2015

Contributor

For a boolean expression this complicated i would turn negative checks into positive ones:

if (compiled.type() == script.getType() && (name == null || name.equals(compiled.name())) {

I would also try to use locals to break it down easier. Something like:

boolean sameType = 
boolean sameName =

if (sameName && sameType) {

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jun 5, 2015

Author Contributor

@rmuir Thanks for the feedback -- I don't think this code is going to remain once the cache key is changed, but if it does I will make sure it's simplified in this form.

@jdconrad jdconrad force-pushed the jdconrad:pr/6653 branch from 4570fc5 to f022622 Jun 3, 2015

jdconrad added a commit to jdconrad/elasticsearch that referenced this pull request Jun 3, 2015

Scripting: Add script name to error messages
Modified ScriptEngineService to pass in a CompiledScript object
to several methods with newly added name and type member variables.
This can in turn be used to give better scripting error messages
with the type of script used and the name of the script.

Required slight modifications to the caching mechanism.

closes elastic#6653
closes elastic#11449
@jdconrad

This comment has been minimized.

Copy link
Contributor Author

jdconrad commented Jun 3, 2015

Thanks to @rmuir @s1monw @javanna for the reviews -- and I'm ready for the next round. I pushed a new commit with changes based on the prior comments, and I tried to add a bit more consistency to the modified error messages.

@@ -263,10 +264,13 @@ public CompiledScript compileInternal(Script script) {
return compiled;
}

// Name will remain null for inline scripts.

This comment has been minimized.

Copy link
@javanna

javanna Jun 4, 2015

Member

is this comment correct? seems to me that we assign it only in case of indexed scripts?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jun 4, 2015

Author Contributor

I skipped file scripts here, as if it was a file script it had already been processed, but added a note to the comment to clarify.

This comment has been minimized.

Copy link
@javanna

javanna Jun 5, 2015

Member

gotcha

// An indexed or inline script may be mistaken for the other since the cache key relies on
// only the code of the script. Two indexed scripts may also have the same code, but different
// names. If this is case, the name and type parameters must be modified accordingly.
// Then cache the most recent version used.

This comment has been minimized.

Copy link
@javanna

javanna Jun 4, 2015

Member

I find this new code block confusing, can we maybe add the script type to the cache key instead or something along those lines?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jun 4, 2015

Author Contributor

@javanna I see three options here:

  1. Decide to leave it as is with the benefit being this is the least invasive; however, it is complicated.
  2. Add name to the cache key (instead of script type) because there are two cases currently where we can confuse different scripts described in the comment. The first is if the code is the same between an inline script and an indexed script, you may get back a compiled script with incorrect name and type information. The second is if two indexed scripts have the same code but different names, you may get back a compiled script with an incorrect name. Adding name to the cache key would solve both problems.
  3. We could leave the cache key as is and cache only the executable script objects instead of compiled script objects. Then we create a new compiled script object to be returned with the current metadata (name, type, and lang). I see the point of the cache as trying to prevent compiling the same script over and over, so this solution solves that problem and might actually be the cleanest way of doing this.

Let me know what you think, and then I'll implement the solution we decide upon. Thanks!

This comment has been minimized.

Copy link
@javanna

javanna Jun 5, 2015

Member

I guess the question is whether it's a problem that two scripts with same code end up being one single entry in the cache, beyond side effect of returning wrong name etc. (which is a bug). If so we should go for 2) and add all the needed info to the cache key so we can distinguish those "same" scripts. Otherwise we could go with 3) too. Both 2. and 3. sound ok to me, which one do you prefer?

This comment has been minimized.

Copy link
@rjernst

rjernst Jun 5, 2015

Member

I don't quite follow all the options here, but why is it not as simple as type + "value"?
file_filename
indexed_name
inline_a+b

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jun 5, 2015

Author Contributor

@rjernst I'll give this some thought. Maybe it makes sense to turn cache key into a method that just returns a concatenated string instead of a full blown object that isn't really necessary?

@jdconrad

This comment has been minimized.

Copy link
Contributor Author

jdconrad commented Jun 4, 2015

@javanna Please take a look at the response I left you when you get a chance.

@javanna

This comment has been minimized.

Copy link
Member

javanna commented Jun 5, 2015

thanks @jdconrad I replied, this looks good on my end besides the comments I left, maybe @uboness can have a look too?

@clintongormley clintongormley changed the title Scripting: Add script type and script name to error messages Add script type and script name to error messages Jun 6, 2015

@javanna

This comment has been minimized.

Copy link
Member

javanna commented Jul 3, 2015

what's the status on this? @uboness did you have time to have a look?

@jdconrad

This comment has been minimized.

Copy link
Contributor Author

jdconrad commented Jul 6, 2015

@javanna No update right now. I have been caught up in other issues and need to come back to this. I like @rjernst 's suggestion of making the CacheKey class into a simple string with all the necessary information to make the key unique as that's what's ultimately happening anyway. What do you think?

@javanna

This comment has been minimized.

Copy link
Member

javanna commented Jul 6, 2015

I think it would be nice if we could get this in, especially given that it's marked breaking. I wouldn't want this PR to be forgotten for 2.0.

@jdconrad

This comment has been minimized.

Copy link
Contributor Author

jdconrad commented Jul 6, 2015

@javanna I will try to get it done before the end of the week then.

@jdconrad jdconrad force-pushed the jdconrad:pr/6653 branch from f022622 Jul 10, 2015

@jdconrad

This comment has been minimized.

Copy link
Contributor Author

jdconrad commented Jul 10, 2015

@rjernst Oops, meant this PR :)

@rjernst

View changes

core/src/main/java/org/elasticsearch/script/CompiledScript.java Outdated
private final String lang;
private final Object compiled;

/**
* Constructor for CompiledScript.
+ + * @param type The type of script to be executed.

This comment has been minimized.

Copy link
@rjernst

rjernst Jul 11, 2015

Member

Looks like a merge issue? (the '+'s)

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jul 11, 2015

Author Contributor

Oops. Merge error from using a patch between the pre and post plugin era. Fixed.

public CompiledScript(String lang, Object compiled) {
this.lang = lang;
this.compiled = compiled;
public CompiledScript(ScriptService.ScriptType type, String name, String lang, Object compiled) {

This comment has been minimized.

Copy link
@rjernst

rjernst Jul 11, 2015

Member

can we move ScriptType out from ScriptService?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jul 11, 2015

Author Contributor

Will do so in a different PR as discussed previously in this PR.

@rjernst

View changes

core/src/main/java/org/elasticsearch/script/ScriptService.java Outdated
String cacheKey = getCacheKey(scriptEngineService, script.getScript());
String cacheKey;

CompiledScript compiledScript;

This comment has been minimized.

Copy link
@rjernst

rjernst Jul 11, 2015

Member

I would move this into the FILE block, and redefine later when it is needed, so the FILE block is clearly on its own? same with cacheKey?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jul 11, 2015

Author Contributor

Done.

@rjernst

View changes

core/src/main/java/org/elasticsearch/script/ScriptService.java Outdated
String code = script.getScript();

if (script.getType() == ScriptType.INDEXED) {
final IndexedScript indexedScript = new IndexedScript(lang, script.getScript());
final IndexedScript indexedScript = new IndexedScript(lang, name);

This comment has been minimized.

Copy link
@rjernst

rjernst Jul 11, 2015

Member

maybe add a comment here that we must lookup the indexed script before the cache because the code may have changed?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jul 11, 2015

Author Contributor

Done.

@rjernst

View changes

core/src/main/java/org/elasticsearch/script/ScriptService.java Outdated
try {
compiledScript = new CompiledScript(type, name, lang, scriptEngineService.compile(code));
} catch (Exception exception) {
compiledScript = new CompiledScript(type, name, lang, null);

This comment has been minimized.

Copy link
@rjernst

rjernst Jul 11, 2015

Member

Can we just print out the pieces type/name/lang here without creating a compiled script just to do a toString()?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jul 11, 2015

Author Contributor

Done.

@rjernst

View changes

core/src/main/java/org/elasticsearch/script/ScriptService.java Outdated
String lang = scriptEngineService.types()[0];
return lang + ":" + script;
return lang + ":" + name + (code != null ? ":" + code : "");

This comment has been minimized.

Copy link
@rjernst

rjernst Jul 11, 2015

Member

In the case of an inline script, won't name already be the entire script? So in this case the cache key would be the code duplicated twice?

This comment has been minimized.

Copy link
@jdconrad

jdconrad Jul 11, 2015

Author Contributor

While this shouldn't affect uniqueness, to avoid confusion I will make it so inline scripts end up being lang:code.

@rjernst

This comment has been minimized.

Copy link
Member

rjernst commented Jul 11, 2015

LGTM, I left a couple small suggestions.

Add script type and script name to error messages
Modified ScriptEngineService to pass in a CompiledScript object
with newly added name and type member variables.
This can in turn be used to give better scripting error messages
with the type of script used and the name of the script.

Required slight modifications to the caching mechanism.

Note that this does not enforce good behavior in that plugins will
have to write exceptions that also output the name of the script
in order to be effective. There was no way to wrap the script
methods in a try/catch block properly further up the chain because
many have script-like objects passed back that can be run at a
later time.

closes #6653
closes #11449

@jdconrad jdconrad force-pushed the jdconrad:pr/6653 branch to f46ba22 Jul 11, 2015

@jdconrad jdconrad closed this in c1137b3 Jul 11, 2015

@jdconrad

This comment has been minimized.

Copy link
Contributor Author

jdconrad commented Jul 11, 2015

@rjernst Thanks for the review.

@jdconrad jdconrad deleted the jdconrad:pr/6653 branch Jan 19, 2016

@jdconrad jdconrad restored the jdconrad:pr/6653 branch Jan 19, 2016

@jdconrad jdconrad deleted the jdconrad:pr/6653 branch Feb 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.