[ML] Move job group validation after parsing (elastic/x-pack-elasticsearch#2207)

Validating job groups during parsing results into
the validation error being wrapped into a parse
exception. The UI then does not display the cause of the
error. Finally, it is conceptually not a parse error, so
it belongs outside the parsing phase.

Original commit: elastic/x-pack-elasticsearch@a03f002bdc

plugin/src/main/java/org/elasticsearch/xpack/ml/job/config/Job.java

@@ -694,11 +694,6 @@ public class Job extends AbstractDiffable<Job> implements Writeable, ToXContentO
 
         public void setGroups(List<String> groups) {
             this.groups = groups == null ? Collections.emptyList() : groups;
-            for (String group : this.groups) {
-                if (MlStrings.isValidId(group) == false) {
-                    throw new IllegalArgumentException(Messages.getMessage(Messages.INVALID_GROUP, group));
-                }
-            }
         }
 
         public Date getCreateTime() {
@@ -994,6 +989,8 @@ public class Job extends AbstractDiffable<Job> implements Writeable, ToXContentO
                 throw new IllegalArgumentException(Messages.getMessage(Messages.JOB_CONFIG_ID_TOO_LONG, MAX_JOB_ID_LENGTH));
             }
 
+            validateGroups();
+
             // Results index name not specified in user input means use the default, so is acceptable in this validation
             if (!Strings.isNullOrEmpty(resultsIndexName) && !MlStrings.isValidId(resultsIndexName)) {
                 throw new IllegalArgumentException(
@@ -1003,6 +1000,14 @@ public class Job extends AbstractDiffable<Job> implements Writeable, ToXContentO
             // Creation time is NOT required in user input, hence validated only on build
         }
 
+        private void validateGroups() {
+            for (String group : this.groups) {
+                if (MlStrings.isValidId(group) == false) {
+                    throw new IllegalArgumentException(Messages.getMessage(Messages.INVALID_GROUP, group));
+                }
+            }
+        }
+
         /**
          * Builds a job with the given {@code createTime} and the current version.
          * This should be used when a new job is created as opposed to {@link #build()}.

plugin/src/test/java/org/elasticsearch/xpack/ml/job/config/JobTests.java

@@ -470,15 +470,17 @@ public class JobTests extends AbstractSerializingTestCase<Job> {
 
     public void testEmptyGroup() {
         Job.Builder builder = buildJobBuilder("foo");
+        builder.setGroups(Arrays.asList("foo-group", ""));
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
-                () -> builder.setGroups(Arrays.asList("foo-group", "")));
+                () -> builder.build());
         assertThat(e.getMessage(), containsString("Invalid group id ''"));
     }
 
     public void testInvalidGroup() {
         Job.Builder builder = buildJobBuilder("foo");
+        builder.setGroups(Arrays.asList("foo-group", "$$$"));
         IllegalArgumentException e = expectThrows(IllegalArgumentException.class,
-                () -> builder.setGroups(Arrays.asList("foo-group", "$$$")));
+                () -> builder.build());
         assertThat(e.getMessage(), containsString("Invalid group id '$$$'"));
     }