Exclude internal fields from job APIs. (elastic#106115)

* Exclude internal fields from job APIs.

* Skip BWC breaking tests

* Remove unused Job.STRICT_PARSER

x-pack/plugin/build.gradle

@@ -109,6 +109,8 @@ tasks.named("yamlRestTestV7CompatTransform").configure { task ->
   )
   task.skipTest("ml/jobs_crud/Test update job", "Behaviour change #89824 - added limit filter to categorization analyzer")
   task.skipTest("ml/jobs_crud/Test create job with delimited format", "removing undocumented functionality")
+  task.skipTest("ml/jobs_crud/Test cannot create job with model snapshot id set", "Exception type has changed.")
+  task.skipTest("ml/validate/Test job config is invalid because model snapshot id set", "Exception type has changed.")
   task.skipTest("ml/datafeeds_crud/Test update datafeed to point to missing job", "behaviour change #44752 - not allowing to update datafeed job_id")
   task.skipTest(
     "ml/datafeeds_crud/Test update datafeed to point to different job",

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/PreviewDatafeedAction.java

@@ -60,7 +60,7 @@ public static class Request extends ActionRequest implements ToXContentObject {
         private static final ObjectParser<Builder, Void> PARSER = new ObjectParser<>("preview_datafeed_action", Request.Builder::new);
         static {
             PARSER.declareObject(Builder::setDatafeedBuilder, DatafeedConfig.STRICT_PARSER, DATAFEED_CONFIG);
-            PARSER.declareObject(Builder::setJobBuilder, Job.STRICT_PARSER, JOB_CONFIG);
+            PARSER.declareObject(Builder::setJobBuilder, Job.REST_REQUEST_PARSER, JOB_CONFIG);
             PARSER.declareString(Builder::setStart, START_TIME);
             PARSER.declareString(Builder::setEnd, END_TIME);
         }

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/PutJobAction.java

@@ -20,7 +20,6 @@
 import org.elasticsearch.xpack.core.ml.job.messages.Messages;
 
 import java.io.IOException;
-import java.util.List;
 import java.util.Objects;
 
 public class PutJobAction extends ActionType<PutJobAction.Response> {
@@ -35,7 +34,7 @@ private PutJobAction() {
     public static class Request extends AcknowledgedRequest<Request> {
 
         public static Request parseRequest(String jobId, XContentParser parser, IndicesOptions indicesOptions) {
-            Job.Builder jobBuilder = Job.STRICT_PARSER.apply(parser, null);
+            Job.Builder jobBuilder = Job.REST_REQUEST_PARSER.apply(parser, null);
             if (jobBuilder.getId() == null) {
                 jobBuilder.setId(jobId);
             } else if (Strings.isNullOrEmpty(jobId) == false && jobId.equals(jobBuilder.getId()) == false) {
@@ -58,14 +57,6 @@ public Request(Job.Builder jobBuilder) {
             // would occur when parsing an old job config that already had duplicate detectors.
             jobBuilder.validateDetectorsAreUnique();
 
-            // Some fields cannot be set at create time
-            List<String> invalidJobCreationSettings = jobBuilder.invalidCreateTimeSettings();
-            if (invalidJobCreationSettings.isEmpty() == false) {
-                throw new IllegalArgumentException(
-                    Messages.getMessage(Messages.JOB_CONFIG_INVALID_CREATE_SETTINGS, String.join(",", invalidJobCreationSettings))
-                );
-            }
-
             this.jobBuilder = jobBuilder;
         }
 

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/action/ValidateJobConfigAction.java

@@ -14,11 +14,9 @@
 import org.elasticsearch.common.io.stream.StreamOutput;
 import org.elasticsearch.xcontent.XContentParser;
 import org.elasticsearch.xpack.core.ml.job.config.Job;
-import org.elasticsearch.xpack.core.ml.job.messages.Messages;
 
 import java.io.IOException;
 import java.util.Date;
-import java.util.List;
 import java.util.Objects;
 
 public class ValidateJobConfigAction extends ActionType<AcknowledgedResponse> {
@@ -32,10 +30,10 @@ protected ValidateJobConfigAction() {
 
     public static class Request extends ActionRequest {
 
-        private Job job;
+        private final Job job;
 
         public static Request parseRequest(XContentParser parser) {
-            Job.Builder jobBuilder = Job.STRICT_PARSER.apply(parser, null);
+            Job.Builder jobBuilder = Job.REST_REQUEST_PARSER.apply(parser, null);
             // When jobs are PUT their ID must be supplied in the URL - assume this will
             // be valid unless an invalid job ID is specified in the JSON to be validated
             jobBuilder.setId(jobBuilder.getId() != null ? jobBuilder.getId() : "ok");
@@ -45,14 +43,6 @@ public static Request parseRequest(XContentParser parser) {
             // would occur when parsing an old job config that already had duplicate detectors.
             jobBuilder.validateDetectorsAreUnique();
 
-            // Some fields cannot be set at create time
-            List<String> invalidJobCreationSettings = jobBuilder.invalidCreateTimeSettings();
-            if (invalidJobCreationSettings.isEmpty() == false) {
-                throw new IllegalArgumentException(
-                    Messages.getMessage(Messages.JOB_CONFIG_INVALID_CREATE_SETTINGS, String.join(",", invalidJobCreationSettings))
-                );
-            }
-
             return new Request(jobBuilder.build(new Date()));
         }
 

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/config/Job.java

@@ -98,8 +98,9 @@ public class Job implements SimpleDiffable<Job>, Writeable, ToXContentObject {
     public static final ParseField RESULTS_FIELD = new ParseField("jobs");
 
     // These parsers follow the pattern that metadata is parsed leniently (to allow for enhancements), whilst config is parsed strictly
-    public static final ObjectParser<Builder, Void> LENIENT_PARSER = createParser(true);
-    public static final ObjectParser<Builder, Void> STRICT_PARSER = createParser(false);
+    public static final ObjectParser<Builder, Void> LENIENT_PARSER = createParser(true, true);
+    // Use the REST request parser to parse a job passed to the API, to disallow setting internal fields.
+    public static final ObjectParser<Builder, Void> REST_REQUEST_PARSER = createParser(false, false);
 
     public static final TimeValue MIN_BACKGROUND_PERSIST_INTERVAL = TimeValue.timeValueHours(1);
 
@@ -114,26 +115,12 @@ public class Job implements SimpleDiffable<Job>, Writeable, ToXContentObject {
     public static final long DEFAULT_MODEL_SNAPSHOT_RETENTION_DAYS = 10;
     public static final long DEFAULT_DAILY_MODEL_SNAPSHOT_RETENTION_AFTER_DAYS = 1;
 
-    private static ObjectParser<Builder, Void> createParser(boolean ignoreUnknownFields) {
+    private static ObjectParser<Builder, Void> createParser(boolean allowInternalFields, boolean ignoreUnknownFields) {
         ObjectParser<Builder, Void> parser = new ObjectParser<>("job_details", ignoreUnknownFields, Builder::new);
 
         parser.declareString(Builder::setId, ID);
-        parser.declareString(Builder::setJobType, JOB_TYPE);
-        parser.declareString(Builder::setJobVersion, JOB_VERSION);
         parser.declareStringArray(Builder::setGroups, GROUPS);
         parser.declareStringOrNull(Builder::setDescription, DESCRIPTION);
-        parser.declareField(
-            Builder::setCreateTime,
-            p -> TimeUtils.parseTimeField(p, CREATE_TIME.getPreferredName()),
-            CREATE_TIME,
-            ValueType.VALUE
-        );
-        parser.declareField(
-            Builder::setFinishedTime,
-            p -> TimeUtils.parseTimeField(p, FINISHED_TIME.getPreferredName()),
-            FINISHED_TIME,
-            ValueType.VALUE
-        );
         parser.declareObject(
             Builder::setAnalysisConfig,
             ignoreUnknownFields ? AnalysisConfig.LENIENT_PARSER : AnalysisConfig.STRICT_PARSER,
@@ -165,17 +152,35 @@ private static ObjectParser<Builder, Void> createParser(boolean ignoreUnknownFie
         parser.declareLong(Builder::setModelSnapshotRetentionDays, MODEL_SNAPSHOT_RETENTION_DAYS);
         parser.declareLong(Builder::setDailyModelSnapshotRetentionAfterDays, DAILY_MODEL_SNAPSHOT_RETENTION_AFTER_DAYS);
         parser.declareField(Builder::setCustomSettings, (p, c) -> p.mapOrdered(), CUSTOM_SETTINGS, ValueType.OBJECT);
-        parser.declareStringOrNull(Builder::setModelSnapshotId, MODEL_SNAPSHOT_ID);
-        parser.declareStringOrNull(Builder::setModelSnapshotMinVersion, MODEL_SNAPSHOT_MIN_VERSION);
         parser.declareString(Builder::setResultsIndexName, RESULTS_INDEX_NAME);
-        parser.declareBoolean(Builder::setDeleting, DELETING);
         parser.declareBoolean(Builder::setAllowLazyOpen, ALLOW_LAZY_OPEN);
-        parser.declareObject(Builder::setBlocked, ignoreUnknownFields ? Blocked.LENIENT_PARSER : Blocked.STRICT_PARSER, BLOCKED);
         parser.declareObject(
             Builder::setDatafeed,
             ignoreUnknownFields ? DatafeedConfig.LENIENT_PARSER : DatafeedConfig.STRICT_PARSER,
             DATAFEED_CONFIG
         );
+
+        if (allowInternalFields) {
+            parser.declareString(Builder::setJobType, JOB_TYPE);
+            parser.declareString(Builder::setJobVersion, JOB_VERSION);
+            parser.declareField(
+                Builder::setCreateTime,
+                p -> TimeUtils.parseTimeField(p, CREATE_TIME.getPreferredName()),
+                CREATE_TIME,
+                ValueType.VALUE
+            );
+            parser.declareField(
+                Builder::setFinishedTime,
+                p -> TimeUtils.parseTimeField(p, FINISHED_TIME.getPreferredName()),
+                FINISHED_TIME,
+                ValueType.VALUE
+            );
+            parser.declareStringOrNull(Builder::setModelSnapshotId, MODEL_SNAPSHOT_ID);
+            parser.declareStringOrNull(Builder::setModelSnapshotMinVersion, MODEL_SNAPSHOT_MIN_VERSION);
+            parser.declareBoolean(Builder::setDeleting, DELETING);
+            parser.declareObject(Builder::setBlocked, ignoreUnknownFields ? Blocked.LENIENT_PARSER : Blocked.STRICT_PARSER, BLOCKED);
+        }
+
         return parser;
     }
 
@@ -1020,26 +1025,6 @@ public Builder setDatafeedIndicesOptionsIfRequired(IndicesOptions indicesOptions
             return this;
         }
 
-        /**
-         * Return the list of fields that have been set and are invalid to
-         * be set when the job is created e.g. model snapshot Id should not
-         * be set at job creation.
-         * @return List of fields set fields that should not be.
-         */
-        public List<String> invalidCreateTimeSettings() {
-            List<String> invalidCreateValues = new ArrayList<>();
-            if (modelSnapshotId != null) {
-                invalidCreateValues.add(MODEL_SNAPSHOT_ID.getPreferredName());
-            }
-            if (finishedTime != null) {
-                invalidCreateValues.add(FINISHED_TIME.getPreferredName());
-            }
-            if (createTime != null) {
-                invalidCreateValues.add(CREATE_TIME.getPreferredName());
-            }
-            return invalidCreateValues;
-        }
-
         @Override
         public void writeTo(StreamOutput out) throws IOException {
             out.writeOptionalString(id);

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ml/job/messages/Messages.java

@@ -222,8 +222,6 @@ public final class Messages {
     public static final String JOB_CONFIG_FUNCTION_REQUIRES_OVERFIELD = "over_field_name must be set when the ''{0}'' function is used";
     public static final String JOB_CONFIG_ID_ALREADY_TAKEN = "The job cannot be created with the Id ''{0}''. The Id is already used.";
     public static final String JOB_CONFIG_ID_TOO_LONG = "The job id cannot contain more than {0,number,integer} characters.";
-    public static final String JOB_CONFIG_INVALID_CREATE_SETTINGS =
-        "The job is configured with fields [{0}] that are illegal to set at job creation";
     public static final String JOB_CONFIG_INVALID_FIELDNAME_CHARS =
         "Invalid field name ''{0}''. Field names including over, by and partition " + "fields cannot contain any of these characters: {1}";
     public static final String JOB_CONFIG_INVALID_FIELDNAME =

x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ml/job/config/JobTests.java

@@ -43,7 +43,6 @@
 import java.util.Collections;
 import java.util.Date;
 import java.util.HashMap;
-import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -101,7 +100,7 @@ protected Writeable.Reader<Job> instanceReader() {
 
     @Override
     protected Job doParseInstance(XContentParser parser) {
-        return Job.STRICT_PARSER.apply(parser, null).build();
+        return Job.LENIENT_PARSER.apply(parser, null).build();
     }
 
     public void testToXContentForInternalStorage() throws IOException {
@@ -119,10 +118,10 @@ public void testToXContentForInternalStorage() throws IOException {
         }
     }
 
-    public void testFutureConfigParse() throws IOException {
+    public void testRestRequestParser_DoesntAllowInternalFields() throws IOException {
         XContentParser parser = XContentFactory.xContent(XContentType.JSON).createParser(XContentParserConfiguration.EMPTY, FUTURE_JOB);
-        XContentParseException e = expectThrows(XContentParseException.class, () -> Job.STRICT_PARSER.apply(parser, null).build());
-        assertEquals("[4:5] [job_details] unknown field [tomorrows_technology_today]", e.getMessage());
+        XContentParseException e = expectThrows(XContentParseException.class, () -> Job.REST_REQUEST_PARSER.apply(parser, null).build());
+        assertEquals("[3:5] [job_details] unknown field [create_time]", e.getMessage());
     }
 
     public void testFutureMetadataParse() throws IOException {
@@ -554,22 +553,6 @@ public void testBuilder_givenTimeFieldInAnalysisConfig() {
         assertThat(e.getMessage(), equalTo(Messages.getMessage(Messages.JOB_CONFIG_TIME_FIELD_NOT_ALLOWED_IN_ANALYSIS_CONFIG)));
     }
 
-    public void testInvalidCreateTimeSettings() {
-        Job.Builder builder = new Job.Builder("invalid-settings");
-        builder.setModelSnapshotId("snapshot-foo");
-        assertEquals(Collections.singletonList(Job.MODEL_SNAPSHOT_ID.getPreferredName()), builder.invalidCreateTimeSettings());
-
-        builder.setCreateTime(new Date());
-        builder.setFinishedTime(new Date());
-
-        Set<String> expected = new HashSet<>();
-        expected.add(Job.CREATE_TIME.getPreferredName());
-        expected.add(Job.FINISHED_TIME.getPreferredName());
-        expected.add(Job.MODEL_SNAPSHOT_ID.getPreferredName());
-
-        assertEquals(expected, new HashSet<>(builder.invalidCreateTimeSettings()));
-    }
-
     public void testEmptyGroup() {
         Job.Builder builder = buildJobBuilder("foo");
         builder.setGroups(Arrays.asList("foo-group", ""));

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/ml/jobs_crud.yml

@@ -1130,7 +1130,7 @@
 "Test cannot create job with model snapshot id set":
 
   - do:
-      catch: /illegal_argument_exception/
+      catch: /x_content_parse_exception/
       ml.put_job:
         job_id: has-model-snapshot-id
         body:  >

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/ml/validate.yml

@@ -76,21 +76,7 @@
 "Test job config is invalid because model snapshot id set":
 
   - do:
-      catch: /illegal_argument_exception/
-      ml.validate:
-        body: >
-          {
-            "model_snapshot_id": "wont-create-with-this-setting",
-            "analysis_config" : {
-                "bucket_span": "1h",
-                "detectors" :[{"function":"metric","field_name":"responsetime","by_field_name":"airline"}]
-            },
-            "data_description" : {
-            }
-          }
-
-  - do:
-      catch: /The job is configured with fields \[model_snapshot_id\] that are illegal to set at job creation/
+      catch: /x_content_parse_exception/
       ml.validate:
         body: >
           {