[ML] Hide internal Job update options from the REST API (#30537)

davidkyle · davidkyle · commit 01ec8a95972e · 2018-05-14T10:35:52.000+01:00
diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/UpdateJobAction.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/UpdateJobAction.java
@@ -45,7 +45,7 @@ public PutJobAction.Response newResponse() {
     public static class Request extends AcknowledgedRequest<UpdateJobAction.Request> implements ToXContentObject {
 
         public static UpdateJobAction.Request parseRequest(String jobId, XContentParser parser) {
-            JobUpdate update = JobUpdate.PARSER.apply(parser, null).setJobId(jobId).build();
+            JobUpdate update = JobUpdate.EXTERNAL_PARSER.apply(parser, null).setJobId(jobId).build();
             return new UpdateJobAction.Request(jobId, update);
         }
 
diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdate.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdate.java
@@ -30,26 +30,34 @@
 public class JobUpdate implements Writeable, ToXContentObject {
     public static final ParseField DETECTORS = new ParseField("detectors");
 
-    public static final ConstructingObjectParser<Builder, Void> PARSER = new ConstructingObjectParser<>(
+    // For internal updates
+    static final ConstructingObjectParser<Builder, Void> INTERNAL_PARSER = new ConstructingObjectParser<>(
+            "job_update", args -> new Builder((String) args[0]));
+
+    // For parsing REST requests
+    public static final ConstructingObjectParser<Builder, Void> EXTERNAL_PARSER = new ConstructingObjectParser<>(
             "job_update", args -> new Builder((String) args[0]));
 
     static {
-        PARSER.declareString(ConstructingObjectParser.optionalConstructorArg(), Job.ID);
-        PARSER.declareStringArray(Builder::setGroups, Job.GROUPS);
-        PARSER.declareStringOrNull(Builder::setDescription, Job.DESCRIPTION);
-        PARSER.declareObjectArray(Builder::setDetectorUpdates, DetectorUpdate.PARSER, DETECTORS);
-        PARSER.declareObject(Builder::setModelPlotConfig, ModelPlotConfig.CONFIG_PARSER, Job.MODEL_PLOT_CONFIG);
-        PARSER.declareObject(Builder::setAnalysisLimits, AnalysisLimits.CONFIG_PARSER, Job.ANALYSIS_LIMITS);
-        PARSER.declareString((builder, val) -> builder.setBackgroundPersistInterval(
-                TimeValue.parseTimeValue(val, Job.BACKGROUND_PERSIST_INTERVAL.getPreferredName())), Job.BACKGROUND_PERSIST_INTERVAL);
-        PARSER.declareLong(Builder::setRenormalizationWindowDays, Job.RENORMALIZATION_WINDOW_DAYS);
-        PARSER.declareLong(Builder::setResultsRetentionDays, Job.RESULTS_RETENTION_DAYS);
-        PARSER.declareLong(Builder::setModelSnapshotRetentionDays, Job.MODEL_SNAPSHOT_RETENTION_DAYS);
-        PARSER.declareStringArray(Builder::setCategorizationFilters, AnalysisConfig.CATEGORIZATION_FILTERS);
-        PARSER.declareField(Builder::setCustomSettings, (p, c) -> p.map(), Job.CUSTOM_SETTINGS, ObjectParser.ValueType.OBJECT);
-        PARSER.declareString(Builder::setModelSnapshotId, Job.MODEL_SNAPSHOT_ID);
-        PARSER.declareLong(Builder::setEstablishedModelMemory, Job.ESTABLISHED_MODEL_MEMORY);
-        PARSER.declareString(Builder::setJobVersion, Job.JOB_VERSION);
+        for (ConstructingObjectParser<Builder, Void> parser : Arrays.asList(INTERNAL_PARSER, EXTERNAL_PARSER)) {
+            parser.declareString(ConstructingObjectParser.optionalConstructorArg(), Job.ID);
+            parser.declareStringArray(Builder::setGroups, Job.GROUPS);
+            parser.declareStringOrNull(Builder::setDescription, Job.DESCRIPTION);
+            parser.declareObjectArray(Builder::setDetectorUpdates, DetectorUpdate.PARSER, DETECTORS);
+            parser.declareObject(Builder::setModelPlotConfig, ModelPlotConfig.CONFIG_PARSER, Job.MODEL_PLOT_CONFIG);
+            parser.declareObject(Builder::setAnalysisLimits, AnalysisLimits.CONFIG_PARSER, Job.ANALYSIS_LIMITS);
+            parser.declareString((builder, val) -> builder.setBackgroundPersistInterval(
+                    TimeValue.parseTimeValue(val, Job.BACKGROUND_PERSIST_INTERVAL.getPreferredName())), Job.BACKGROUND_PERSIST_INTERVAL);
+            parser.declareLong(Builder::setRenormalizationWindowDays, Job.RENORMALIZATION_WINDOW_DAYS);
+            parser.declareLong(Builder::setResultsRetentionDays, Job.RESULTS_RETENTION_DAYS);
+            parser.declareLong(Builder::setModelSnapshotRetentionDays, Job.MODEL_SNAPSHOT_RETENTION_DAYS);
+            parser.declareStringArray(Builder::setCategorizationFilters, AnalysisConfig.CATEGORIZATION_FILTERS);
+            parser.declareField(Builder::setCustomSettings, (p, c) -> p.map(), Job.CUSTOM_SETTINGS, ObjectParser.ValueType.OBJECT);
+        }
+        // These fields should not be set by a REST request
+        INTERNAL_PARSER.declareString(Builder::setModelSnapshotId, Job.MODEL_SNAPSHOT_ID);
+        INTERNAL_PARSER.declareLong(Builder::setEstablishedModelMemory, Job.ESTABLISHED_MODEL_MEMORY);
+        INTERNAL_PARSER.declareString(Builder::setJobVersion, Job.JOB_VERSION);
     }
 
     private final String jobId;
@@ -224,14 +232,14 @@ public Long getEstablishedModelMemory() {
         return establishedModelMemory;
     }
 
-    public boolean isAutodetectProcessUpdate() {
-        return modelPlotConfig != null || detectorUpdates != null;
-    }
-
     public Version getJobVersion() {
         return jobVersion;
     }
 
+    public boolean isAutodetectProcessUpdate() {
+        return modelPlotConfig != null || detectorUpdates != null;
+    }
+
     @Override
     public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
         builder.startObject();
@@ -332,7 +340,7 @@ public Set<String> getUpdateFields() {
     /**
      * Updates {@code source} with the new values in this object returning a new {@link Job}.
      *
-     * @param source Source job to be updated
+     * @param source              Source job to be updated
      * @param maxModelMemoryLimit The maximum model memory allowed
      * @return A new job equivalent to {@code source} updated.
      */
diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdateTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobUpdateTests.java
@@ -26,6 +26,8 @@
 
 public class JobUpdateTests extends AbstractSerializingTestCase<JobUpdate> {
 
+    private boolean useInternalParser = randomBoolean();
+
     @Override
     protected JobUpdate createTestInstance() {
         JobUpdate.Builder update = new JobUpdate.Builder(randomAlphaOfLength(4));
@@ -84,13 +86,13 @@ protected JobUpdate createTestInstance() {
         if (randomBoolean()) {
             update.setCustomSettings(Collections.singletonMap(randomAlphaOfLength(10), randomAlphaOfLength(10)));
         }
-        if (randomBoolean()) {
+        if (useInternalParser && randomBoolean()) {
             update.setModelSnapshotId(randomAlphaOfLength(10));
         }
-        if (randomBoolean()) {
+        if (useInternalParser && randomBoolean()) {
             update.setEstablishedModelMemory(randomNonNegativeLong());
         }
-        if (randomBoolean()) {
+        if (useInternalParser && randomBoolean()) {
             update.setJobVersion(randomFrom(Version.CURRENT, Version.V_6_2_0, Version.V_6_1_0));
         }
 
@@ -104,7 +106,11 @@ protected Writeable.Reader<JobUpdate> instanceReader() {
 
     @Override
     protected JobUpdate doParseInstance(XContentParser parser) {
-        return JobUpdate.PARSER.apply(parser, null).build();
+        if (useInternalParser) {
+            return JobUpdate.INTERNAL_PARSER.apply(parser, null).build();
+        } else {
+            return JobUpdate.EXTERNAL_PARSER.apply(parser, null).build();
+        }
     }
 
     public void testMergeWithJob() {
@@ -141,7 +147,7 @@ public void testMergeWithJob() {
         JobUpdate update = updateBuilder.build();
 
         Job.Builder jobBuilder = new Job.Builder("foo");
-        jobBuilder.setGroups(Arrays.asList("group-1"));
+        jobBuilder.setGroups(Collections.singletonList("group-1"));
         Detector.Builder d1 = new Detector.Builder("info_content", "domain");
         d1.setOverFieldName("mlcategory");
         Detector.Builder d2 = new Detector.Builder("min", "field");
diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/delete_model_snapshot.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/ml/delete_model_snapshot.yml
@@ -88,7 +88,24 @@ setup:
             "description": "second",
             "latest_record_time_stamp": "2016-06-01T00:00:00Z",
             "latest_result_time_stamp": "2016-06-01T00:00:00Z",
-            "snapshot_doc_count": 3
+            "snapshot_doc_count": 3,
+            "model_size_stats": {
+                "job_id" : "delete-model-snapshot",
+                "result_type" : "model_size_stats",
+                "model_bytes" : 0,
+                "total_by_field_count" : 101,
+                "total_over_field_count" : 0,
+                "total_partition_field_count" : 0,
+                "bucket_allocation_failures_count" : 0,
+                "memory_status" : "ok",
+                "log_time" : 1495808248662,
+                "timestamp" : 1495808248662
+            },
+            "quantiles": {
+              "job_id": "delete-model-snapshot",
+              "timestamp": 1495808248662,
+              "quantile_state": "quantiles-1"
+            }
           }
 
   - do:
@@ -106,12 +123,10 @@ setup:
   - do:
       headers:
         Authorization: "Basic eF9wYWNrX3Jlc3RfdXNlcjp4LXBhY2stdGVzdC1wYXNzd29yZA==" # run as x_pack_rest_user, i.e. the test setup superuser
-      xpack.ml.update_job:
+      xpack.ml.revert_model_snapshot:
         job_id: delete-model-snapshot
-        body:  >
-          {
-            "model_snapshot_id": "active-snapshot"
-          }
+        snapshot_id: "active-snapshot"
+
 
 ---
 "Test delete snapshot missing snapshotId":

Original file line number	Diff line number	Diff line change
`@@ -45,7 +45,7 @@ public PutJobAction.Response newResponse() {`
`45`	`45`	`public static class Request extends AcknowledgedRequest<UpdateJobAction.Request> implements ToXContentObject {`
`46`	`46`
`47`	`47`	`public static UpdateJobAction.Request parseRequest(String jobId, XContentParser parser) {`
`48`		`- JobUpdate update = JobUpdate.PARSER.apply(parser, null).setJobId(jobId).build();`
	`48`	`+ JobUpdate update = JobUpdate.EXTERNAL_PARSER.apply(parser, null).setJobId(jobId).build();`
`49`	`49`	`return new UpdateJobAction.Request(jobId, update);`
`50`	`50`	`}`
`51`	`51`