Ingest: Add validation and strong typing to sourceAndMetdata map #87673

stu-elastic · 2022-06-14T22:59:02Z

Adds IngestSourceAndMetdata to replace the sourceAndMetadata map.

This validates metadata values when they are added to the map for use in
scripts and other process as well as provides typed getters and for use
inside of server.

This change lays the foundation for strongly typed Metadata access in scripting.

Changes the type of the version paramter in IngestDocument from Long to long and moves it to the third argument, so all required values occur before nullable arguments. The IngestService expects a non-null version for a document and will throw an NullPointerException if one is not provided. Related: elastic#87309

Adds IngestSourceAndMetdata to replace the sourceAndMetadata map. This class validates metadata values when they are added to the map for use in scripts and other process as well as provides typed getters and for use inside of server.

…earch into ingest_validating-map

…_validating-map

… checking against Map.Entry

…IngestSourceAndMetadata javadocs

rjernst

Not a full review, but a few quick thoughts

rjernst · 2022-06-15T16:18:25Z

modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/AppendProcessorTests.java

        }
    }

-    public void testAppendMetadataExceptVersion() throws Exception {


why is this test removed?

It's not valid to append to any metadata field (and thus create an array list) based on my tests in 8.2. This was correctly failing validation.

rjernst · 2022-06-15T16:18:54Z

...s/ingest-common/src/test/java/org/elasticsearch/ingest/common/DotExpanderProcessorTests.java

        processor = new DotExpanderProcessor("_tag", null, null, "foo.bar");
        processor.execute(document);
-        assertThat(document.getSourceAndMetadata().size(), equalTo(1));
+        assertThat(document.getSourceAndMetadata().size(), equalTo(2));


Why did the map size change?

_version is added in the test constructor so it doesn't have to be added in every test that uses IngestDocument.

This seems trappy. Just above here the IngestDocument is created. If we need helpers to not have to specify version or other metadata fields, let's add helper methods, but not make it magical. If I were debugging this code, I would be very confused why I see 2 here.

It's just version because it always has to be there. If we internally separate out source and metadata then we can have this check source.

Will validate that version exists in another PR in a way that allows these tests to stay the same.

rjernst · 2022-06-15T16:20:36Z

server/src/main/java/org/elasticsearch/ingest/IngestDocument.java

        return "IngestDocument{" + " sourceAndMetadata=" + sourceAndMetadata + ", ingestMetadata=" + ingestMetadata + '}';
    }

-    public enum Metadata {


Can we keep this here? It creates a lot of churn, and I don't think there is any real reason it needs to be inside the implementation specific map.

rjernst · 2022-06-15T16:21:48Z

server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java

+ * The map is expected to be used by processors where-as the typed getter and setters should
+ * be used by server code where possible.
+ */
+public class IngestSourceAndMetadata extends ValidatingMap {


We should be able to make this an impl detail, the type doesn't need to be exposed, so it can be package private?

rjernst · 2022-06-15T16:23:14Z

server/src/main/java/org/elasticsearch/script/field/ValidatingMap.java

+ * {@link #merge(Object, Object, BiFunction)} family of methods, via {@link Map.Entry#setValue(Object)},
+ * via the linked Set from {@link #entrySet()} or {@link Collection#remove(Object)} from the linked collection via {@link #values()}
+ */
+public class ValidatingMap extends AbstractMap<String, Object> {


This can be package private? Or perhaps this doesn't need to be a separate class, we just need it for ingest right now, let's fold this into the real impl class and split if there is a need in the future?

Removed as a separate class.

rjernst · 2022-06-15T16:25:50Z

server/src/main/java/org/elasticsearch/script/field/ValidatingMap.java

+         * @throws IllegalArgumentException if the validator does not allow the Entry to be removed
+         */
+        @Override
+        public void remove() {


Wouldn't the iterator remove eventually delegate to a remove on the map, which will run the validator? Or why is a validator run at all? It seems like a hacky way to disallow removal.

There is this map and the wrapped map. We're trying to proxy changes to the wrapped map.

This iterator is needed because AbstractSet requires subclasses to implement iterator(). If we just take the wrapped maps entry set, the validators will not be called.

Ok, but why are the validators run on removal?

You cannot remove _version. If scripts remove _version then either we need to return a sentinel value from getVersion() (Long.MIN_VALUE or similar) or we have to throw an IllegalArgumentException

Validators are still run on removal because a validator may raise on removal, however this change does not have any such validator.

rjernst · 2022-06-15T16:31:25Z

server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java

+    /**
+     * One time operation that removes all metadata values and their validators and returns the underlying map, containing only source keys
+     */
+    public Map<String, Object> extractSource() {


Shouldn't this build a copy? Alternatively, what if source was kept separate, and delegated to. So we have separate source and metadata maps internal to this meta-map. It makes the lookup slightly slower, but the metadata one would be first and very quick since it is so small a number of fields, and it makes the extraction at the end trivial (none necessary).

The existing pattern of use in IngestDocument is to remove the Metadata and then pass resulting map as the source. The IngestDocument is thrown away so there's no need to allocate another, potentially large, map to hold the source to avoid modification.

Alternatively, what if source was kept separate, and delegated to.

We can do that, it would slightly complicate EntrySet but that's not a problem

source and metadata maps are separated.

…_validating-map

…_document-test-constructor

…stic/elasticsearch into ingest_validating-map

…pass a validator

elasticmachine · 2022-06-22T18:55:27Z

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

elasticmachine · 2022-06-22T18:55:28Z

Pinging @elastic/es-data-management (Team:Data Management)

…_validating-map

…for version type error

rjernst

This is looking good. A few more comments

rjernst · 2022-06-23T20:49:38Z

modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/AppendProcessorTests.java

        }
    }

-    public void testAppendMetadataExceptVersion() throws Exception {


Was this test just abusing metadata?

Yes, the only metadata that doesn't fail is _type and I think that's because it's missing validation.

rjernst · 2022-06-23T20:58:28Z

server/src/main/java/org/elasticsearch/ingest/IngestDocument.java

+    /**
+     * Build an IngestDocument from values read via deserialization
+     */
+    public static IngestDocument of(Map<String, Object> source, Map<String, Object> metadata, Map<String, Object> ingestMetadata) {


There are a ton of different constructors (package private?) and then also these of methods. Can we consolidate these? It seems like we shouldn't need like 6 different ways to create an IngestDocument?

I will take another pass.

Down to the same number as before.

rjernst · 2022-06-23T20:59:29Z

server/src/main/java/org/elasticsearch/ingest/IngestDocument.java

-            metadataMap.put(metadata, sourceAndMetadata.remove(metadata.getFieldName()));
-        }
-        return metadataMap;
+    public Map<String, Object> getSourceAndMetadata() {


Why do we need this separately? IngestSourceAndMetadata implements Map, so we should be able to combine this method and the one below?

We lose the fact that it's an IngestSourceAndMetadata if we make the signature a Map<String, Object> (which loses access to the typed getters) and if we return an IngestSourceAndMetadata then many of the tests for the ingest processors fail to compile because IngestSourceAndMetadata is package private, eg

modules/ingest-geoip/src/test/java/org/elasticsearch/ingest/geoip/GeoIpProcessorFactoryTests.java:497: error: IngestSourceAndMetadata.get(Object) is defined in an inaccessible class or interface Map<?, ?> geoData = (Map<?, ?>) ingestDocument.getIngestSourceAndMetadata().get("geoip");

Similar in org.elasticsearch.action.ingest.AsyncIngestProcessorIT.

rjernst · 2022-06-23T21:00:03Z

server/src/main/java/org/elasticsearch/ingest/IngestDocument.java

+    /**
+     * Get all Metadata values in a Map
+     */
+    public Map<String, Object> getMetadata() {


Can users get call getSourceAndMetadata() and call the appropriate method instead of adding access here?

org.elasticsearch.action.ingest.WriteableIngestDocument can't see the package private org.elasticsearch.ingest.IngestSourceAndMetadata

rjernst · 2022-06-23T21:00:56Z

server/src/main/java/org/elasticsearch/ingest/IngestDocument.java

     */
-    public Map<String, Object> getSourceAndMetadata() {
-        return this.sourceAndMetadata;
+    public static ZonedDateTime getTimestamp(Map<String, Object> ingestMetadata) {


This seems like an odd place for a utility method. Is there another place it could live, perhaps on metadata itself?

Moved to IngestSourceAndMetadata.

rjernst · 2022-06-23T21:05:16Z

server/src/test/java/org/elasticsearch/ingest/IngestSourceAndMetadataTests.java

+    IngestSourceAndMetadata map;
+
+    @Override
+    public void setUp() throws Exception {


this seems unnecesary, maybe leftover from debugging?

rjernst · 2022-06-23T21:07:33Z

server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java

+
+    /**
+     * map of key to validating function. Should throw {@link IllegalArgumentException} on invalid value, otherwise return identity
+      */


nit: spacing on javadoc off :)

rjernst · 2022-06-23T21:08:16Z

server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java

+    protected final ZonedDateTime timestamp;
+
+    /**
+     * map of key to validating function. Should throw {@link IllegalArgumentException} on invalid value, otherwise return identity


Why make it return anything? If validators throw IAE, then they could just be Consumers?

Changed to BiConsumers

rjernst · 2022-06-23T21:09:40Z

server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java

+     * @param timestamp the timestamp of ingestion
+     * @throws IllegalArgumentException if a validator fails for a given key
+     */
+    public static IngestSourceAndMetadata ofMixedSourceAndMetadata(Map<String, Object> sourceAndMetadata, ZonedDateTime timestamp) {


Similar to IngestDocument, there seem to be a lot of different ways to construct an IngestSourceAndMetadata. Can these be consolidated?

Reduced to two.

rjernst · 2022-06-23T21:10:19Z

server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java

+        put(IngestDocument.Metadata.VERSION.getFieldName(), version);
+    }
+
+    // timestamp isn't backed by the map


why isn't timestamp part of the metadata map?

It's a new field (#80038) so I didn't think it was necessary to backport it into the ctx map and create a potential bwc issue.

stu-elastic · 2022-06-24T16:46:23Z

@elasticmachine run elasticsearch-ci/part-2

…gest_validating-map

stu-elastic · 2022-06-27T19:10:39Z

@elasticmachine run elasticsearch-ci/packaging-tests-unix-sample

rjernst

Looks fine. I left a couple more thoughts.

rjernst · 2022-06-27T20:01:06Z

server/src/main/java/org/elasticsearch/ingest/IngestDocument.java

-            metadataMap.put(metadata, sourceAndMetadata.get(metadata.getFieldName()));
-        }
-        return metadataMap;
+    public IngestSourceAndMetadata getIngestSourceAndMetadata() {


since IngestSourceAndMetadata is package private, this method can be as well?

Made package private.

rjernst · 2022-06-27T20:04:33Z

server/src/main/java/org/elasticsearch/ingest/IngestDocument.java

+            IngestSourceAndMetadata.getTimestamp(ingestMetadata),
+            IngestSourceAndMetadata.VALIDATORS
+        );
+        this.ingestMetadata = ingestMetadata != null ? ingestMetadata : new HashMap<>();


Doesn't this need to be the metadata from sourceAndMetadata? If the assumption is it should already exist in the passed in sourceAndMetadata, then shouldn't ingestMetadata always be non null? The naming here is a bit confusing because "sourceAndMetadata" implies it contains the metadata, but then ingestMetadata is also passed in. When this was package private it's scoped more narrowly, but making this ctor public opens this up to trappy usage.

There are two metadatas, there's the metadata available via ctx and separate "ingest" metadata that is used by processors and stores processor-only information like the pipeline and the key/value for the ForEachProcessor

Removed HashMap instantiation on null.

rjernst · 2022-06-27T20:14:01Z

server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java

+        return timestamp;
+    }
+
+    // These are not available to scripts


If they are not available to scripts, why are they in the metadata map? It doesn't appear to be added when the IngestSourceAndMetadata is created, so when and how does it get in there?

They are set in the SimluatePipelineRequest and can be set via the SetProcessor

rjernst · 2022-06-27T20:27:39Z

test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java

-    public static IngestDocument ofSourceAndIngest(Map<String, Object> sourceAndMetadata, Map<String, Object> ingestMetadata) {
-        return new IngestDocument(sourceAndMetadata, ingestMetadata);
+    public static IngestDocument ofSourceAndMetadata(Map<String, Object> sourceAndMetadata) {
+        return new IngestDocument(sourceAndMetadata, new HashMap<>());


Doesn't this need to be the metadata from the sourceAndMetadata?

No, there's a separate IngestMetadata for internal processor use.

…on-null

…gest_validating-map

stu-elastic added 5 commits June 14, 2022 14:35

Fix doc and unit tests

9a21f12

simulate sets version to Versions.MATCH_ANY

d1286b1

Merge branch 'ingest_long-version' of github.com:stu-elastic/elastics…

466287a

…earch into ingest_validating-map

stu-elastic added the WIP label Jun 14, 2022

elasticsearchmachine added the v8.4.0 label Jun 14, 2022

stu-elastic added 11 commits June 14, 2022 18:08

Fix test compile issues

05db222

spotless

02246f3

Merge branch 'master' of github.com:elastic/elasticsearch into ingest…

4d7883d

…_validating-map

Clean up some tests

aece824

more test cleanup

d7bbe07

Merge branch 'master' of github.com:elastic/elasticsearch into ingest…

94ba063

…_validating-map

update size for version, check for entries individually to avoid type…

5444453

… checking against Map.Entry

spotless

567d159

Fix xpack tests by using the test constructor, add ValidatingMap and …

d7c61a8

…IngestSourceAndMetadata javadocs

version added in test constructor, no need to add in test

0121dff

comment hashcode in xcontent test, produce valid random metadata

b07a484

rjernst reviewed Jun 15, 2022

View reviewed changes

stu-elastic added 11 commits June 16, 2022 18:05

Ingest: move IngestDocument test ctor usage to static builder

5e5a899

Add javadoc and comments

1801596

revert modules/ingest-common/build.gradle

4d27346

map constructor and fromWire javadoc

145cd6b

Use map in CircleProcessorTests

7feb3f7

WIP

5b4fdf0

Move test constructors to test framework

26582c0

Merge branch 'master' of github.com:elastic/elasticsearch into ingest…

4f72efb

…_validating-map

Merge branch 'master' of github.com:elastic/elasticsearch into ingest…

a20642b

…_document-test-constructor

Merge branch 'ingest_document-test-constructor' of github.com:stu-ela…

b4dffc8

…stic/elasticsearch into ingest_validating-map

getMetadata returns strings

ae3e171

spotless

79fe344

stu-elastic added :Core/Infra/Scripting Scripting abstractions, Painless, and Mustache :Data Management/Ingest Node Execution or management of Ingest Pipelines including GeoIP >refactoring and removed WIP labels Jun 22, 2022

stu-elastic added 2 commits June 22, 2022 13:14

Move IngestSourceAndMetadata package private

281a6ce

Continue hiding IngestSourceAndMetdata in tests by adding ability to …

6b52e40

…pass a validator

stu-elastic marked this pull request as ready for review June 22, 2022 18:55

elasticmachine added Team:Core/Infra Meta label for core/infra team Team:Data Management Meta label for data/management team labels Jun 22, 2022

stu-elastic mentioned this pull request Jun 22, 2022

Script: Add Metadata to ingest context. #87309

Merged

stu-elastic added 3 commits June 22, 2022 13:59

Merge branch 'master' of github.com:elastic/elasticsearch into ingest…

9181ea6

…_validating-map

use correct helper in getString test

e39ab10

WriteableIngest presize, typo in primate, more tests, type and value …

1ff3adb

…for version type error

rjernst reviewed Jun 23, 2022

View reviewed changes

stu-elastic added 3 commits June 24, 2022 10:55

Validators are BiConsumers, fix TestIG javadoc, rm extra setup call

e8ecf2a

remove return from validator javadoc

70becc6

clean up consumer validators

30b89e0

stu-elastic added 3 commits June 24, 2022 16:08

Remove some IngestDocument and IngestSourceAndMetadata constructors

1460ae9

Move getTimestamp from IngestDocument to IngestSourceAndMetadata

13f6898

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

0849a2f

…gest_validating-map

rjernst approved these changes Jun 27, 2022

View reviewed changes

stu-elastic added 2 commits June 27, 2022 15:56

getIngestSourceAndMetadata pkg private, expect ingestMetadata to be n…

2bad2d0

…on-null

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

9de0da6

…gest_validating-map

stu-elastic merged commit 5a2d91c into elastic:master Jun 27, 2022

Ingest: Add validation and strong typing to sourceAndMetdata map #87673

Ingest: Add validation and strong typing to sourceAndMetdata map #87673

Uh oh!

Conversation

stu-elastic commented Jun 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjernst left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stu-elastic Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stu-elastic Jun 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jun 22, 2022

Uh oh!

elasticmachine commented Jun 22, 2022

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stu-elastic commented Jun 14, 2022 •

edited

Loading

rjernst left a comment •

edited

Loading

stu-elastic Jun 22, 2022 •

edited

Loading

stu-elastic Jun 15, 2022 •

edited

Loading

stu-elastic Jun 24, 2022 •

edited

Loading