From c4512c0a5f272d3bc8356ff184d6679e5796afc6 Mon Sep 17 00:00:00 2001 From: Przemyslaw Witek Date: Tue, 26 May 2020 14:29:00 +0200 Subject: [PATCH 1/6] Introduce Annotation.event field --- .../xpack/core/ml/annotations/Annotation.java | 91 ++++++++++++++++--- .../core/ml/annotations_index_mappings.json | 3 + .../core/ml/annotations/AnnotationTests.java | 3 +- .../xpack/ml/datafeed/DatafeedJob.java | 3 +- .../output/AutodetectResultProcessor.java | 3 +- .../xpack/ml/datafeed/DatafeedJobTests.java | 6 +- .../AutodetectResultProcessorTests.java | 3 +- 7 files changed, 94 insertions(+), 18 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java index 8136487b91ad5..34b2f205b9d18 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java @@ -14,15 +14,44 @@ import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.ToXContentObject; import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.xpack.core.common.time.TimeUtils; import org.elasticsearch.xpack.core.ml.job.config.Job; import java.io.IOException; import java.util.Date; +import java.util.Locale; import java.util.Objects; public class Annotation implements ToXContentObject, Writeable { + public enum Type { + ANNOTATION; + + public static Type fromString(String value) { + return valueOf(value.toUpperCase(Locale.ROOT)); + } + + @Override + public String toString() { + return name().toLowerCase(Locale.ROOT); + } + } + + public enum Event { + DELAYED_DATA, + MODEL_SNAPSHOT_STORED; + + public static Event fromString(String value) { + return valueOf(value.toUpperCase(Locale.ROOT)); + } + + @Override + public String toString() { + return name().toLowerCase(Locale.ROOT); + } + } + public static final ParseField ANNOTATION = new ParseField("annotation"); public static final ParseField CREATE_TIME = new ParseField("create_time"); public static final ParseField CREATE_USERNAME = new ParseField("create_username"); @@ -31,6 +60,7 @@ public class Annotation implements ToXContentObject, Writeable { public static final ParseField MODIFIED_TIME = new ParseField("modified_time"); public static final ParseField MODIFIED_USERNAME = new ParseField("modified_username"); public static final ParseField TYPE = new ParseField("type"); + public static final ParseField EVENT = new ParseField("event"); public static final ParseField DETECTOR_INDEX = new ParseField("detector_index"); public static final ParseField PARTITION_FIELD_NAME = new ParseField("partition_field_name"); public static final ParseField PARTITION_FIELD_VALUE = new ParseField("partition_field_value"); @@ -39,7 +69,7 @@ public class Annotation implements ToXContentObject, Writeable { public static final ParseField BY_FIELD_NAME = new ParseField("by_field_name"); public static final ParseField BY_FIELD_VALUE = new ParseField("by_field_value"); - public static final ObjectParser PARSER = new ObjectParser<>(TYPE.getPreferredName(), true, Builder::new); + public static final ObjectParser PARSER = new ObjectParser<>(ANNOTATION.getPreferredName(), true, Builder::new); static { PARSER.declareString(Builder::setAnnotation, ANNOTATION); @@ -54,7 +84,18 @@ public class Annotation implements ToXContentObject, Writeable { PARSER.declareField(Builder::setModifiedTime, p -> TimeUtils.parseTimeField(p, MODIFIED_TIME.getPreferredName()), MODIFIED_TIME, ObjectParser.ValueType.VALUE); PARSER.declareString(Builder::setModifiedUsername, MODIFIED_USERNAME); - PARSER.declareString(Builder::setType, TYPE); + PARSER.declareField(Builder::setType, p -> { + if (p.currentToken() == XContentParser.Token.VALUE_STRING) { + return Type.fromString(p.text()); + } + throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); + }, TYPE, ObjectParser.ValueType.STRING); + PARSER.declareField(Builder::setEvent, p -> { + if (p.currentToken() == XContentParser.Token.VALUE_STRING) { + return Event.fromString(p.text()); + } + throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); + }, EVENT, ObjectParser.ValueType.STRING); PARSER.declareInt(Builder::setDetectorIndex, DETECTOR_INDEX); PARSER.declareString(Builder::setPartitionFieldName, PARTITION_FIELD_NAME); PARSER.declareString(Builder::setPartitionFieldValue, PARTITION_FIELD_VALUE); @@ -75,7 +116,8 @@ public class Annotation implements ToXContentObject, Writeable { private final String jobId; private final Date modifiedTime; private final String modifiedUsername; - private final String type; + private final Type type; + private final Event event; /** * Scope-related fields. */ @@ -88,8 +130,9 @@ public class Annotation implements ToXContentObject, Writeable { private final String byFieldValue; private Annotation(String annotation, Date createTime, String createUsername, Date timestamp, Date endTimestamp, String jobId, - Date modifiedTime, String modifiedUsername, String type, Integer detectorIndex, String partitionFieldName, - String partitionFieldValue, String overFieldName, String overFieldValue, String byFieldName, String byFieldValue) { + Date modifiedTime, String modifiedUsername, Type type, Event event, Integer detectorIndex, + String partitionFieldName, String partitionFieldValue, String overFieldName, String overFieldValue, + String byFieldName, String byFieldValue) { this.annotation = Objects.requireNonNull(annotation); this.createTime = Objects.requireNonNull(createTime); this.createUsername = Objects.requireNonNull(createUsername); @@ -99,6 +142,7 @@ private Annotation(String annotation, Date createTime, String createUsername, Da this.modifiedTime = modifiedTime; this.modifiedUsername = modifiedUsername; this.type = Objects.requireNonNull(type); + this.event = event; this.detectorIndex = detectorIndex; this.partitionFieldName = partitionFieldName; this.partitionFieldValue = partitionFieldValue; @@ -125,8 +169,9 @@ public Annotation(StreamInput in) throws IOException { modifiedTime = null; } modifiedUsername = in.readOptionalString(); - type = in.readString(); + type = Type.fromString(in.readString()); if (in.getVersion().onOrAfter(Version.V_8_0_0)) { + event = in.readBoolean() ? in.readEnum(Event.class) : null; detectorIndex = in.readOptionalInt(); partitionFieldName = in.readOptionalString(); partitionFieldValue = in.readOptionalString(); @@ -135,6 +180,7 @@ public Annotation(StreamInput in) throws IOException { byFieldName = in.readOptionalString(); byFieldValue = in.readOptionalString(); } else { + event = null; detectorIndex = null; partitionFieldName = null; partitionFieldValue = null; @@ -165,8 +211,14 @@ public void writeTo(StreamOutput out) throws IOException { out.writeBoolean(false); } out.writeOptionalString(modifiedUsername); - out.writeString(type); + out.writeString(type.toString()); if (out.getVersion().onOrAfter(Version.V_8_0_0)) { + if (event != null) { + out.writeBoolean(true); + out.writeEnum(event); + } else { + out.writeBoolean(false); + } out.writeOptionalInt(detectorIndex); out.writeOptionalString(partitionFieldName); out.writeOptionalString(partitionFieldValue); @@ -209,10 +261,14 @@ public String getModifiedUsername() { return modifiedUsername; } - public String getType() { + public Type getType() { return type; } + public Event getEvent() { + return event; + } + public Integer getDetectorIndex() { return detectorIndex; } @@ -261,6 +317,9 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws builder.field(MODIFIED_USERNAME.getPreferredName(), modifiedUsername); } builder.field(TYPE.getPreferredName(), type); + if (event != null) { + builder.field(EVENT.getPreferredName(), event); + } if (detectorIndex != null) { builder.field(DETECTOR_INDEX.getPreferredName(), detectorIndex); } @@ -289,7 +348,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws @Override public int hashCode() { return Objects.hash( - annotation, createTime, createUsername, timestamp, endTimestamp, jobId, modifiedTime, modifiedUsername, type, + annotation, createTime, createUsername, timestamp, endTimestamp, jobId, modifiedTime, modifiedUsername, type, event, detectorIndex, partitionFieldName, partitionFieldValue, overFieldName, overFieldValue, byFieldName, byFieldValue); } @@ -311,6 +370,7 @@ public boolean equals(Object obj) { Objects.equals(modifiedTime, other.modifiedTime) && Objects.equals(modifiedUsername, other.modifiedUsername) && Objects.equals(type, other.type) && + Objects.equals(event, other.event) && Objects.equals(detectorIndex, other.detectorIndex) && Objects.equals(partitionFieldName, other.partitionFieldName) && Objects.equals(partitionFieldValue, other.partitionFieldValue) && @@ -338,7 +398,8 @@ public static class Builder { private String jobId; private Date modifiedTime; private String modifiedUsername; - private String type; + private Type type; + private Event event; private Integer detectorIndex; private String partitionFieldName; private String partitionFieldValue; @@ -360,6 +421,7 @@ public Builder(Annotation other) { this.modifiedTime = other.modifiedTime == null ? null : new Date(other.modifiedTime.getTime()); this.modifiedUsername = other.modifiedUsername; this.type = other.type; + this.event = other.event; this.detectorIndex = other.detectorIndex; this.partitionFieldName = other.partitionFieldName; this.partitionFieldValue = other.partitionFieldValue; @@ -409,11 +471,16 @@ public Builder setModifiedUsername(String modifiedUsername) { return this; } - public Builder setType(String type) { + public Builder setType(Type type) { this.type = Objects.requireNonNull(type); return this; } + public Builder setEvent(Event event) { + this.event = event; + return this; + } + public Builder setDetectorIndex(Integer index) { this.detectorIndex = index; return this; @@ -451,7 +518,7 @@ public Builder setByFieldValue(String value) { public Annotation build() { return new Annotation( - annotation, createTime, createUsername, timestamp, endTimestamp, jobId, modifiedTime, modifiedUsername, type, + annotation, createTime, createUsername, timestamp, endTimestamp, jobId, modifiedTime, modifiedUsername, type, event, detectorIndex, partitionFieldName, partitionFieldValue, overFieldName, overFieldValue, byFieldName, byFieldValue); } } diff --git a/x-pack/plugin/core/src/main/resources/org/elasticsearch/xpack/core/ml/annotations_index_mappings.json b/x-pack/plugin/core/src/main/resources/org/elasticsearch/xpack/core/ml/annotations_index_mappings.json index 563a317bc9dd3..17ebf089de809 100644 --- a/x-pack/plugin/core/src/main/resources/org/elasticsearch/xpack/core/ml/annotations_index_mappings.json +++ b/x-pack/plugin/core/src/main/resources/org/elasticsearch/xpack/core/ml/annotations_index_mappings.json @@ -31,6 +31,9 @@ "type" : { "type" : "keyword" }, + "event" : { + "type" : "keyword" + }, "detector_index" : { "type" : "integer" }, diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationTests.java index 98e40e9525958..624574fb0acce 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationTests.java @@ -35,7 +35,8 @@ public static Annotation randomAnnotation(String jobId) { .setJobId(jobId) .setModifiedTime(randomBoolean() ? new Date(randomNonNegativeLong()) : null) .setModifiedUsername(randomBoolean() ? randomAlphaOfLengthBetween(5, 20) : null) - .setType(randomAlphaOfLengthBetween(10, 15)) + .setType(randomFrom(Annotation.Type.values())) + .setEvent(randomBoolean() ? randomFrom(Annotation.Event.values()) : null) .setDetectorIndex(randomBoolean() ? randomIntBetween(0, 10) : null) .setPartitionFieldName(randomBoolean() ? randomAlphaOfLengthBetween(5, 20) : null) .setPartitionFieldValue(randomBoolean() ? randomAlphaOfLengthBetween(5, 20) : null) diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/DatafeedJob.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/DatafeedJob.java index 22ebfda9dab59..3836284a1017d 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/DatafeedJob.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/DatafeedJob.java @@ -244,7 +244,8 @@ private Annotation createDelayedDataAnnotation(Date startTime, Date endTime, Str .setJobId(jobId) .setModifiedTime(currentTime) .setModifiedUsername(XPackUser.NAME) - .setType("annotation") + .setType(Annotation.Type.ANNOTATION) + .setEvent(Annotation.Event.DELAYED_DATA) .build(); } diff --git a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/process/autodetect/output/AutodetectResultProcessor.java b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/process/autodetect/output/AutodetectResultProcessor.java index d0ee4dec2c8c0..82dc5a4c1d24c 100644 --- a/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/process/autodetect/output/AutodetectResultProcessor.java +++ b/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/process/autodetect/output/AutodetectResultProcessor.java @@ -368,7 +368,8 @@ private Annotation createModelSnapshotAnnotation(ModelSnapshot modelSnapshot) { .setJobId(jobId) .setModifiedTime(currentTime) .setModifiedUsername(XPackUser.NAME) - .setType("annotation") + .setType(Annotation.Type.ANNOTATION) + .setEvent(Annotation.Event.MODEL_SNAPSHOT_STORED) .build(); } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/datafeed/DatafeedJobTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/datafeed/DatafeedJobTests.java index 03fdc5f52351c..790f1ea505b89 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/datafeed/DatafeedJobTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/datafeed/DatafeedJobTests.java @@ -296,7 +296,8 @@ public void testRealtimeRun() throws Exception { .setJobId(jobId) .setModifiedTime(new Date(annotationCreateTime)) .setModifiedUsername(XPackUser.NAME) - .setType("annotation") + .setType(Annotation.Type.ANNOTATION) + .setEvent(Annotation.Event.DELAYED_DATA) .build(); BytesReference expectedSource = BytesReference.bytes(expectedAnnotation.toXContent(XContentFactory.jsonBuilder(), ToXContent.EMPTY_PARAMS)); @@ -342,7 +343,8 @@ public void testRealtimeRun() throws Exception { .setJobId(jobId) .setModifiedTime(new Date(annotationUpdateTime)) .setModifiedUsername(XPackUser.NAME) - .setType("annotation") + .setType(Annotation.Type.ANNOTATION) + .setEvent(Annotation.Event.DELAYED_DATA) .build(); BytesReference expectedSource = BytesReference.bytes(expectedUpdatedAnnotation.toXContent(XContentFactory.jsonBuilder(), ToXContent.EMPTY_PARAMS)); diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/output/AutodetectResultProcessorTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/output/AutodetectResultProcessorTests.java index bef641619ecd9..e34a6d479b29f 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/output/AutodetectResultProcessorTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/job/process/autodetect/output/AutodetectResultProcessorTests.java @@ -396,7 +396,8 @@ public void testProcessResult_modelSnapshot() { .setJobId(JOB_ID) .setModifiedTime(Date.from(CURRENT_TIME)) .setModifiedUsername(XPackUser.NAME) - .setType("annotation") + .setType(Annotation.Type.ANNOTATION) + .setEvent(Annotation.Event.MODEL_SNAPSHOT_STORED) .build(); assertThat(annotation, is(equalTo(expectedAnnotation))); From c220db1626c2f39ef827677ac64b6ac3e2df6e2e Mon Sep 17 00:00:00 2001 From: Przemyslaw Witek Date: Fri, 29 May 2020 14:44:50 +0200 Subject: [PATCH 2/6] Use Annotation parser rather than Annotation.Builder throughout the code --- .../elasticsearch/xpack/core/ml/annotations/Annotation.java | 6 +++++- .../xpack/core/ml/annotations/AnnotationTests.java | 2 +- .../xpack/ml/integration/AutodetectResultProcessorIT.java | 2 +- .../xpack/ml/annotations/AnnotationPersisterTests.java | 2 +- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java index 34b2f205b9d18..ffd36888f015b 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java @@ -69,7 +69,11 @@ public String toString() { public static final ParseField BY_FIELD_NAME = new ParseField("by_field_name"); public static final ParseField BY_FIELD_VALUE = new ParseField("by_field_value"); - public static final ObjectParser PARSER = new ObjectParser<>(ANNOTATION.getPreferredName(), true, Builder::new); + public static Annotation fromXContent(XContentParser parser, Void context) { + return PARSER.apply(parser, context).build(); + } + + private static final ObjectParser PARSER = new ObjectParser<>(ANNOTATION.getPreferredName(), true, Builder::new); static { PARSER.declareString(Builder::setAnnotation, ANNOTATION); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationTests.java index 624574fb0acce..0d1260052e681 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationTests.java @@ -17,7 +17,7 @@ public class AnnotationTests extends AbstractSerializingTestCase { @Override protected Annotation doParseInstance(XContentParser parser) { - return Annotation.PARSER.apply(parser, null).build(); + return Annotation.fromXContent(parser, null); } @Override diff --git a/x-pack/plugin/ml/src/internalClusterTest/java/org/elasticsearch/xpack/ml/integration/AutodetectResultProcessorIT.java b/x-pack/plugin/ml/src/internalClusterTest/java/org/elasticsearch/xpack/ml/integration/AutodetectResultProcessorIT.java index 1982babc8042e..607cc560cb0f7 100644 --- a/x-pack/plugin/ml/src/internalClusterTest/java/org/elasticsearch/xpack/ml/integration/AutodetectResultProcessorIT.java +++ b/x-pack/plugin/ml/src/internalClusterTest/java/org/elasticsearch/xpack/ml/integration/AutodetectResultProcessorIT.java @@ -682,7 +682,7 @@ private List getAnnotations() throws Exception { private Annotation parseAnnotation(BytesReference source) throws IOException { try (XContentParser parser = createParser(jsonXContent, source)) { - return Annotation.PARSER.parse(parser, null).build(); + return Annotation.fromXContent(parser, null); } } diff --git a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/annotations/AnnotationPersisterTests.java b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/annotations/AnnotationPersisterTests.java index b1f5c91aa4d4e..c308d872da261 100644 --- a/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/annotations/AnnotationPersisterTests.java +++ b/x-pack/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/annotations/AnnotationPersisterTests.java @@ -234,7 +234,7 @@ private static BulkItemResponse bulkItemFailure(String docId) { private Annotation parseAnnotation(BytesReference source) throws IOException { try (XContentParser parser = createParser(jsonXContent, source)) { - return Annotation.PARSER.parse(parser, null).build(); + return Annotation.fromXContent(parser, null); } } } From 606b44315870b75237248afbdf668b5727c41b2d Mon Sep 17 00:00:00 2001 From: Przemyslaw Witek Date: Thu, 28 May 2020 11:04:49 +0200 Subject: [PATCH 3/6] Apply review comments --- .../xpack/core/ml/annotations/Annotation.java | 16 ++++++-- .../core/ml/annotations/AnnotationTests.java | 39 +++++++++++++++++++ 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java index ffd36888f015b..b0a4035bfc531 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java @@ -19,17 +19,25 @@ import org.elasticsearch.xpack.core.ml.job.config.Job; import java.io.IOException; +import java.util.Arrays; import java.util.Date; import java.util.Locale; +import java.util.Map; import java.util.Objects; +import java.util.function.Function; + +import static java.util.stream.Collectors.toMap; public class Annotation implements ToXContentObject, Writeable { public enum Type { - ANNOTATION; + ANNOTATION, + COMMENT; + + private static Map lookupByName = Arrays.stream(values()).collect(toMap(Type::name, Function.identity())); public static Type fromString(String value) { - return valueOf(value.toUpperCase(Locale.ROOT)); + return lookupByName.getOrDefault(value.toUpperCase(Locale.ROOT), ANNOTATION); } @Override @@ -42,8 +50,10 @@ public enum Event { DELAYED_DATA, MODEL_SNAPSHOT_STORED; + private static Map lookupByName = Arrays.stream(values()).collect(toMap(Event::name, Function.identity())); + public static Event fromString(String value) { - return valueOf(value.toUpperCase(Locale.ROOT)); + return lookupByName.get(value.toUpperCase(Locale.ROOT)); } @Override diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationTests.java index 0d1260052e681..8a255ab8ee567 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationTests.java @@ -5,13 +5,19 @@ */ package org.elasticsearch.xpack.core.ml.annotations; +import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.test.AbstractSerializingTestCase; +import java.io.IOException; import java.util.Date; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; public class AnnotationTests extends AbstractSerializingTestCase { @@ -58,4 +64,37 @@ public void testCopyConstructor() { assertThat(testAnnotation, equalTo(new Annotation.Builder(testAnnotation).build())); } } + + public void testParsingTypeFieldIsLenient() throws IOException { + XContentBuilder json = XContentBuilder.builder(JsonXContent.jsonXContent) + .startObject() + .field("annotation", "some text") + .field("create_time", 2_000_000) + .field("create_username", "some user") + .field("timestamp", 1_000_000) + .field("type", "bad_type") + .endObject(); + + try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(json))) { + Annotation annotation = doParseInstance(parser); + assertThat(annotation.getType(), is(equalTo(Annotation.Type.ANNOTATION))); + } + } + + public void testParsingEventFieldIsLenient() throws IOException { + XContentBuilder json = XContentBuilder.builder(JsonXContent.jsonXContent) + .startObject() + .field("annotation", "some text") + .field("create_time", 2_000_000) + .field("create_username", "some user") + .field("timestamp", 1_000_000) + .field("type", "annotation") + .field("event", "bad_event") + .endObject(); + + try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(json))) { + Annotation annotation = doParseInstance(parser); + assertThat(annotation.getEvent(), is(nullValue())); + } + } } From b0e372eea565bab05413cf9b3806fd1ee5cf0616 Mon Sep 17 00:00:00 2001 From: Przemyslaw Witek Date: Thu, 28 May 2020 13:16:43 +0200 Subject: [PATCH 4/6] Add Event.USER enum value for user-generated annotations --- .../org/elasticsearch/xpack/core/ml/annotations/Annotation.java | 1 + 1 file changed, 1 insertion(+) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java index b0a4035bfc531..5c3bcb5ff50b0 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java @@ -47,6 +47,7 @@ public String toString() { } public enum Event { + USER, DELAYED_DATA, MODEL_SNAPSHOT_STORED; From c13a5ce3c07f28dbd2779e2b3aba4341b75b823f Mon Sep 17 00:00:00 2001 From: Przemyslaw Witek Date: Mon, 1 Jun 2020 17:29:12 +0200 Subject: [PATCH 5/6] Make Annotation parser strict --- .../xpack/core/ml/annotations/Annotation.java | 21 +++++----- .../core/ml/annotations/AnnotationTests.java | 39 ------------------- 2 files changed, 9 insertions(+), 51 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java index 5c3bcb5ff50b0..ad8c09623802f 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java @@ -19,14 +19,9 @@ import org.elasticsearch.xpack.core.ml.job.config.Job; import java.io.IOException; -import java.util.Arrays; import java.util.Date; import java.util.Locale; -import java.util.Map; import java.util.Objects; -import java.util.function.Function; - -import static java.util.stream.Collectors.toMap; public class Annotation implements ToXContentObject, Writeable { @@ -34,10 +29,8 @@ public enum Type { ANNOTATION, COMMENT; - private static Map lookupByName = Arrays.stream(values()).collect(toMap(Type::name, Function.identity())); - public static Type fromString(String value) { - return lookupByName.getOrDefault(value.toUpperCase(Locale.ROOT), ANNOTATION); + return valueOf(value.toUpperCase(Locale.ROOT)); } @Override @@ -51,10 +44,8 @@ public enum Event { DELAYED_DATA, MODEL_SNAPSHOT_STORED; - private static Map lookupByName = Arrays.stream(values()).collect(toMap(Event::name, Function.identity())); - public static Event fromString(String value) { - return lookupByName.get(value.toUpperCase(Locale.ROOT)); + return valueOf(value.toUpperCase(Locale.ROOT)); } @Override @@ -80,11 +71,17 @@ public String toString() { public static final ParseField BY_FIELD_NAME = new ParseField("by_field_name"); public static final ParseField BY_FIELD_VALUE = new ParseField("by_field_value"); + /** + * Parses {@link Annotation} using a strict parser. + */ public static Annotation fromXContent(XContentParser parser, Void context) { return PARSER.apply(parser, context).build(); } - private static final ObjectParser PARSER = new ObjectParser<>(ANNOTATION.getPreferredName(), true, Builder::new); + /** + * Strict parser for cases when {@link Annotation} is returned from C++ as an ML result. + */ + private static final ObjectParser PARSER = new ObjectParser<>(ANNOTATION.getPreferredName(), false, Builder::new); static { PARSER.declareString(Builder::setAnnotation, ANNOTATION); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationTests.java index 8a255ab8ee567..0d1260052e681 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/annotations/AnnotationTests.java @@ -5,19 +5,13 @@ */ package org.elasticsearch.xpack.core.ml.annotations; -import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.io.stream.Writeable; -import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.test.AbstractSerializingTestCase; -import java.io.IOException; import java.util.Date; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.is; -import static org.hamcrest.Matchers.nullValue; public class AnnotationTests extends AbstractSerializingTestCase { @@ -64,37 +58,4 @@ public void testCopyConstructor() { assertThat(testAnnotation, equalTo(new Annotation.Builder(testAnnotation).build())); } } - - public void testParsingTypeFieldIsLenient() throws IOException { - XContentBuilder json = XContentBuilder.builder(JsonXContent.jsonXContent) - .startObject() - .field("annotation", "some text") - .field("create_time", 2_000_000) - .field("create_username", "some user") - .field("timestamp", 1_000_000) - .field("type", "bad_type") - .endObject(); - - try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(json))) { - Annotation annotation = doParseInstance(parser); - assertThat(annotation.getType(), is(equalTo(Annotation.Type.ANNOTATION))); - } - } - - public void testParsingEventFieldIsLenient() throws IOException { - XContentBuilder json = XContentBuilder.builder(JsonXContent.jsonXContent) - .startObject() - .field("annotation", "some text") - .field("create_time", 2_000_000) - .field("create_username", "some user") - .field("timestamp", 1_000_000) - .field("type", "annotation") - .field("event", "bad_event") - .endObject(); - - try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(json))) { - Annotation annotation = doParseInstance(parser); - assertThat(annotation.getEvent(), is(nullValue())); - } - } } From 0624aa0c1c717fa1fe0005060600b23449d6a15a Mon Sep 17 00:00:00 2001 From: Przemyslaw Witek Date: Mon, 1 Jun 2020 18:19:30 +0200 Subject: [PATCH 6/6] Rename PARSER to STRICT_PARSER --- .../xpack/core/ml/annotations/Annotation.java | 38 +++++++++---------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java index ad8c09623802f..68c1635491b01 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/annotations/Annotation.java @@ -75,46 +75,46 @@ public String toString() { * Parses {@link Annotation} using a strict parser. */ public static Annotation fromXContent(XContentParser parser, Void context) { - return PARSER.apply(parser, context).build(); + return STRICT_PARSER.apply(parser, context).build(); } /** * Strict parser for cases when {@link Annotation} is returned from C++ as an ML result. */ - private static final ObjectParser PARSER = new ObjectParser<>(ANNOTATION.getPreferredName(), false, Builder::new); + private static final ObjectParser STRICT_PARSER = new ObjectParser<>(ANNOTATION.getPreferredName(), false, Builder::new); static { - PARSER.declareString(Builder::setAnnotation, ANNOTATION); - PARSER.declareField(Builder::setCreateTime, + STRICT_PARSER.declareString(Builder::setAnnotation, ANNOTATION); + STRICT_PARSER.declareField(Builder::setCreateTime, p -> TimeUtils.parseTimeField(p, CREATE_TIME.getPreferredName()), CREATE_TIME, ObjectParser.ValueType.VALUE); - PARSER.declareString(Builder::setCreateUsername, CREATE_USERNAME); - PARSER.declareField(Builder::setTimestamp, + STRICT_PARSER.declareString(Builder::setCreateUsername, CREATE_USERNAME); + STRICT_PARSER.declareField(Builder::setTimestamp, p -> TimeUtils.parseTimeField(p, TIMESTAMP.getPreferredName()), TIMESTAMP, ObjectParser.ValueType.VALUE); - PARSER.declareField(Builder::setEndTimestamp, + STRICT_PARSER.declareField(Builder::setEndTimestamp, p -> TimeUtils.parseTimeField(p, END_TIMESTAMP.getPreferredName()), END_TIMESTAMP, ObjectParser.ValueType.VALUE); - PARSER.declareString(Builder::setJobId, Job.ID); - PARSER.declareField(Builder::setModifiedTime, + STRICT_PARSER.declareString(Builder::setJobId, Job.ID); + STRICT_PARSER.declareField(Builder::setModifiedTime, p -> TimeUtils.parseTimeField(p, MODIFIED_TIME.getPreferredName()), MODIFIED_TIME, ObjectParser.ValueType.VALUE); - PARSER.declareString(Builder::setModifiedUsername, MODIFIED_USERNAME); - PARSER.declareField(Builder::setType, p -> { + STRICT_PARSER.declareString(Builder::setModifiedUsername, MODIFIED_USERNAME); + STRICT_PARSER.declareField(Builder::setType, p -> { if (p.currentToken() == XContentParser.Token.VALUE_STRING) { return Type.fromString(p.text()); } throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); }, TYPE, ObjectParser.ValueType.STRING); - PARSER.declareField(Builder::setEvent, p -> { + STRICT_PARSER.declareField(Builder::setEvent, p -> { if (p.currentToken() == XContentParser.Token.VALUE_STRING) { return Event.fromString(p.text()); } throw new IllegalArgumentException("Unsupported token [" + p.currentToken() + "]"); }, EVENT, ObjectParser.ValueType.STRING); - PARSER.declareInt(Builder::setDetectorIndex, DETECTOR_INDEX); - PARSER.declareString(Builder::setPartitionFieldName, PARTITION_FIELD_NAME); - PARSER.declareString(Builder::setPartitionFieldValue, PARTITION_FIELD_VALUE); - PARSER.declareString(Builder::setOverFieldName, OVER_FIELD_NAME); - PARSER.declareString(Builder::setOverFieldValue, OVER_FIELD_VALUE); - PARSER.declareString(Builder::setByFieldName, BY_FIELD_NAME); - PARSER.declareString(Builder::setByFieldValue, BY_FIELD_VALUE); + STRICT_PARSER.declareInt(Builder::setDetectorIndex, DETECTOR_INDEX); + STRICT_PARSER.declareString(Builder::setPartitionFieldName, PARTITION_FIELD_NAME); + STRICT_PARSER.declareString(Builder::setPartitionFieldValue, PARTITION_FIELD_VALUE); + STRICT_PARSER.declareString(Builder::setOverFieldName, OVER_FIELD_NAME); + STRICT_PARSER.declareString(Builder::setOverFieldValue, OVER_FIELD_VALUE); + STRICT_PARSER.declareString(Builder::setByFieldName, BY_FIELD_NAME); + STRICT_PARSER.declareString(Builder::setByFieldValue, BY_FIELD_VALUE); } private final String annotation;