From bf2085f96243afdcc811e293da12813fd55e8760 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Fri, 8 Jul 2022 15:18:44 -0500 Subject: [PATCH 01/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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 bdf6bfdfd04fd8153720cd11995aa7c785105ad4 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 11 Jul 2022 18:07:07 -0500 Subject: [PATCH 08/11] 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 09/11] 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 dd8b711a7cbec72eb0e28f6da8c4d0a15ce35ee5 Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Mon, 11 Jul 2022 23:34:30 -0500 Subject: [PATCH 10/11] Avoid ConcurrentModificationException by copying keySet for EntrySet --- .../org/elasticsearch/ingest/IngestSourceAndMetadata.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java b/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java index a041891d51371..16a79d6c7f074 100644 --- a/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java +++ b/server/src/main/java/org/elasticsearch/ingest/IngestSourceAndMetadata.java @@ -20,6 +20,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; @@ -132,7 +133,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())); } /** From 3cfdbbb3f570959665cb5169df916b08acbd182c Mon Sep 17 00:00:00 2001 From: Stuart Tettemer Date: Tue, 12 Jul 2022 05:51:06 -0500 Subject: [PATCH 11/11] IllegalArgumentException -> IllegalStateException for bad type in MetadataTests.testGetNumber --- .../src/test/java/org/elasticsearch/script/MetadataTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/script/MetadataTests.java b/server/src/test/java/org/elasticsearch/script/MetadataTests.java index 068c15ac1470f..5469bd91ec9f4 100644 --- a/server/src/test/java/org/elasticsearch/script/MetadataTests.java +++ b/server/src/test/java/org/elasticsearch/script/MetadataTests.java @@ -56,7 +56,7 @@ public void testGetNumber() { 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")); + 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"));