Provide more context variables in update scripts #5724
Conversation
| if (fetchedTimestamp instanceof String) { | ||
| timestamp = (String) fetchedTimestamp; | ||
| } else { | ||
| timestamp = fetchedTimestamp.toString(); |
There was a problem hiding this comment.
String.toString just returns this, so you could remove the instanceof check and cast after you ensure it's non-null.
|
Changed what @pickypg proposed + rebased to current master. |
|
@dakrone please could you take a look at this |
| "ctx._source.parent = ctx._parent;\n" + | ||
| "ctx._source.routing = ctx._routing;\n" + | ||
| "ctx._source.timestamp = ctx._timestamp;\n" + | ||
| "ctx._source.ttl = ctx._ttl;\n") |
There was a problem hiding this comment.
In this example, you can use assert directly in the script itself, so you could do
.setScript("assert ctx._version == 1 : \"version should be 1\"" +
... etc ...If you would like to (I think either way is fine, just depends on personal preference).
|
@ofavre this looks pretty good, can you rebase on the latest master since this is an older PR and make sure this still works now that Groovy is the default scripting language? |
| assertEquals("parentId1", getResponse.getSourceAsMap().get("parent")); | ||
| assertEquals("routing1", getResponse.getSourceAsMap().get("routing")); | ||
| assertEquals(timestamp, getResponse.getSourceAsMap().get("timestamp")); | ||
| assertEquals((double)111211211 - (postUpdateTs - postIndexTs), ((Number)getResponse.getSourceAsMap().get("ttl")).doubleValue(), postIndexTs - preIndexTs + postUpdateTs - preUpdateTs); |
There was a problem hiding this comment.
I think this assert in particular (the timestamp one) could be greatly simplified by asserting inside of the script itself instead of setting a new field value.
ofavre
force-pushed
the
update-ctx-vars
branch
from
8e96ae3
to
d098005
Compare
|
Thanks for the suggestions, they were valuable, and I managed to simplify the ttl assertion a bit. |
| @@ -522,6 +522,99 @@ public void testUpdateRequestWithScriptAndShouldUpsertDoc() throws Exception { | |||
| } | |||
|
|
|||
| @Test | |||
| public void testContextVariables() throws Exception { | |||
| createIndex(); | |||
There was a problem hiding this comment.
This empty createIndex causes the tests to fail, it should be createIndex("test")
|
@ofavre thanks for the update! I left one more comment about a failing test, if you can fix that I'll merge this in |
ofavre
force-pushed
the
update-ctx-vars
branch
from
d098005
to
c4f35ab
Compare
|
I somehow rebase on a 3 month old master. |
clintongormley
added
review
:Core/Infra/Scripting
| if (fetchedTimestamp != null) { | ||
| timestamp = fetchedTimestamp.toString(); | ||
| } else { | ||
| timestamp = originalTimestamp.toString(); |
There was a problem hiding this comment.
originalTimestamp can be null here, causing an NPE
|
Hi @ofavre I left another comment, running the tests with your change causes the following to fail: |
In addition to `_source`, the following variables are available through the `ctx` map: `_index`, `_type`, `_id`, `_version`, `_routing`, `_parent`, `_timestamp`, `_ttl`. Some of these fields are more useful still within the context of an Update By Query, see elastic#1607, elastic#2230, elastic#2231.
ofavre
force-pushed
the
update-ctx-vars
branch
from
c4f35ab
to
8038920
Compare
|
You're right, I've added a check for nullity in the else clause. |
|
Pushed to 1.x and master, thanks @ofavre ! |
|
Thank you @clintongormley and @dakrone for pushing this forward! ;-) |
clintongormley
added
:Distributed/CRUD
In addition to
_source, the following variables are available through thectxmap:_index,_type,_id,_version,_routing,_parent,_timestamp,_ttl.Some of these fields are more useful still within the context of an Update By Query, see #1607, #2230, #2231.