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
Remove deprecated 1.x script and template syntax #19387
Remove deprecated 1.x script and template syntax #19387
Conversation
@@ -49,7 +49,7 @@ public void testUpdateRequest() throws Exception { | |||
UpdateRequest request = new UpdateRequest("test", "type", "1"); | |||
// simple script | |||
request.source(XContentFactory.jsonBuilder().startObject() | |||
.field("script", "script1") | |||
.startObject("script").field("inline", "script1").endObject() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this change is correct it is not necessary. An inline script with no parameters can have two forms:
"script": "scirpt contents"
or:
"script": {
"inline": "script contents"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, the test failed if I didn't make this change. I'll change the parsing this and other places so that a script string value is also accepted as an inline script.
@martijnvg I left some comments mainly on the tests but its looking good |
@@ -645,7 +645,7 @@ public UpdateRequest source(BytesReference source) throws Exception { | |||
while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { | |||
if (token == XContentParser.Token.FIELD_NAME) { | |||
currentFieldName = parser.currentName(); | |||
} else if ("script".equals(currentFieldName) && token == XContentParser.Token.START_OBJECT) { | |||
} else if ("script".equals(currentFieldName)) { | |||
//here we don't have settings available, unable to throw strict deprecation exceptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this comment out of date now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it is. I'll remove it.
@martijnvg thanks for the quick update to this PR, I left a very minor comment but LGTM now |
bd2d66b
to
bdf4bfa
Compare
Pushed via: 2c3165d |
@martijnvg do you know what version of ES this syntax was first deprecated in? This change hits Kibana and I'm trying to figure out how we (Kibana) could be more proactive about recognizing and fixing use of deprecated features. I checked alpha4 and I don't seem to get any deprecation logs for the old syntax there. |
The old Script API was deprecated from 2.0.0 although I don't think we had the deprecation logger at that point so it might have been in one of the minor version that the logging of the deprecation was enabled. It was in the release notes though, I'll try to find the link to it |
This is the PR that introduced the new script API in 2.0.0-beta1: #11164 and this is a link to the relevant section in the breaking changes document: Hope that helps |
Elasticsearch has removed the abbreviated script syntax that we were using in Kibana. This change updates all of Kibana's queries and filters to use the newer object syntax wherever scripted fields are valid. This commit also includes a change to pass the lang param when sorting by scripted field which fixes an error that was being thrown. Fixes: elastic#7788 Fixes: elastic#5676 Related: elastic/elasticsearch#19387
Elasticsearch has removed the abbreviated script syntax that we were using in Kibana. This change updates all of Kibana's queries and filters to use the newer object syntax wherever scripted fields are valid. This commit also includes a change to pass the lang param when sorting by scripted field which fixes an error that was being thrown. Fixes: elastic#7788 Fixes: elastic#5676 Related: elastic/elasticsearch#19387 Former-commit-id: ebb9cd2
PR for #13729