From bf2085f96243afdcc811e293da12813fd55e8760 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Fri, 8 Jul 2022 15:18:44 -0500 Subject: [PATCH 01/24] WIP --- .../java/org/elasticsearch/script/CtxMap.java | 104 +++++++++ .../elasticsearch/script/MapWrappable.java | 26 +++ .../org/elasticsearch/script/MapWrapper.java | 210 ++++++++++++++++++ .../org/elasticsearch/script/Metadata.java | 6 +- 4 files changed, 345 insertions(+), 1 deletion(-) create mode 100644 server/src/main/java/org/elasticsearch/script/CtxMap.java create mode 100644 server/src/main/java/org/elasticsearch/script/MapWrappable.java create mode 100644 server/src/main/java/org/elasticsearch/script/MapWrapper.java diff --git a/server/src/main/java/org/elasticsearch/script/CtxMap.java b/server/src/main/java/org/elasticsearch/script/CtxMap.java new file mode 100644 index 0000000000000..446d33c34a8f7 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/script/CtxMap.java @@ -0,0 +1,104 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.script; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +public class CtxMap extends MapWrapper { + + protected Map source; + protected M metadata; + + CtxMap(Map source, M metadata) { + super(new SourceAndMetadataMap(source, metadata)); + this.source = source; + this.metadata = metadata; + } + + public Map getSource() { + return source; + } + + public M getMetadata() { + return metadata; + } + + private static class SourceAndMetadataMap implements MapWrappable { + protected Map source; + protected MapWrappable metadata; + + SourceAndMetadataMap(Map source, MapWrappable metadata) { + this.source = source; + this.metadata = metadata; + } + + @Override + public boolean ownsKey(String key) { + return true; + } + + @Override + public Object put(String key, Object value) { + if (metadata.ownsKey(key)) { + return metadata.put(key, value); + } + return source.put(key, value); + } + + @Override + public boolean containsKey(String key) { + return source.containsKey(key) || metadata.containsKey(key); + } + + @Override + public boolean containsValue(Object value) { + return source.containsValue(value) || metadata.containsValue(value); + } + + @Override + public Object get(String key) { + if (metadata.ownsKey(key)) { + return metadata.get(key); + } + return source.get(key); + } + + @Override + public Object remove(String key) { + if (metadata.ownsKey(key)) { + return metadata.remove(key); + } + return source.remove(key); + } + + @Override + public List keys() { + List keys = new ArrayList<>(size()); + keys.addAll(metadata.keys()); + keys.addAll(source.keySet()); + return keys; + } + + @Override + public int size() { + return metadata.size() + source.size(); + } + + @Override + public MapWrappable clone() { + return new CtxMap( + new HashMap<>(source), + metadata.clone() + ); + } + } +} diff --git a/server/src/main/java/org/elasticsearch/script/MapWrappable.java b/server/src/main/java/org/elasticsearch/script/MapWrappable.java new file mode 100644 index 0000000000000..8c532e8967f69 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/script/MapWrappable.java @@ -0,0 +1,26 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.script; + +import java.util.List; + +/** + * A class that can easily be wrapped in a Map interface by {@link MapWrapper} + */ +public interface MapWrappable { + boolean ownsKey(String key); + Object put(String key, Object value); + boolean containsKey(String key); + boolean containsValue(Object value); + Object get(String key); + Object remove(String key); + List keys(); + int size(); + MapWrappable clone(); +} diff --git a/server/src/main/java/org/elasticsearch/script/MapWrapper.java b/server/src/main/java/org/elasticsearch/script/MapWrapper.java new file mode 100644 index 0000000000000..d220f8c5ff192 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/script/MapWrapper.java @@ -0,0 +1,210 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.script; + +import java.util.AbstractCollection; +import java.util.AbstractMap; +import java.util.AbstractSet; +import java.util.Collection; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; + +/** + * Implements a Map interface, including {@link Map.Entry} and {@link Iterator} for classes that + * implement {@link MapWrappable}. + */ +class MapWrapper extends AbstractMap { + private EntrySet entrySet; // cache to avoid recreation + protected final MapWrappable wrapped; + + MapWrapper(MapWrappable wrapped) { + this.wrapped = wrapped; + } + + /** + * Returns an entrySet that respects the validators of the map. + */ + @Override + public Set> entrySet() { + if (entrySet == null) { + entrySet = new EntrySet(wrapped.keys()); + } + return entrySet; + } + + /** + * Associate a key with a value. If the key has a validator, it is applied before association. + * @throws IllegalArgumentException if value does not pass validation for the given key + */ + @Override + public Object put(String key, Object value) { + return wrapped.put(key, value); + } + + /** + * Remove the mapping of key. If the key has a validator, it is checked before key removal. + * @throws IllegalArgumentException if the validator does not allow the key to be removed + */ + @Override + public Object remove(Object key) { + // uses map directly to avoid AbstractMaps linear time implementation using entrySet() + if (key instanceof String str) { + return wrapped.remove(str); + } + return null; + } + + /** + * Clear entire map. For each key in the map with a validator, that validator is checked as in {@link #remove(Object)}. + * @throws IllegalArgumentException if any validator does not allow the key to be removed, in this case the map is may have been + * modified + */ + @Override + public void clear() { + // AbstractMap uses entrySet().clear(), it should be quicker to run through the metadata keys, then call the wrapped maps clear + for (String key : wrapped.keys()) { + wrapped.remove(key); + } + } + + @Override + public int size() { + // uses map directly to avoid creating an EntrySet via AbstractMaps implementation, which returns entrySet().size() + return wrapped.size(); + } + + @Override + public boolean containsValue(Object value) { + // uses map directly to avoid AbstractMaps linear time implementation using entrySet() + return wrapped.containsValue(value); + } + + @Override + public boolean containsKey(Object key) { + // uses map directly to avoid AbstractMaps linear time implementation using entrySet() + if (key instanceof String str) { + return wrapped.containsKey(str); + } + return false; + } + + @Override + public Object get(Object key) { + // uses map directly to avoid AbstractMaps linear time implementation using entrySet() + if (key instanceof String str) { + return wrapped.get(str); + } + return null; + } + + /** + * Set of entries of the wrapped map that calls the appropriate validator before changing an entries value or removing an entry. + * + * Inherits {@link AbstractSet#removeAll(Collection)}, which calls the overridden {@link #remove(Object)} which performs validation. + * + * Inherits {@link AbstractCollection#retainAll(Collection)} and {@link AbstractCollection#clear()}, which both use + * {@link EntrySetIterator#remove()} for removal. + */ + class EntrySet extends AbstractSet> { + List wrappedKeys; + + EntrySet(List wrappedKeys) { + this.wrappedKeys = wrappedKeys; + } + + @Override + public Iterator> iterator() { + return new EntrySetIterator(wrappedKeys.iterator()); + } + + @Override + public int size() { + return wrappedKeys.size(); + } + + @Override + public boolean remove(Object o) { + if (o instanceof Map.Entry entry) { + if (entry.getKey()instanceof String key) { + if (wrapped.containsKey(key)) { + if (Objects.equals(entry.getValue(), wrapped.get(key))) { + wrapped.remove(key); + return true; + } + } + } + } + return false; + } + } + + /** + * Iterator over the wrapped map that returns a validating {@link Entry} on {@link #next()} and validates on {@link #remove()}. + * + * {@link #remove()} is called by remove in {@link AbstractMap#values()}, {@link AbstractMap#keySet()}, {@link AbstractMap#clear()} via + * {@link AbstractSet#clear()} + */ + class EntrySetIterator implements Iterator> { + final Iterator wrappedIter; + Map.Entry cur; + + EntrySetIterator(Iterator wrappedIter) { + this.wrappedIter = wrappedIter; + } + + @Override + public boolean hasNext() { + return wrappedIter.hasNext(); + } + + @Override + public Map.Entry next() { + return cur = new Entry(wrappedIter.next()); + } + + /** + * Remove current entry from the backing Map. Checks the Entry's key's validator, if one exists, before removal. + * @throws IllegalArgumentException if the validator does not allow the Entry to be removed + * @throws IllegalStateException if remove is called before {@link #next()} + */ + @Override + public void remove() { + if (cur == null) { + throw new IllegalStateException(); + } + wrapped.remove(cur.getKey()); + } + } + + class Entry implements Map.Entry { + final String key; + + Entry(String key) { + this.key = key; + } + + @Override + public String getKey() { + return key; + } + + @Override + public Object getValue() { + return wrapped.get(key); + } + + @Override + public Object setValue(Object value) { + return wrapped.put(key, value); + } + } +} diff --git a/server/src/main/java/org/elasticsearch/script/Metadata.java b/server/src/main/java/org/elasticsearch/script/Metadata.java index e598a211c109a..cae0d47469f66 100644 --- a/server/src/main/java/org/elasticsearch/script/Metadata.java +++ b/server/src/main/java/org/elasticsearch/script/Metadata.java @@ -9,11 +9,15 @@ package org.elasticsearch.script; import java.time.ZonedDateTime; +import java.util.Map; /** * Ingest and update metadata available to write scripts */ -public interface Metadata { +public class Metadata implements MapWrappable { + + protected final Map map; + /** * The destination index */ From e0ff58aa9a28d9f8d29148850a167d66efaadbf8 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Fri, 8 Jul 2022 17:42:32 -0500 Subject: [PATCH 02/24] Split Metadata off off IngestSourceAndMetadata --- .../common/DotExpanderProcessorTests.java | 4 +- .../ingest/common/RenameProcessorTests.java | 6 +- .../elasticsearch/ingest/IngestClientIT.java | 2 +- .../ingest/WriteableIngestDocument.java | 9 +- .../elasticsearch/ingest/IngestDocument.java | 25 +- .../elasticsearch/ingest/IngestService.java | 22 +- .../ingest/IngestSourceAndMetadata.java | 384 +++--------------- .../java/org/elasticsearch/script/CtxMap.java | 104 ----- .../elasticsearch/script/MapWrappable.java | 26 -- .../org/elasticsearch/script/MapWrapper.java | 210 ---------- .../org/elasticsearch/script/Metadata.java | 302 +++++++++++++- .../ingest/SimulateExecutionServiceTests.java | 2 +- .../SimulatePipelineRequestParsingTests.java | 33 +- .../ingest/WriteableIngestDocumentTests.java | 13 +- .../ingest/IngestServiceTests.java | 2 +- .../ingest/IngestSourceAndMetadataTests.java | 139 ++++--- .../elasticsearch/script/MetadataTests.java | 25 ++ .../ingest/TestIngestDocument.java | 21 +- .../elasticsearch/script/TestMetadata.java | 24 ++ .../results/InferenceResultsTestCase.java | 11 +- 20 files changed, 513 insertions(+), 851 deletions(-) delete mode 100644 server/src/main/java/org/elasticsearch/script/CtxMap.java delete mode 100644 server/src/main/java/org/elasticsearch/script/MapWrappable.java delete mode 100644 server/src/main/java/org/elasticsearch/script/MapWrapper.java create mode 100644 server/src/test/java/org/elasticsearch/script/MetadataTests.java create mode 100644 test/framework/src/main/java/org/elasticsearch/script/TestMetadata.java diff --git a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/DotExpanderProcessorTests.java b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/DotExpanderProcessorTests.java index 1714717d0e6d3..fc17506555edb 100644 --- a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/DotExpanderProcessorTests.java +++ b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/DotExpanderProcessorTests.java @@ -48,7 +48,7 @@ public void testEscapeFields() throws Exception { processor = new DotExpanderProcessor("_tag", null, null, "foo.bar"); processor.execute(document); assertThat(document.getSource().size(), equalTo(1)); - assertThat(document.getMetadataMap().size(), equalTo(1)); // the default version + assertThat(document.getMetadata().size(), equalTo(1)); // the default version assertThat(document.getFieldValue("foo.bar", List.class).size(), equalTo(2)); assertThat(document.getFieldValue("foo.bar.0", String.class), equalTo("baz2")); assertThat(document.getFieldValue("foo.bar.1", String.class), equalTo("baz1")); @@ -60,7 +60,7 @@ public void testEscapeFields() throws Exception { processor = new DotExpanderProcessor("_tag", null, null, "foo.bar"); processor.execute(document); assertThat(document.getSource().size(), equalTo(1)); - assertThat(document.getMetadataMap().size(), equalTo(1)); // the default version + assertThat(document.getMetadata().size(), equalTo(1)); // the default version assertThat(document.getFieldValue("foo.bar", List.class).size(), equalTo(2)); assertThat(document.getFieldValue("foo.bar.0", Integer.class), equalTo(1)); assertThat(document.getFieldValue("foo.bar.1", String.class), equalTo("2")); diff --git a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RenameProcessorTests.java b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RenameProcessorTests.java index 4cab0b999c248..32566e82baf80 100644 --- a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RenameProcessorTests.java +++ b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RenameProcessorTests.java @@ -140,11 +140,11 @@ public void testRenameAtomicOperationSetFails() throws Exception { Map metadata = new HashMap<>(); metadata.put("list", Collections.singletonList("item")); - IngestDocument ingestDocument = TestIngestDocument.ofMetadataWithValidator(metadata, Map.of("new_field", (k, v) -> { + IngestDocument ingestDocument = TestIngestDocument.ofMetadataWithValidator(metadata, Map.of("new_field", (o, k, v) -> { if (v != null) { throw new UnsupportedOperationException(); } - }, "list", (k, v) -> {})); + }, "list", (o, k, v) -> {})); Processor processor = createRenameProcessor("list", "new_field", false); try { processor.execute(ingestDocument); @@ -160,7 +160,7 @@ public void testRenameAtomicOperationRemoveFails() throws Exception { Map metadata = new HashMap<>(); metadata.put("list", Collections.singletonList("item")); - IngestDocument ingestDocument = TestIngestDocument.ofMetadataWithValidator(metadata, Map.of("list", (k, v) -> { + IngestDocument ingestDocument = TestIngestDocument.ofMetadataWithValidator(metadata, Map.of("list", (o, k, v) -> { if (v == null) { throw new UnsupportedOperationException(); } diff --git a/server/src/internalClusterTest/java/org/elasticsearch/ingest/IngestClientIT.java b/server/src/internalClusterTest/java/org/elasticsearch/ingest/IngestClientIT.java index 2e69cdff0fe80..8407d72eb728e 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/ingest/IngestClientIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/ingest/IngestClientIT.java @@ -115,7 +115,7 @@ public void testSimulate() throws Exception { source.put("processed", true); IngestDocument ingestDocument = new IngestDocument("index", "id", Versions.MATCH_ANY, null, null, source); assertThat(simulateDocumentBaseResult.getIngestDocument().getSource(), equalTo(ingestDocument.getSource())); - assertThat(simulateDocumentBaseResult.getIngestDocument().getMetadataMap(), equalTo(ingestDocument.getMetadataMap())); + assertThat(simulateDocumentBaseResult.getIngestDocument().getMetadata(), equalTo(ingestDocument.getMetadata())); assertThat(simulateDocumentBaseResult.getFailure(), nullValue()); // cleanup diff --git a/server/src/main/java/org/elasticsearch/action/ingest/WriteableIngestDocument.java b/server/src/main/java/org/elasticsearch/action/ingest/WriteableIngestDocument.java index e9e2882763e33..ade4a99133712 100644 --- a/server/src/main/java/org/elasticsearch/action/ingest/WriteableIngestDocument.java +++ b/server/src/main/java/org/elasticsearch/action/ingest/WriteableIngestDocument.java @@ -107,10 +107,11 @@ IngestDocument getIngestDocument() { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(DOC_FIELD); - Map metadataMap = ingestDocument.getMetadataMap(); - for (Map.Entry metadata : metadataMap.entrySet()) { - if (metadata.getValue() != null) { - builder.field(metadata.getKey(), metadata.getValue().toString()); + org.elasticsearch.script.Metadata metadata = ingestDocument.getMetadata(); + for (String key : metadata.keys()) { + Object value = metadata.get(key); + if (value != null) { + builder.field(key, value.toString()); } } if (builder.getRestApiVersion() == RestApiVersion.V_7) { diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java index e6e8f428efb3a..c70c1a5ca36fe 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java @@ -68,7 +68,7 @@ public IngestDocument(String index, String id, long version, String routing, Ver source ); this.ingestMetadata = new HashMap<>(); - this.ingestMetadata.put(TIMESTAMP, sourceAndMetadata.getTimestamp()); + this.ingestMetadata.put(TIMESTAMP, sourceAndMetadata.getMetadata().getTimestamp()); } /** @@ -76,12 +76,7 @@ public IngestDocument(String index, String id, long version, String routing, Ver */ public IngestDocument(IngestDocument other) { this( - new IngestSourceAndMetadata( - deepCopyMap(other.sourceAndMetadata.getSource()), - deepCopyMap(other.sourceAndMetadata.getMetadata()), - other.getIngestSourceAndMetadata().timestamp, - other.getIngestSourceAndMetadata().validators - ), + new IngestSourceAndMetadata(deepCopyMap(other.sourceAndMetadata.getSource()), other.sourceAndMetadata.getMetadata().clone()), deepCopyMap(other.ingestMetadata) ); } @@ -90,17 +85,16 @@ public IngestDocument(IngestDocument other) { * Constructor to create an IngestDocument from its constituent maps. The maps are shallow copied. */ public IngestDocument(Map sourceAndMetadata, Map ingestMetadata) { + // TODO(stu): fix Tuple, Map> sm = IngestSourceAndMetadata.splitSourceAndMetadata(sourceAndMetadata); this.sourceAndMetadata = new IngestSourceAndMetadata( sm.v1(), - sm.v2(), - IngestSourceAndMetadata.getTimestamp(ingestMetadata), - IngestSourceAndMetadata.VALIDATORS + new org.elasticsearch.script.Metadata(sm.v2(), IngestSourceAndMetadata.getTimestamp(ingestMetadata)) ); this.ingestMetadata = new HashMap<>(ingestMetadata); this.ingestMetadata.computeIfPresent(TIMESTAMP, (k, v) -> { if (v instanceof String) { - return this.sourceAndMetadata.getTimestamp(); + return this.sourceAndMetadata.getMetadata().getTimestamp(); } return v; }); @@ -737,18 +731,11 @@ public IngestSourceAndMetadata getIngestSourceAndMetadata() { return sourceAndMetadata; } - /** - * Get all Metadata values in a Map - */ - public Map getMetadataMap() { - return sourceAndMetadata.getMetadata(); - } - /** * Get the strongly typed metadata */ public org.elasticsearch.script.Metadata getMetadata() { - return sourceAndMetadata; + return sourceAndMetadata.getMetadata(); } /** diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestService.java b/server/src/main/java/org/elasticsearch/ingest/IngestService.java index 67f0abae7b23d..152c9a80f7d62 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestService.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestService.java @@ -897,27 +897,27 @@ private void innerExecute( itemDroppedHandler.accept(slot); handler.accept(null); } else { - IngestSourceAndMetadata sourceAndMetadata = ingestDocument.getIngestSourceAndMetadata(); + org.elasticsearch.script.Metadata metadata = ingestDocument.getMetadata(); // it's fine to set all metadata fields all the time, as ingest document holds their starting values // before ingestion, which might also get modified during ingestion. - indexRequest.index(sourceAndMetadata.getIndex()); - indexRequest.id(sourceAndMetadata.getId()); - indexRequest.routing(sourceAndMetadata.getRouting()); - indexRequest.version(sourceAndMetadata.getVersion()); - if (sourceAndMetadata.getVersionType() != null) { - indexRequest.versionType(VersionType.fromString(sourceAndMetadata.getVersionType())); + indexRequest.index(metadata.getIndex()); + indexRequest.id(metadata.getId()); + indexRequest.routing(metadata.getRouting()); + indexRequest.version(metadata.getVersion()); + if (metadata.getVersionType() != null) { + indexRequest.versionType(VersionType.fromString(metadata.getVersionType())); } Number number; - if ((number = sourceAndMetadata.getIfSeqNo()) != null) { + if ((number = metadata.getIfSeqNo()) != null) { indexRequest.setIfSeqNo(number.longValue()); } - if ((number = sourceAndMetadata.getIfPrimaryTerm()) != null) { + if ((number = metadata.getIfPrimaryTerm()) != null) { indexRequest.setIfPrimaryTerm(number.longValue()); } try { boolean ensureNoSelfReferences = ingestDocument.doNoSelfReferencesCheck(); - indexRequest.source(sourceAndMetadata.getSource(), indexRequest.getContentType(), ensureNoSelfReferences); + indexRequest.source(ingestDocument.getSource(), indexRequest.getContentType(), ensureNoSelfReferences); } catch (IllegalArgumentException ex) { // An IllegalArgumentException can be thrown when an ingest // processor creates a source map that is self-referencing. @@ -933,7 +933,7 @@ private void innerExecute( return; } Map map; - if ((map = sourceAndMetadata.getDynamicTemplates()) != null) { + if ((map = metadata.getDynamicTemplates()) != null) { Map mergedDynamicTemplates = new HashMap<>(indexRequest.getDynamicTemplates()); mergedDynamicTemplates.putAll(map); indexRequest.setDynamicTemplates(mergedDynamicTemplates); diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java b/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java index 68d779be37812..7978c8d4e08ed 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java @@ -17,16 +17,13 @@ import java.util.AbstractCollection; import java.util.AbstractMap; import java.util.AbstractSet; -import java.util.Arrays; import java.util.Collection; import java.util.HashMap; -import java.util.HashSet; import java.util.Iterator; +import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.function.BiConsumer; -import java.util.stream.Collectors; /** * Map containing ingest source and metadata. @@ -41,36 +38,10 @@ * * The map is expected to be used by processors, server code should the typed getter and setters where possible. */ -class IngestSourceAndMetadata extends AbstractMap implements Metadata { - protected final ZonedDateTime timestamp; - - /** - * map of key to validating function. Should throw {@link IllegalArgumentException} on invalid value - */ - static final Map> VALIDATORS = Map.of( - IngestDocument.Metadata.INDEX.getFieldName(), - IngestSourceAndMetadata::stringValidator, - IngestDocument.Metadata.ID.getFieldName(), - IngestSourceAndMetadata::stringValidator, - IngestDocument.Metadata.ROUTING.getFieldName(), - IngestSourceAndMetadata::stringValidator, - IngestDocument.Metadata.VERSION.getFieldName(), - IngestSourceAndMetadata::versionValidator, - IngestDocument.Metadata.VERSION_TYPE.getFieldName(), - IngestSourceAndMetadata::versionTypeValidator, - IngestDocument.Metadata.DYNAMIC_TEMPLATES.getFieldName(), - IngestSourceAndMetadata::mapValidator, - IngestDocument.Metadata.IF_SEQ_NO.getFieldName(), - IngestSourceAndMetadata::longValidator, - IngestDocument.Metadata.IF_PRIMARY_TERM.getFieldName(), - IngestSourceAndMetadata::longValidator, - IngestDocument.Metadata.TYPE.getFieldName(), - IngestSourceAndMetadata::stringValidator - ); +class IngestSourceAndMetadata extends AbstractMap { protected final Map source; - protected final Map metadata; - protected final Map> validators; + protected final Metadata metadata; private EntrySet entrySet; // cache to avoid recreation /** @@ -85,7 +56,7 @@ class IngestSourceAndMetadata extends AbstractMap implements Met ZonedDateTime timestamp, Map source ) { - this(new HashMap<>(source), metadataMap(index, id, version, routing, versionType), timestamp, VALIDATORS); + this(new HashMap<>(source), new Metadata(index, id, version, routing, versionType, timestamp)); } /** @@ -93,38 +64,10 @@ class IngestSourceAndMetadata extends AbstractMap implements Met * * @param source the source document map * @param metadata the metadata map - * @param timestamp the time of ingestion - * @param validators validators to run on metadata map, if a key is in this map, the value is stored in metadata. - * if null, use the default validators from {@link #VALIDATORS} */ - IngestSourceAndMetadata( - Map source, - Map metadata, - ZonedDateTime timestamp, - Map> validators - ) { + IngestSourceAndMetadata(Map source, Metadata metadata) { this.source = source != null ? source : new HashMap<>(); - this.metadata = metadata != null ? metadata : new HashMap<>(); - this.timestamp = timestamp; - this.validators = validators != null ? validators : VALIDATORS; - validateMetadata(); - } - - /** - * Create the backing metadata map with the standard contents assuming default validators. - */ - protected static Map metadataMap(String index, String id, long version, String routing, VersionType versionType) { - Map metadata = Maps.newHashMapWithExpectedSize(IngestDocument.Metadata.values().length); - metadata.put(IngestDocument.Metadata.INDEX.getFieldName(), index); - metadata.put(IngestDocument.Metadata.ID.getFieldName(), id); - metadata.put(IngestDocument.Metadata.VERSION.getFieldName(), version); - if (routing != null) { - metadata.put(IngestDocument.Metadata.ROUTING.getFieldName(), routing); - } - if (versionType != null) { - metadata.put(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), VersionType.toString(versionType)); - } - return metadata; + this.metadata = metadata; } /** @@ -132,11 +75,12 @@ protected static Map metadataMap(String index, String id, long v */ public static Tuple, Map> splitSourceAndMetadata(Map sourceAndMetadata) { if (sourceAndMetadata instanceof IngestSourceAndMetadata ingestSourceAndMetadata) { - return new Tuple<>(new HashMap<>(ingestSourceAndMetadata.source), new HashMap<>(ingestSourceAndMetadata.metadata)); + return new Tuple<>(new HashMap<>(ingestSourceAndMetadata.source), new HashMap<>(ingestSourceAndMetadata.metadata.mapCopy())); } Map metadata = Maps.newHashMapWithExpectedSize(IngestDocument.Metadata.values().length); Map source = new HashMap<>(sourceAndMetadata); - for (String metadataName : VALIDATORS.keySet()) { + for (IngestDocument.Metadata ingestDocumentMetadata : IngestDocument.Metadata.values()) { + String metadataName = ingestDocumentMetadata.getFieldName(); if (sourceAndMetadata.containsKey(metadataName)) { metadata.put(metadataName, source.remove(metadataName)); } @@ -171,105 +115,17 @@ public Map getSource() { /** * get the metadata map, if externally modified then the guarantees of this class are not enforced */ - public Map getMetadata() { + public Metadata getMetadata() { return metadata; } - // These are available to scripts - public String getIndex() { - return getString(IngestDocument.Metadata.INDEX.getFieldName()); - } - - public void setIndex(String index) { - put(IngestDocument.Metadata.INDEX.getFieldName(), index); - } - - public String getId() { - return getString(IngestDocument.Metadata.ID.getFieldName()); - } - - public void setId(String id) { - put(IngestDocument.Metadata.ID.getFieldName(), id); - } - - public String getRouting() { - return getString(IngestDocument.Metadata.ROUTING.getFieldName()); - } - - public void setRouting(String routing) { - put(IngestDocument.Metadata.ROUTING.getFieldName(), routing); - } - - public String getVersionType() { - return getString(IngestDocument.Metadata.VERSION_TYPE.getFieldName()); - } - - public void setVersionType(String versionType) { - put(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), versionType); - } - - public long getVersion() { - Number version = getNumber(IngestDocument.Metadata.VERSION.getFieldName()); - assert version != null : IngestDocument.Metadata.VERSION.getFieldName() + " validation allowed null version"; - return version.longValue(); - } - - public void setVersion(long version) { - put(IngestDocument.Metadata.VERSION.getFieldName(), version); - } - - // timestamp isn't backed by the map - public ZonedDateTime getTimestamp() { - return timestamp; - } - - // These are not available to scripts - public Number getIfSeqNo() { - return getNumber(IngestDocument.Metadata.IF_SEQ_NO.getFieldName()); - } - - public Number getIfPrimaryTerm() { - return getNumber(IngestDocument.Metadata.IF_PRIMARY_TERM.getFieldName()); - } - - @SuppressWarnings("unchecked") - public Map getDynamicTemplates() { - return (Map) metadata.get(IngestDocument.Metadata.DYNAMIC_TEMPLATES.getFieldName()); - } - - /** - * Check that all metadata map contains only valid metadata and no extraneous keys and source map contains no metadata - */ - protected void validateMetadata() { - int numMetadata = 0; - for (Map.Entry> entry : validators.entrySet()) { - String key = entry.getKey(); - if (metadata.containsKey(key)) { - numMetadata++; - } - entry.getValue().accept(key, metadata.get(key)); - if (source.containsKey(key)) { - throw new IllegalArgumentException("Unexpected metadata key [" + key + "] in source with value [" + source.get(key) + "]"); - } - } - if (numMetadata < metadata.size()) { - Set keys = new HashSet<>(metadata.keySet()); - keys.removeAll(validators.keySet()); - throw new IllegalArgumentException( - "Unexpected metadata keys [" - + keys.stream().sorted().map(k -> k + ":" + metadata.get(k)).collect(Collectors.joining(", ")) - + "]" - ); - } - } - /** * Returns an entrySet that respects the validators of the map. */ @Override public Set> entrySet() { if (entrySet == null) { - entrySet = new EntrySet(source.entrySet(), metadata.entrySet()); + entrySet = new EntrySet(source.entrySet(), metadata.keys()); } return entrySet; } @@ -280,9 +136,7 @@ public Set> entrySet() { */ @Override public Object put(String key, Object value) { - BiConsumer validator = validators.get(key); - if (validator != null) { - validator.accept(key, value); + if (metadata.ownsKey(key)) { return metadata.put(key, value); } return source.put(key, value); @@ -295,11 +149,9 @@ public Object put(String key, Object value) { @Override public Object remove(Object key) { // uses map directly to avoid AbstractMaps linear time implementation using entrySet() - if (key instanceof String strKey) { - BiConsumer validator = validators.get(key); - if (validator != null) { - validator.accept(strKey, null); - return metadata.remove(key); + if (key instanceof String str) { + if (metadata.ownsKey(str)) { + return metadata.remove(str); } } return source.remove(key); @@ -312,12 +164,9 @@ public Object remove(Object key) { @Override public void clear() { // AbstractMap uses entrySet().clear(), it should be quicker to run through the validators, then call the wrapped maps clear - validators.forEach((k, v) -> { - if (metadata.containsKey(k)) { - v.accept(k, null); - } - }); - metadata.clear(); + for (String key : metadata.keys()) { + metadata.remove(key); + } source.clear(); } @@ -336,42 +185,23 @@ public boolean containsValue(Object value) { @Override public boolean containsKey(Object key) { // uses map directly to avoid AbstractMaps linear time implementation using entrySet() - return metadata.containsKey(key) || source.containsKey(key); + if (key instanceof String str) { + return metadata.containsKey(str) || source.containsKey(key); + } + return source.containsKey(key); } @Override public Object get(Object key) { // uses map directly to avoid AbstractMaps linear time implementation using entrySet() - if (validators.get(key) != null) { - return metadata.get(key); + if (key instanceof String str) { + if (metadata.ownsKey(str)) { + return metadata.get(str); + } } return source.get(key); } - /** - * Get the String version of the value associated with {@code key}, or null - */ - public String getString(Object key) { - return Objects.toString(get(key), null); - } - - /** - * Get the {@link Number} associated with key, or null - * @throws IllegalArgumentException if the value is not a {@link Number} - */ - public Number getNumber(Object key) { - Object value = get(key); - if (value == null) { - return null; - } - if (value instanceof Number number) { - return number; - } - throw new IllegalArgumentException( - "unexpected type for [" + key + "] with value [" + value + "], expected Number, got [" + value.getClass().getName() + "]" - ); - } - /** * Set of entries of the wrapped map that calls the appropriate validator before changing an entries value or removing an entry. * @@ -382,32 +212,31 @@ public Number getNumber(Object key) { */ class EntrySet extends AbstractSet> { Set> sourceSet; - Set> metadataSet; + List metadataKeys; - EntrySet(Set> sourceSet, Set> metadataSet) { + EntrySet(Set> sourceSet, List metadataKeys) { this.sourceSet = sourceSet; - this.metadataSet = metadataSet; + this.metadataKeys = metadataKeys; } @Override public Iterator> iterator() { - return new EntrySetIterator(sourceSet.iterator(), metadataSet.iterator()); + return new EntrySetIterator(sourceSet.iterator(), metadataKeys.iterator()); } @Override public int size() { - return sourceSet.size() + metadataSet.size(); + return sourceSet.size() + metadataKeys.size(); } @Override public boolean remove(Object o) { - if (metadataSet.contains(o)) { - if (o instanceof Map.Entry entry) { - if (entry.getKey()instanceof String key) { - BiConsumer validator = validators.get(key); - if (validator != null) { - validator.accept(key, null); - return metadataSet.remove(o); + if (o instanceof Map.Entry entry) { + if (entry.getKey()instanceof String key) { + if (metadata.containsKey(key)) { + if (Objects.equals(entry.getValue(), metadata.get(key))) { + metadata.remove(key); + return true; } } } @@ -424,25 +253,25 @@ public boolean remove(Object o) { */ class EntrySetIterator implements Iterator> { final Iterator> sourceIter; - final Iterator> metadataIter; + final Iterator metadataKeyIter; boolean sourceCur = true; - Entry cur; + Map.Entry cur; - EntrySetIterator(Iterator> sourceIter, Iterator> metadataIter) { + EntrySetIterator(Iterator> sourceIter, Iterator metadataKeyIter) { this.sourceIter = sourceIter; - this.metadataIter = metadataIter; + this.metadataKeyIter = metadataKeyIter; } @Override public boolean hasNext() { - return sourceIter.hasNext() || metadataIter.hasNext(); + return sourceIter.hasNext() || metadataKeyIter.hasNext(); } @Override public Map.Entry next() { sourceCur = sourceIter.hasNext(); - return cur = new Entry(sourceCur ? sourceIter.next() : metadataIter.next(), sourceCur); + return cur = sourceCur ? sourceIter.next() : new Entry(metadataKeyIter.next()); } /** @@ -458,145 +287,34 @@ public void remove() { if (sourceCur) { sourceIter.remove(); } else { - BiConsumer validator = validators.get(cur.getKey()); - if (validator != null) { - validator.accept(cur.getKey(), null); - } - metadataIter.remove(); + metadata.remove(cur.getKey()); } } } /** - * Wrapped Map.Entry that calls the key's validator on {@link #setValue(Object)} + * Map.Entry that stores metadata key and calls into {@link #metadata} for {@link #setValue} */ class Entry implements Map.Entry { - final Map.Entry entry; - final boolean isSource; + final String key; - Entry(Map.Entry entry, boolean isSource) { - this.entry = entry; - this.isSource = isSource; + Entry(String key) { + this.key = key; } @Override public String getKey() { - return entry.getKey(); + return key; } @Override public Object getValue() { - return entry.getValue(); + return metadata.get(key); } - /** - * Associate the value with the Entry's key in the linked Map. If the Entry's key has a validator, it is applied before association - * @throws IllegalArgumentException if value does not pass validation for the Entry's key - */ @Override public Object setValue(Object value) { - if (isSource == false) { - BiConsumer validator = validators.get(entry.getKey()); - if (validator != null) { - validator.accept(entry.getKey(), value); - } - } - return entry.setValue(value); - } - } - - /** - * Allow a String or null - */ - protected static void stringValidator(String key, Object value) { - if (value == null || value instanceof String) { - return; - } - throw new IllegalArgumentException( - key + " must be null or a String but was [" + value + "] with type [" + value.getClass().getName() + "]" - ); - } - - /** - * Allow Numbers that can be represented as longs without loss of precision - */ - protected static void longValidator(String key, Object value) { - if (value == null) { - return; // Allow null version for now - } - if (value instanceof Number number) { - long version = number.longValue(); - // did we round? - if (number.doubleValue() == version) { - return; - } - } - throw new IllegalArgumentException( - key + " may only be set to an int or a long but was [" + value + "] with type [" + value.getClass().getName() + "]" - ); - } - - /** - * Version must be non-null and representable as a long without loss of precision - */ - protected static void versionValidator(String key, Object value) { - if (value == null) { - throw new IllegalArgumentException(key + " cannot be null"); - } - longValidator(key, value); - } - - /** - * Allow lower case Strings that map to VersionType values, or null - */ - protected static void versionTypeValidator(String key, Object value) { - if (value == null) { - return; - } - if (value instanceof String versionType) { - try { - VersionType.fromString(versionType); - return; - } catch (IllegalArgumentException ignored) {} - } - throw new IllegalArgumentException( - key - + " must be a null or one of [" - + Arrays.stream(VersionType.values()).map(vt -> VersionType.toString(vt)).collect(Collectors.joining(", ")) - + "] but was [" - + value - + "] with type [" - + value.getClass().getName() - + "]" - ); - } - - /** - * Allow maps - */ - protected static void mapValidator(String key, Object value) { - if (value == null || value instanceof Map) { - return; + return metadata.put(key, value); } - throw new IllegalArgumentException( - key + " must be a null or a Map but was [" + value + "] with type [" + value.getClass().getName() + "]" - ); - } - - @Override - public boolean equals(Object o) { - if (this == o) return true; - if ((o instanceof IngestSourceAndMetadata) == false) return false; - if (super.equals(o) == false) return false; - IngestSourceAndMetadata that = (IngestSourceAndMetadata) o; - return Objects.equals(timestamp, that.timestamp) - && source.equals(that.source) - && metadata.equals(that.metadata) - && validators.equals(that.validators); - } - - @Override - public int hashCode() { - return Objects.hash(timestamp, source, metadata, validators); } } diff --git a/server/src/main/java/org/elasticsearch/script/CtxMap.java b/server/src/main/java/org/elasticsearch/script/CtxMap.java deleted file mode 100644 index 446d33c34a8f7..0000000000000 --- a/server/src/main/java/org/elasticsearch/script/CtxMap.java +++ /dev/null @@ -1,104 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0 and the Server Side Public License, v 1; you may not use this file except - * in compliance with, at your election, the Elastic License 2.0 or the Server - * Side Public License, v 1. - */ - -package org.elasticsearch.script; - -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - -public class CtxMap extends MapWrapper { - - protected Map source; - protected M metadata; - - CtxMap(Map source, M metadata) { - super(new SourceAndMetadataMap(source, metadata)); - this.source = source; - this.metadata = metadata; - } - - public Map getSource() { - return source; - } - - public M getMetadata() { - return metadata; - } - - private static class SourceAndMetadataMap implements MapWrappable { - protected Map source; - protected MapWrappable metadata; - - SourceAndMetadataMap(Map source, MapWrappable metadata) { - this.source = source; - this.metadata = metadata; - } - - @Override - public boolean ownsKey(String key) { - return true; - } - - @Override - public Object put(String key, Object value) { - if (metadata.ownsKey(key)) { - return metadata.put(key, value); - } - return source.put(key, value); - } - - @Override - public boolean containsKey(String key) { - return source.containsKey(key) || metadata.containsKey(key); - } - - @Override - public boolean containsValue(Object value) { - return source.containsValue(value) || metadata.containsValue(value); - } - - @Override - public Object get(String key) { - if (metadata.ownsKey(key)) { - return metadata.get(key); - } - return source.get(key); - } - - @Override - public Object remove(String key) { - if (metadata.ownsKey(key)) { - return metadata.remove(key); - } - return source.remove(key); - } - - @Override - public List keys() { - List keys = new ArrayList<>(size()); - keys.addAll(metadata.keys()); - keys.addAll(source.keySet()); - return keys; - } - - @Override - public int size() { - return metadata.size() + source.size(); - } - - @Override - public MapWrappable clone() { - return new CtxMap( - new HashMap<>(source), - metadata.clone() - ); - } - } -} diff --git a/server/src/main/java/org/elasticsearch/script/MapWrappable.java b/server/src/main/java/org/elasticsearch/script/MapWrappable.java deleted file mode 100644 index 8c532e8967f69..0000000000000 --- a/server/src/main/java/org/elasticsearch/script/MapWrappable.java +++ /dev/null @@ -1,26 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0 and the Server Side Public License, v 1; you may not use this file except - * in compliance with, at your election, the Elastic License 2.0 or the Server - * Side Public License, v 1. - */ - -package org.elasticsearch.script; - -import java.util.List; - -/** - * A class that can easily be wrapped in a Map interface by {@link MapWrapper} - */ -public interface MapWrappable { - boolean ownsKey(String key); - Object put(String key, Object value); - boolean containsKey(String key); - boolean containsValue(Object value); - Object get(String key); - Object remove(String key); - List keys(); - int size(); - MapWrappable clone(); -} diff --git a/server/src/main/java/org/elasticsearch/script/MapWrapper.java b/server/src/main/java/org/elasticsearch/script/MapWrapper.java deleted file mode 100644 index d220f8c5ff192..0000000000000 --- a/server/src/main/java/org/elasticsearch/script/MapWrapper.java +++ /dev/null @@ -1,210 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0 and the Server Side Public License, v 1; you may not use this file except - * in compliance with, at your election, the Elastic License 2.0 or the Server - * Side Public License, v 1. - */ - -package org.elasticsearch.script; - -import java.util.AbstractCollection; -import java.util.AbstractMap; -import java.util.AbstractSet; -import java.util.Collection; -import java.util.Iterator; -import java.util.List; -import java.util.Map; -import java.util.Objects; -import java.util.Set; - -/** - * Implements a Map interface, including {@link Map.Entry} and {@link Iterator} for classes that - * implement {@link MapWrappable}. - */ -class MapWrapper extends AbstractMap { - private EntrySet entrySet; // cache to avoid recreation - protected final MapWrappable wrapped; - - MapWrapper(MapWrappable wrapped) { - this.wrapped = wrapped; - } - - /** - * Returns an entrySet that respects the validators of the map. - */ - @Override - public Set> entrySet() { - if (entrySet == null) { - entrySet = new EntrySet(wrapped.keys()); - } - return entrySet; - } - - /** - * Associate a key with a value. If the key has a validator, it is applied before association. - * @throws IllegalArgumentException if value does not pass validation for the given key - */ - @Override - public Object put(String key, Object value) { - return wrapped.put(key, value); - } - - /** - * Remove the mapping of key. If the key has a validator, it is checked before key removal. - * @throws IllegalArgumentException if the validator does not allow the key to be removed - */ - @Override - public Object remove(Object key) { - // uses map directly to avoid AbstractMaps linear time implementation using entrySet() - if (key instanceof String str) { - return wrapped.remove(str); - } - return null; - } - - /** - * Clear entire map. For each key in the map with a validator, that validator is checked as in {@link #remove(Object)}. - * @throws IllegalArgumentException if any validator does not allow the key to be removed, in this case the map is may have been - * modified - */ - @Override - public void clear() { - // AbstractMap uses entrySet().clear(), it should be quicker to run through the metadata keys, then call the wrapped maps clear - for (String key : wrapped.keys()) { - wrapped.remove(key); - } - } - - @Override - public int size() { - // uses map directly to avoid creating an EntrySet via AbstractMaps implementation, which returns entrySet().size() - return wrapped.size(); - } - - @Override - public boolean containsValue(Object value) { - // uses map directly to avoid AbstractMaps linear time implementation using entrySet() - return wrapped.containsValue(value); - } - - @Override - public boolean containsKey(Object key) { - // uses map directly to avoid AbstractMaps linear time implementation using entrySet() - if (key instanceof String str) { - return wrapped.containsKey(str); - } - return false; - } - - @Override - public Object get(Object key) { - // uses map directly to avoid AbstractMaps linear time implementation using entrySet() - if (key instanceof String str) { - return wrapped.get(str); - } - return null; - } - - /** - * Set of entries of the wrapped map that calls the appropriate validator before changing an entries value or removing an entry. - * - * Inherits {@link AbstractSet#removeAll(Collection)}, which calls the overridden {@link #remove(Object)} which performs validation. - * - * Inherits {@link AbstractCollection#retainAll(Collection)} and {@link AbstractCollection#clear()}, which both use - * {@link EntrySetIterator#remove()} for removal. - */ - class EntrySet extends AbstractSet> { - List wrappedKeys; - - EntrySet(List wrappedKeys) { - this.wrappedKeys = wrappedKeys; - } - - @Override - public Iterator> iterator() { - return new EntrySetIterator(wrappedKeys.iterator()); - } - - @Override - public int size() { - return wrappedKeys.size(); - } - - @Override - public boolean remove(Object o) { - if (o instanceof Map.Entry entry) { - if (entry.getKey()instanceof String key) { - if (wrapped.containsKey(key)) { - if (Objects.equals(entry.getValue(), wrapped.get(key))) { - wrapped.remove(key); - return true; - } - } - } - } - return false; - } - } - - /** - * Iterator over the wrapped map that returns a validating {@link Entry} on {@link #next()} and validates on {@link #remove()}. - * - * {@link #remove()} is called by remove in {@link AbstractMap#values()}, {@link AbstractMap#keySet()}, {@link AbstractMap#clear()} via - * {@link AbstractSet#clear()} - */ - class EntrySetIterator implements Iterator> { - final Iterator wrappedIter; - Map.Entry cur; - - EntrySetIterator(Iterator wrappedIter) { - this.wrappedIter = wrappedIter; - } - - @Override - public boolean hasNext() { - return wrappedIter.hasNext(); - } - - @Override - public Map.Entry next() { - return cur = new Entry(wrappedIter.next()); - } - - /** - * Remove current entry from the backing Map. Checks the Entry's key's validator, if one exists, before removal. - * @throws IllegalArgumentException if the validator does not allow the Entry to be removed - * @throws IllegalStateException if remove is called before {@link #next()} - */ - @Override - public void remove() { - if (cur == null) { - throw new IllegalStateException(); - } - wrapped.remove(cur.getKey()); - } - } - - class Entry implements Map.Entry { - final String key; - - Entry(String key) { - this.key = key; - } - - @Override - public String getKey() { - return key; - } - - @Override - public Object getValue() { - return wrapped.get(key); - } - - @Override - public Object setValue(Object value) { - return wrapped.put(key, value); - } - } -} diff --git a/server/src/main/java/org/elasticsearch/script/Metadata.java b/server/src/main/java/org/elasticsearch/script/Metadata.java index cae0d47469f66..10a93ac373d1e 100644 --- a/server/src/main/java/org/elasticsearch/script/Metadata.java +++ b/server/src/main/java/org/elasticsearch/script/Metadata.java @@ -8,57 +8,317 @@ package org.elasticsearch.script; +import org.elasticsearch.common.util.Maps; +import org.elasticsearch.index.VersionType; +import org.elasticsearch.ingest.IngestDocument; + import java.time.ZonedDateTime; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.stream.Collectors; /** * Ingest and update metadata available to write scripts */ -public class Metadata implements MapWrappable { +public class Metadata { + protected static final String INDEX = "_index"; + protected static final String ID = "_id"; + protected static final String ROUTING = "_routing"; + protected static final String VERSION_TYPE = "_version_type"; + protected static final String VERSION = "_version"; + protected static final String IF_SEQ_NO = "_if_seq_no"; + protected static final String IF_PRIMARY_TERM = "_if_primary_term"; + protected static final String DYNAMIC_TEMPLATES = "_dynamic_templates"; + + protected static final Map VALIDATORS = Map.of( + INDEX, + Metadata::stringValidator, + ID, + Metadata::stringValidator, + ROUTING, + Metadata::stringValidator, + VERSION_TYPE, + Metadata::stringValidator, + VERSION, + Metadata::notNullLongValidator, + IF_SEQ_NO, + Metadata::longValidator, + IF_PRIMARY_TERM, + Metadata::longValidator, + DYNAMIC_TEMPLATES, + Metadata::mapValidator + ); protected final Map map; + protected final ZonedDateTime timestamp; + protected final Map validators; + + public Metadata(String index, String id, long version, String routing, VersionType versionType, ZonedDateTime timestamp) { + this(metadataMap(index, id, version, routing, versionType), timestamp, VALIDATORS); + } + + public Metadata(Map map, ZonedDateTime timestamp) { + this(map, timestamp, VALIDATORS); + } + + Metadata(Map map, ZonedDateTime timestamp, Map validators) { + this.map = map; + this.timestamp = timestamp; + this.validators = validators; + validateMetadata(); + } /** - * The destination index + * Create the backing metadata map with the standard contents assuming default validators. */ - String getIndex(); - - void setIndex(String index); + protected static Map metadataMap(String index, String id, long version, String routing, VersionType versionType) { + Map metadata = Maps.newHashMapWithExpectedSize(IngestDocument.Metadata.values().length); + metadata.put(IngestDocument.Metadata.INDEX.getFieldName(), index); + metadata.put(IngestDocument.Metadata.ID.getFieldName(), id); + metadata.put(IngestDocument.Metadata.VERSION.getFieldName(), version); + if (routing != null) { + metadata.put(IngestDocument.Metadata.ROUTING.getFieldName(), routing); + } + if (versionType != null) { + metadata.put(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), VersionType.toString(versionType)); + } + return metadata; + } /** - * The document id + * Check that all metadata map contains only valid metadata and no extraneous keys and source map contains no metadata */ - String getId(); + protected void validateMetadata() { + int numMetadata = 0; + for (Map.Entry entry : validators.entrySet()) { + String key = entry.getKey(); + if (map.containsKey(key)) { + numMetadata++; + } + entry.getValue().accept(MapOperation.INIT, key, map.get(key)); + } + if (numMetadata < map.size()) { + Set keys = new HashSet<>(map.keySet()); + keys.removeAll(validators.keySet()); + throw new IllegalArgumentException( + "Unexpected metadata keys [" + keys.stream().sorted().map(k -> k + ":" + map.get(k)).collect(Collectors.joining(", ")) + "]" + ); + } + } + + // These are available to scripts + public String getIndex() { + return getString(INDEX); + } + + public void setIndex(String index) { + put(INDEX, index); + } + + public String getId() { + return getString(ID); + } + + public void setId(String id) { + put(ID, id); + } + + public String getRouting() { + return getString(ROUTING); + } + + public void setRouting(String routing) { + put(ROUTING, routing); + } + + public String getVersionType() { + return getString(VERSION_TYPE); + } + + public void setVersionType(String versionType) { + put(VERSION_TYPE, versionType); + } + + public long getVersion() { + return getNumber(VERSION).longValue(); + } + + public void setVersion(long version) { + put(VERSION, version); + } + + public ZonedDateTime getTimestamp() { + return timestamp; + } + + // These are not available to scripts + public Number getIfSeqNo() { + return getNumber(IF_SEQ_NO); + } - void setId(String id); + public Number getIfPrimaryTerm() { + return getNumber(IF_PRIMARY_TERM); + } + + @SuppressWarnings("unchecked") + public Map getDynamicTemplates() { + return (Map) get(DYNAMIC_TEMPLATES); + } /** - * The document routing string + * Get the String version of the value associated with {@code key}, or null */ - String getRouting(); + public String getString(String key) { + return Objects.toString(get(key), null); + } + + /** + * Get the {@link Number} associated with key, or null + * @throws IllegalArgumentException if the value is not a {@link Number} + */ + public Number getNumber(String key) { + Object value = get(key); + if (value == null) { + return null; + } + if (value instanceof Number number) { + return number; + } + throw new IllegalArgumentException( + "unexpected type for [" + key + "] with value [" + value + "], expected Number, got [" + value.getClass().getName() + "]" + ); + } + + public boolean ownsKey(String key) { + return validators.containsKey(key); + } + + public Object put(String key, Object value) { + Validator v = validators.getOrDefault(key, this::badKey); + v.accept(MapOperation.UPDATE, key, value); + return map.put(key, value); + } + + public boolean containsKey(String key) { + return map.containsKey(key); + } + + public boolean containsValue(Object value) { + return map.containsValue(value); + } + + public Object get(String key) { + return map.get(key); + } - void setRouting(String routing); + public Object remove(String key) { + Validator v = validators.getOrDefault(key, this::badKey); + v.accept(MapOperation.REMOVE, key, null); + return map.remove(key); + } + + public List keys() { + return new ArrayList<>(map.keySet()); + } + + public int size() { + return map.size(); + } + + @Override + public Metadata clone() { + return new Metadata(new HashMap<>(map), timestamp, new HashMap<>(validators)); + } + + public Map mapCopy() { + // This is used for tests, can be removed when Metadata implements HashMap + return new HashMap<>(map); + } /** - * The version of the document + * Allow a String or null */ - long getVersion(); + protected static void stringValidator(MapOperation op, String key, Object value) { + if (op == MapOperation.REMOVE || value == null || value instanceof String) { + return; + } + throw new IllegalArgumentException( + key + " must be null or a String but was [" + value + "] with type [" + value.getClass().getName() + "]" + ); + } - void setVersion(long version); + /** + * Allow Numbers that can be represented as longs without loss of precision + */ + public static void longValidator(MapOperation op, String key, Object value) { + if (op == MapOperation.REMOVE || value == null) { + return; + } + if (value instanceof Number number) { + long version = number.longValue(); + // did we round? + if (number.doubleValue() == version) { + return; + } + } + throw new IllegalArgumentException( + key + " may only be set to an int or a long but was [" + value + "] with type [" + value.getClass().getName() + "]" + ); + } + + protected static void notNullLongValidator(MapOperation op, String key, Object value) { + if (op == MapOperation.REMOVE || value == null) { + throw new IllegalArgumentException(key + " cannot be removed or set to null"); + } + longValidator(op, key, value); + } /** - * The version type of the document, {@link org.elasticsearch.index.VersionType} as a lower-case string. + * Allow maps */ - String getVersionType(); + public static void mapValidator(MapOperation op, String key, Object value) { + if (op == MapOperation.REMOVE || value == null || value instanceof Map) { + return; + } + throw new IllegalArgumentException( + key + " must be a null or a Map but was [" + value + "] with type [" + value.getClass().getName() + "]" + ); + } + + private void badKey(MapOperation op, String key, Object value) { + throw new IllegalArgumentException( + "unexpected metadata key [" + + key + + "], expected one of [" + + validators.keySet().stream().sorted().collect(Collectors.joining(", ")) + + "]" + ); + } /** - * Set the version type of the document. - * @param versionType {@link org.elasticsearch.index.VersionType} as a lower-case string + * The operation being performed on the value in the map. + * INIT: Initial value - the metadata value as passed into this class + * UPDATE: the metadata is being set to a different value + * REMOVE: the metadata mapping is being removed */ - void setVersionType(String versionType); + public enum MapOperation { + INIT, + UPDATE, + REMOVE + } /** - * Timestamp of this ingestion or update + * A "TriConsumer" that tests if the {@link MapOperation}, the metadata key and value are valid. + * + * throws IllegalArgumentException if the given triple is invalid */ - ZonedDateTime getTimestamp(); + @FunctionalInterface + public interface Validator { + void accept(MapOperation op, String key, Object value); + } } diff --git a/server/src/test/java/org/elasticsearch/action/ingest/SimulateExecutionServiceTests.java b/server/src/test/java/org/elasticsearch/action/ingest/SimulateExecutionServiceTests.java index 958c59f67a838..f609114832a70 100644 --- a/server/src/test/java/org/elasticsearch/action/ingest/SimulateExecutionServiceTests.java +++ b/server/src/test/java/org/elasticsearch/action/ingest/SimulateExecutionServiceTests.java @@ -378,7 +378,7 @@ public boolean isAsync() { for (int id = 0; id < numDocs; id++) { SimulateDocumentBaseResult result = (SimulateDocumentBaseResult) response.getResults().get(id); assertThat( - result.getIngestDocument().getMetadataMap().get(IngestDocument.Metadata.ID.getFieldName()), + result.getIngestDocument().getMetadata().get(IngestDocument.Metadata.ID.getFieldName()), equalTo(Integer.toString(id)) ); assertThat(result.getIngestDocument().getSourceAndMetadata().get("processed"), is(true)); diff --git a/server/src/test/java/org/elasticsearch/action/ingest/SimulatePipelineRequestParsingTests.java b/server/src/test/java/org/elasticsearch/action/ingest/SimulatePipelineRequestParsingTests.java index e906a2d619cb0..bdc14e32c2a6f 100644 --- a/server/src/test/java/org/elasticsearch/action/ingest/SimulatePipelineRequestParsingTests.java +++ b/server/src/test/java/org/elasticsearch/action/ingest/SimulatePipelineRequestParsingTests.java @@ -100,9 +100,8 @@ public void testParseUsingPipelineStore() throws Exception { Iterator> expectedDocsIterator = expectedDocs.iterator(); for (IngestDocument ingestDocument : actualRequest.documents()) { Map expectedDocument = expectedDocsIterator.next(); - Map metadataMap = ingestDocument.getMetadataMap(); - assertThat(metadataMap.get(INDEX.getFieldName()), equalTo(expectedDocument.get(INDEX.getFieldName()))); - assertThat(metadataMap.get(ID.getFieldName()), equalTo(expectedDocument.get(ID.getFieldName()))); + assertThat(ingestDocument.getMetadata().get(INDEX.getFieldName()), equalTo(expectedDocument.get(INDEX.getFieldName()))); + assertThat(ingestDocument.getMetadata().get(ID.getFieldName()), equalTo(expectedDocument.get(ID.getFieldName()))); assertThat(ingestDocument.getSource(), equalTo(expectedDocument.get(Fields.SOURCE))); } @@ -196,14 +195,14 @@ public void testParseWithProvidedPipeline() throws Exception { Iterator> expectedDocsIterator = expectedDocs.iterator(); for (IngestDocument ingestDocument : actualRequest.documents()) { Map expectedDocument = expectedDocsIterator.next(); - Map metadataMap = ingestDocument.getMetadataMap(); - assertThat(metadataMap.get(INDEX.getFieldName()), equalTo(expectedDocument.get(INDEX.getFieldName()))); - assertThat(metadataMap.get(ID.getFieldName()), equalTo(expectedDocument.get(ID.getFieldName()))); - assertThat(metadataMap.get(ROUTING.getFieldName()), equalTo(expectedDocument.get(ROUTING.getFieldName()))); - assertThat(metadataMap.get(VERSION.getFieldName()), equalTo(expectedDocument.get(VERSION.getFieldName()))); - assertThat(metadataMap.get(VERSION_TYPE.getFieldName()), equalTo(expectedDocument.get(VERSION_TYPE.getFieldName()))); - assertThat(metadataMap.get(IF_SEQ_NO.getFieldName()), equalTo(expectedDocument.get(IF_SEQ_NO.getFieldName()))); - assertThat(metadataMap.get(IF_PRIMARY_TERM.getFieldName()), equalTo(expectedDocument.get(IF_PRIMARY_TERM.getFieldName()))); + org.elasticsearch.script.Metadata metadata = ingestDocument.getMetadata(); + assertThat(metadata.get(INDEX.getFieldName()), equalTo(expectedDocument.get(INDEX.getFieldName()))); + assertThat(metadata.get(ID.getFieldName()), equalTo(expectedDocument.get(ID.getFieldName()))); + assertThat(metadata.get(ROUTING.getFieldName()), equalTo(expectedDocument.get(ROUTING.getFieldName()))); + assertThat(metadata.get(VERSION.getFieldName()), equalTo(expectedDocument.get(VERSION.getFieldName()))); + assertThat(metadata.get(VERSION_TYPE.getFieldName()), equalTo(expectedDocument.get(VERSION_TYPE.getFieldName()))); + assertThat(metadata.get(IF_SEQ_NO.getFieldName()), equalTo(expectedDocument.get(IF_SEQ_NO.getFieldName()))); + assertThat(metadata.get(IF_PRIMARY_TERM.getFieldName()), equalTo(expectedDocument.get(IF_PRIMARY_TERM.getFieldName()))); assertThat(ingestDocument.getSource(), equalTo(expectedDocument.get(Fields.SOURCE))); } @@ -349,12 +348,12 @@ public void testIngestPipelineWithDocumentsWithType() throws Exception { Iterator> expectedDocsIterator = expectedDocs.iterator(); for (IngestDocument ingestDocument : actualRequest.documents()) { Map expectedDocument = expectedDocsIterator.next(); - Map metadataMap = ingestDocument.getMetadataMap(); - assertThat(metadataMap.get(INDEX.getFieldName()), equalTo(expectedDocument.get(INDEX.getFieldName()))); - assertThat(metadataMap.get(ID.getFieldName()), equalTo(expectedDocument.get(ID.getFieldName()))); - assertThat(metadataMap.get(ROUTING.getFieldName()), equalTo(expectedDocument.get(ROUTING.getFieldName()))); - assertThat(metadataMap.get(VERSION.getFieldName()), equalTo(expectedDocument.get(VERSION.getFieldName()))); - assertThat(metadataMap.get(VERSION_TYPE.getFieldName()), equalTo(expectedDocument.get(VERSION_TYPE.getFieldName()))); + org.elasticsearch.script.Metadata metadata = ingestDocument.getMetadata(); + assertThat(metadata.get(INDEX.getFieldName()), equalTo(expectedDocument.get(INDEX.getFieldName()))); + assertThat(metadata.get(ID.getFieldName()), equalTo(expectedDocument.get(ID.getFieldName()))); + assertThat(metadata.get(ROUTING.getFieldName()), equalTo(expectedDocument.get(ROUTING.getFieldName()))); + assertThat(metadata.get(VERSION.getFieldName()), equalTo(expectedDocument.get(VERSION.getFieldName()))); + assertThat(metadata.get(VERSION_TYPE.getFieldName()), equalTo(expectedDocument.get(VERSION_TYPE.getFieldName()))); assertThat(ingestDocument.getSource(), equalTo(expectedDocument.get(Fields.SOURCE))); } assertThat(actualRequest.pipeline().getId(), equalTo(SIMULATED_PIPELINE_ID)); diff --git a/server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java b/server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java index 03007d1e84712..92d4abdec5207 100644 --- a/server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java +++ b/server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java @@ -148,19 +148,18 @@ public void testToXContent() throws IOException { Map toXContentSource = (Map) toXContentDoc.get("_source"); Map toXContentIngestMetadata = (Map) toXContentDoc.get("_ingest"); - Map metadataMap = ingestDocument.getMetadataMap(); - for (Map.Entry metadata : metadataMap.entrySet()) { - String fieldName = metadata.getKey(); - if (metadata.getValue() == null) { + for (String fieldName : ingestDocument.getMetadata().keys()) { + Object value = ingestDocument.getMetadata().get(fieldName); + if (value == null) { assertThat(toXContentDoc.containsKey(fieldName), is(false)); } else { - assertThat(toXContentDoc.get(fieldName), equalTo(metadata.getValue().toString())); + assertThat(toXContentDoc.get(fieldName), equalTo(value.toString())); } } - Map sourceAndMetadata = Maps.newMapWithExpectedSize(toXContentSource.size() + metadataMap.size()); + Map sourceAndMetadata = Maps.newMapWithExpectedSize(toXContentSource.size() + ingestDocument.getMetadata().size()); sourceAndMetadata.putAll(toXContentSource); - sourceAndMetadata.putAll(metadataMap); + ingestDocument.getMetadata().keys().forEach(k -> sourceAndMetadata.put(k, ingestDocument.getMetadata().get(k))); IngestDocument serializedIngestDocument = new IngestDocument(sourceAndMetadata, toXContentIngestMetadata); // TODO(stu): is this test correct? Comparing against ingestDocument fails due to incorrectly failed byte array comparisons assertThat(serializedIngestDocument, equalTo(serializedIngestDocument)); diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java index 7fcb2b07a1f81..85b13ba1f6d0d 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java @@ -2230,7 +2230,7 @@ private class IngestDocumentMatcher implements ArgumentMatcher { public boolean matches(IngestDocument other) { // ingest metadata and IngestSourceAndMetadata will not be the same (timestamp differs every time) return Objects.equals(ingestDocument.getSource(), other.getSource()) - && Objects.equals(ingestDocument.getMetadataMap(), other.getMetadataMap()); + && Objects.equals(ingestDocument.getMetadata(), other.getMetadata()); } } diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestSourceAndMetadataTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestSourceAndMetadataTests.java index f0085abda82ee..cb2db8d3980ae 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestSourceAndMetadataTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestSourceAndMetadataTests.java @@ -9,6 +9,8 @@ package org.elasticsearch.ingest; import org.elasticsearch.index.VersionType; +import org.elasticsearch.script.Metadata; +import org.elasticsearch.script.TestMetadata; import org.elasticsearch.test.ESTestCase; import java.util.HashMap; @@ -17,15 +19,13 @@ import java.util.Map; import java.util.Set; -import static org.elasticsearch.ingest.TestIngestDocument.replaceValidator; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.hasEntry; -import static org.hamcrest.Matchers.notNullValue; public class IngestSourceAndMetadataTests extends ESTestCase { IngestSourceAndMetadata map; + Metadata md; public void testSettersAndGetters() { Map metadata = new HashMap<>(); @@ -37,31 +37,32 @@ public void testSettersAndGetters() { metadata.put("_if_primary_term", 10000); metadata.put("_version_type", "internal"); metadata.put("_dynamic_templates", Map.of("foo", "bar")); - map = new IngestSourceAndMetadata(new HashMap<>(), metadata, null, null); - assertEquals("myIndex", map.getIndex()); - map.setIndex("myIndex2"); - assertEquals("myIndex2", map.getIndex()); + map = new IngestSourceAndMetadata(new HashMap<>(), new Metadata(metadata, null)); + md = map.getMetadata(); + assertEquals("myIndex", md.getIndex()); + md.setIndex("myIndex2"); + assertEquals("myIndex2", md.getIndex()); - assertEquals("myId", map.getId()); - map.setId("myId2"); - assertEquals("myId2", map.getId()); + assertEquals("myId", md.getId()); + md.setId("myId2"); + assertEquals("myId2", md.getId()); - assertEquals("myRouting", map.getRouting()); - map.setRouting("myRouting2"); - assertEquals("myRouting2", map.getRouting()); + assertEquals("myRouting", md.getRouting()); + md.setRouting("myRouting2"); + assertEquals("myRouting2", md.getRouting()); - assertEquals(20, map.getVersion()); - map.setVersion(10); - assertEquals(10, map.getVersion()); + assertEquals(20, md.getVersion()); + md.setVersion(10); + assertEquals(10, md.getVersion()); - assertEquals("internal", map.getVersionType()); - map.setVersionType("external_gte"); - assertEquals("external_gte", map.getVersionType()); + assertEquals("internal", md.getVersionType()); + md.setVersionType("external_gte"); + assertEquals("external_gte", md.getVersionType()); - assertEquals(Map.of("foo", "bar"), map.getDynamicTemplates()); + assertEquals(Map.of("foo", "bar"), md.getDynamicTemplates()); - assertEquals(500, map.getIfSeqNo()); - assertEquals(10000, map.getIfPrimaryTerm()); + assertEquals(500, md.getIfSeqNo()); + assertEquals(10000, md.getIfPrimaryTerm()); } public void testGetString() { @@ -76,12 +77,13 @@ public String toString() { } }); source.put("missing", null); - map = new IngestSourceAndMetadata(source, metadata, null, replaceValidator("_version", IngestSourceAndMetadata::longValidator)); - assertNull(map.getString("missing")); - assertNull(map.getString("no key")); - assertEquals("myToString()", map.getString("toStr")); - assertEquals("myStr", map.getString("str")); - assertEquals("myRouting", map.getString("_routing")); + map = new IngestSourceAndMetadata(source, TestMetadata.withNullableVersion(metadata)); + md = map.getMetadata(); + assertNull(md.getString("missing")); + assertNull(md.getString("no key")); + assertEquals("myToString()", md.getString("toStr")); + assertEquals("myStr", md.getString("str")); + assertEquals("myRouting", md.getString("_routing")); } public void testGetNumber() { @@ -90,12 +92,13 @@ public void testGetNumber() { Map source = new HashMap<>(); source.put("number", "NaN"); source.put("missing", null); - map = new IngestSourceAndMetadata(source, metadata, null, null); - assertEquals(Long.MAX_VALUE, map.getNumber("_version")); - IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> map.getNumber("number")); + map = new IngestSourceAndMetadata(source, new Metadata(metadata, null)); + md = map.getMetadata(); + assertEquals(Long.MAX_VALUE, md.getNumber("_version")); + IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> md.getNumber("number")); assertEquals("unexpected type for [number] with value [NaN], expected Number, got [java.lang.String]", err.getMessage()); - assertNull(map.getNumber("missing")); - assertNull(map.getNumber("no key")); + assertNull(md.getNumber("missing")); + assertNull(md.getNumber("no key")); } public void testInvalidMetadata() { @@ -103,7 +106,7 @@ public void testInvalidMetadata() { metadata.put("_version", Double.MAX_VALUE); IllegalArgumentException err = expectThrows( IllegalArgumentException.class, - () -> new IngestSourceAndMetadata(new HashMap<>(), metadata, null, null) + () -> new IngestSourceAndMetadata(new HashMap<>(), new Metadata(metadata, null)) ); assertThat(err.getMessage(), containsString("_version may only be set to an int or a long but was [")); assertThat(err.getMessage(), containsString("] with type [java.lang.Double]")); @@ -114,7 +117,7 @@ public void testSourceInMetadata() { source.put("_version", 25); IllegalArgumentException err = expectThrows( IllegalArgumentException.class, - () -> new IngestSourceAndMetadata(source, source, null, null) + () -> new IngestSourceAndMetadata(source, new Metadata(source, null)) ); assertEquals("Unexpected metadata key [_version] in source with value [25]", err.getMessage()); } @@ -126,7 +129,7 @@ public void testExtraMetadata() { metadata.put("routing", "myRouting"); IllegalArgumentException err = expectThrows( IllegalArgumentException.class, - () -> new IngestSourceAndMetadata(new HashMap<>(), metadata, null, null) + () -> new IngestSourceAndMetadata(new HashMap<>(), new Metadata(metadata, null)) ); assertEquals("Unexpected metadata keys [routing:myRouting, version:567]", err.getMessage()); } @@ -135,7 +138,7 @@ public void testPutSource() { Map metadata = new HashMap<>(); metadata.put("_version", 123); Map source = new HashMap<>(); - map = new IngestSourceAndMetadata(source, metadata, null, null); + map = new IngestSourceAndMetadata(source, new Metadata(metadata, null)); } public void testRemove() { @@ -143,11 +146,11 @@ public void testRemove() { String canRemove = "canRemove"; Map metadata = new HashMap<>(); metadata.put(cannotRemove, "value"); - map = new IngestSourceAndMetadata(new HashMap<>(), metadata, null, Map.of(cannotRemove, (k, v) -> { + map = new IngestSourceAndMetadata(new HashMap<>(), new TestMetadata(metadata, Map.of(cannotRemove, (o, k, v) -> { if (v == null) { throw new IllegalArgumentException(k + " cannot be null or removed"); } - }, canRemove, (k, v) -> {})); + }, canRemove, (o, k, v) -> {}))); String msg = "cannotRemove cannot be null or removed"; IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> map.remove(cannotRemove)); assertEquals(msg, err.getMessage()); @@ -208,7 +211,8 @@ public void testEntryAndIterator() { source.put("foo", "bar"); source.put("baz", "qux"); source.put("noz", "zon"); - map = new IngestSourceAndMetadata(source, metadata, null, replaceValidator("_version", IngestSourceAndMetadata::longValidator)); + map = new IngestSourceAndMetadata(source, TestMetadata.withNullableVersion(metadata)); + md = map.getMetadata(); for (Map.Entry entry : map.entrySet()) { if ("foo".equals(entry.getKey())) { @@ -218,7 +222,7 @@ public void testEntryAndIterator() { } } assertEquals("changed", map.get("foo")); - assertEquals("external_gte", map.getVersionType()); + assertEquals("external_gte", md.getVersionType()); assertEquals(5, map.entrySet().size()); assertEquals(5, map.size()); @@ -235,7 +239,7 @@ public void testEntryAndIterator() { } } - assertNull(map.getVersionType()); + assertNull(md.getVersionType()); assertFalse(map.containsKey("baz")); assertTrue(map.containsKey("_version")); assertTrue(map.containsKey("foo")); @@ -247,7 +251,7 @@ public void testEntryAndIterator() { } public void testContainsValue() { - map = new IngestSourceAndMetadata(Map.of("myField", "fieldValue"), Map.of("_version", 5678), null, null); + map = new IngestSourceAndMetadata(Map.of("myField", "fieldValue"), new Metadata(Map.of("_version", 5678), null)); assertTrue(map.containsValue(5678)); assertFalse(map.containsValue(5679)); assertTrue(map.containsValue("fieldValue")); @@ -256,39 +260,40 @@ public void testContainsValue() { public void testValidators() { map = new IngestSourceAndMetadata("myIndex", "myId", 1234, "myRouting", VersionType.EXTERNAL, null, new HashMap<>()); + md = map.getMetadata(); IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> map.put("_index", 555)); assertEquals("_index must be null or a String but was [555] with type [java.lang.Integer]", err.getMessage()); - assertEquals("myIndex", map.getIndex()); + assertEquals("myIndex", md.getIndex()); err = expectThrows(IllegalArgumentException.class, () -> map.put("_id", 555)); assertEquals("_id must be null or a String but was [555] with type [java.lang.Integer]", err.getMessage()); - assertEquals("myId", map.getId()); + assertEquals("myId", md.getId()); map.put("_id", "myId2"); - assertEquals("myId2", map.getId()); + assertEquals("myId2", md.getId()); err = expectThrows(IllegalArgumentException.class, () -> map.put("_routing", 555)); assertEquals("_routing must be null or a String but was [555] with type [java.lang.Integer]", err.getMessage()); - assertEquals("myRouting", map.getRouting()); + assertEquals("myRouting", md.getRouting()); map.put("_routing", "myRouting2"); - assertEquals("myRouting2", map.getRouting()); + assertEquals("myRouting2", md.getRouting()); err = expectThrows(IllegalArgumentException.class, () -> map.put("_version", "five-five-five")); assertEquals( "_version may only be set to an int or a long but was [five-five-five] with type [java.lang.String]", err.getMessage() ); - assertEquals(1234, map.getVersion()); + assertEquals(1234, md.getVersion()); map.put("_version", 555); - assertEquals(555, map.getVersion()); + assertEquals(555, md.getVersion()); err = expectThrows(IllegalArgumentException.class, () -> map.put("_version_type", "vt")); assertEquals( "_version_type must be a null or one of [internal, external, external_gte] but was [vt] with type [java.lang.String]", err.getMessage() ); - assertEquals("external", map.getVersionType()); + assertEquals("external", md.getVersionType()); map.put("_version_type", "internal"); - assertEquals("internal", map.getVersionType()); + assertEquals("internal", md.getVersionType()); err = expectThrows(IllegalArgumentException.class, () -> map.put("_version_type", VersionType.EXTERNAL.toString())); assertEquals( "_version_type must be a null or one of [internal, external, external_gte] but was [EXTERNAL] with type [java.lang.String]", @@ -300,8 +305,8 @@ public void testValidators() { + " [org.elasticsearch.index.VersionType$2]", err.getMessage() ); - assertEquals("internal", map.getVersionType()); - err = expectThrows(IllegalArgumentException.class, () -> map.setVersionType(VersionType.EXTERNAL.toString())); + assertEquals("internal", md.getVersionType()); + err = expectThrows(IllegalArgumentException.class, () -> md.setVersionType(VersionType.EXTERNAL.toString())); assertEquals( "_version_type must be a null or one of [internal, external, external_gte] but was [EXTERNAL] with type [java.lang.String]", err.getMessage() @@ -311,33 +316,27 @@ public void testValidators() { assertEquals("_dynamic_templates must be a null or a Map but was [5] with type [java.lang.String]", err.getMessage()); Map dt = Map.of("a", "b"); map.put("_dynamic_templates", dt); - assertThat(dt, equalTo(map.getDynamicTemplates())); - } - - public void testDefaultValidatorForAllMetadata() { - for (IngestDocument.Metadata m : IngestDocument.Metadata.values()) { - assertThat(IngestSourceAndMetadata.VALIDATORS, hasEntry(equalTo(m.getFieldName()), notNullValue())); - } - assertEquals(IngestDocument.Metadata.values().length, IngestSourceAndMetadata.VALIDATORS.size()); + assertThat(dt, equalTo(md.getDynamicTemplates())); } public void testHandlesAllVersionTypes() { - Map md = new HashMap<>(); - md.put("_version", 1234); - map = new IngestSourceAndMetadata(new HashMap<>(), md, null, null); - assertNull(map.getVersionType()); + Map mdRawMap = new HashMap<>(); + mdRawMap.put("_version", 1234); + map = new IngestSourceAndMetadata(new HashMap<>(), new Metadata(mdRawMap, null)); + md = map.getMetadata(); + assertNull(md.getVersionType()); for (VersionType vt : VersionType.values()) { - map.setVersionType(VersionType.toString(vt)); + md.setVersionType(VersionType.toString(vt)); assertEquals(VersionType.toString(vt), map.get("_version_type")); } for (VersionType vt : VersionType.values()) { map.put("_version_type", VersionType.toString(vt)); - assertEquals(vt.toString().toLowerCase(Locale.ROOT), map.getVersionType()); + assertEquals(vt.toString().toLowerCase(Locale.ROOT), md.getVersionType()); } - map.setVersionType(null); - assertNull(map.getVersionType()); + md.setVersionType(null); + assertNull(md.getVersionType()); } private static class TestEntry implements Map.Entry { diff --git a/server/src/test/java/org/elasticsearch/script/MetadataTests.java b/server/src/test/java/org/elasticsearch/script/MetadataTests.java new file mode 100644 index 0000000000000..c241d14d93b1b --- /dev/null +++ b/server/src/test/java/org/elasticsearch/script/MetadataTests.java @@ -0,0 +1,25 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.script; + +import org.elasticsearch.ingest.IngestDocument; +import org.elasticsearch.test.ESTestCase; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasEntry; +import static org.hamcrest.Matchers.notNullValue; + +public class MetadataTests extends ESTestCase { + public void testDefaultValidatorForAllMetadata() { + for (IngestDocument.Metadata m : IngestDocument.Metadata.values()) { + assertThat(Metadata.VALIDATORS, hasEntry(equalTo(m.getFieldName()), notNullValue())); + } + assertEquals(IngestDocument.Metadata.values().length, Metadata.VALIDATORS.size()); + } +} diff --git a/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java b/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java index b6b6949a07290..ffd9ca324f63b 100644 --- a/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java +++ b/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java @@ -11,11 +11,12 @@ import org.elasticsearch.common.lucene.uid.Versions; import org.elasticsearch.core.Tuple; import org.elasticsearch.index.VersionType; +import org.elasticsearch.script.Metadata; +import org.elasticsearch.script.TestMetadata; import org.elasticsearch.test.ESTestCase; import java.util.HashMap; import java.util.Map; -import java.util.function.BiConsumer; /** * Construct ingest documents for testing purposes @@ -36,10 +37,8 @@ public static IngestDocument withNullableVersion(Map sourceAndMe * _versions. Normally null _version is not allowed, but many tests don't care about that invariant. */ public static IngestDocument ofIngestWithNullableVersion(Map sourceAndMetadata, Map ingestMetadata) { - Map> validators = replaceValidator(VERSION, IngestSourceAndMetadata::longValidator); Tuple, Map> sm = IngestSourceAndMetadata.splitSourceAndMetadata(sourceAndMetadata); - IngestSourceAndMetadata withNullableVersion = new IngestSourceAndMetadata(sm.v1(), sm.v2(), null, validators); - return new IngestDocument(withNullableVersion, ingestMetadata); + return new IngestDocument(new IngestSourceAndMetadata(sm.v1(), TestMetadata.withNullableVersion(sm.v2())), ingestMetadata); } /** @@ -53,22 +52,12 @@ public static IngestDocument withDefaultVersion(Map sourceAndMet return new IngestDocument(sourceAndMetadata, new HashMap<>()); } - /** - * Return the default validator map with a single validator replaced, if that validator was already present in the default validators - * map - */ - protected static Map> replaceValidator(String key, BiConsumer validator) { - Map> validators = new HashMap<>(IngestSourceAndMetadata.VALIDATORS); - validators.computeIfPresent(key, (k, v) -> validator); - return validators; - } - /** * Create an IngestDocument with a metadata map and validators. The metadata map is passed by reference, not copied, so callers * can observe changes to the map directly. */ - public static IngestDocument ofMetadataWithValidator(Map metadata, Map> validators) { - return new IngestDocument(new IngestSourceAndMetadata(new HashMap<>(), metadata, null, validators), new HashMap<>()); + public static IngestDocument ofMetadataWithValidator(Map metadata, Map validators) { + return new IngestDocument(new IngestSourceAndMetadata(new HashMap<>(), new TestMetadata(metadata, validators)), new HashMap<>()); } /** diff --git a/test/framework/src/main/java/org/elasticsearch/script/TestMetadata.java b/test/framework/src/main/java/org/elasticsearch/script/TestMetadata.java new file mode 100644 index 0000000000000..1ce7dcf26d249 --- /dev/null +++ b/test/framework/src/main/java/org/elasticsearch/script/TestMetadata.java @@ -0,0 +1,24 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.script; + +import java.util.HashMap; +import java.util.Map; + +public class TestMetadata extends Metadata { + public TestMetadata(Map map, Map validators) { + super(map, null, validators); + } + + public static TestMetadata withNullableVersion(Map map) { + Map updatedValidators = new HashMap<>(VALIDATORS); + updatedValidators.replace(VERSION, Metadata::longValidator); + return new TestMetadata(map, updatedValidators); + } +} diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/results/InferenceResultsTestCase.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/results/InferenceResultsTestCase.java index 1663c63eeeae3..701e348c48cdf 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/results/InferenceResultsTestCase.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/results/InferenceResultsTestCase.java @@ -55,14 +55,15 @@ public void testWriteToDocAndSerialize() throws IOException { InferenceResults.writeResult(inferenceResult, document, parentField, modelId); try (XContentBuilder builder = XContentFactory.jsonBuilder()) { builder.startObject(); - Map metadataMap = document.getMetadataMap(); - for (Map.Entry metadata : metadataMap.entrySet()) { - if (metadata.getValue() != null) { - builder.field(metadata.getKey(), metadata.getValue().toString()); + org.elasticsearch.script.Metadata metadata = document.getMetadata(); + for (String key : metadata.keys()) { + Object value = metadata.get(key); + if (value != null) { + builder.field(key, value.toString()); } } Map source = IngestDocument.deepCopyMap(document.getSourceAndMetadata()); - metadataMap.keySet().forEach(mD -> source.remove(mD)); + metadata.keys().forEach(source::remove); builder.field("_source", source); builder.field("_ingest", document.getIngestMetadata()); builder.endObject(); From 4ea1791064ada4adf7bb48ee71bfa1572bc24760 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Fri, 8 Jul 2022 18:16:40 -0500 Subject: [PATCH 03/24] Update metadata javadoc --- .../ingest/IngestSourceAndMetadata.java | 6 +-- .../org/elasticsearch/script/Metadata.java | 52 ++++++++++++++++--- .../elasticsearch/script/TestMetadata.java | 3 ++ 3 files changed, 52 insertions(+), 9 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java b/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java index 7978c8d4e08ed..9a95401d04a34 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java @@ -136,7 +136,7 @@ public Set> entrySet() { */ @Override public Object put(String key, Object value) { - if (metadata.ownsKey(key)) { + if (metadata.isMetadata(key)) { return metadata.put(key, value); } return source.put(key, value); @@ -150,7 +150,7 @@ public Object put(String key, Object value) { public Object remove(Object key) { // uses map directly to avoid AbstractMaps linear time implementation using entrySet() if (key instanceof String str) { - if (metadata.ownsKey(str)) { + if (metadata.isMetadata(str)) { return metadata.remove(str); } } @@ -195,7 +195,7 @@ public boolean containsKey(Object key) { public Object get(Object key) { // uses map directly to avoid AbstractMaps linear time implementation using entrySet() if (key instanceof String str) { - if (metadata.ownsKey(str)) { + if (metadata.isMetadata(str)) { return metadata.get(str); } } diff --git a/server/src/main/java/org/elasticsearch/script/Metadata.java b/server/src/main/java/org/elasticsearch/script/Metadata.java index 10a93ac373d1e..d63a0dc0b2081 100644 --- a/server/src/main/java/org/elasticsearch/script/Metadata.java +++ b/server/src/main/java/org/elasticsearch/script/Metadata.java @@ -23,7 +23,13 @@ import java.util.stream.Collectors; /** - * Ingest and update metadata available to write scripts + * Ingest and update metadata available to write scripts. + * + * Provides a map-like interface for backwards compatibility with the ctx map. + * + * Validates all updates to performed via the map-like interface. + * + * Provides getters and setters for script usage. */ public class Metadata { protected static final String INDEX = "_index"; @@ -194,38 +200,65 @@ public Number getNumber(String key) { ); } - public boolean ownsKey(String key) { + /** + * Is this key a Metadata key? A {@link #remove}d key would return false for {@link #containsKey(String)} but true for + * this call. + */ + public boolean isMetadata(String key) { return validators.containsKey(key); } + /** + * Create the mapping from key to value. + * @throws IllegalArgumentException if {@link #isMetadata(String)} is false or the key cannot be updated to the value. + */ public Object put(String key, Object value) { Validator v = validators.getOrDefault(key, this::badKey); v.accept(MapOperation.UPDATE, key, value); return map.put(key, value); } + /** + * Does the metadata contain the key? + */ public boolean containsKey(String key) { return map.containsKey(key); } + /** + * Does the metadata contain the value. + */ public boolean containsValue(Object value) { return map.containsValue(value); } + /** + * Get the value associated with {@param key} + */ public Object get(String key) { return map.get(key); } + /** + * Remove the mapping associated with {@param key} + * @throws IllegalArgumentException if {@link #isMetadata(String)} is false or the key cannot be removed. + */ public Object remove(String key) { Validator v = validators.getOrDefault(key, this::badKey); v.accept(MapOperation.REMOVE, key, null); return map.remove(key); } + /** + * Return the list of keys with mappings + */ public List keys() { return new ArrayList<>(map.keySet()); } + /** + * The number of metadata keys currently mapped. + */ public int size() { return map.size(); } @@ -235,13 +268,14 @@ public Metadata clone() { return new Metadata(new HashMap<>(map), timestamp, new HashMap<>(validators)); } + // Return a copy of the backing map public Map mapCopy() { - // This is used for tests, can be removed when Metadata implements HashMap return new HashMap<>(map); } /** - * Allow a String or null + * Allow a String or null. + * @throws IllegalArgumentException if {@param value} is neither a {@link String} nor null */ protected static void stringValidator(MapOperation op, String key, Object value) { if (op == MapOperation.REMOVE || value == null || value instanceof String) { @@ -253,7 +287,8 @@ protected static void stringValidator(MapOperation op, String key, Object value) } /** - * Allow Numbers that can be represented as longs without loss of precision + * Allow Numbers that can be represented as longs without loss of precision or null + * @throws IllegalArgumentException if the value cannot be represented as a long */ public static void longValidator(MapOperation op, String key, Object value) { if (op == MapOperation.REMOVE || value == null) { @@ -271,6 +306,10 @@ public static void longValidator(MapOperation op, String key, Object value) { ); } + /** + * Same as {@link #longValidator(MapOperation, String, Object)} but {@param value} cannot be null. + * @throws IllegalArgumentException if value is null or cannot be represented as a long. + */ protected static void notNullLongValidator(MapOperation op, String key, Object value) { if (op == MapOperation.REMOVE || value == null) { throw new IllegalArgumentException(key + " cannot be removed or set to null"); @@ -279,7 +318,8 @@ protected static void notNullLongValidator(MapOperation op, String key, Object v } /** - * Allow maps + * Allow maps. + * @throws IllegalArgumentException if {@param value} is not a {@link Map} */ public static void mapValidator(MapOperation op, String key, Object value) { if (op == MapOperation.REMOVE || value == null || value instanceof Map) { diff --git a/test/framework/src/main/java/org/elasticsearch/script/TestMetadata.java b/test/framework/src/main/java/org/elasticsearch/script/TestMetadata.java index 1ce7dcf26d249..87a45ea857549 100644 --- a/test/framework/src/main/java/org/elasticsearch/script/TestMetadata.java +++ b/test/framework/src/main/java/org/elasticsearch/script/TestMetadata.java @@ -11,6 +11,9 @@ import java.util.HashMap; import java.util.Map; +/** + * An implementation of {@link Metadata} with customizable {@link org.elasticsearch.script.Metadata.Validator}s for use in testing. + */ public class TestMetadata extends Metadata { public TestMetadata(Map map, Map validators) { super(map, null, validators); From 72440108d81837bf2b44bf1b7997ba54d4e892c1 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Fri, 8 Jul 2022 18:41:11 -0500 Subject: [PATCH 04/24] Add hashcode and equals to make the WritableIngestDocumentTests pass --- .../ingest/IngestSourceAndMetadata.java | 14 ++++++++++++++ .../java/org/elasticsearch/script/Metadata.java | 16 ++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java b/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java index 9a95401d04a34..a4e1181f6b5dc 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java @@ -317,4 +317,18 @@ public Object setValue(Object value) { return metadata.put(key, value); } } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + if (super.equals(o) == false) return false; + IngestSourceAndMetadata that = (IngestSourceAndMetadata) o; + return Objects.equals(source, that.source) && Objects.equals(metadata, that.metadata); + } + + @Override + public int hashCode() { + return Objects.hash(source, metadata); + } } diff --git a/server/src/main/java/org/elasticsearch/script/Metadata.java b/server/src/main/java/org/elasticsearch/script/Metadata.java index d63a0dc0b2081..a967e55268f66 100644 --- a/server/src/main/java/org/elasticsearch/script/Metadata.java +++ b/server/src/main/java/org/elasticsearch/script/Metadata.java @@ -37,6 +37,7 @@ public class Metadata { protected static final String ROUTING = "_routing"; protected static final String VERSION_TYPE = "_version_type"; protected static final String VERSION = "_version"; + protected static final String TYPE = "_type"; // type is deprecated so it's supported in the map but not available as a getter protected static final String IF_SEQ_NO = "_if_seq_no"; protected static final String IF_PRIMARY_TERM = "_if_primary_term"; protected static final String DYNAMIC_TEMPLATES = "_dynamic_templates"; @@ -52,6 +53,8 @@ public class Metadata { Metadata::stringValidator, VERSION, Metadata::notNullLongValidator, + TYPE, + Metadata::stringValidator, IF_SEQ_NO, Metadata::longValidator, IF_PRIMARY_TERM, @@ -361,4 +364,17 @@ public enum MapOperation { public interface Validator { void accept(MapOperation op, String key, Object value); } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + Metadata metadata = (Metadata) o; + return Objects.equals(map, metadata.map) && Objects.equals(timestamp, metadata.timestamp); + } + + @Override + public int hashCode() { + return Objects.hash(map, timestamp); + } } From 75f85bd3beac3a3ac612383de36b1fad6fe8a2f2 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 11 Jul 2022 10:47:35 -0500 Subject: [PATCH 05/24] keys() -> getKeys(), don't cache entrySet, check metadata in source, return vt validator, doc matcher only checks metadata map --- .../ingest/WriteableIngestDocument.java | 2 +- .../ingest/IngestSourceAndMetadata.java | 19 +++++--- .../org/elasticsearch/script/Metadata.java | 48 ++++++++++++++++++- .../ingest/WriteableIngestDocumentTests.java | 4 +- .../ingest/IngestServiceTests.java | 2 +- .../ingest/IngestSourceAndMetadataTests.java | 48 +++++++++++-------- .../results/InferenceResultsTestCase.java | 4 +- 7 files changed, 92 insertions(+), 35 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/ingest/WriteableIngestDocument.java b/server/src/main/java/org/elasticsearch/action/ingest/WriteableIngestDocument.java index ade4a99133712..dab2d5526932c 100644 --- a/server/src/main/java/org/elasticsearch/action/ingest/WriteableIngestDocument.java +++ b/server/src/main/java/org/elasticsearch/action/ingest/WriteableIngestDocument.java @@ -108,7 +108,7 @@ IngestDocument getIngestDocument() { public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(DOC_FIELD); org.elasticsearch.script.Metadata metadata = ingestDocument.getMetadata(); - for (String key : metadata.keys()) { + for (String key : metadata.getKeys()) { Object value = metadata.get(key); if (value != null) { builder.field(key, value.toString()); diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java b/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java index a4e1181f6b5dc..7682b8714634e 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java @@ -24,6 +24,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.stream.Collectors; /** * Map containing ingest source and metadata. @@ -42,7 +43,6 @@ class IngestSourceAndMetadata extends AbstractMap { protected final Map source; protected final Metadata metadata; - private EntrySet entrySet; // cache to avoid recreation /** * Create an IngestSourceAndMetadata with the given metadata, source and default validators @@ -60,7 +60,7 @@ class IngestSourceAndMetadata extends AbstractMap { } /** - * Create IngestSourceAndMetadata with custom validators. + * Create IngestSourceAndMetadata from a source and metadata * * @param source the source document map * @param metadata the metadata map @@ -68,6 +68,14 @@ class IngestSourceAndMetadata extends AbstractMap { IngestSourceAndMetadata(Map source, Metadata metadata) { this.source = source != null ? source : new HashMap<>(); this.metadata = metadata; + Set badKeys = metadata.metadataKeys(this.source.keySet()); + if (badKeys.size() > 0) { + throw new IllegalArgumentException( + "unexpected metadata [" + + badKeys.stream().sorted().map(k -> k + ":" + this.source.get(k)).collect(Collectors.joining(", ")) + + "] in source" + ); + } } /** @@ -124,10 +132,7 @@ public Metadata getMetadata() { */ @Override public Set> entrySet() { - if (entrySet == null) { - entrySet = new EntrySet(source.entrySet(), metadata.keys()); - } - return entrySet; + return new EntrySet(source.entrySet(), metadata.getKeys()); } /** @@ -164,7 +169,7 @@ public Object remove(Object key) { @Override public void clear() { // AbstractMap uses entrySet().clear(), it should be quicker to run through the validators, then call the wrapped maps clear - for (String key : metadata.keys()) { + for (String key : metadata.getKeys()) { metadata.remove(key); } source.clear(); diff --git a/server/src/main/java/org/elasticsearch/script/Metadata.java b/server/src/main/java/org/elasticsearch/script/Metadata.java index a967e55268f66..d0fd6d75a71af 100644 --- a/server/src/main/java/org/elasticsearch/script/Metadata.java +++ b/server/src/main/java/org/elasticsearch/script/Metadata.java @@ -14,6 +14,8 @@ import java.time.ZonedDateTime; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -50,7 +52,7 @@ public class Metadata { ROUTING, Metadata::stringValidator, VERSION_TYPE, - Metadata::stringValidator, + Metadata::versionTypeValidator, VERSION, Metadata::notNullLongValidator, TYPE, @@ -255,7 +257,7 @@ public Object remove(String key) { /** * Return the list of keys with mappings */ - public List keys() { + public List getKeys() { return new ArrayList<>(map.keySet()); } @@ -276,6 +278,22 @@ public Map mapCopy() { return new HashMap<>(map); } + /** + * Get the metadata keys inside the {@param other} set + */ + public Set metadataKeys(Set other) { + Set keys = null; + for (String key : validators.keySet()) { + if (other.contains(key)) { + if (keys == null) { + keys = new HashSet<>(); + } + keys.add(key); + } + } + return keys != null ? keys : Collections.emptySet(); + } + /** * Allow a String or null. * @throws IllegalArgumentException if {@param value} is neither a {@link String} nor null @@ -333,6 +351,32 @@ public static void mapValidator(MapOperation op, String key, Object value) { ); } + /** + * Allow lower case Strings that map to VersionType values, or null. + * @throws IllegalArgumentException if {@param value} cannot be converted via {@link VersionType#fromString(String)} + */ + protected static void versionTypeValidator(MapOperation op, String key, Object value) { + if (op == MapOperation.REMOVE || value == null) { + return; + } + if (value instanceof String versionType) { + try { + VersionType.fromString(versionType); + return; + } catch (IllegalArgumentException ignored) {} + } + throw new IllegalArgumentException( + key + + " must be a null or one of [" + + Arrays.stream(VersionType.values()).map(vt -> VersionType.toString(vt)).collect(Collectors.joining(", ")) + + "] but was [" + + value + + "] with type [" + + value.getClass().getName() + + "]" + ); + } + private void badKey(MapOperation op, String key, Object value) { throw new IllegalArgumentException( "unexpected metadata key [" diff --git a/server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java b/server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java index 92d4abdec5207..4929a56296bea 100644 --- a/server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java +++ b/server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java @@ -148,7 +148,7 @@ public void testToXContent() throws IOException { Map toXContentSource = (Map) toXContentDoc.get("_source"); Map toXContentIngestMetadata = (Map) toXContentDoc.get("_ingest"); - for (String fieldName : ingestDocument.getMetadata().keys()) { + for (String fieldName : ingestDocument.getMetadata().getKeys()) { Object value = ingestDocument.getMetadata().get(fieldName); if (value == null) { assertThat(toXContentDoc.containsKey(fieldName), is(false)); @@ -159,7 +159,7 @@ public void testToXContent() throws IOException { Map sourceAndMetadata = Maps.newMapWithExpectedSize(toXContentSource.size() + ingestDocument.getMetadata().size()); sourceAndMetadata.putAll(toXContentSource); - ingestDocument.getMetadata().keys().forEach(k -> sourceAndMetadata.put(k, ingestDocument.getMetadata().get(k))); + ingestDocument.getMetadata().getKeys().forEach(k -> sourceAndMetadata.put(k, ingestDocument.getMetadata().get(k))); IngestDocument serializedIngestDocument = new IngestDocument(sourceAndMetadata, toXContentIngestMetadata); // TODO(stu): is this test correct? Comparing against ingestDocument fails due to incorrectly failed byte array comparisons assertThat(serializedIngestDocument, equalTo(serializedIngestDocument)); diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java index 85b13ba1f6d0d..2c102de79019f 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java @@ -2230,7 +2230,7 @@ private class IngestDocumentMatcher implements ArgumentMatcher { public boolean matches(IngestDocument other) { // ingest metadata and IngestSourceAndMetadata will not be the same (timestamp differs every time) return Objects.equals(ingestDocument.getSource(), other.getSource()) - && Objects.equals(ingestDocument.getMetadata(), other.getMetadata()); + && Objects.equals(ingestDocument.getMetadata().mapCopy(), other.getMetadata().mapCopy()); } } diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestSourceAndMetadataTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestSourceAndMetadataTests.java index cb2db8d3980ae..fac543fa05864 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestSourceAndMetadataTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestSourceAndMetadataTests.java @@ -67,37 +67,37 @@ public void testSettersAndGetters() { public void testGetString() { Map metadata = new HashMap<>(); - metadata.put("_routing", "myRouting"); - Map source = new HashMap<>(); - source.put("str", "myStr"); - source.put("toStr", new Object() { + metadata.put("a", "A"); + metadata.put("b", new Object() { @Override public String toString() { return "myToString()"; } }); - source.put("missing", null); - map = new IngestSourceAndMetadata(source, TestMetadata.withNullableVersion(metadata)); + metadata.put("c", null); + metadata.put("d", 1234); + map = new IngestSourceAndMetadata(new HashMap<>(), new TestMetadata(metadata, allowAllValidators("a", "b", "c", "d"))); md = map.getMetadata(); - assertNull(md.getString("missing")); + assertNull(md.getString("c")); assertNull(md.getString("no key")); - assertEquals("myToString()", md.getString("toStr")); - assertEquals("myStr", md.getString("str")); - assertEquals("myRouting", md.getString("_routing")); + assertEquals("myToString()", md.getString("b")); + assertEquals("A", md.getString("a")); + assertEquals("1234", md.getString("d")); } public void testGetNumber() { Map metadata = new HashMap<>(); - metadata.put("_version", Long.MAX_VALUE); - Map source = new HashMap<>(); - source.put("number", "NaN"); - source.put("missing", null); - map = new IngestSourceAndMetadata(source, new Metadata(metadata, null)); + metadata.put("a", Long.MAX_VALUE); + metadata.put("b", Double.MAX_VALUE); + metadata.put("c", "NaN"); + metadata.put("d", null); + map = new IngestSourceAndMetadata(new HashMap<>(), new TestMetadata(metadata, allowAllValidators("a", "b", "c", "d"))); md = map.getMetadata(); - assertEquals(Long.MAX_VALUE, md.getNumber("_version")); - IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> md.getNumber("number")); - assertEquals("unexpected type for [number] with value [NaN], expected Number, got [java.lang.String]", err.getMessage()); - assertNull(md.getNumber("missing")); + assertEquals(Long.MAX_VALUE, md.getNumber("a")); + assertEquals(Double.MAX_VALUE, md.getNumber("b")); + IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> md.getNumber("c")); + assertEquals("unexpected type for [c] with value [NaN], expected Number, got [java.lang.String]", err.getMessage()); + assertNull(md.getNumber("d")); assertNull(md.getNumber("no key")); } @@ -119,7 +119,7 @@ public void testSourceInMetadata() { IllegalArgumentException.class, () -> new IngestSourceAndMetadata(source, new Metadata(source, null)) ); - assertEquals("Unexpected metadata key [_version] in source with value [25]", err.getMessage()); + assertEquals("unexpected metadata [_version:25] in source", err.getMessage()); } public void testExtraMetadata() { @@ -363,4 +363,12 @@ public Object setValue(Object value) { throw new UnsupportedOperationException(); } } + + private static Map allowAllValidators(String... keys) { + Map validators = new HashMap<>(); + for (String key : keys) { + validators.put(key, (o, k, v) -> {}); + } + return validators; + } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/results/InferenceResultsTestCase.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/results/InferenceResultsTestCase.java index 701e348c48cdf..74916aee9296d 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/results/InferenceResultsTestCase.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/results/InferenceResultsTestCase.java @@ -56,14 +56,14 @@ public void testWriteToDocAndSerialize() throws IOException { try (XContentBuilder builder = XContentFactory.jsonBuilder()) { builder.startObject(); org.elasticsearch.script.Metadata metadata = document.getMetadata(); - for (String key : metadata.keys()) { + for (String key : metadata.getKeys()) { Object value = metadata.get(key); if (value != null) { builder.field(key, value.toString()); } } Map source = IngestDocument.deepCopyMap(document.getSourceAndMetadata()); - metadata.keys().forEach(source::remove); + metadata.getKeys().forEach(source::remove); builder.field("_source", source); builder.field("_ingest", document.getIngestMetadata()); builder.endObject(); From aebad9a0faff12419ccfdea51f63776b350d8e0d Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 11 Jul 2022 10:51:57 -0500 Subject: [PATCH 06/24] Return raw map --- .../java/org/elasticsearch/ingest/IngestDocument.java | 1 - .../org/elasticsearch/ingest/IngestSourceAndMetadata.java | 2 +- .../src/main/java/org/elasticsearch/script/Metadata.java | 8 +++++--- .../java/org/elasticsearch/ingest/IngestServiceTests.java | 2 +- 4 files changed, 7 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java index c70c1a5ca36fe..a77d5d57b3170 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java @@ -85,7 +85,6 @@ public IngestDocument(IngestDocument other) { * Constructor to create an IngestDocument from its constituent maps. The maps are shallow copied. */ public IngestDocument(Map sourceAndMetadata, Map ingestMetadata) { - // TODO(stu): fix Tuple, Map> sm = IngestSourceAndMetadata.splitSourceAndMetadata(sourceAndMetadata); this.sourceAndMetadata = new IngestSourceAndMetadata( sm.v1(), diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java b/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java index 7682b8714634e..592c135d46501 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java @@ -83,7 +83,7 @@ class IngestSourceAndMetadata extends AbstractMap { */ public static Tuple, Map> splitSourceAndMetadata(Map sourceAndMetadata) { if (sourceAndMetadata instanceof IngestSourceAndMetadata ingestSourceAndMetadata) { - return new Tuple<>(new HashMap<>(ingestSourceAndMetadata.source), new HashMap<>(ingestSourceAndMetadata.metadata.mapCopy())); + return new Tuple<>(new HashMap<>(ingestSourceAndMetadata.source), new HashMap<>(ingestSourceAndMetadata.metadata.getMap())); } Map metadata = Maps.newHashMapWithExpectedSize(IngestDocument.Metadata.values().length); Map source = new HashMap<>(sourceAndMetadata); diff --git a/server/src/main/java/org/elasticsearch/script/Metadata.java b/server/src/main/java/org/elasticsearch/script/Metadata.java index d0fd6d75a71af..aa2b9a7d8c383 100644 --- a/server/src/main/java/org/elasticsearch/script/Metadata.java +++ b/server/src/main/java/org/elasticsearch/script/Metadata.java @@ -273,9 +273,11 @@ public Metadata clone() { return new Metadata(new HashMap<>(map), timestamp, new HashMap<>(validators)); } - // Return a copy of the backing map - public Map mapCopy() { - return new HashMap<>(map); + /** + * Get the backing map, if modified then the guarantees of this class may not hold + */ + public Map getMap() { + return map; } /** diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java index 2c102de79019f..39d93a0691856 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java @@ -2230,7 +2230,7 @@ private class IngestDocumentMatcher implements ArgumentMatcher { public boolean matches(IngestDocument other) { // ingest metadata and IngestSourceAndMetadata will not be the same (timestamp differs every time) return Objects.equals(ingestDocument.getSource(), other.getSource()) - && Objects.equals(ingestDocument.getMetadata().mapCopy(), other.getMetadata().mapCopy()); + && Objects.equals(ingestDocument.getMetadata().getMap(), other.getMetadata().getMap()); } } From 0c2e0236247be842355aa232f81ce1099b7398df Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 11 Jul 2022 11:04:28 -0500 Subject: [PATCH 07/24] Better javadoc for metadata --- .../java/org/elasticsearch/script/Metadata.java | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/script/Metadata.java b/server/src/main/java/org/elasticsearch/script/Metadata.java index aa2b9a7d8c383..1cb9ce6e9a30f 100644 --- a/server/src/main/java/org/elasticsearch/script/Metadata.java +++ b/server/src/main/java/org/elasticsearch/script/Metadata.java @@ -28,10 +28,18 @@ * Ingest and update metadata available to write scripts. * * Provides a map-like interface for backwards compatibility with the ctx map. - * - * Validates all updates to performed via the map-like interface. + * - {@link #put(String, Object)} + * - {@link #get(String)} + * - {@link #remove(String)} + * - {@link #containsKey(String)} + * - {@link #containsValue(Object)} + * - {@link #getKeys()} for iteration + * - {@link #size()} + * - {@link #isMetadata(String)} for determining if a key is a metadata key * * Provides getters and setters for script usage. + * + * Validates all updates whether originating in map-like interface or setters. */ public class Metadata { protected static final String INDEX = "_index"; @@ -66,9 +74,11 @@ public class Metadata { ); protected final Map map; - protected final ZonedDateTime timestamp; protected final Map validators; + // timestamp is new to ingest metadata, so it doesn't need to be backed by the map for back compat + protected final ZonedDateTime timestamp; + public Metadata(String index, String id, long version, String routing, VersionType versionType, ZonedDateTime timestamp) { this(metadataMap(index, id, version, routing, versionType), timestamp, VALIDATORS); } From 20d8019be69fbbe64b91d4705966ff0345479142 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 11 Jul 2022 11:47:02 -0500 Subject: [PATCH 08/24] Script: Move Map impl from IngestSourceAndMetadata to CtxMap --- .../elasticsearch/ingest/IngestCtxMap.java | 88 +++++++++++++++++++ .../elasticsearch/ingest/IngestDocument.java | 28 +++--- .../CtxMap.java} | 85 +++++------------- ...adataTests.java => IngestCtxMapTests.java} | 28 +++--- .../ingest/IngestServiceTests.java | 2 +- .../ingest/TestIngestDocument.java | 6 +- 6 files changed, 136 insertions(+), 101 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java rename server/src/main/java/org/elasticsearch/{ingest/IngestSourceAndMetadata.java => script/CtxMap.java} (76%) rename server/src/test/java/org/elasticsearch/ingest/{IngestSourceAndMetadataTests.java => IngestCtxMapTests.java} (91%) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java new file mode 100644 index 0000000000000..51a6178087fab --- /dev/null +++ b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java @@ -0,0 +1,88 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.ingest; + +import org.elasticsearch.core.Tuple; +import org.elasticsearch.index.VersionType; +import org.elasticsearch.script.CtxMap; +import org.elasticsearch.script.Metadata; + +import java.time.ZonedDateTime; +import java.util.Arrays; +import java.util.HashMap; +import java.util.Map; +import java.util.stream.Collectors; + +/** + * Map containing ingest source and metadata. + * + * The Metadata values in {@link IngestDocument.Metadata} are validated when put in the map. + * _index, _id and _routing must be a String or null + * _version_type must be a lower case VersionType or null + * _version must be representable as a long without loss of precision or null + * _dyanmic_templates must be a map + * _if_seq_no must be a long or null + * _if_primary_term must be a long or null + * + * The map is expected to be used by processors, server code should the typed getter and setters where possible. + */ +class IngestCtxMap extends CtxMap { + + /** + * Create an IngestCtxMap with the given metadata, source and default validators + */ + IngestCtxMap( + String index, + String id, + long version, + String routing, + VersionType versionType, + ZonedDateTime timestamp, + Map source + ) { + super(new HashMap<>(source), new Metadata(index, id, version, routing, versionType, timestamp)); + } + + /** + * Create IngestCtxMap from a source and metadata + * + * @param source the source document map + * @param metadata the metadata map + */ + IngestCtxMap(Map source, Metadata metadata) { + super(source, metadata); + } + + /** + * Returns a new metadata map and the existing source map with metadata removed. + */ + public static Tuple, Map> splitSourceAndMetadata(Map sourceAndMetadata) { + return CtxMap.splitSourceAndMetadata( + sourceAndMetadata, + Arrays.stream(IngestDocument.Metadata.values()).map(IngestDocument.Metadata::getFieldName).collect(Collectors.toSet()) + ); + } + + /** + * Fetch the timestamp from the ingestMetadata, if it exists + * @return the timestamp for the document or null + */ + public static ZonedDateTime getTimestamp(Map ingestMetadata) { + if (ingestMetadata == null) { + return null; + } + Object ts = ingestMetadata.get(IngestDocument.TIMESTAMP); + if (ts instanceof ZonedDateTime timestamp) { + return timestamp; + } else if (ts instanceof String str) { + return ZonedDateTime.parse(str); + } + return null; + } +} diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java index a77d5d57b3170..dcb5b4e090567 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java @@ -49,7 +49,7 @@ public final class IngestDocument { static final String TIMESTAMP = "timestamp"; - private final IngestSourceAndMetadata sourceAndMetadata; + private final IngestCtxMap sourceAndMetadata; private final Map ingestMetadata; // Contains all pipelines that have been executed for this document @@ -58,15 +58,7 @@ public final class IngestDocument { private boolean doNoSelfReferencesCheck = false; public IngestDocument(String index, String id, long version, String routing, VersionType versionType, Map source) { - this.sourceAndMetadata = new IngestSourceAndMetadata( - index, - id, - version, - routing, - versionType, - ZonedDateTime.now(ZoneOffset.UTC), - source - ); + this.sourceAndMetadata = new IngestCtxMap(index, id, version, routing, versionType, ZonedDateTime.now(ZoneOffset.UTC), source); this.ingestMetadata = new HashMap<>(); this.ingestMetadata.put(TIMESTAMP, sourceAndMetadata.getMetadata().getTimestamp()); } @@ -76,7 +68,7 @@ public IngestDocument(String index, String id, long version, String routing, Ver */ public IngestDocument(IngestDocument other) { this( - new IngestSourceAndMetadata(deepCopyMap(other.sourceAndMetadata.getSource()), other.sourceAndMetadata.getMetadata().clone()), + new IngestCtxMap(deepCopyMap(other.sourceAndMetadata.getSource()), other.sourceAndMetadata.getMetadata().clone()), deepCopyMap(other.ingestMetadata) ); } @@ -85,10 +77,10 @@ public IngestDocument(IngestDocument other) { * Constructor to create an IngestDocument from its constituent maps. The maps are shallow copied. */ public IngestDocument(Map sourceAndMetadata, Map ingestMetadata) { - Tuple, Map> sm = IngestSourceAndMetadata.splitSourceAndMetadata(sourceAndMetadata); - this.sourceAndMetadata = new IngestSourceAndMetadata( + Tuple, Map> sm = IngestCtxMap.splitSourceAndMetadata(sourceAndMetadata); + this.sourceAndMetadata = new IngestCtxMap( sm.v1(), - new org.elasticsearch.script.Metadata(sm.v2(), IngestSourceAndMetadata.getTimestamp(ingestMetadata)) + new org.elasticsearch.script.Metadata(sm.v2(), IngestCtxMap.getTimestamp(ingestMetadata)) ); this.ingestMetadata = new HashMap<>(ingestMetadata); this.ingestMetadata.computeIfPresent(TIMESTAMP, (k, v) -> { @@ -102,7 +94,7 @@ public IngestDocument(Map sourceAndMetadata, Map /** * Constructor to create an IngestDocument from its constituent maps */ - IngestDocument(IngestSourceAndMetadata sourceAndMetadata, Map ingestMetadata) { + IngestDocument(IngestCtxMap sourceAndMetadata, Map ingestMetadata) { this.sourceAndMetadata = sourceAndMetadata; this.ingestMetadata = ingestMetadata; } @@ -724,9 +716,9 @@ public Map getSourceAndMetadata() { } /** - * Get source and metadata map as {@link IngestSourceAndMetadata} + * Get source and metadata map as {@link IngestCtxMap} */ - public IngestSourceAndMetadata getIngestSourceAndMetadata() { + public IngestCtxMap getIngestSourceAndMetadata() { return sourceAndMetadata; } @@ -763,7 +755,7 @@ public static Object deepCopy(Object value) { for (Map.Entry entry : mapValue.entrySet()) { copy.put(entry.getKey(), deepCopy(entry.getValue())); } - // TODO(stu): should this check for IngestSourceAndMetadata in addition to Map? + // TODO(stu): should this check for IngestCtxMap in addition to Map? return copy; } else if (value instanceof List listValue) { List copy = new ArrayList<>(listValue.size()); diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java b/server/src/main/java/org/elasticsearch/script/CtxMap.java similarity index 76% rename from server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java rename to server/src/main/java/org/elasticsearch/script/CtxMap.java index 592c135d46501..80fa299206571 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java +++ b/server/src/main/java/org/elasticsearch/script/CtxMap.java @@ -6,14 +6,11 @@ * Side Public License, v 1. */ -package org.elasticsearch.ingest; +package org.elasticsearch.script; import org.elasticsearch.common.util.Maps; import org.elasticsearch.core.Tuple; -import org.elasticsearch.index.VersionType; -import org.elasticsearch.script.Metadata; -import java.time.ZonedDateTime; import java.util.AbstractCollection; import java.util.AbstractMap; import java.util.AbstractSet; @@ -26,46 +23,17 @@ import java.util.Set; import java.util.stream.Collectors; -/** - * Map containing ingest source and metadata. - * - * The Metadata values in {@link IngestDocument.Metadata} are validated when put in the map. - * _index, _id and _routing must be a String or null - * _version_type must be a lower case VersionType or null - * _version must be representable as a long without loss of precision or null - * _dyanmic_templates must be a map - * _if_seq_no must be a long or null - * _if_primary_term must be a long or null - * - * The map is expected to be used by processors, server code should the typed getter and setters where possible. - */ -class IngestSourceAndMetadata extends AbstractMap { - +public class CtxMap extends AbstractMap { protected final Map source; protected final Metadata metadata; /** - * Create an IngestSourceAndMetadata with the given metadata, source and default validators - */ - IngestSourceAndMetadata( - String index, - String id, - long version, - String routing, - VersionType versionType, - ZonedDateTime timestamp, - Map source - ) { - this(new HashMap<>(source), new Metadata(index, id, version, routing, versionType, timestamp)); - } - - /** - * Create IngestSourceAndMetadata from a source and metadata + * Create CtxMap from a source and metadata * * @param source the source document map * @param metadata the metadata map */ - IngestSourceAndMetadata(Map source, Metadata metadata) { + protected CtxMap(Map source, Metadata metadata) { this.source = source != null ? source : new HashMap<>(); this.metadata = metadata; Set badKeys = metadata.metadataKeys(this.source.keySet()); @@ -81,38 +49,25 @@ class IngestSourceAndMetadata extends AbstractMap { /** * Returns a new metadata map and the existing source map with metadata removed. */ - public static Tuple, Map> splitSourceAndMetadata(Map sourceAndMetadata) { - if (sourceAndMetadata instanceof IngestSourceAndMetadata ingestSourceAndMetadata) { - return new Tuple<>(new HashMap<>(ingestSourceAndMetadata.source), new HashMap<>(ingestSourceAndMetadata.metadata.getMap())); + public static Tuple, Map> splitSourceAndMetadata( + Map sourceAndMetadata, + Set metadataKeys + ) { + if (sourceAndMetadata instanceof CtxMap ctxMap) { + return new Tuple<>(new HashMap<>(ctxMap.source), new HashMap<>(ctxMap.metadata.getMap())); } - Map metadata = Maps.newHashMapWithExpectedSize(IngestDocument.Metadata.values().length); + + Map metadata = Maps.newHashMapWithExpectedSize(metadataKeys.size()); Map source = new HashMap<>(sourceAndMetadata); - for (IngestDocument.Metadata ingestDocumentMetadata : IngestDocument.Metadata.values()) { - String metadataName = ingestDocumentMetadata.getFieldName(); - if (sourceAndMetadata.containsKey(metadataName)) { - metadata.put(metadataName, source.remove(metadataName)); + + for (String metadataKey : metadataKeys) { + if (sourceAndMetadata.containsKey(metadataKey)) { + metadata.put(metadataKey, source.remove(metadataKey)); } } return new Tuple<>(source, metadata); } - /** - * Fetch the timestamp from the ingestMetadata, if it exists - * @return the timestamp for the document or null - */ - public static ZonedDateTime getTimestamp(Map ingestMetadata) { - if (ingestMetadata == null) { - return null; - } - Object ts = ingestMetadata.get(IngestDocument.TIMESTAMP); - if (ts instanceof ZonedDateTime timestamp) { - return timestamp; - } else if (ts instanceof String str) { - return ZonedDateTime.parse(str); - } - return null; - } - /** * get the source map, if externally modified then the guarantees of this class are not enforced */ @@ -326,14 +281,14 @@ public Object setValue(Object value) { @Override public boolean equals(Object o) { if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; + if ((o instanceof CtxMap) == false) return false; if (super.equals(o) == false) return false; - IngestSourceAndMetadata that = (IngestSourceAndMetadata) o; - return Objects.equals(source, that.source) && Objects.equals(metadata, that.metadata); + CtxMap ctxMap = (CtxMap) o; + return source.equals(ctxMap.source) && metadata.equals(ctxMap.metadata); } @Override public int hashCode() { - return Objects.hash(source, metadata); + return Objects.hash(super.hashCode(), source, metadata); } } diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestSourceAndMetadataTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestCtxMapTests.java similarity index 91% rename from server/src/test/java/org/elasticsearch/ingest/IngestSourceAndMetadataTests.java rename to server/src/test/java/org/elasticsearch/ingest/IngestCtxMapTests.java index fac543fa05864..098794dde902b 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestSourceAndMetadataTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestCtxMapTests.java @@ -22,9 +22,9 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; -public class IngestSourceAndMetadataTests extends ESTestCase { +public class IngestCtxMapTests extends ESTestCase { - IngestSourceAndMetadata map; + IngestCtxMap map; Metadata md; public void testSettersAndGetters() { @@ -37,7 +37,7 @@ public void testSettersAndGetters() { metadata.put("_if_primary_term", 10000); metadata.put("_version_type", "internal"); metadata.put("_dynamic_templates", Map.of("foo", "bar")); - map = new IngestSourceAndMetadata(new HashMap<>(), new Metadata(metadata, null)); + map = new IngestCtxMap(new HashMap<>(), new Metadata(metadata, null)); md = map.getMetadata(); assertEquals("myIndex", md.getIndex()); md.setIndex("myIndex2"); @@ -76,7 +76,7 @@ public String toString() { }); metadata.put("c", null); metadata.put("d", 1234); - map = new IngestSourceAndMetadata(new HashMap<>(), new TestMetadata(metadata, allowAllValidators("a", "b", "c", "d"))); + map = new IngestCtxMap(new HashMap<>(), new TestMetadata(metadata, allowAllValidators("a", "b", "c", "d"))); md = map.getMetadata(); assertNull(md.getString("c")); assertNull(md.getString("no key")); @@ -91,7 +91,7 @@ public void testGetNumber() { metadata.put("b", Double.MAX_VALUE); metadata.put("c", "NaN"); metadata.put("d", null); - map = new IngestSourceAndMetadata(new HashMap<>(), new TestMetadata(metadata, allowAllValidators("a", "b", "c", "d"))); + map = new IngestCtxMap(new HashMap<>(), new TestMetadata(metadata, allowAllValidators("a", "b", "c", "d"))); md = map.getMetadata(); assertEquals(Long.MAX_VALUE, md.getNumber("a")); assertEquals(Double.MAX_VALUE, md.getNumber("b")); @@ -106,7 +106,7 @@ public void testInvalidMetadata() { metadata.put("_version", Double.MAX_VALUE); IllegalArgumentException err = expectThrows( IllegalArgumentException.class, - () -> new IngestSourceAndMetadata(new HashMap<>(), new Metadata(metadata, null)) + () -> new IngestCtxMap(new HashMap<>(), new Metadata(metadata, null)) ); assertThat(err.getMessage(), containsString("_version may only be set to an int or a long but was [")); assertThat(err.getMessage(), containsString("] with type [java.lang.Double]")); @@ -117,7 +117,7 @@ public void testSourceInMetadata() { source.put("_version", 25); IllegalArgumentException err = expectThrows( IllegalArgumentException.class, - () -> new IngestSourceAndMetadata(source, new Metadata(source, null)) + () -> new IngestCtxMap(source, new Metadata(source, null)) ); assertEquals("unexpected metadata [_version:25] in source", err.getMessage()); } @@ -129,7 +129,7 @@ public void testExtraMetadata() { metadata.put("routing", "myRouting"); IllegalArgumentException err = expectThrows( IllegalArgumentException.class, - () -> new IngestSourceAndMetadata(new HashMap<>(), new Metadata(metadata, null)) + () -> new IngestCtxMap(new HashMap<>(), new Metadata(metadata, null)) ); assertEquals("Unexpected metadata keys [routing:myRouting, version:567]", err.getMessage()); } @@ -138,7 +138,7 @@ public void testPutSource() { Map metadata = new HashMap<>(); metadata.put("_version", 123); Map source = new HashMap<>(); - map = new IngestSourceAndMetadata(source, new Metadata(metadata, null)); + map = new IngestCtxMap(source, new Metadata(metadata, null)); } public void testRemove() { @@ -146,7 +146,7 @@ public void testRemove() { String canRemove = "canRemove"; Map metadata = new HashMap<>(); metadata.put(cannotRemove, "value"); - map = new IngestSourceAndMetadata(new HashMap<>(), new TestMetadata(metadata, Map.of(cannotRemove, (o, k, v) -> { + map = new IngestCtxMap(new HashMap<>(), new TestMetadata(metadata, Map.of(cannotRemove, (o, k, v) -> { if (v == null) { throw new IllegalArgumentException(k + " cannot be null or removed"); } @@ -211,7 +211,7 @@ public void testEntryAndIterator() { source.put("foo", "bar"); source.put("baz", "qux"); source.put("noz", "zon"); - map = new IngestSourceAndMetadata(source, TestMetadata.withNullableVersion(metadata)); + map = new IngestCtxMap(source, TestMetadata.withNullableVersion(metadata)); md = map.getMetadata(); for (Map.Entry entry : map.entrySet()) { @@ -251,7 +251,7 @@ public void testEntryAndIterator() { } public void testContainsValue() { - map = new IngestSourceAndMetadata(Map.of("myField", "fieldValue"), new Metadata(Map.of("_version", 5678), null)); + map = new IngestCtxMap(Map.of("myField", "fieldValue"), new Metadata(Map.of("_version", 5678), null)); assertTrue(map.containsValue(5678)); assertFalse(map.containsValue(5679)); assertTrue(map.containsValue("fieldValue")); @@ -259,7 +259,7 @@ public void testContainsValue() { } public void testValidators() { - map = new IngestSourceAndMetadata("myIndex", "myId", 1234, "myRouting", VersionType.EXTERNAL, null, new HashMap<>()); + map = new IngestCtxMap("myIndex", "myId", 1234, "myRouting", VersionType.EXTERNAL, null, new HashMap<>()); md = map.getMetadata(); IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> map.put("_index", 555)); assertEquals("_index must be null or a String but was [555] with type [java.lang.Integer]", err.getMessage()); @@ -322,7 +322,7 @@ public void testValidators() { public void testHandlesAllVersionTypes() { Map mdRawMap = new HashMap<>(); mdRawMap.put("_version", 1234); - map = new IngestSourceAndMetadata(new HashMap<>(), new Metadata(mdRawMap, null)); + map = new IngestCtxMap(new HashMap<>(), new Metadata(mdRawMap, null)); md = map.getMetadata(); assertNull(md.getVersionType()); for (VersionType vt : VersionType.values()) { diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java index 39d93a0691856..6182e4709a6b8 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java @@ -2228,7 +2228,7 @@ private class IngestDocumentMatcher implements ArgumentMatcher { @Override public boolean matches(IngestDocument other) { - // ingest metadata and IngestSourceAndMetadata will not be the same (timestamp differs every time) + // ingest metadata and IngestCtxMap will not be the same (timestamp differs every time) return Objects.equals(ingestDocument.getSource(), other.getSource()) && Objects.equals(ingestDocument.getMetadata().getMap(), other.getMetadata().getMap()); } diff --git a/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java b/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java index ffd9ca324f63b..58afd55d38480 100644 --- a/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java +++ b/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java @@ -37,8 +37,8 @@ public static IngestDocument withNullableVersion(Map sourceAndMe * _versions. Normally null _version is not allowed, but many tests don't care about that invariant. */ public static IngestDocument ofIngestWithNullableVersion(Map sourceAndMetadata, Map ingestMetadata) { - Tuple, Map> sm = IngestSourceAndMetadata.splitSourceAndMetadata(sourceAndMetadata); - return new IngestDocument(new IngestSourceAndMetadata(sm.v1(), TestMetadata.withNullableVersion(sm.v2())), ingestMetadata); + Tuple, Map> sm = IngestCtxMap.splitSourceAndMetadata(sourceAndMetadata); + return new IngestDocument(new IngestCtxMap(sm.v1(), TestMetadata.withNullableVersion(sm.v2())), ingestMetadata); } /** @@ -57,7 +57,7 @@ public static IngestDocument withDefaultVersion(Map sourceAndMet * can observe changes to the map directly. */ public static IngestDocument ofMetadataWithValidator(Map metadata, Map validators) { - return new IngestDocument(new IngestSourceAndMetadata(new HashMap<>(), new TestMetadata(metadata, validators)), new HashMap<>()); + return new IngestDocument(new IngestCtxMap(new HashMap<>(), new TestMetadata(metadata, validators)), new HashMap<>()); } /** From 86b35edb54d49b9f389546b5b83c27bb561f484f Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 11 Jul 2022 17:06:26 -0500 Subject: [PATCH 09/24] Script: Configuration driven validation for CtxMap Adds FieldProperties record that configures how to validate fields. Fields have a type, are writeable or read-only, and nullable or not and may have an additional validation useful for Set/Enum validation. Splits IngestMetadata from Metadata in preparation for new Metdata subclasses. --- .../ingest/common/RenameProcessorTests.java | 6 +- .../elasticsearch/ingest/IngestCtxMap.java | 81 +++++- .../elasticsearch/ingest/IngestDocument.java | 7 +- .../java/org/elasticsearch/script/CtxMap.java | 5 + .../org/elasticsearch/script/Metadata.java | 260 ++++++------------ .../ingest/IngestCtxMapTests.java | 39 ++- .../elasticsearch/script/MetadataTests.java | 25 -- .../ingest/TestIngestCtxMetadata.java | 26 ++ .../ingest/TestIngestDocument.java | 17 +- .../elasticsearch/script/TestMetadata.java | 27 -- 10 files changed, 226 insertions(+), 267 deletions(-) delete mode 100644 server/src/test/java/org/elasticsearch/script/MetadataTests.java create mode 100644 test/framework/src/main/java/org/elasticsearch/ingest/TestIngestCtxMetadata.java delete mode 100644 test/framework/src/main/java/org/elasticsearch/script/TestMetadata.java diff --git a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RenameProcessorTests.java b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RenameProcessorTests.java index 32566e82baf80..4cab0b999c248 100644 --- a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RenameProcessorTests.java +++ b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RenameProcessorTests.java @@ -140,11 +140,11 @@ public void testRenameAtomicOperationSetFails() throws Exception { Map metadata = new HashMap<>(); metadata.put("list", Collections.singletonList("item")); - IngestDocument ingestDocument = TestIngestDocument.ofMetadataWithValidator(metadata, Map.of("new_field", (o, k, v) -> { + IngestDocument ingestDocument = TestIngestDocument.ofMetadataWithValidator(metadata, Map.of("new_field", (k, v) -> { if (v != null) { throw new UnsupportedOperationException(); } - }, "list", (o, k, v) -> {})); + }, "list", (k, v) -> {})); Processor processor = createRenameProcessor("list", "new_field", false); try { processor.execute(ingestDocument); @@ -160,7 +160,7 @@ public void testRenameAtomicOperationRemoveFails() throws Exception { Map metadata = new HashMap<>(); metadata.put("list", Collections.singletonList("item")); - IngestDocument ingestDocument = TestIngestDocument.ofMetadataWithValidator(metadata, Map.of("list", (o, k, v) -> { + IngestDocument ingestDocument = TestIngestDocument.ofMetadataWithValidator(metadata, Map.of("list", (k, v) -> { if (v == null) { throw new UnsupportedOperationException(); } diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java index 51a6178087fab..dc5bf06697c51 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java @@ -8,7 +8,7 @@ package org.elasticsearch.ingest; -import org.elasticsearch.core.Tuple; +import org.elasticsearch.common.util.Maps; import org.elasticsearch.index.VersionType; import org.elasticsearch.script.CtxMap; import org.elasticsearch.script.Metadata; @@ -46,7 +46,7 @@ class IngestCtxMap extends CtxMap { ZonedDateTime timestamp, Map source ) { - super(new HashMap<>(source), new Metadata(index, id, version, routing, versionType, timestamp)); + super(new HashMap<>(source), new IngestMetadata(index, id, version, routing, versionType, timestamp)); } /** @@ -59,16 +59,6 @@ class IngestCtxMap extends CtxMap { super(source, metadata); } - /** - * Returns a new metadata map and the existing source map with metadata removed. - */ - public static Tuple, Map> splitSourceAndMetadata(Map sourceAndMetadata) { - return CtxMap.splitSourceAndMetadata( - sourceAndMetadata, - Arrays.stream(IngestDocument.Metadata.values()).map(IngestDocument.Metadata::getFieldName).collect(Collectors.toSet()) - ); - } - /** * Fetch the timestamp from the ingestMetadata, if it exists * @return the timestamp for the document or null @@ -85,4 +75,71 @@ public static ZonedDateTime getTimestamp(Map ingestMetadata) { } return null; } + + static class IngestMetadata extends Metadata { + private static final FieldProperty UPDATABLE_STRING = new FieldProperty<>(String.class, true, true, null); + static final Map> PROPERTIES = Map.of( + INDEX, + UPDATABLE_STRING, + ID, + UPDATABLE_STRING, + ROUTING, + UPDATABLE_STRING, + VERSION_TYPE, + new FieldProperty<>(String.class, true, true, (k, v) -> { + try { + VersionType.fromString(v); + return; + } catch (IllegalArgumentException ignored) {} + throw new IllegalArgumentException( + k + + " must be a null or one of [" + + Arrays.stream(VersionType.values()).map(vt -> VersionType.toString(vt)).collect(Collectors.joining(", ")) + + "] but was [" + + v + + "] with type [" + + v.getClass().getName() + + "]" + ); + }), + VERSION, + new FieldProperty<>(Number.class, false, true, FieldProperty.LONGABLE_NUMBER), + TYPE, + new FieldProperty<>(String.class, false, false, null), + IF_SEQ_NO, + new FieldProperty<>(Number.class, true, true, FieldProperty.LONGABLE_NUMBER), + IF_PRIMARY_TERM, + new FieldProperty<>(Number.class, true, true, FieldProperty.LONGABLE_NUMBER), + DYNAMIC_TEMPLATES, + new FieldProperty<>(Map.class, true, true, null) + ); + + protected final ZonedDateTime timestamp; + + IngestMetadata(String index, String id, long version, String routing, VersionType versionType, ZonedDateTime timestamp) { + this(metadataMap(index, id, version, routing, versionType), timestamp); + } + + IngestMetadata(Map metadata, ZonedDateTime timestamp) { + super(metadata, PROPERTIES); + this.timestamp = timestamp; + } + + /** + * Create the backing metadata map with the standard contents assuming default validators. + */ + protected static Map metadataMap(String index, String id, long version, String routing, VersionType versionType) { + Map metadata = Maps.newHashMapWithExpectedSize(IngestDocument.Metadata.values().length); + metadata.put(IngestDocument.Metadata.INDEX.getFieldName(), index); + metadata.put(IngestDocument.Metadata.ID.getFieldName(), id); + metadata.put(IngestDocument.Metadata.VERSION.getFieldName(), version); + if (routing != null) { + metadata.put(IngestDocument.Metadata.ROUTING.getFieldName(), routing); + } + if (versionType != null) { + metadata.put(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), VersionType.toString(versionType)); + } + return metadata; + } + } } diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java index dcb5b4e090567..81ecfa39bccd0 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java @@ -77,10 +77,13 @@ public IngestDocument(IngestDocument other) { * Constructor to create an IngestDocument from its constituent maps. The maps are shallow copied. */ public IngestDocument(Map sourceAndMetadata, Map ingestMetadata) { - Tuple, Map> sm = IngestCtxMap.splitSourceAndMetadata(sourceAndMetadata); + Tuple, Map> sm = IngestCtxMap.splitSourceAndMetadata( + sourceAndMetadata, + Arrays.stream(IngestDocument.Metadata.values()).map(IngestDocument.Metadata::getFieldName).collect(Collectors.toSet()) + ); this.sourceAndMetadata = new IngestCtxMap( sm.v1(), - new org.elasticsearch.script.Metadata(sm.v2(), IngestCtxMap.getTimestamp(ingestMetadata)) + new IngestCtxMap.IngestMetadata(sm.v2(), IngestCtxMap.getTimestamp(ingestMetadata)) ); this.ingestMetadata = new HashMap<>(ingestMetadata); this.ingestMetadata.computeIfPresent(TIMESTAMP, (k, v) -> { diff --git a/server/src/main/java/org/elasticsearch/script/CtxMap.java b/server/src/main/java/org/elasticsearch/script/CtxMap.java index 80fa299206571..2f82ed05b30a0 100644 --- a/server/src/main/java/org/elasticsearch/script/CtxMap.java +++ b/server/src/main/java/org/elasticsearch/script/CtxMap.java @@ -23,6 +23,11 @@ import java.util.Set; import java.util.stream.Collectors; +/** + * A scripting ctx map with metadata for write ingest contexts. Delegates all metadata updates to metadata and + * all other updates to source. Implements the {@link Map} interface for backwards compatibility while performing + * validation via {@link Metadata}. + */ public class CtxMap extends AbstractMap { protected final Map source; protected final Metadata metadata; diff --git a/server/src/main/java/org/elasticsearch/script/Metadata.java b/server/src/main/java/org/elasticsearch/script/Metadata.java index 1cb9ce6e9a30f..27ef71906d7d4 100644 --- a/server/src/main/java/org/elasticsearch/script/Metadata.java +++ b/server/src/main/java/org/elasticsearch/script/Metadata.java @@ -8,13 +8,8 @@ package org.elasticsearch.script; -import org.elasticsearch.common.util.Maps; -import org.elasticsearch.index.VersionType; -import org.elasticsearch.ingest.IngestDocument; - import java.time.ZonedDateTime; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; @@ -22,6 +17,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.function.BiConsumer; import java.util.stream.Collectors; /** @@ -52,80 +48,31 @@ public class Metadata { protected static final String IF_PRIMARY_TERM = "_if_primary_term"; protected static final String DYNAMIC_TEMPLATES = "_dynamic_templates"; - protected static final Map VALIDATORS = Map.of( - INDEX, - Metadata::stringValidator, - ID, - Metadata::stringValidator, - ROUTING, - Metadata::stringValidator, - VERSION_TYPE, - Metadata::versionTypeValidator, - VERSION, - Metadata::notNullLongValidator, - TYPE, - Metadata::stringValidator, - IF_SEQ_NO, - Metadata::longValidator, - IF_PRIMARY_TERM, - Metadata::longValidator, - DYNAMIC_TEMPLATES, - Metadata::mapValidator - ); - protected final Map map; - protected final Map validators; - - // timestamp is new to ingest metadata, so it doesn't need to be backed by the map for back compat - protected final ZonedDateTime timestamp; - - public Metadata(String index, String id, long version, String routing, VersionType versionType, ZonedDateTime timestamp) { - this(metadataMap(index, id, version, routing, versionType), timestamp, VALIDATORS); - } + protected final Map> properties; + protected static final FieldProperty BAD_KEY = new FieldProperty<>(null, false, false, null); - public Metadata(Map map, ZonedDateTime timestamp) { - this(map, timestamp, VALIDATORS); - } - - Metadata(Map map, ZonedDateTime timestamp, Map validators) { + public Metadata(Map map, Map> properties) { this.map = map; - this.timestamp = timestamp; - this.validators = validators; + this.properties = properties; validateMetadata(); } - /** - * Create the backing metadata map with the standard contents assuming default validators. - */ - protected static Map metadataMap(String index, String id, long version, String routing, VersionType versionType) { - Map metadata = Maps.newHashMapWithExpectedSize(IngestDocument.Metadata.values().length); - metadata.put(IngestDocument.Metadata.INDEX.getFieldName(), index); - metadata.put(IngestDocument.Metadata.ID.getFieldName(), id); - metadata.put(IngestDocument.Metadata.VERSION.getFieldName(), version); - if (routing != null) { - metadata.put(IngestDocument.Metadata.ROUTING.getFieldName(), routing); - } - if (versionType != null) { - metadata.put(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), VersionType.toString(versionType)); - } - return metadata; - } - /** * Check that all metadata map contains only valid metadata and no extraneous keys and source map contains no metadata */ protected void validateMetadata() { int numMetadata = 0; - for (Map.Entry entry : validators.entrySet()) { + for (Map.Entry> entry : properties.entrySet()) { String key = entry.getKey(); if (map.containsKey(key)) { numMetadata++; } - entry.getValue().accept(MapOperation.INIT, key, map.get(key)); + entry.getValue().check(MapOperation.INIT, key, map.get(key)); } if (numMetadata < map.size()) { Set keys = new HashSet<>(map.keySet()); - keys.removeAll(validators.keySet()); + keys.removeAll(properties.keySet()); throw new IllegalArgumentException( "Unexpected metadata keys [" + keys.stream().sorted().map(k -> k + ":" + map.get(k)).collect(Collectors.joining(", ")) + "]" ); @@ -174,7 +121,7 @@ public void setVersion(long version) { } public ZonedDateTime getTimestamp() { - return timestamp; + throw new UnsupportedOperationException("unimplemented"); } // These are not available to scripts @@ -220,7 +167,7 @@ public Number getNumber(String key) { * this call. */ public boolean isMetadata(String key) { - return validators.containsKey(key); + return properties.containsKey(key); } /** @@ -228,8 +175,7 @@ public boolean isMetadata(String key) { * @throws IllegalArgumentException if {@link #isMetadata(String)} is false or the key cannot be updated to the value. */ public Object put(String key, Object value) { - Validator v = validators.getOrDefault(key, this::badKey); - v.accept(MapOperation.UPDATE, key, value); + properties.getOrDefault(key, Metadata.BAD_KEY).check(MapOperation.UPDATE, key, value); return map.put(key, value); } @@ -259,8 +205,7 @@ public Object get(String key) { * @throws IllegalArgumentException if {@link #isMetadata(String)} is false or the key cannot be removed. */ public Object remove(String key) { - Validator v = validators.getOrDefault(key, this::badKey); - v.accept(MapOperation.REMOVE, key, null); + properties.getOrDefault(key, Metadata.BAD_KEY).check(MapOperation.REMOVE, key, null); return map.remove(key); } @@ -280,7 +225,7 @@ public int size() { @Override public Metadata clone() { - return new Metadata(new HashMap<>(map), timestamp, new HashMap<>(validators)); + return new Metadata(new HashMap<>(map), new HashMap<>(properties)); } /** @@ -295,7 +240,7 @@ public Map getMap() { */ public Set metadataKeys(Set other) { Set keys = null; - for (String key : validators.keySet()) { + for (String key : properties.keySet()) { if (other.contains(key)) { if (keys == null) { keys = new HashSet<>(); @@ -306,99 +251,6 @@ public Set metadataKeys(Set other) { return keys != null ? keys : Collections.emptySet(); } - /** - * Allow a String or null. - * @throws IllegalArgumentException if {@param value} is neither a {@link String} nor null - */ - protected static void stringValidator(MapOperation op, String key, Object value) { - if (op == MapOperation.REMOVE || value == null || value instanceof String) { - return; - } - throw new IllegalArgumentException( - key + " must be null or a String but was [" + value + "] with type [" + value.getClass().getName() + "]" - ); - } - - /** - * Allow Numbers that can be represented as longs without loss of precision or null - * @throws IllegalArgumentException if the value cannot be represented as a long - */ - public static void longValidator(MapOperation op, String key, Object value) { - if (op == MapOperation.REMOVE || value == null) { - return; - } - if (value instanceof Number number) { - long version = number.longValue(); - // did we round? - if (number.doubleValue() == version) { - return; - } - } - throw new IllegalArgumentException( - key + " may only be set to an int or a long but was [" + value + "] with type [" + value.getClass().getName() + "]" - ); - } - - /** - * Same as {@link #longValidator(MapOperation, String, Object)} but {@param value} cannot be null. - * @throws IllegalArgumentException if value is null or cannot be represented as a long. - */ - protected static void notNullLongValidator(MapOperation op, String key, Object value) { - if (op == MapOperation.REMOVE || value == null) { - throw new IllegalArgumentException(key + " cannot be removed or set to null"); - } - longValidator(op, key, value); - } - - /** - * Allow maps. - * @throws IllegalArgumentException if {@param value} is not a {@link Map} - */ - public static void mapValidator(MapOperation op, String key, Object value) { - if (op == MapOperation.REMOVE || value == null || value instanceof Map) { - return; - } - throw new IllegalArgumentException( - key + " must be a null or a Map but was [" + value + "] with type [" + value.getClass().getName() + "]" - ); - } - - /** - * Allow lower case Strings that map to VersionType values, or null. - * @throws IllegalArgumentException if {@param value} cannot be converted via {@link VersionType#fromString(String)} - */ - protected static void versionTypeValidator(MapOperation op, String key, Object value) { - if (op == MapOperation.REMOVE || value == null) { - return; - } - if (value instanceof String versionType) { - try { - VersionType.fromString(versionType); - return; - } catch (IllegalArgumentException ignored) {} - } - throw new IllegalArgumentException( - key - + " must be a null or one of [" - + Arrays.stream(VersionType.values()).map(vt -> VersionType.toString(vt)).collect(Collectors.joining(", ")) - + "] but was [" - + value - + "] with type [" - + value.getClass().getName() - + "]" - ); - } - - private void badKey(MapOperation op, String key, Object value) { - throw new IllegalArgumentException( - "unexpected metadata key [" - + key - + "], expected one of [" - + validators.keySet().stream().sorted().collect(Collectors.joining(", ")) - + "]" - ); - } - /** * The operation being performed on the value in the map. * INIT: Initial value - the metadata value as passed into this class @@ -411,26 +263,72 @@ public enum MapOperation { REMOVE } - /** - * A "TriConsumer" that tests if the {@link MapOperation}, the metadata key and value are valid. - * - * throws IllegalArgumentException if the given triple is invalid - */ - @FunctionalInterface - public interface Validator { - void accept(MapOperation op, String key, Object value); - } + public record FieldProperty (Class type, boolean nullable, boolean writable, BiConsumer extendedValidation) { - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; - Metadata metadata = (Metadata) o; - return Objects.equals(map, metadata.map) && Objects.equals(timestamp, metadata.timestamp); - } + public static BiConsumer LONGABLE_NUMBER = (k, v) -> { + long version = v.longValue(); + // did we round? + if (v.doubleValue() == version) { + return; + } + throw new IllegalArgumentException( + k + " may only be set to an int or a long but was [" + v + "] with type [" + v.getClass().getName() + "]" + ); + }; + + public static FieldProperty ALLOW_ALL = new FieldProperty<>(null, true, true, null); + + @SuppressWarnings("fallthrough") + public void check(MapOperation op, String key, Object value) { + switch (op) { + case UPDATE: + if (writable == false) { + throw new IllegalArgumentException(key + " cannot be updated"); + } + // fall through + + case INIT: + if (value == null) { + if (nullable == false) { + throw new IllegalArgumentException(key + " cannot be null"); + } + } else { + checkType(key, value); + } + break; + + case REMOVE: + if (writable == false || nullable == false) { + throw new IllegalArgumentException(key + " cannot be removed"); + } + break; + + default: + throw new IllegalArgumentException("unexpected op [" + op + "] for key [" + key + "] and value [" + value + "]"); - @Override - public int hashCode() { - return Objects.hash(map, timestamp); + } + } + + @SuppressWarnings("unchecked") + private void checkType(String key, Object value) { + if (type == null) { + return; + } + if (type.isAssignableFrom(value.getClass()) == false) { + throw new IllegalArgumentException( + key + + " [" + + value + + "] is wrong type, expected assignable to [" + + type.getName() + + "], not [" + + value.getClass().getName() + + "]" + ); + } + if (extendedValidation != null) { + extendedValidation.accept(key, (T) value); + } + } } } diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestCtxMapTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestCtxMapTests.java index 098794dde902b..e5575910ad881 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestCtxMapTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestCtxMapTests.java @@ -10,7 +10,6 @@ import org.elasticsearch.index.VersionType; import org.elasticsearch.script.Metadata; -import org.elasticsearch.script.TestMetadata; import org.elasticsearch.test.ESTestCase; import java.util.HashMap; @@ -21,6 +20,8 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasEntry; +import static org.hamcrest.Matchers.notNullValue; public class IngestCtxMapTests extends ESTestCase { @@ -76,7 +77,7 @@ public String toString() { }); metadata.put("c", null); metadata.put("d", 1234); - map = new IngestCtxMap(new HashMap<>(), new TestMetadata(metadata, allowAllValidators("a", "b", "c", "d"))); + map = new IngestCtxMap(new HashMap<>(), new TestIngestCtxMetadata(metadata, allowAllValidators("a", "b", "c", "d"))); md = map.getMetadata(); assertNull(md.getString("c")); assertNull(md.getString("no key")); @@ -91,7 +92,7 @@ public void testGetNumber() { metadata.put("b", Double.MAX_VALUE); metadata.put("c", "NaN"); metadata.put("d", null); - map = new IngestCtxMap(new HashMap<>(), new TestMetadata(metadata, allowAllValidators("a", "b", "c", "d"))); + map = new IngestCtxMap(new HashMap<>(), new TestIngestCtxMetadata(metadata, allowAllValidators("a", "b", "c", "d"))); md = map.getMetadata(); assertEquals(Long.MAX_VALUE, md.getNumber("a")); assertEquals(Double.MAX_VALUE, md.getNumber("b")); @@ -146,11 +147,18 @@ public void testRemove() { String canRemove = "canRemove"; Map metadata = new HashMap<>(); metadata.put(cannotRemove, "value"); - map = new IngestCtxMap(new HashMap<>(), new TestMetadata(metadata, Map.of(cannotRemove, (o, k, v) -> { - if (v == null) { - throw new IllegalArgumentException(k + " cannot be null or removed"); - } - }, canRemove, (o, k, v) -> {}))); + map = new IngestCtxMap( + new HashMap<>(), + new TestIngestCtxMetadata( + metadata, + Map.of( + cannotRemove, + new Metadata.FieldProperty<>(String.class, false, true, null), + canRemove, + new Metadata.FieldProperty<>(String.class, true, true, null) + ) + ) + ); String msg = "cannotRemove cannot be null or removed"; IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> map.remove(cannotRemove)); assertEquals(msg, err.getMessage()); @@ -211,7 +219,7 @@ public void testEntryAndIterator() { source.put("foo", "bar"); source.put("baz", "qux"); source.put("noz", "zon"); - map = new IngestCtxMap(source, TestMetadata.withNullableVersion(metadata)); + map = new IngestCtxMap(source, TestIngestCtxMetadata.withNullableVersion(metadata)); md = map.getMetadata(); for (Map.Entry entry : map.entrySet()) { @@ -364,11 +372,18 @@ public Object setValue(Object value) { } } - private static Map allowAllValidators(String... keys) { - Map validators = new HashMap<>(); + private static Map> allowAllValidators(String... keys) { + Map> validators = new HashMap<>(); for (String key : keys) { - validators.put(key, (o, k, v) -> {}); + validators.put(key, Metadata.FieldProperty.ALLOW_ALL); } return validators; } + + public void testDefaultFieldPropertiesForAllMetadata() { + for (IngestDocument.Metadata m : IngestDocument.Metadata.values()) { + assertThat(IngestCtxMap.IngestMetadata.PROPERTIES, hasEntry(equalTo(m.getFieldName()), notNullValue())); + } + assertEquals(IngestDocument.Metadata.values().length, IngestCtxMap.IngestMetadata.PROPERTIES.size()); + } } diff --git a/server/src/test/java/org/elasticsearch/script/MetadataTests.java b/server/src/test/java/org/elasticsearch/script/MetadataTests.java deleted file mode 100644 index c241d14d93b1b..0000000000000 --- a/server/src/test/java/org/elasticsearch/script/MetadataTests.java +++ /dev/null @@ -1,25 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0 and the Server Side Public License, v 1; you may not use this file except - * in compliance with, at your election, the Elastic License 2.0 or the Server - * Side Public License, v 1. - */ - -package org.elasticsearch.script; - -import org.elasticsearch.ingest.IngestDocument; -import org.elasticsearch.test.ESTestCase; - -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.hasEntry; -import static org.hamcrest.Matchers.notNullValue; - -public class MetadataTests extends ESTestCase { - public void testDefaultValidatorForAllMetadata() { - for (IngestDocument.Metadata m : IngestDocument.Metadata.values()) { - assertThat(Metadata.VALIDATORS, hasEntry(equalTo(m.getFieldName()), notNullValue())); - } - assertEquals(IngestDocument.Metadata.values().length, Metadata.VALIDATORS.size()); - } -} diff --git a/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestCtxMetadata.java b/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestCtxMetadata.java new file mode 100644 index 0000000000000..2040fa5ee9322 --- /dev/null +++ b/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestCtxMetadata.java @@ -0,0 +1,26 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.ingest; + +import org.elasticsearch.script.Metadata; + +import java.util.HashMap; +import java.util.Map; + +public class TestIngestCtxMetadata extends Metadata { + public TestIngestCtxMetadata(Map map, Map> properties) { + super(map, properties); + } + + public static TestIngestCtxMetadata withNullableVersion(Map map) { + Map> updatedProperties = new HashMap<>(IngestCtxMap.IngestMetadata.PROPERTIES); + updatedProperties.replace(VERSION, new Metadata.FieldProperty<>(Number.class, true, true, FieldProperty.LONGABLE_NUMBER)); + return new TestIngestCtxMetadata(map, updatedProperties); + } +} diff --git a/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java b/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java index 58afd55d38480..a9135b80ae790 100644 --- a/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java +++ b/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java @@ -12,11 +12,13 @@ import org.elasticsearch.core.Tuple; import org.elasticsearch.index.VersionType; import org.elasticsearch.script.Metadata; -import org.elasticsearch.script.TestMetadata; import org.elasticsearch.test.ESTestCase; +import java.util.Arrays; import java.util.HashMap; import java.util.Map; +import java.util.function.BiConsumer; +import java.util.stream.Collectors; /** * Construct ingest documents for testing purposes @@ -37,8 +39,11 @@ public static IngestDocument withNullableVersion(Map sourceAndMe * _versions. Normally null _version is not allowed, but many tests don't care about that invariant. */ public static IngestDocument ofIngestWithNullableVersion(Map sourceAndMetadata, Map ingestMetadata) { - Tuple, Map> sm = IngestCtxMap.splitSourceAndMetadata(sourceAndMetadata); - return new IngestDocument(new IngestCtxMap(sm.v1(), TestMetadata.withNullableVersion(sm.v2())), ingestMetadata); + Tuple, Map> sm = IngestCtxMap.splitSourceAndMetadata( + sourceAndMetadata, + Arrays.stream(IngestDocument.Metadata.values()).map(IngestDocument.Metadata::getFieldName).collect(Collectors.toSet()) + ); + return new IngestDocument(new IngestCtxMap(sm.v1(), TestIngestCtxMetadata.withNullableVersion(sm.v2())), ingestMetadata); } /** @@ -56,8 +61,10 @@ public static IngestDocument withDefaultVersion(Map sourceAndMet * Create an IngestDocument with a metadata map and validators. The metadata map is passed by reference, not copied, so callers * can observe changes to the map directly. */ - public static IngestDocument ofMetadataWithValidator(Map metadata, Map validators) { - return new IngestDocument(new IngestCtxMap(new HashMap<>(), new TestMetadata(metadata, validators)), new HashMap<>()); + public static IngestDocument ofMetadataWithValidator(Map metadata, Map> validators) { + Map> properties = new HashMap<>(); + validators.forEach((k, v) -> properties.put(k, new Metadata.FieldProperty<>(Object.class, true, true, v))); + return new IngestDocument(new IngestCtxMap(new HashMap<>(), new TestIngestCtxMetadata(metadata, properties)), new HashMap<>()); } /** diff --git a/test/framework/src/main/java/org/elasticsearch/script/TestMetadata.java b/test/framework/src/main/java/org/elasticsearch/script/TestMetadata.java deleted file mode 100644 index 87a45ea857549..0000000000000 --- a/test/framework/src/main/java/org/elasticsearch/script/TestMetadata.java +++ /dev/null @@ -1,27 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0 and the Server Side Public License, v 1; you may not use this file except - * in compliance with, at your election, the Elastic License 2.0 or the Server - * Side Public License, v 1. - */ - -package org.elasticsearch.script; - -import java.util.HashMap; -import java.util.Map; - -/** - * An implementation of {@link Metadata} with customizable {@link org.elasticsearch.script.Metadata.Validator}s for use in testing. - */ -public class TestMetadata extends Metadata { - public TestMetadata(Map map, Map validators) { - super(map, null, validators); - } - - public static TestMetadata withNullableVersion(Map map) { - Map updatedValidators = new HashMap<>(VALIDATORS); - updatedValidators.replace(VERSION, Metadata::longValidator); - return new TestMetadata(map, updatedValidators); - } -} From 29dd6eca796913d3064baaf3ca0b646bb90618a5 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 11 Jul 2022 17:27:22 -0500 Subject: [PATCH 10/24] Fix hashCode in CtxMap --- server/src/main/java/org/elasticsearch/script/CtxMap.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/script/CtxMap.java b/server/src/main/java/org/elasticsearch/script/CtxMap.java index 80fa299206571..3ee6b52b468fd 100644 --- a/server/src/main/java/org/elasticsearch/script/CtxMap.java +++ b/server/src/main/java/org/elasticsearch/script/CtxMap.java @@ -289,6 +289,6 @@ public boolean equals(Object o) { @Override public int hashCode() { - return Objects.hash(super.hashCode(), source, metadata); + return Objects.hash(source, metadata); } } From 472b824fe945b1c328b98afa3e8e8d040c6c11e8 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 11 Jul 2022 17:27:22 -0500 Subject: [PATCH 11/24] Fix hashCode in CtxMap --- server/src/main/java/org/elasticsearch/script/CtxMap.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/script/CtxMap.java b/server/src/main/java/org/elasticsearch/script/CtxMap.java index 2f82ed05b30a0..d7916e91fc450 100644 --- a/server/src/main/java/org/elasticsearch/script/CtxMap.java +++ b/server/src/main/java/org/elasticsearch/script/CtxMap.java @@ -294,6 +294,6 @@ public boolean equals(Object o) { @Override public int hashCode() { - return Objects.hash(super.hashCode(), source, metadata); + return Objects.hash(source, metadata); } } From bdf6bfdfd04fd8153720cd11995aa7c785105ad4 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 11 Jul 2022 18:07:07 -0500 Subject: [PATCH 12/24] Check metadata map in testSimulate --- .../java/org/elasticsearch/ingest/IngestClientIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/ingest/IngestClientIT.java b/server/src/internalClusterTest/java/org/elasticsearch/ingest/IngestClientIT.java index 8407d72eb728e..9b05421d479d2 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/ingest/IngestClientIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/ingest/IngestClientIT.java @@ -115,7 +115,7 @@ public void testSimulate() throws Exception { source.put("processed", true); IngestDocument ingestDocument = new IngestDocument("index", "id", Versions.MATCH_ANY, null, null, source); assertThat(simulateDocumentBaseResult.getIngestDocument().getSource(), equalTo(ingestDocument.getSource())); - assertThat(simulateDocumentBaseResult.getIngestDocument().getMetadata(), equalTo(ingestDocument.getMetadata())); + assertThat(simulateDocumentBaseResult.getIngestDocument().getMetadata().getMap(), equalTo(ingestDocument.getMetadata().getMap())); assertThat(simulateDocumentBaseResult.getFailure(), nullValue()); // cleanup From 16007251a6c8f543787cf8e2d23dccb4c50be615 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 11 Jul 2022 20:25:11 -0500 Subject: [PATCH 13/24] keySet(), Sets.intersection, isAvailable, protected validator & getters --- .../ingest/WriteableIngestDocument.java | 2 +- .../ingest/IngestSourceAndMetadata.java | 18 +++---- .../org/elasticsearch/script/Metadata.java | 42 +++++------------ .../ingest/WriteableIngestDocumentTests.java | 4 +- .../ingest/IngestSourceAndMetadataTests.java | 44 ----------------- .../elasticsearch/script/MetadataTests.java | 47 +++++++++++++++++++ .../results/InferenceResultsTestCase.java | 4 +- 7 files changed, 73 insertions(+), 88 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/ingest/WriteableIngestDocument.java b/server/src/main/java/org/elasticsearch/action/ingest/WriteableIngestDocument.java index dab2d5526932c..3a7e3c11fa141 100644 --- a/server/src/main/java/org/elasticsearch/action/ingest/WriteableIngestDocument.java +++ b/server/src/main/java/org/elasticsearch/action/ingest/WriteableIngestDocument.java @@ -108,7 +108,7 @@ IngestDocument getIngestDocument() { public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(DOC_FIELD); org.elasticsearch.script.Metadata metadata = ingestDocument.getMetadata(); - for (String key : metadata.getKeys()) { + for (String key : metadata.keySet()) { Object value = metadata.get(key); if (value != null) { builder.field(key, value.toString()); diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java b/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java index 592c135d46501..a041891d51371 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java @@ -9,6 +9,7 @@ package org.elasticsearch.ingest; import org.elasticsearch.common.util.Maps; +import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.core.Tuple; import org.elasticsearch.index.VersionType; import org.elasticsearch.script.Metadata; @@ -20,7 +21,6 @@ import java.util.Collection; import java.util.HashMap; import java.util.Iterator; -import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; @@ -68,7 +68,7 @@ class IngestSourceAndMetadata extends AbstractMap { IngestSourceAndMetadata(Map source, Metadata metadata) { this.source = source != null ? source : new HashMap<>(); this.metadata = metadata; - Set badKeys = metadata.metadataKeys(this.source.keySet()); + Set badKeys = Sets.intersection(this.metadata.keySet(), this.source.keySet()); if (badKeys.size() > 0) { throw new IllegalArgumentException( "unexpected metadata [" @@ -132,7 +132,7 @@ public Metadata getMetadata() { */ @Override public Set> entrySet() { - return new EntrySet(source.entrySet(), metadata.getKeys()); + return new EntrySet(source.entrySet(), metadata.keySet()); } /** @@ -141,7 +141,7 @@ public Set> entrySet() { */ @Override public Object put(String key, Object value) { - if (metadata.isMetadata(key)) { + if (metadata.isAvailable(key)) { return metadata.put(key, value); } return source.put(key, value); @@ -155,7 +155,7 @@ public Object put(String key, Object value) { public Object remove(Object key) { // uses map directly to avoid AbstractMaps linear time implementation using entrySet() if (key instanceof String str) { - if (metadata.isMetadata(str)) { + if (metadata.isAvailable(str)) { return metadata.remove(str); } } @@ -169,7 +169,7 @@ public Object remove(Object key) { @Override public void clear() { // AbstractMap uses entrySet().clear(), it should be quicker to run through the validators, then call the wrapped maps clear - for (String key : metadata.getKeys()) { + for (String key : metadata.keySet()) { metadata.remove(key); } source.clear(); @@ -200,7 +200,7 @@ public boolean containsKey(Object key) { public Object get(Object key) { // uses map directly to avoid AbstractMaps linear time implementation using entrySet() if (key instanceof String str) { - if (metadata.isMetadata(str)) { + if (metadata.isAvailable(str)) { return metadata.get(str); } } @@ -217,9 +217,9 @@ public Object get(Object key) { */ class EntrySet extends AbstractSet> { Set> sourceSet; - List metadataKeys; + Set metadataKeys; - EntrySet(Set> sourceSet, List metadataKeys) { + EntrySet(Set> sourceSet, Set metadataKeys) { this.sourceSet = sourceSet; this.metadataKeys = metadataKeys; } diff --git a/server/src/main/java/org/elasticsearch/script/Metadata.java b/server/src/main/java/org/elasticsearch/script/Metadata.java index 1cb9ce6e9a30f..8118e4f5f0cb7 100644 --- a/server/src/main/java/org/elasticsearch/script/Metadata.java +++ b/server/src/main/java/org/elasticsearch/script/Metadata.java @@ -13,12 +13,10 @@ import org.elasticsearch.ingest.IngestDocument; import java.time.ZonedDateTime; -import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; -import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Set; @@ -33,9 +31,9 @@ * - {@link #remove(String)} * - {@link #containsKey(String)} * - {@link #containsValue(Object)} - * - {@link #getKeys()} for iteration + * - {@link #keySet()} for iteration * - {@link #size()} - * - {@link #isMetadata(String)} for determining if a key is a metadata key + * - {@link #isAvailable(String)} for determining if a key is a metadata key * * Provides getters and setters for script usage. * @@ -194,7 +192,7 @@ public Map getDynamicTemplates() { /** * Get the String version of the value associated with {@code key}, or null */ - public String getString(String key) { + protected String getString(String key) { return Objects.toString(get(key), null); } @@ -202,7 +200,7 @@ public String getString(String key) { * Get the {@link Number} associated with key, or null * @throws IllegalArgumentException if the value is not a {@link Number} */ - public Number getNumber(String key) { + protected Number getNumber(String key) { Object value = get(key); if (value == null) { return null; @@ -210,7 +208,7 @@ public Number getNumber(String key) { if (value instanceof Number number) { return number; } - throw new IllegalArgumentException( + throw new IllegalStateException( "unexpected type for [" + key + "] with value [" + value + "], expected Number, got [" + value.getClass().getName() + "]" ); } @@ -219,13 +217,13 @@ public Number getNumber(String key) { * Is this key a Metadata key? A {@link #remove}d key would return false for {@link #containsKey(String)} but true for * this call. */ - public boolean isMetadata(String key) { + public boolean isAvailable(String key) { return validators.containsKey(key); } /** * Create the mapping from key to value. - * @throws IllegalArgumentException if {@link #isMetadata(String)} is false or the key cannot be updated to the value. + * @throws IllegalArgumentException if {@link #isAvailable(String)} is false or the key cannot be updated to the value. */ public Object put(String key, Object value) { Validator v = validators.getOrDefault(key, this::badKey); @@ -256,7 +254,7 @@ public Object get(String key) { /** * Remove the mapping associated with {@param key} - * @throws IllegalArgumentException if {@link #isMetadata(String)} is false or the key cannot be removed. + * @throws IllegalArgumentException if {@link #isAvailable(String)} is false or the key cannot be removed. */ public Object remove(String key) { Validator v = validators.getOrDefault(key, this::badKey); @@ -267,8 +265,8 @@ public Object remove(String key) { /** * Return the list of keys with mappings */ - public List getKeys() { - return new ArrayList<>(map.keySet()); + public Set keySet() { + return Collections.unmodifiableSet(map.keySet()); } /** @@ -290,22 +288,6 @@ public Map getMap() { return map; } - /** - * Get the metadata keys inside the {@param other} set - */ - public Set metadataKeys(Set other) { - Set keys = null; - for (String key : validators.keySet()) { - if (other.contains(key)) { - if (keys == null) { - keys = new HashSet<>(); - } - keys.add(key); - } - } - return keys != null ? keys : Collections.emptySet(); - } - /** * Allow a String or null. * @throws IllegalArgumentException if {@param value} is neither a {@link String} nor null @@ -323,7 +305,7 @@ protected static void stringValidator(MapOperation op, String key, Object value) * Allow Numbers that can be represented as longs without loss of precision or null * @throws IllegalArgumentException if the value cannot be represented as a long */ - public static void longValidator(MapOperation op, String key, Object value) { + protected static void longValidator(MapOperation op, String key, Object value) { if (op == MapOperation.REMOVE || value == null) { return; } @@ -354,7 +336,7 @@ protected static void notNullLongValidator(MapOperation op, String key, Object v * Allow maps. * @throws IllegalArgumentException if {@param value} is not a {@link Map} */ - public static void mapValidator(MapOperation op, String key, Object value) { + protected static void mapValidator(MapOperation op, String key, Object value) { if (op == MapOperation.REMOVE || value == null || value instanceof Map) { return; } diff --git a/server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java b/server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java index 4929a56296bea..6d70802bab021 100644 --- a/server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java +++ b/server/src/test/java/org/elasticsearch/action/ingest/WriteableIngestDocumentTests.java @@ -148,7 +148,7 @@ public void testToXContent() throws IOException { Map toXContentSource = (Map) toXContentDoc.get("_source"); Map toXContentIngestMetadata = (Map) toXContentDoc.get("_ingest"); - for (String fieldName : ingestDocument.getMetadata().getKeys()) { + for (String fieldName : ingestDocument.getMetadata().keySet()) { Object value = ingestDocument.getMetadata().get(fieldName); if (value == null) { assertThat(toXContentDoc.containsKey(fieldName), is(false)); @@ -159,7 +159,7 @@ public void testToXContent() throws IOException { Map sourceAndMetadata = Maps.newMapWithExpectedSize(toXContentSource.size() + ingestDocument.getMetadata().size()); sourceAndMetadata.putAll(toXContentSource); - ingestDocument.getMetadata().getKeys().forEach(k -> sourceAndMetadata.put(k, ingestDocument.getMetadata().get(k))); + ingestDocument.getMetadata().keySet().forEach(k -> sourceAndMetadata.put(k, ingestDocument.getMetadata().get(k))); IngestDocument serializedIngestDocument = new IngestDocument(sourceAndMetadata, toXContentIngestMetadata); // TODO(stu): is this test correct? Comparing against ingestDocument fails due to incorrectly failed byte array comparisons assertThat(serializedIngestDocument, equalTo(serializedIngestDocument)); diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestSourceAndMetadataTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestSourceAndMetadataTests.java index fac543fa05864..34a05e9ef2e03 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestSourceAndMetadataTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestSourceAndMetadataTests.java @@ -65,42 +65,6 @@ public void testSettersAndGetters() { assertEquals(10000, md.getIfPrimaryTerm()); } - public void testGetString() { - Map metadata = new HashMap<>(); - metadata.put("a", "A"); - metadata.put("b", new Object() { - @Override - public String toString() { - return "myToString()"; - } - }); - metadata.put("c", null); - metadata.put("d", 1234); - map = new IngestSourceAndMetadata(new HashMap<>(), new TestMetadata(metadata, allowAllValidators("a", "b", "c", "d"))); - md = map.getMetadata(); - assertNull(md.getString("c")); - assertNull(md.getString("no key")); - assertEquals("myToString()", md.getString("b")); - assertEquals("A", md.getString("a")); - assertEquals("1234", md.getString("d")); - } - - public void testGetNumber() { - Map metadata = new HashMap<>(); - metadata.put("a", Long.MAX_VALUE); - metadata.put("b", Double.MAX_VALUE); - metadata.put("c", "NaN"); - metadata.put("d", null); - map = new IngestSourceAndMetadata(new HashMap<>(), new TestMetadata(metadata, allowAllValidators("a", "b", "c", "d"))); - md = map.getMetadata(); - assertEquals(Long.MAX_VALUE, md.getNumber("a")); - assertEquals(Double.MAX_VALUE, md.getNumber("b")); - IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> md.getNumber("c")); - assertEquals("unexpected type for [c] with value [NaN], expected Number, got [java.lang.String]", err.getMessage()); - assertNull(md.getNumber("d")); - assertNull(md.getNumber("no key")); - } - public void testInvalidMetadata() { Map metadata = new HashMap<>(); metadata.put("_version", Double.MAX_VALUE); @@ -363,12 +327,4 @@ public Object setValue(Object value) { throw new UnsupportedOperationException(); } } - - private static Map allowAllValidators(String... keys) { - Map validators = new HashMap<>(); - for (String key : keys) { - validators.put(key, (o, k, v) -> {}); - } - return validators; - } } diff --git a/server/src/test/java/org/elasticsearch/script/MetadataTests.java b/server/src/test/java/org/elasticsearch/script/MetadataTests.java index c241d14d93b1b..068c15ac1470f 100644 --- a/server/src/test/java/org/elasticsearch/script/MetadataTests.java +++ b/server/src/test/java/org/elasticsearch/script/MetadataTests.java @@ -11,15 +11,62 @@ import org.elasticsearch.ingest.IngestDocument; import org.elasticsearch.test.ESTestCase; +import java.util.HashMap; +import java.util.Map; + import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasEntry; import static org.hamcrest.Matchers.notNullValue; public class MetadataTests extends ESTestCase { + Metadata md; + public void testDefaultValidatorForAllMetadata() { for (IngestDocument.Metadata m : IngestDocument.Metadata.values()) { assertThat(Metadata.VALIDATORS, hasEntry(equalTo(m.getFieldName()), notNullValue())); } assertEquals(IngestDocument.Metadata.values().length, Metadata.VALIDATORS.size()); } + + public void testGetString() { + Map metadata = new HashMap<>(); + metadata.put("a", "A"); + metadata.put("b", new Object() { + @Override + public String toString() { + return "myToString()"; + } + }); + metadata.put("c", null); + metadata.put("d", 1234); + md = new TestMetadata(metadata, allowAllValidators("a", "b", "c", "d")); + assertNull(md.getString("c")); + assertNull(md.getString("no key")); + assertEquals("myToString()", md.getString("b")); + assertEquals("A", md.getString("a")); + assertEquals("1234", md.getString("d")); + } + + public void testGetNumber() { + Map metadata = new HashMap<>(); + metadata.put("a", Long.MAX_VALUE); + metadata.put("b", Double.MAX_VALUE); + metadata.put("c", "NaN"); + metadata.put("d", null); + md = new TestMetadata(metadata, allowAllValidators("a", "b", "c", "d")); + assertEquals(Long.MAX_VALUE, md.getNumber("a")); + assertEquals(Double.MAX_VALUE, md.getNumber("b")); + IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> md.getNumber("c")); + assertEquals("unexpected type for [c] with value [NaN], expected Number, got [java.lang.String]", err.getMessage()); + assertNull(md.getNumber("d")); + assertNull(md.getNumber("no key")); + } + + private static Map allowAllValidators(String... keys) { + Map validators = new HashMap<>(); + for (String key : keys) { + validators.put(key, (o, k, v) -> {}); + } + return validators; + } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/results/InferenceResultsTestCase.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/results/InferenceResultsTestCase.java index 74916aee9296d..58e58bd9eb1f0 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/results/InferenceResultsTestCase.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/inference/results/InferenceResultsTestCase.java @@ -56,14 +56,14 @@ public void testWriteToDocAndSerialize() throws IOException { try (XContentBuilder builder = XContentFactory.jsonBuilder()) { builder.startObject(); org.elasticsearch.script.Metadata metadata = document.getMetadata(); - for (String key : metadata.getKeys()) { + for (String key : metadata.keySet()) { Object value = metadata.get(key); if (value != null) { builder.field(key, value.toString()); } } Map source = IngestDocument.deepCopyMap(document.getSourceAndMetadata()); - metadata.getKeys().forEach(source::remove); + metadata.keySet().forEach(source::remove); builder.field("_source", source); builder.field("_ingest", document.getIngestMetadata()); builder.endObject(); From f9ac7ae508b1645e0383015fdf67d3ac9cc6d86c Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 11 Jul 2022 21:15:09 -0500 Subject: [PATCH 14/24] Fix unit tests --- .../elasticsearch/ingest/IngestCtxMap.java | 2 +- .../java/org/elasticsearch/script/CtxMap.java | 4 +- .../org/elasticsearch/script/Metadata.java | 2 - .../ingest/IngestCtxMapTests.java | 44 ++++++++++++------- .../elasticsearch/script/MetadataTests.java | 18 ++------ 5 files changed, 35 insertions(+), 35 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java index dc5bf06697c51..9e98d6386d2be 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java @@ -105,7 +105,7 @@ static class IngestMetadata extends Metadata { VERSION, new FieldProperty<>(Number.class, false, true, FieldProperty.LONGABLE_NUMBER), TYPE, - new FieldProperty<>(String.class, false, false, null), + new FieldProperty<>(String.class, true, false, null), IF_SEQ_NO, new FieldProperty<>(Number.class, true, true, FieldProperty.LONGABLE_NUMBER), IF_PRIMARY_TERM, diff --git a/server/src/main/java/org/elasticsearch/script/CtxMap.java b/server/src/main/java/org/elasticsearch/script/CtxMap.java index 683a2b816069a..7301052a27a7e 100644 --- a/server/src/main/java/org/elasticsearch/script/CtxMap.java +++ b/server/src/main/java/org/elasticsearch/script/CtxMap.java @@ -17,6 +17,7 @@ import java.util.AbstractSet; import java.util.Collection; import java.util.HashMap; +import java.util.HashSet; import java.util.Iterator; import java.util.Map; import java.util.Objects; @@ -92,7 +93,8 @@ public Metadata getMetadata() { */ @Override public Set> entrySet() { - return new EntrySet(source.entrySet(), metadata.keySet()); + // Make a copy of the Metadata.keySet() to avoid a ConcurrentModificationException when removing a value from the iterator + return new EntrySet(source.entrySet(), new HashSet<>(metadata.keySet())); } /** diff --git a/server/src/main/java/org/elasticsearch/script/Metadata.java b/server/src/main/java/org/elasticsearch/script/Metadata.java index 471acc901bc4d..09b655e64b26f 100644 --- a/server/src/main/java/org/elasticsearch/script/Metadata.java +++ b/server/src/main/java/org/elasticsearch/script/Metadata.java @@ -9,8 +9,6 @@ package org.elasticsearch.script; import java.time.ZonedDateTime; -import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestCtxMapTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestCtxMapTests.java index eba6fb1a20148..f7a3761ebf0f2 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestCtxMapTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestCtxMapTests.java @@ -20,6 +20,8 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasEntry; +import static org.hamcrest.Matchers.notNullValue; public class IngestCtxMapTests extends ESTestCase { @@ -36,7 +38,7 @@ public void testSettersAndGetters() { metadata.put("_if_primary_term", 10000); metadata.put("_version_type", "internal"); metadata.put("_dynamic_templates", Map.of("foo", "bar")); - map = new IngestCtxMap(new HashMap<>(), new Metadata(metadata, null)); + map = new IngestCtxMap(new HashMap<>(), new IngestCtxMap.IngestMetadata(metadata, null)); md = map.getMetadata(); assertEquals("myIndex", md.getIndex()); md.setIndex("myIndex2"); @@ -69,7 +71,7 @@ public void testInvalidMetadata() { metadata.put("_version", Double.MAX_VALUE); IllegalArgumentException err = expectThrows( IllegalArgumentException.class, - () -> new IngestCtxMap(new HashMap<>(), new Metadata(metadata, null)) + () -> new IngestCtxMap(new HashMap<>(), new IngestCtxMap.IngestMetadata(metadata, null)) ); assertThat(err.getMessage(), containsString("_version may only be set to an int or a long but was [")); assertThat(err.getMessage(), containsString("] with type [java.lang.Double]")); @@ -80,7 +82,7 @@ public void testSourceInMetadata() { source.put("_version", 25); IllegalArgumentException err = expectThrows( IllegalArgumentException.class, - () -> new IngestCtxMap(source, new Metadata(source, null)) + () -> new IngestCtxMap(source, new IngestCtxMap.IngestMetadata(source, null)) ); assertEquals("unexpected metadata [_version:25] in source", err.getMessage()); } @@ -92,7 +94,7 @@ public void testExtraMetadata() { metadata.put("routing", "myRouting"); IllegalArgumentException err = expectThrows( IllegalArgumentException.class, - () -> new IngestCtxMap(new HashMap<>(), new Metadata(metadata, null)) + () -> new IngestCtxMap(new HashMap<>(), new IngestCtxMap.IngestMetadata(metadata, null)) ); assertEquals("Unexpected metadata keys [routing:myRouting, version:567]", err.getMessage()); } @@ -101,7 +103,7 @@ public void testPutSource() { Map metadata = new HashMap<>(); metadata.put("_version", 123); Map source = new HashMap<>(); - map = new IngestCtxMap(source, new Metadata(metadata, null)); + map = new IngestCtxMap(source, new IngestCtxMap.IngestMetadata(metadata, null)); } public void testRemove() { @@ -121,15 +123,15 @@ public void testRemove() { ) ) ); - String msg = "cannotRemove cannot be null or removed"; + String msg = "cannotRemove cannot be removed"; IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> map.remove(cannotRemove)); assertEquals(msg, err.getMessage()); err = expectThrows(IllegalArgumentException.class, () -> map.put(cannotRemove, null)); - assertEquals(msg, err.getMessage()); + assertEquals("cannotRemove cannot be null", err.getMessage()); err = expectThrows(IllegalArgumentException.class, () -> map.entrySet().iterator().next().setValue(null)); - assertEquals(msg, err.getMessage()); + assertEquals("cannotRemove cannot be null", err.getMessage()); err = expectThrows(IllegalArgumentException.class, () -> { Iterator> it = map.entrySet().iterator(); @@ -221,7 +223,7 @@ public void testEntryAndIterator() { } public void testContainsValue() { - map = new IngestCtxMap(Map.of("myField", "fieldValue"), new Metadata(Map.of("_version", 5678), null)); + map = new IngestCtxMap(Map.of("myField", "fieldValue"), new IngestCtxMap.IngestMetadata(Map.of("_version", 5678), null)); assertTrue(map.containsValue(5678)); assertFalse(map.containsValue(5679)); assertTrue(map.containsValue("fieldValue")); @@ -232,24 +234,24 @@ public void testValidators() { map = new IngestCtxMap("myIndex", "myId", 1234, "myRouting", VersionType.EXTERNAL, null, new HashMap<>()); md = map.getMetadata(); IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> map.put("_index", 555)); - assertEquals("_index must be null or a String but was [555] with type [java.lang.Integer]", err.getMessage()); + assertEquals("_index [555] is wrong type, expected assignable to [java.lang.String], not [java.lang.Integer]", err.getMessage()); assertEquals("myIndex", md.getIndex()); err = expectThrows(IllegalArgumentException.class, () -> map.put("_id", 555)); - assertEquals("_id must be null or a String but was [555] with type [java.lang.Integer]", err.getMessage()); + assertEquals("_id [555] is wrong type, expected assignable to [java.lang.String], not [java.lang.Integer]", err.getMessage()); assertEquals("myId", md.getId()); map.put("_id", "myId2"); assertEquals("myId2", md.getId()); err = expectThrows(IllegalArgumentException.class, () -> map.put("_routing", 555)); - assertEquals("_routing must be null or a String but was [555] with type [java.lang.Integer]", err.getMessage()); + assertEquals("_routing [555] is wrong type, expected assignable to [java.lang.String], not [java.lang.Integer]", err.getMessage()); assertEquals("myRouting", md.getRouting()); map.put("_routing", "myRouting2"); assertEquals("myRouting2", md.getRouting()); err = expectThrows(IllegalArgumentException.class, () -> map.put("_version", "five-five-five")); assertEquals( - "_version may only be set to an int or a long but was [five-five-five] with type [java.lang.String]", + "_version [five-five-five] is wrong type, expected assignable to [java.lang.Number], not [java.lang.String]", err.getMessage() ); assertEquals(1234, md.getVersion()); @@ -271,7 +273,7 @@ public void testValidators() { ); err = expectThrows(IllegalArgumentException.class, () -> map.put("_version_type", VersionType.EXTERNAL)); assertEquals( - "_version_type must be a null or one of [internal, external, external_gte] but was [EXTERNAL] with type" + "_version_type [EXTERNAL] is wrong type, expected assignable to [java.lang.String], not" + " [org.elasticsearch.index.VersionType$2]", err.getMessage() ); @@ -283,7 +285,10 @@ public void testValidators() { ); err = expectThrows(IllegalArgumentException.class, () -> map.put("_dynamic_templates", "5")); - assertEquals("_dynamic_templates must be a null or a Map but was [5] with type [java.lang.String]", err.getMessage()); + assertEquals( + "_dynamic_templates [5] is wrong type, expected assignable to [java.util.Map], not [java.lang.String]", + err.getMessage() + ); Map dt = Map.of("a", "b"); map.put("_dynamic_templates", dt); assertThat(dt, equalTo(md.getDynamicTemplates())); @@ -292,7 +297,7 @@ public void testValidators() { public void testHandlesAllVersionTypes() { Map mdRawMap = new HashMap<>(); mdRawMap.put("_version", 1234); - map = new IngestCtxMap(new HashMap<>(), new Metadata(mdRawMap, null)); + map = new IngestCtxMap(new HashMap<>(), new IngestCtxMap.IngestMetadata(mdRawMap, null)); md = map.getMetadata(); assertNull(md.getVersionType()); for (VersionType vt : VersionType.values()) { @@ -309,6 +314,13 @@ public void testHandlesAllVersionTypes() { assertNull(md.getVersionType()); } + public void testDefaultFieldPropertiesForAllMetadata() { + for (IngestDocument.Metadata m : IngestDocument.Metadata.values()) { + assertThat(IngestCtxMap.IngestMetadata.PROPERTIES, hasEntry(equalTo(m.getFieldName()), notNullValue())); + } + assertEquals(IngestDocument.Metadata.values().length, IngestCtxMap.IngestMetadata.PROPERTIES.size()); + } + private static class TestEntry implements Map.Entry { String key; Object value; diff --git a/server/src/test/java/org/elasticsearch/script/MetadataTests.java b/server/src/test/java/org/elasticsearch/script/MetadataTests.java index c3e9c599076b2..1b3d5574b496b 100644 --- a/server/src/test/java/org/elasticsearch/script/MetadataTests.java +++ b/server/src/test/java/org/elasticsearch/script/MetadataTests.java @@ -8,26 +8,14 @@ package org.elasticsearch.script; -import org.elasticsearch.ingest.IngestDocument; import org.elasticsearch.test.ESTestCase; import java.util.HashMap; import java.util.Map; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.hasEntry; -import static org.hamcrest.Matchers.notNullValue; - public class MetadataTests extends ESTestCase { Metadata md; - public void testDefaultFieldPropertiesForAllMetadata() { - for (IngestDocument.Metadata m : IngestDocument.Metadata.values()) { - assertThat(IngestCtxMap.IngestMetadata.PROPERTIES, hasEntry(equalTo(m.getFieldName()), notNullValue())); - } - assertEquals(IngestDocument.Metadata.values().length, IngestCtxMap.IngestMetadata.PROPERTIES.size()); - } - public void testGetString() { Map metadata = new HashMap<>(); metadata.put("a", "A"); @@ -39,7 +27,7 @@ public String toString() { }); metadata.put("c", null); metadata.put("d", 1234); - md = new TestMetadata(metadata, allowAllValidators("a", "b", "c", "d")); + md = new Metadata(metadata, allowAllValidators("a", "b", "c", "d")); assertNull(md.getString("c")); assertNull(md.getString("no key")); assertEquals("myToString()", md.getString("b")); @@ -53,10 +41,10 @@ public void testGetNumber() { metadata.put("b", Double.MAX_VALUE); metadata.put("c", "NaN"); metadata.put("d", null); - md = new TestMetadata(metadata, allowAllValidators("a", "b", "c", "d")); + md = new Metadata(metadata, allowAllValidators("a", "b", "c", "d")); assertEquals(Long.MAX_VALUE, md.getNumber("a")); assertEquals(Double.MAX_VALUE, md.getNumber("b")); - IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> md.getNumber("c")); + IllegalStateException err = expectThrows(IllegalStateException.class, () -> md.getNumber("c")); assertEquals("unexpected type for [c] with value [NaN], expected Number, got [java.lang.String]", err.getMessage()); assertNull(md.getNumber("d")); assertNull(md.getNumber("no key")); From 15328b18780faa248b472109ade03d9b391c2e66 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 11 Jul 2022 21:20:07 -0500 Subject: [PATCH 15/24] FieldProperty javadoc --- .../src/main/java/org/elasticsearch/script/Metadata.java | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/script/Metadata.java b/server/src/main/java/org/elasticsearch/script/Metadata.java index 09b655e64b26f..e2e7551e4cb9f 100644 --- a/server/src/main/java/org/elasticsearch/script/Metadata.java +++ b/server/src/main/java/org/elasticsearch/script/Metadata.java @@ -245,6 +245,13 @@ public enum MapOperation { REMOVE } + /** + * The properties of a metadata field. + * @param type - the class of the field. Updates must be assignable to this type. If null, no type checking is performed. + * @param nullable - can the field value be null and can it be removed + * @param writable - can the field be updated after the initial set + * @param extendedValidation - value validation after type checking, may be used for values that may be one of a set + */ public record FieldProperty (Class type, boolean nullable, boolean writable, BiConsumer extendedValidation) { public static BiConsumer LONGABLE_NUMBER = (k, v) -> { From 824cdc85cedfe2e66d42551550e051ab5c2212b9 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 11 Jul 2022 21:38:38 -0500 Subject: [PATCH 16/24] Implement getTimestamp() in IngestMetadata --- .../src/main/java/org/elasticsearch/ingest/IngestCtxMap.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java index 9e98d6386d2be..d1fddd11c3a9c 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java @@ -141,5 +141,10 @@ protected static Map metadataMap(String index, String id, long v } return metadata; } + + @Override + public ZonedDateTime getTimestamp() { + return timestamp; + } } } From c39bb9d1d0cf50684c115abdf0ed7d76b1c613ce Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 11 Jul 2022 22:15:57 -0500 Subject: [PATCH 17/24] Use extended validators, add hashcode and equals for Metadata --- .../ingest/common/RenameProcessorTests.java | 25 +++++++++++-------- .../org/elasticsearch/script/Metadata.java | 13 ++++++++++ .../ingest/TestIngestDocument.java | 5 +--- 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RenameProcessorTests.java b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RenameProcessorTests.java index 4cab0b999c248..5908fc8784d8f 100644 --- a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RenameProcessorTests.java +++ b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/RenameProcessorTests.java @@ -13,6 +13,7 @@ import org.elasticsearch.ingest.RandomDocumentPicks; import org.elasticsearch.ingest.TestIngestDocument; import org.elasticsearch.ingest.TestTemplateService; +import org.elasticsearch.script.Metadata; import org.elasticsearch.test.ESTestCase; import java.util.ArrayList; @@ -140,11 +141,14 @@ public void testRenameAtomicOperationSetFails() throws Exception { Map metadata = new HashMap<>(); metadata.put("list", Collections.singletonList("item")); - IngestDocument ingestDocument = TestIngestDocument.ofMetadataWithValidator(metadata, Map.of("new_field", (k, v) -> { - if (v != null) { - throw new UnsupportedOperationException(); - } - }, "list", (k, v) -> {})); + IngestDocument ingestDocument = TestIngestDocument.ofMetadataWithValidator( + metadata, + Map.of("new_field", new Metadata.FieldProperty<>(Object.class, true, true, (k, v) -> { + if (v != null) { + throw new UnsupportedOperationException(); + } + }), "list", new Metadata.FieldProperty<>(Object.class, true, true, null)) + ); Processor processor = createRenameProcessor("list", "new_field", false); try { processor.execute(ingestDocument); @@ -160,16 +164,15 @@ public void testRenameAtomicOperationRemoveFails() throws Exception { Map metadata = new HashMap<>(); metadata.put("list", Collections.singletonList("item")); - IngestDocument ingestDocument = TestIngestDocument.ofMetadataWithValidator(metadata, Map.of("list", (k, v) -> { - if (v == null) { - throw new UnsupportedOperationException(); - } - })); + IngestDocument ingestDocument = TestIngestDocument.ofMetadataWithValidator( + metadata, + Map.of("list", new Metadata.FieldProperty<>(Object.class, false, true, null)) + ); Processor processor = createRenameProcessor("list", "new_field", false); try { processor.execute(ingestDocument); fail("processor execute should have failed"); - } catch (UnsupportedOperationException e) { + } catch (IllegalArgumentException e) { // the set failed, the old field has not been removed assertThat(ingestDocument.getSourceAndMetadata().containsKey("list"), equalTo(true)); assertThat(ingestDocument.getSourceAndMetadata().containsKey("new_field"), equalTo(false)); diff --git a/server/src/main/java/org/elasticsearch/script/Metadata.java b/server/src/main/java/org/elasticsearch/script/Metadata.java index e2e7551e4cb9f..c435dff5ca7fa 100644 --- a/server/src/main/java/org/elasticsearch/script/Metadata.java +++ b/server/src/main/java/org/elasticsearch/script/Metadata.java @@ -233,6 +233,19 @@ public Map getMap() { return map; } + @Override + public boolean equals(Object o) { + if (this == o) return true; + if ((o instanceof Metadata) == false) return false; + Metadata metadata = (Metadata) o; + return map.equals(metadata.map); + } + + @Override + public int hashCode() { + return Objects.hash(map); + } + /** * The operation being performed on the value in the map. * INIT: Initial value - the metadata value as passed into this class diff --git a/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java b/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java index a9135b80ae790..9dbb74ea80608 100644 --- a/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java +++ b/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java @@ -17,7 +17,6 @@ import java.util.Arrays; import java.util.HashMap; import java.util.Map; -import java.util.function.BiConsumer; import java.util.stream.Collectors; /** @@ -61,9 +60,7 @@ public static IngestDocument withDefaultVersion(Map sourceAndMet * Create an IngestDocument with a metadata map and validators. The metadata map is passed by reference, not copied, so callers * can observe changes to the map directly. */ - public static IngestDocument ofMetadataWithValidator(Map metadata, Map> validators) { - Map> properties = new HashMap<>(); - validators.forEach((k, v) -> properties.put(k, new Metadata.FieldProperty<>(Object.class, true, true, v))); + public static IngestDocument ofMetadataWithValidator(Map metadata, Map> properties) { return new IngestDocument(new IngestCtxMap(new HashMap<>(), new TestIngestCtxMetadata(metadata, properties)), new HashMap<>()); } From e81f5fa0b32428681a2613bf35a9807c28a6027e Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 11 Jul 2022 23:16:12 -0500 Subject: [PATCH 18/24] _type can be updated --- server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java index d1fddd11c3a9c..6bb6003a96c5a 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java @@ -105,7 +105,7 @@ static class IngestMetadata extends Metadata { VERSION, new FieldProperty<>(Number.class, false, true, FieldProperty.LONGABLE_NUMBER), TYPE, - new FieldProperty<>(String.class, true, false, null), + new FieldProperty<>(String.class, true, true, null), IF_SEQ_NO, new FieldProperty<>(Number.class, true, true, FieldProperty.LONGABLE_NUMBER), IF_PRIMARY_TERM, From 7276bb0c3efada0cbd6e1c889aee58877b24d99a Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 11 Jul 2022 23:20:17 -0500 Subject: [PATCH 19/24] Fix testExecutePropagateAllMetadataUpdates rather than allow _type to be updateable --- server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java | 2 +- .../test/java/org/elasticsearch/ingest/IngestServiceTests.java | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java index 6bb6003a96c5a..d1fddd11c3a9c 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java @@ -105,7 +105,7 @@ static class IngestMetadata extends Metadata { VERSION, new FieldProperty<>(Number.class, false, true, FieldProperty.LONGABLE_NUMBER), TYPE, - new FieldProperty<>(String.class, true, true, null), + new FieldProperty<>(String.class, true, false, null), IF_SEQ_NO, new FieldProperty<>(Number.class, true, true, FieldProperty.LONGABLE_NUMBER), IF_PRIMARY_TERM, diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java index 6182e4709a6b8..7c64cf31f52aa 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestServiceTests.java @@ -1153,6 +1153,8 @@ public void testExecutePropagateAllMetadataUpdates() throws Exception { ingestDocument.setFieldValue(metadata.getFieldName(), ifPrimaryTerm); } else if (metadata == IngestDocument.Metadata.DYNAMIC_TEMPLATES) { ingestDocument.setFieldValue(metadata.getFieldName(), Map.of("foo", "bar")); + } else if (metadata == IngestDocument.Metadata.TYPE) { + // can't update _type } else { ingestDocument.setFieldValue(metadata.getFieldName(), "update" + metadata.getFieldName()); } From 9c7b3895f3c37a9616c5364be3c7c3369c72294b Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Tue, 12 Jul 2022 16:19:33 -0500 Subject: [PATCH 20/24] Remove getIngestSourceAndMetadata --- .../common/ScriptProcessorFactoryTests.java | 2 +- .../elasticsearch/ingest/IngestDocument.java | 7 ----- .../elasticsearch/script/TestMetadata.java | 27 ------------------- 3 files changed, 1 insertion(+), 35 deletions(-) delete mode 100644 test/framework/src/main/java/org/elasticsearch/script/TestMetadata.java diff --git a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/ScriptProcessorFactoryTests.java b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/ScriptProcessorFactoryTests.java index 6fb39fa0fb803..7476eb2216dc6 100644 --- a/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/ScriptProcessorFactoryTests.java +++ b/modules/ingest-common/src/test/java/org/elasticsearch/ingest/common/ScriptProcessorFactoryTests.java @@ -159,7 +159,7 @@ public void testInlineIsCompiled() throws Exception { assertThat(processor.getScript().getParams(), equalTo(Collections.emptyMap())); assertNotNull(processor.getPrecompiledIngestScriptFactory()); IngestDocument doc = TestIngestDocument.emptyIngestDocument(); - Map ctx = TestIngestDocument.emptyIngestDocument().getIngestSourceAndMetadata(); + Map ctx = TestIngestDocument.emptyIngestDocument().getSourceAndMetadata(); processor.getPrecompiledIngestScriptFactory().newInstance(null, doc.getMetadata(), ctx).execute(); assertThat(ctx.get("foo"), equalTo("bar")); } diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java index 81ecfa39bccd0..c80e477606b0b 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java @@ -718,13 +718,6 @@ public Map getSourceAndMetadata() { return sourceAndMetadata; } - /** - * Get source and metadata map as {@link IngestCtxMap} - */ - public IngestCtxMap getIngestSourceAndMetadata() { - return sourceAndMetadata; - } - /** * Get the strongly typed metadata */ diff --git a/test/framework/src/main/java/org/elasticsearch/script/TestMetadata.java b/test/framework/src/main/java/org/elasticsearch/script/TestMetadata.java deleted file mode 100644 index 87a45ea857549..0000000000000 --- a/test/framework/src/main/java/org/elasticsearch/script/TestMetadata.java +++ /dev/null @@ -1,27 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0 and the Server Side Public License, v 1; you may not use this file except - * in compliance with, at your election, the Elastic License 2.0 or the Server - * Side Public License, v 1. - */ - -package org.elasticsearch.script; - -import java.util.HashMap; -import java.util.Map; - -/** - * An implementation of {@link Metadata} with customizable {@link org.elasticsearch.script.Metadata.Validator}s for use in testing. - */ -public class TestMetadata extends Metadata { - public TestMetadata(Map map, Map validators) { - super(map, null, validators); - } - - public static TestMetadata withNullableVersion(Map map) { - Map updatedValidators = new HashMap<>(VALIDATORS); - updatedValidators.replace(VERSION, Metadata::longValidator); - return new TestMetadata(map, updatedValidators); - } -} From 5a45f97a1c5f3a472fcce1a066cfd8ff94651ecb Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Tue, 12 Jul 2022 18:13:15 -0500 Subject: [PATCH 21/24] Don't clone properties, add MetadataTests --- .../org/elasticsearch/script/Metadata.java | 5 +- .../elasticsearch/script/MetadataTests.java | 207 ++++++++++++++++++ 2 files changed, 210 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/script/Metadata.java b/server/src/main/java/org/elasticsearch/script/Metadata.java index c435dff5ca7fa..6750c79061475 100644 --- a/server/src/main/java/org/elasticsearch/script/Metadata.java +++ b/server/src/main/java/org/elasticsearch/script/Metadata.java @@ -52,7 +52,7 @@ public class Metadata { public Metadata(Map map, Map> properties) { this.map = map; - this.properties = properties; + this.properties = Collections.unmodifiableMap(properties); validateMetadata(); } @@ -223,7 +223,8 @@ public int size() { @Override public Metadata clone() { - return new Metadata(new HashMap<>(map), new HashMap<>(properties)); + // properties is an UnmodifiableMap, no need to create a copy + return new Metadata(new HashMap<>(map), properties); } /** diff --git a/server/src/test/java/org/elasticsearch/script/MetadataTests.java b/server/src/test/java/org/elasticsearch/script/MetadataTests.java index 1b3d5574b496b..843c5a6c1c97c 100644 --- a/server/src/test/java/org/elasticsearch/script/MetadataTests.java +++ b/server/src/test/java/org/elasticsearch/script/MetadataTests.java @@ -10,11 +10,14 @@ import org.elasticsearch.test.ESTestCase; +import java.util.Collections; import java.util.HashMap; import java.util.Map; +import java.util.Set; public class MetadataTests extends ESTestCase { Metadata md; + private static final Metadata.FieldProperty STRING_PROP = new Metadata.FieldProperty<>(String.class, true, true, null); public void testGetString() { Map metadata = new HashMap<>(); @@ -57,4 +60,208 @@ private static Map> allowAllValidators(String. } return validators; } + + public void testValidateMetadata() { + IllegalArgumentException err = expectThrows( + IllegalArgumentException.class, + () -> new Metadata(Map.of("foo", 1), Map.of("foo", STRING_PROP)) + ); + assertEquals("foo [1] is wrong type, expected assignable to [java.lang.String], not [java.lang.Integer]", err.getMessage()); + + err = expectThrows( + IllegalArgumentException.class, + () -> new Metadata(Map.of("foo", "abc", "bar", "def"), Map.of("foo", STRING_PROP)) + ); + assertEquals("Unexpected metadata keys [bar:def]", err.getMessage()); + } + + public void testIsAvailable() { + md = new Metadata(Map.of("bar", "baz"), Map.of("foo", STRING_PROP, "bar", STRING_PROP)); + assertTrue(md.isAvailable("bar")); + assertTrue(md.isAvailable("foo")); + } + + public void testPut() { + md = new Metadata(new HashMap<>(Map.of("foo", "bar")), Map.of("foo", STRING_PROP)); + assertEquals("bar", md.get("foo")); + md.put("foo", "baz"); + assertEquals("baz", md.get("foo")); + md.put("foo", null); + assertNull(md.get("foo")); + IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> md.put("foo", 1)); + assertEquals("foo [1] is wrong type, expected assignable to [java.lang.String], not [java.lang.Integer]", err.getMessage()); + } + + public void testContainsKey() { + md = new Metadata(new HashMap<>(Map.of("foo", "bar")), Map.of("foo", STRING_PROP, "baz", STRING_PROP)); + assertTrue(md.containsKey("foo")); + assertFalse(md.containsKey("baz")); + assertTrue(md.isAvailable("baz")); + } + + public void testContainsValue() { + md = new Metadata(new HashMap<>(Map.of("foo", "bar")), Map.of("foo", STRING_PROP, "baz", STRING_PROP)); + assertTrue(md.containsValue("bar")); + assertFalse(md.containsValue("foo")); + } + + public void testRemove() { + md = new Metadata( + new HashMap<>(Map.of("foo", "bar", "baz", "qux")), + Map.of("foo", STRING_PROP, "baz", new Metadata.FieldProperty<>(String.class, false, true, null)) + ); + assertTrue(md.containsKey("foo")); + md.remove("foo"); + assertFalse(md.containsKey("foo")); + + assertTrue(md.containsKey("baz")); + IllegalArgumentException err = expectThrows(IllegalArgumentException.class, () -> md.remove("baz")); + assertEquals("baz cannot be removed", err.getMessage()); + } + + public void testKeySetAndSize() { + md = new Metadata(new HashMap<>(Map.of("foo", "bar", "baz", "qux")), Map.of("foo", STRING_PROP, "baz", STRING_PROP)); + Set expected = Set.of("foo", "baz"); + assertEquals(expected, md.keySet()); + assertEquals(2, md.size()); + md.remove("foo"); + assertEquals(Set.of("baz"), md.keySet()); + assertEquals(1, md.size()); + md.put("foo", "abc"); + assertEquals(expected, md.keySet()); + assertEquals(2, md.size()); + md.remove("foo"); + md.remove("baz"); + assertEquals(Collections.emptySet(), md.keySet()); + assertEquals(0, md.size()); + } + + public void testTestClone() { + md = new Metadata(new HashMap<>(Map.of("foo", "bar", "baz", "qux")), Map.of("foo", STRING_PROP, "baz", STRING_PROP)); + Metadata md2 = md.clone(); + md2.remove("foo"); + md.remove("baz"); + assertEquals("bar", md.get("foo")); + assertNull(md2.get("foo")); + assertNull(md.get("baz")); + assertEquals("qux", md2.get("baz")); + } + + public void testFieldPropertyType() { + Metadata.FieldProperty aProp = new Metadata.FieldProperty<>(A.class, true, true, null); + aProp.check(Metadata.MapOperation.UPDATE, "a", new A()); + aProp.check(Metadata.MapOperation.INIT, "a", new A()); + aProp.check(Metadata.MapOperation.UPDATE, "a", new B()); + aProp.check(Metadata.MapOperation.INIT, "a", new B()); + + IllegalArgumentException err = expectThrows( + IllegalArgumentException.class, + () -> aProp.check(Metadata.MapOperation.UPDATE, "a", new C()) + ); + String expected = "a [I'm C] is wrong type, expected assignable to [org.elasticsearch.script.MetadataTests$A], not" + + " [org.elasticsearch.script.MetadataTests$C]"; + assertEquals(expected, err.getMessage()); + err = expectThrows(IllegalArgumentException.class, () -> aProp.check(Metadata.MapOperation.INIT, "a", new C())); + assertEquals(expected, err.getMessage()); + } + + static class A {} + + static class B extends A {} + + static class C { + @Override + public String toString() { + return "I'm C"; + } + } + + public void testFieldPropertyNullable() { + Metadata.FieldProperty cantNull = new Metadata.FieldProperty<>(String.class, false, true, null); + Metadata.FieldProperty canNull = new Metadata.FieldProperty<>(String.class, true, true, null); + + IllegalArgumentException err; + { + Metadata.MapOperation op = Metadata.MapOperation.INIT; + err = expectThrows(IllegalArgumentException.class, () -> cantNull.check(op, "a", null)); + assertEquals("a cannot be null", err.getMessage()); + canNull.check(op, "a", null); + } + + { + Metadata.MapOperation op = Metadata.MapOperation.UPDATE; + err = expectThrows(IllegalArgumentException.class, () -> cantNull.check(op, "a", null)); + assertEquals("a cannot be null", err.getMessage()); + canNull.check(Metadata.MapOperation.UPDATE, "a", null); + } + + { + Metadata.MapOperation op = Metadata.MapOperation.REMOVE; + err = expectThrows(IllegalArgumentException.class, () -> cantNull.check(op, "a", null)); + assertEquals("a cannot be removed", err.getMessage()); + canNull.check(Metadata.MapOperation.REMOVE, "a", null); + } + } + + public void testFieldPropertyWritable() { + Metadata.FieldProperty writable = new Metadata.FieldProperty<>(String.class, true, true, null); + Metadata.FieldProperty readonly = new Metadata.FieldProperty<>(String.class, true, false, null); + + String key = "a"; + String value = "abc"; + + IllegalArgumentException err; + { + Metadata.MapOperation op = Metadata.MapOperation.INIT; + writable.check(op, key, value); + readonly.check(op, key, value); + } + + { + Metadata.MapOperation op = Metadata.MapOperation.UPDATE; + err = expectThrows(IllegalArgumentException.class, () -> readonly.check(op, key, value)); + assertEquals("a cannot be updated", err.getMessage()); + writable.check(op, key, value); + } + + { + Metadata.MapOperation op = Metadata.MapOperation.REMOVE; + err = expectThrows(IllegalArgumentException.class, () -> readonly.check(op, key, value)); + assertEquals("a cannot be removed", err.getMessage()); + writable.check(op, key, value); + } + } + + public void testFieldPropertyExtendedValidation() { + Metadata.FieldProperty any = new Metadata.FieldProperty<>(Number.class, true, true, null); + Metadata.FieldProperty odd = new Metadata.FieldProperty<>(Number.class, true, true, (k, v) -> { + if (v.intValue() % 2 == 0) { + throw new IllegalArgumentException("not odd"); + } + }); + + String key = "a"; + int value = 2; + + IllegalArgumentException err; + { + Metadata.MapOperation op = Metadata.MapOperation.INIT; + any.check(op, key, value); + err = expectThrows(IllegalArgumentException.class, () -> odd.check(op, key, value)); + assertEquals("not odd", err.getMessage()); + } + + { + Metadata.MapOperation op = Metadata.MapOperation.UPDATE; + any.check(op, key, value); + err = expectThrows(IllegalArgumentException.class, () -> odd.check(op, key, value)); + assertEquals("not odd", err.getMessage()); + } + + { + Metadata.MapOperation op = Metadata.MapOperation.REMOVE; + any.check(op, key, value); + odd.check(op, key, value); + } + } } From dc1fc58051c251aa78456a721affbb5bb67171f3 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Wed, 13 Jul 2022 10:01:35 -0500 Subject: [PATCH 22/24] IngestMetadata -> IngestDocMetadata, remove splitSourceAndMetadta, fix validate javadoc --- .../elasticsearch/ingest/IngestCtxMap.java | 76 +--------------- .../ingest/IngestDocMetadata.java | 90 +++++++++++++++++++ .../elasticsearch/ingest/IngestDocument.java | 30 +++---- .../java/org/elasticsearch/script/CtxMap.java | 24 ----- .../org/elasticsearch/script/Metadata.java | 2 +- .../ingest/IngestCtxMapTests.java | 14 +-- .../ingest/TestIngestCtxMetadata.java | 2 +- .../ingest/TestIngestDocument.java | 17 ++-- 8 files changed, 125 insertions(+), 130 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/ingest/IngestDocMetadata.java diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java index d1fddd11c3a9c..b648051669567 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java @@ -8,16 +8,13 @@ package org.elasticsearch.ingest; -import org.elasticsearch.common.util.Maps; import org.elasticsearch.index.VersionType; import org.elasticsearch.script.CtxMap; import org.elasticsearch.script.Metadata; import java.time.ZonedDateTime; -import java.util.Arrays; import java.util.HashMap; import java.util.Map; -import java.util.stream.Collectors; /** * Map containing ingest source and metadata. @@ -46,7 +43,7 @@ class IngestCtxMap extends CtxMap { ZonedDateTime timestamp, Map source ) { - super(new HashMap<>(source), new IngestMetadata(index, id, version, routing, versionType, timestamp)); + super(new HashMap<>(source), new IngestDocMetadata(index, id, version, routing, versionType, timestamp)); } /** @@ -76,75 +73,4 @@ public static ZonedDateTime getTimestamp(Map ingestMetadata) { return null; } - static class IngestMetadata extends Metadata { - private static final FieldProperty UPDATABLE_STRING = new FieldProperty<>(String.class, true, true, null); - static final Map> PROPERTIES = Map.of( - INDEX, - UPDATABLE_STRING, - ID, - UPDATABLE_STRING, - ROUTING, - UPDATABLE_STRING, - VERSION_TYPE, - new FieldProperty<>(String.class, true, true, (k, v) -> { - try { - VersionType.fromString(v); - return; - } catch (IllegalArgumentException ignored) {} - throw new IllegalArgumentException( - k - + " must be a null or one of [" - + Arrays.stream(VersionType.values()).map(vt -> VersionType.toString(vt)).collect(Collectors.joining(", ")) - + "] but was [" - + v - + "] with type [" - + v.getClass().getName() - + "]" - ); - }), - VERSION, - new FieldProperty<>(Number.class, false, true, FieldProperty.LONGABLE_NUMBER), - TYPE, - new FieldProperty<>(String.class, true, false, null), - IF_SEQ_NO, - new FieldProperty<>(Number.class, true, true, FieldProperty.LONGABLE_NUMBER), - IF_PRIMARY_TERM, - new FieldProperty<>(Number.class, true, true, FieldProperty.LONGABLE_NUMBER), - DYNAMIC_TEMPLATES, - new FieldProperty<>(Map.class, true, true, null) - ); - - protected final ZonedDateTime timestamp; - - IngestMetadata(String index, String id, long version, String routing, VersionType versionType, ZonedDateTime timestamp) { - this(metadataMap(index, id, version, routing, versionType), timestamp); - } - - IngestMetadata(Map metadata, ZonedDateTime timestamp) { - super(metadata, PROPERTIES); - this.timestamp = timestamp; - } - - /** - * Create the backing metadata map with the standard contents assuming default validators. - */ - protected static Map metadataMap(String index, String id, long version, String routing, VersionType versionType) { - Map metadata = Maps.newHashMapWithExpectedSize(IngestDocument.Metadata.values().length); - metadata.put(IngestDocument.Metadata.INDEX.getFieldName(), index); - metadata.put(IngestDocument.Metadata.ID.getFieldName(), id); - metadata.put(IngestDocument.Metadata.VERSION.getFieldName(), version); - if (routing != null) { - metadata.put(IngestDocument.Metadata.ROUTING.getFieldName(), routing); - } - if (versionType != null) { - metadata.put(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), VersionType.toString(versionType)); - } - return metadata; - } - - @Override - public ZonedDateTime getTimestamp() { - return timestamp; - } - } } diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocMetadata.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocMetadata.java new file mode 100644 index 0000000000000..0897f1a3175e4 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocMetadata.java @@ -0,0 +1,90 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.ingest; + +import org.elasticsearch.common.util.Maps; +import org.elasticsearch.index.VersionType; +import org.elasticsearch.script.Metadata; + +import java.time.ZonedDateTime; +import java.util.Arrays; +import java.util.Map; +import java.util.stream.Collectors; + +class IngestDocMetadata extends Metadata { + private static final FieldProperty UPDATABLE_STRING = new FieldProperty<>(String.class, true, true, null); + static final Map> PROPERTIES = Map.of( + INDEX, + UPDATABLE_STRING, + ID, + UPDATABLE_STRING, + ROUTING, + UPDATABLE_STRING, + VERSION_TYPE, + new FieldProperty<>(String.class, true, true, (k, v) -> { + try { + VersionType.fromString(v); + return; + } catch (IllegalArgumentException ignored) {} + throw new IllegalArgumentException( + k + + " must be a null or one of [" + + Arrays.stream(VersionType.values()).map(vt -> VersionType.toString(vt)).collect(Collectors.joining(", ")) + + "] but was [" + + v + + "] with type [" + + v.getClass().getName() + + "]" + ); + }), + VERSION, + new FieldProperty<>(Number.class, false, true, FieldProperty.LONGABLE_NUMBER), + TYPE, + new FieldProperty<>(String.class, true, false, null), + IF_SEQ_NO, + new FieldProperty<>(Number.class, true, true, FieldProperty.LONGABLE_NUMBER), + IF_PRIMARY_TERM, + new FieldProperty<>(Number.class, true, true, FieldProperty.LONGABLE_NUMBER), + DYNAMIC_TEMPLATES, + new FieldProperty<>(Map.class, true, true, null) + ); + + protected final ZonedDateTime timestamp; + + IngestDocMetadata(String index, String id, long version, String routing, VersionType versionType, ZonedDateTime timestamp) { + this(metadataMap(index, id, version, routing, versionType), timestamp); + } + + IngestDocMetadata(Map metadata, ZonedDateTime timestamp) { + super(metadata, PROPERTIES); + this.timestamp = timestamp; + } + + /** + * Create the backing metadata map with the standard contents assuming default validators. + */ + protected static Map metadataMap(String index, String id, long version, String routing, VersionType versionType) { + Map metadata = Maps.newHashMapWithExpectedSize(IngestDocument.Metadata.values().length); + metadata.put(IngestDocument.Metadata.INDEX.getFieldName(), index); + metadata.put(IngestDocument.Metadata.ID.getFieldName(), id); + metadata.put(IngestDocument.Metadata.VERSION.getFieldName(), version); + if (routing != null) { + metadata.put(IngestDocument.Metadata.ROUTING.getFieldName(), routing); + } + if (versionType != null) { + metadata.put(IngestDocument.Metadata.VERSION_TYPE.getFieldName(), VersionType.toString(versionType)); + } + return metadata; + } + + @Override + public ZonedDateTime getTimestamp() { + return timestamp; + } +} diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java index c80e477606b0b..715ba748e6049 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestDocument.java @@ -12,7 +12,6 @@ import org.elasticsearch.common.util.LazyMap; import org.elasticsearch.common.util.Maps; import org.elasticsearch.common.util.set.Sets; -import org.elasticsearch.core.Tuple; import org.elasticsearch.index.VersionType; import org.elasticsearch.index.mapper.IdFieldMapper; import org.elasticsearch.index.mapper.IndexFieldMapper; @@ -77,21 +76,22 @@ public IngestDocument(IngestDocument other) { * Constructor to create an IngestDocument from its constituent maps. The maps are shallow copied. */ public IngestDocument(Map sourceAndMetadata, Map ingestMetadata) { - Tuple, Map> sm = IngestCtxMap.splitSourceAndMetadata( - sourceAndMetadata, - Arrays.stream(IngestDocument.Metadata.values()).map(IngestDocument.Metadata::getFieldName).collect(Collectors.toSet()) - ); - this.sourceAndMetadata = new IngestCtxMap( - sm.v1(), - new IngestCtxMap.IngestMetadata(sm.v2(), IngestCtxMap.getTimestamp(ingestMetadata)) - ); - this.ingestMetadata = new HashMap<>(ingestMetadata); - this.ingestMetadata.computeIfPresent(TIMESTAMP, (k, v) -> { - if (v instanceof String) { - return this.sourceAndMetadata.getMetadata().getTimestamp(); + Map source; + Map metadata; + if (sourceAndMetadata instanceof IngestCtxMap ingestCtxMap) { + source = new HashMap<>(ingestCtxMap.getSource()); + metadata = new HashMap<>(ingestCtxMap.getMetadata().getMap()); + } else { + metadata = Maps.newHashMapWithExpectedSize(Metadata.METADATA_NAMES.size()); + source = new HashMap<>(sourceAndMetadata); + for (String key : Metadata.METADATA_NAMES) { + if (sourceAndMetadata.containsKey(key)) { + metadata.put(key, source.remove(key)); + } } - return v; - }); + } + this.ingestMetadata = new HashMap<>(ingestMetadata); + this.sourceAndMetadata = new IngestCtxMap(source, new IngestDocMetadata(metadata, IngestCtxMap.getTimestamp(ingestMetadata))); } /** diff --git a/server/src/main/java/org/elasticsearch/script/CtxMap.java b/server/src/main/java/org/elasticsearch/script/CtxMap.java index 7301052a27a7e..d66514127043a 100644 --- a/server/src/main/java/org/elasticsearch/script/CtxMap.java +++ b/server/src/main/java/org/elasticsearch/script/CtxMap.java @@ -8,9 +8,7 @@ package org.elasticsearch.script; -import org.elasticsearch.common.util.Maps; import org.elasticsearch.common.util.set.Sets; -import org.elasticsearch.core.Tuple; import java.util.AbstractCollection; import java.util.AbstractMap; @@ -52,28 +50,6 @@ protected CtxMap(Map source, Metadata metadata) { } } - /** - * Returns a new metadata map and the existing source map with metadata removed. - */ - public static Tuple, Map> splitSourceAndMetadata( - Map sourceAndMetadata, - Set metadataKeys - ) { - if (sourceAndMetadata instanceof CtxMap ctxMap) { - return new Tuple<>(new HashMap<>(ctxMap.source), new HashMap<>(ctxMap.metadata.getMap())); - } - - Map metadata = Maps.newHashMapWithExpectedSize(metadataKeys.size()); - Map source = new HashMap<>(sourceAndMetadata); - - for (String metadataKey : metadataKeys) { - if (sourceAndMetadata.containsKey(metadataKey)) { - metadata.put(metadataKey, source.remove(metadataKey)); - } - } - return new Tuple<>(source, metadata); - } - /** * get the source map, if externally modified then the guarantees of this class are not enforced */ diff --git a/server/src/main/java/org/elasticsearch/script/Metadata.java b/server/src/main/java/org/elasticsearch/script/Metadata.java index 6750c79061475..f84e6d5502b61 100644 --- a/server/src/main/java/org/elasticsearch/script/Metadata.java +++ b/server/src/main/java/org/elasticsearch/script/Metadata.java @@ -57,7 +57,7 @@ public Metadata(Map map, Map> propertie } /** - * Check that all metadata map contains only valid metadata and no extraneous keys and source map contains no metadata + * Check that all metadata map contains only valid metadata and no extraneous keys */ protected void validateMetadata() { int numMetadata = 0; diff --git a/server/src/test/java/org/elasticsearch/ingest/IngestCtxMapTests.java b/server/src/test/java/org/elasticsearch/ingest/IngestCtxMapTests.java index 3327d30420478..dab30f6c13b91 100644 --- a/server/src/test/java/org/elasticsearch/ingest/IngestCtxMapTests.java +++ b/server/src/test/java/org/elasticsearch/ingest/IngestCtxMapTests.java @@ -36,7 +36,7 @@ public void testSettersAndGetters() { metadata.put("_if_primary_term", 10000); metadata.put("_version_type", "internal"); metadata.put("_dynamic_templates", Map.of("foo", "bar")); - map = new IngestCtxMap(new HashMap<>(), new IngestCtxMap.IngestMetadata(metadata, null)); + map = new IngestCtxMap(new HashMap<>(), new IngestDocMetadata(metadata, null)); md = map.getMetadata(); assertEquals("myIndex", md.getIndex()); md.setIndex("myIndex2"); @@ -69,7 +69,7 @@ public void testInvalidMetadata() { metadata.put("_version", Double.MAX_VALUE); IllegalArgumentException err = expectThrows( IllegalArgumentException.class, - () -> new IngestCtxMap(new HashMap<>(), new IngestCtxMap.IngestMetadata(metadata, null)) + () -> new IngestCtxMap(new HashMap<>(), new IngestDocMetadata(metadata, null)) ); assertThat(err.getMessage(), containsString("_version may only be set to an int or a long but was [")); assertThat(err.getMessage(), containsString("] with type [java.lang.Double]")); @@ -80,7 +80,7 @@ public void testSourceInMetadata() { source.put("_version", 25); IllegalArgumentException err = expectThrows( IllegalArgumentException.class, - () -> new IngestCtxMap(source, new IngestCtxMap.IngestMetadata(source, null)) + () -> new IngestCtxMap(source, new IngestDocMetadata(source, null)) ); assertEquals("unexpected metadata [_version:25] in source", err.getMessage()); } @@ -92,7 +92,7 @@ public void testExtraMetadata() { metadata.put("routing", "myRouting"); IllegalArgumentException err = expectThrows( IllegalArgumentException.class, - () -> new IngestCtxMap(new HashMap<>(), new IngestCtxMap.IngestMetadata(metadata, null)) + () -> new IngestCtxMap(new HashMap<>(), new IngestDocMetadata(metadata, null)) ); assertEquals("Unexpected metadata keys [routing:myRouting, version:567]", err.getMessage()); } @@ -101,7 +101,7 @@ public void testPutSource() { Map metadata = new HashMap<>(); metadata.put("_version", 123); Map source = new HashMap<>(); - map = new IngestCtxMap(source, new IngestCtxMap.IngestMetadata(metadata, null)); + map = new IngestCtxMap(source, new IngestDocMetadata(metadata, null)); } public void testRemove() { @@ -221,7 +221,7 @@ public void testEntryAndIterator() { } public void testContainsValue() { - map = new IngestCtxMap(Map.of("myField", "fieldValue"), new IngestCtxMap.IngestMetadata(Map.of("_version", 5678), null)); + map = new IngestCtxMap(Map.of("myField", "fieldValue"), new IngestDocMetadata(Map.of("_version", 5678), null)); assertTrue(map.containsValue(5678)); assertFalse(map.containsValue(5679)); assertTrue(map.containsValue("fieldValue")); @@ -295,7 +295,7 @@ public void testValidators() { public void testHandlesAllVersionTypes() { Map mdRawMap = new HashMap<>(); mdRawMap.put("_version", 1234); - map = new IngestCtxMap(new HashMap<>(), new IngestCtxMap.IngestMetadata(mdRawMap, null)); + map = new IngestCtxMap(new HashMap<>(), new IngestDocMetadata(mdRawMap, null)); md = map.getMetadata(); assertNull(md.getVersionType()); for (VersionType vt : VersionType.values()) { diff --git a/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestCtxMetadata.java b/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestCtxMetadata.java index 2040fa5ee9322..1e83eaca5c55e 100644 --- a/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestCtxMetadata.java +++ b/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestCtxMetadata.java @@ -19,7 +19,7 @@ public TestIngestCtxMetadata(Map map, Map map) { - Map> updatedProperties = new HashMap<>(IngestCtxMap.IngestMetadata.PROPERTIES); + Map> updatedProperties = new HashMap<>(IngestDocMetadata.PROPERTIES); updatedProperties.replace(VERSION, new Metadata.FieldProperty<>(Number.class, true, true, FieldProperty.LONGABLE_NUMBER)); return new TestIngestCtxMetadata(map, updatedProperties); } diff --git a/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java b/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java index 9dbb74ea80608..3998cf6db1aa5 100644 --- a/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java +++ b/test/framework/src/main/java/org/elasticsearch/ingest/TestIngestDocument.java @@ -9,15 +9,14 @@ package org.elasticsearch.ingest; import org.elasticsearch.common.lucene.uid.Versions; +import org.elasticsearch.common.util.Maps; import org.elasticsearch.core.Tuple; import org.elasticsearch.index.VersionType; import org.elasticsearch.script.Metadata; import org.elasticsearch.test.ESTestCase; -import java.util.Arrays; import java.util.HashMap; import java.util.Map; -import java.util.stream.Collectors; /** * Construct ingest documents for testing purposes @@ -38,11 +37,15 @@ public static IngestDocument withNullableVersion(Map sourceAndMe * _versions. Normally null _version is not allowed, but many tests don't care about that invariant. */ public static IngestDocument ofIngestWithNullableVersion(Map sourceAndMetadata, Map ingestMetadata) { - Tuple, Map> sm = IngestCtxMap.splitSourceAndMetadata( - sourceAndMetadata, - Arrays.stream(IngestDocument.Metadata.values()).map(IngestDocument.Metadata::getFieldName).collect(Collectors.toSet()) - ); - return new IngestDocument(new IngestCtxMap(sm.v1(), TestIngestCtxMetadata.withNullableVersion(sm.v2())), ingestMetadata); + Map source = new HashMap<>(sourceAndMetadata); + Map metadata = Maps.newHashMapWithExpectedSize(IngestDocument.Metadata.values().length); + for (IngestDocument.Metadata m : IngestDocument.Metadata.values()) { + String key = m.getFieldName(); + if (sourceAndMetadata.containsKey(key)) { + metadata.put(key, source.remove(key)); + } + } + return new IngestDocument(new IngestCtxMap(source, TestIngestCtxMetadata.withNullableVersion(metadata)), ingestMetadata); } /** From a08afb8d9a0d201709ec8594e892dd77162c324e Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Wed, 13 Jul 2022 13:03:49 -0500 Subject: [PATCH 23/24] Don't copy source --- .../main/java/org/elasticsearch/ingest/IngestCtxMap.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java index b648051669567..e3cdc13e1c11e 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java @@ -13,7 +13,6 @@ import org.elasticsearch.script.Metadata; import java.time.ZonedDateTime; -import java.util.HashMap; import java.util.Map; /** @@ -32,7 +31,9 @@ class IngestCtxMap extends CtxMap { /** - * Create an IngestCtxMap with the given metadata, source and default validators + * Create an IngestCtxMap with the given metadata, source and default validators. + * + * Source is not copied, callers can observe changes to source. */ IngestCtxMap( String index, @@ -43,7 +44,7 @@ class IngestCtxMap extends CtxMap { ZonedDateTime timestamp, Map source ) { - super(new HashMap<>(source), new IngestDocMetadata(index, id, version, routing, versionType, timestamp)); + super(source, new IngestDocMetadata(index, id, version, routing, versionType, timestamp)); } /** From 8eb42c6f46f7c8a3590c7039c00800ed416c2d52 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Wed, 13 Jul 2022 13:50:16 -0500 Subject: [PATCH 24/24] Revert "Don't copy source" This reverts commit a08afb8d9a0d201709ec8594e892dd77162c324e. --- .../main/java/org/elasticsearch/ingest/IngestCtxMap.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java index e3cdc13e1c11e..b648051669567 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestCtxMap.java @@ -13,6 +13,7 @@ import org.elasticsearch.script.Metadata; import java.time.ZonedDateTime; +import java.util.HashMap; import java.util.Map; /** @@ -31,9 +32,7 @@ class IngestCtxMap extends CtxMap { /** - * Create an IngestCtxMap with the given metadata, source and default validators. - * - * Source is not copied, callers can observe changes to source. + * Create an IngestCtxMap with the given metadata, source and default validators */ IngestCtxMap( String index, @@ -44,7 +43,7 @@ class IngestCtxMap extends CtxMap { ZonedDateTime timestamp, Map source ) { - super(source, new IngestDocMetadata(index, id, version, routing, versionType, timestamp)); + super(new HashMap<>(source), new IngestDocMetadata(index, id, version, routing, versionType, timestamp)); } /**