Script: Metadata for update context #88333

stu-elastic · 2022-07-06T21:58:14Z

Adds the metadata() API call and a Metadata class for the Update context.

There are different metadata available in the update context depending
on whether it is an update or an insert (via upsert).

For update, scripts can read index, id, routing, version and timestamp.

For insert, scripts can read index, id and timestamp.

Scripts can always read and write the op but the available ops are different.

Updates allow 'noop', 'index' and 'delete'.
Inserts allow 'noop' and 'create'.

Refs: #86472

…gest_update_meta

Adds the metadata() API call and a Metadata class for the Update context There are different metadata available in the update context depending on whether it is an update or an insert (via upsert). For update, scripts can read index, id, routing, version and timestamp. For insert, scripts can read index, id and timestamp. Scripts can always read and write the op but the available ops are different. Updates allow 'noop', 'index' and 'delete'. Inserts allow 'noop' and 'create'. Refs: elastic#86472

…inancy for UpdateRequestTests.testUpdateScript

elasticmachine · 2022-07-07T01:36:54Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

elasticsearchmachine · 2022-07-07T01:37:15Z

Hi @stu-elastic, I've created a changelog YAML for you.

elasticmachine · 2022-07-07T01:59:34Z

Pinging @elastic/clients-team (Team:Clients)

jdconrad

I realized we have talked about this, but after viewing the code I was wondering again what if each context did have its own Metadata and the base class for this took in the Map of validators? The numerous comments in the Metadata class about how something is needed only for one context, but not another seems like it'll be hard to maintain. We also wouldn't have to have methods on the class that don't make sense for a specific context. You still wouldn't have to alias the name because it's part of the allow list specific to each context.

I can see there's a lot of refactoring work as well as you mentioned to me. Since I'm just coming back into this project it's tricky for me to differentiate what's refactoring versus what's added here that's new. Would you be able to split the refactoring into a first PR please?

…asticsearch into 220712-update-metadata-0

…sticsearch into 220712-update-metadata-0

…0712-update-metadata-0

…sticsearch into 220712-update-metadata-0

…to Metadata base class

stu-elastic · 2022-07-13T22:22:05Z

@rjernst and @jdconrad, I've changed this PR to use infrastructure from #88401 and #88458. Can you take another pass?

rjernst

Thanks for the update Stu! After the other PRs this is much more straightforward and easier to understand. I think there is a backcompat issue to address, but otherwise I just have a few minor comments.

rjernst · 2022-07-14T20:49:45Z

server/src/main/java/org/elasticsearch/script/UpdateCtxMap.java

+    @SuppressWarnings("unchecked")
+    public Map<String, Object> getSource() {
+        Map<String, Object> wrapped = super.getSource();
+        Object rawSource = wrapped.get(SOURCE);


Since ctx._source seems to be the more common pattern (only ingest differs?), wouldn't it make more sense for IngestCtxMap to be the special case, and CtxMap has the implementation from here?

rjernst · 2022-07-14T20:53:28Z

server/src/main/java/org/elasticsearch/script/UpdateMetadata.java

+    @Override
+    public void setOp(String op) {
+        if (LEGACY_NOOP_STRING.equals(op)) {
+            throw new IllegalArgumentException(LEGACY_NOOP_STRING + " is deprecated, use 'noop' instead");


This doesn't seem like a deprecation, an exception would mean an error. I think we need to support the existing case without error for now.

It is supported in the map, so there's no bwc issue, what do you think about removing the " is deprecated" language, disallow it in set but translate it in getOp?

That's asymmetric, but the addition of this class seems like the time and place to make that change.

Ok, I see now. I think your suggestion is good.

Ok, keeping it.

rjernst · 2022-07-14T21:35:26Z

...g-painless/src/main/resources/org/elasticsearch/painless/org.elasticsearch.script.update.txt

+    String getId()
+    String getRouting()
+    long getVersion()
+    String getOp()


We discussed having get and set op outside of the metadata on each script type that needs it, as it isn't really metadata about the document. What happened with that?

If it's on the script, then it conflicts with any variables named op, so it would be a backwards compatibility issue.

After thinking about it some more, it seems like a high cost to introduce another concept in addition to field and metadata for this one-off. Maybe there's a better way to handle it?

I don't feel strongly about it being outside of metadata. The potential conflict with variable names is both unfortunate, and something I think we need to look at fixing (via shadowing), because it's not just with class getters, but also any additional arguments added to the execute method.

The potential conflict with variable names is both unfortunate, and something I think we need to look at fixing (via shadowing), because it's not just with class getters, but also any additional arguments added to the execute method.

Agreed. It's a tricky change but worth doing because it allows us to make changes without backward compatibility issues. It's come up a few times now.

rjernst · 2022-07-14T21:50:00Z

server/src/main/java/org/elasticsearch/script/UpdateMetadata.java

+        SET_ONCE_LONG
+    );
+
+    protected static BiConsumer<String, String> setValidator(Set<String> valid) {


nit: the naming here makes it look like a setter. perhaps stringSetValidator?

rjernst · 2022-07-14T21:51:33Z

server/src/main/java/org/elasticsearch/script/UpdateMetadata.java

+    @Override
+    public String getOp() {
+        String op = super.getOp();
+        if (LEGACY_NOOP_STRING.equals(op) || op == null) {


I don't think we can do this translation for the "legacy" value here, it would be breaking. We should look at how/when to deprecate the two noop values outside of this PR.

I don't think it's breaking since the Map interface still allows it, it's just if the script wants to use the metadata interface. I can allow it here but still reject it in setOp(String), what do you think?

The legacy case makes sense, but why is null converted to noop?

op is nullable for backwards compatibility.

Before this change, if op were null, lenientFromString would translate it to UpdateOp.NONE.

UpdateOp.NONE is the same as 'noop' now.

I suppose we should also restrict null in setOp like we're already doing with "none".

In fact, there shouldn't be validation for op at all in the map, as it all gets translated to UpdateOp.NONE.

rjernst · 2022-07-14T21:53:01Z

server/src/main/java/org/elasticsearch/script/UpsertMetadata.java

+
+    @Override
+    public String getRouting() {
+        throw new IllegalArgumentException("routing is unavailable for insert");


UnsupportedOperationException would be more typical. We should use the same anywhere else we are failing a get/set method (a user should never see it since we woulnd't have allowed the method on scripting to begin with).

Changed to UnsupportedOperationException.

It is still whitelisted because an Update script may run in "Update mode" or "Insert-via-upsert mode" and this allows a user to write a script that will work in either case. If we split the Update context then a script that works for "Update mode" would fail to compile for "Insert-via-upsert mode".

If we split the Update context then a script

I thought we were already planning to do that?

We discussed it a while ago, but it'd be weird to have an script sent to the update endpoint fail to compile based on the whether or not the document exists.

rjernst · 2022-07-14T21:54:50Z

server/src/main/java/org/elasticsearch/script/UpdateCtxMap.java

+        if (rawSource instanceof Map<?, ?> map) {
+            return (Map<String, Object>) map;
+        }
+        throw new IllegalArgumentException(


How could this occur? Since we control the ctx map, wouldn't this be a coding error? Should this be an assert instead?

An assert brings down the node. Do you think we'd get the same feedback with an Exception without the customer pain? Perhaps IllegalStateException.

An assert brings down the node

It won't for a couple reasons. The uncaught exception handler does not consider AssertionError to be fatal. But also it would only be tripped if running with asserts enabled, which doesn't happen in production. We run tests with asserts enabled, and use asserts for things like this in many other parts of the code specifically so that we can catch coding errors in tests.

Changed to assert

… in Upsert

jdconrad

The new changes look fine to me. Please wait for @rjernst to have another look before committing.

rjernst

LGTM.

stu-elastic added 8 commits July 5, 2022 18:02

WIP ingest update metadata

9b4dae5

Create validators

7d52c59

Merge branch 'master' of github.com:stu-elastic/elasticsearch into in…

158156d

…gest_update_meta

revert "sourceAndMetadata" from "ingestContext"

3fab662

revert "getContextMetadata" -> "getMetadata"

205df38

Update javadoc

c35d5a7

rest tests

5fd7d3f

stu-elastic added the >enhancement label Jul 6, 2022

elasticsearchmachine added the v8.4.0 label Jul 6, 2022

stu-elastic added 5 commits July 6, 2022 17:12

add opValidator javadocs

73e1743

Add hashcode for WriteableIngestDocumentTests.testFromXContent and le…

0804227

…inancy for UpdateRequestTests.testUpdateScript

create is valid in scripts

c86cd15

Pass valid ops

4ee36ed

spotless UpdateSourceAndMetadata

c9359d5

stu-elastic added the :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache label Jul 7, 2022

stu-elastic marked this pull request as ready for review July 7, 2022 01:36

elasticmachine added the Team:Core/Infra Meta label for core/infra team label Jul 7, 2022

stu-elastic requested a review from jdconrad July 7, 2022 01:36

Update docs/changelog/88333.yaml

7c35fc2

sethmlarson added the Team:Clients Meta label for clients team label Jul 7, 2022

stu-elastic requested a review from rjernst July 7, 2022 17:02

jdconrad reviewed Jul 7, 2022

View reviewed changes

stu-elastic added 4 commits July 8, 2022 15:18

WIP

bf2085f

Split Metadata off off IngestSourceAndMetadata

e0ff58a

Update metadata javadoc

4ea1791

Add hashcode and equals to make the WritableIngestDocumentTests pass

7244010

stu-elastic added 17 commits July 12, 2022 18:13

Don't clone properties, add MetadataTests

5a45f97

Merge branch 'ingest_ctx_map_field_prop' of github.com:stu-elastic/el…

4975f93

…asticsearch into 220712-update-metadata-0

_type is nullable

1a24d71

Merge branch '220712-update-metadata-0' of github.com:stu-elastic/ela…

e9d86f9

…sticsearch into 220712-update-metadata-0

Add imports

12f9d3b

Add BulkMetadata for reference

bb96306

Merge branch 'master' of github.com:stu-elastic/elasticsearch into 22…

aa92c58

…0712-update-metadata-0

Merge branch '220712-update-metadata-0' of github.com:stu-elastic/ela…

cab9d28

…sticsearch into 220712-update-metadata-0

Remove BulkMetadata

7ba7cb9

Move Update and Upsert Metadatas to top level, move op and timestamp …

df2f149

…to Metadata base class

Fix timestamp key

2e56fad

Merge master

a2e754b

Use CtxMap and Metadata subclasses

491f635

Remove SourceAndMetadataMap implementation

fc84418

UpdateScript only needs ctx

3a3020c

Remove old comment

b164b75

Remove <p> in javadoc for UpdateMetadata

05b56bf

rjernst reviewed Jul 14, 2022

View reviewed changes

stu-elastic added 3 commits July 18, 2022 12:11

wrap in CtxMap, stringSetValidator, Unsupported instead of IllegalArg…

092fcc1

… in Upsert

Fix UpsertCtxMapTests expected exceptions

f0e8143

Assert map in getSource, avoid deprecated language in setOp

ae60d2a

jdconrad approved these changes Jul 18, 2022

View reviewed changes

stu-elastic added 2 commits July 18, 2022 18:50

setOp cannot be null

4f04d8f

op valiation in update is done by the getter/setter

7cc0f02

rjernst approved these changes Jul 19, 2022

View reviewed changes

stu-elastic merged commit fa25c31 into elastic:master Jul 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Script: Metadata for update context #88333

Script: Metadata for update context #88333

stu-elastic commented Jul 6, 2022 •

edited

Loading

elasticmachine commented Jul 7, 2022

elasticsearchmachine commented Jul 7, 2022

elasticmachine commented Jul 7, 2022

jdconrad left a comment

stu-elastic commented Jul 13, 2022

rjernst left a comment

rjernst Jul 14, 2022

stu-elastic Jul 18, 2022

rjernst Jul 14, 2022

stu-elastic Jul 18, 2022 •

edited

Loading

rjernst Jul 18, 2022

stu-elastic Jul 18, 2022

rjernst Jul 14, 2022

stu-elastic Jul 18, 2022

rjernst Jul 18, 2022

stu-elastic Jul 18, 2022

rjernst Jul 14, 2022

stu-elastic Jul 18, 2022

rjernst Jul 14, 2022

stu-elastic Jul 18, 2022

rjernst Jul 18, 2022

stu-elastic Jul 18, 2022

stu-elastic Jul 18, 2022

stu-elastic Jul 19, 2022

rjernst Jul 14, 2022

stu-elastic Jul 18, 2022

rjernst Jul 18, 2022

stu-elastic Jul 18, 2022

rjernst Jul 14, 2022

stu-elastic Jul 18, 2022

rjernst Jul 18, 2022 •

edited

Loading

stu-elastic Jul 18, 2022

jdconrad left a comment

rjernst left a comment

Script: Metadata for update context #88333

Script: Metadata for update context #88333

Conversation

stu-elastic commented Jul 6, 2022 • edited Loading

elasticmachine commented Jul 7, 2022

elasticsearchmachine commented Jul 7, 2022

elasticmachine commented Jul 7, 2022

jdconrad left a comment

Choose a reason for hiding this comment

stu-elastic commented Jul 13, 2022

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stu-elastic Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjernst Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdconrad left a comment

Choose a reason for hiding this comment

rjernst left a comment

Choose a reason for hiding this comment

stu-elastic commented Jul 6, 2022 •

edited

Loading

stu-elastic Jul 18, 2022 •

edited

Loading

rjernst Jul 18, 2022 •

edited

Loading