[ML] Improve mapping clash error message (elastic/x-pack-elasticsearch#1968)

relates elastic/x-pack-elasticsearch#1751

Original commit: elastic/x-pack-elasticsearch@01c221bf42

plugin/src/main/java/org/elasticsearch/xpack/ml/job/JobManager.java

@@ -43,6 +43,8 @@ import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
 import java.util.function.Consumer;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
 /**
@@ -191,13 +193,17 @@ public class JobManager extends AbstractComponent {
 
             @Override
             public void onFailure(Exception e) {
-                if (e instanceof IllegalArgumentException
+                if (e instanceof IllegalArgumentException) {
-                        && e.getMessage().matches("mapper \\[.*\\] of different type, current_type \\[.*\\], merged_type \\[.*\\]")) {
+                    // the underlying error differs depending on which way around the clashing fields are seen
-                    actionListener.onFailure(ExceptionsHelper.badRequestException(Messages.JOB_CONFIG_MAPPING_TYPE_CLASH, e));
+                    Matcher matcher = Pattern.compile("(?:mapper|Can't merge a non object mapping) \\[(.*)\\] (?:of different type, " +
-                } else {
+                            "current_type \\[.*\\], merged_type|with an object mapping) \\[.*\\]").matcher(e.getMessage());
-                    actionListener.onFailure(e);
+                    if (matcher.matches()) {
+                        String msg = Messages.getMessage(Messages.JOB_CONFIG_MAPPING_TYPE_CLASH, matcher.group(1));
+                        actionListener.onFailure(ExceptionsHelper.badRequestException(msg, e));
+                        return;
                     }
-
+                }
+                actionListener.onFailure(e);
             }
         };
 

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

@@ -601,8 +601,8 @@ public class AnalysisConfig implements ToXContentObject, Writeable {
             String prevTermField = null;
             for (String termField : termFields) {
                 if (prevTermField != null && termField.startsWith(prevTermField + ".")) {
-                    throw ExceptionsHelper.badRequestException("Fields " + prevTermField + " and " + termField +
+                    throw ExceptionsHelper.badRequestException("Fields [" + prevTermField + "] and [" + termField +
-                            " cannot both be used in the same analysis_config");
+                            "] cannot both be used in the same analysis_config");
                 }
                 prevTermField = termField;
             }

plugin/src/main/java/org/elasticsearch/xpack/ml/job/messages/Messages.java

@@ -135,8 +135,7 @@ public final class Messages {
     public static final String JOB_CONFIG_DETECTOR_OVER_DISALLOWED =
             "''over'' is not a permitted value for {0}";
     public static final String JOB_CONFIG_MAPPING_TYPE_CLASH =
-            "A field has a different mapping type to an existing field with the same name. " +
+            "This job would cause a mapping clash with existing field [{0}] - avoid the clash by assigning a dedicated results index";
-                    "Use the 'results_index_name' setting to assign the job to another index";
     public static final String JOB_CONFIG_TIME_FIELD_NOT_ALLOWED_IN_ANALYSIS_CONFIG =
             "data_description.time_field may not be used in the analysis_config";
 

plugin/src/test/java/org/elasticsearch/xpack/ml/integration/MlJobIT.java

@@ -339,9 +339,17 @@ public class MlJobIT extends ESRestTestCase {
                 "}";
 
         String jobId1 = "job-with-response-field";
-        String byFieldName1 = "response";
+        String byFieldName1;
         String jobId2 = "job-will-fail-with-mapping-error-on-response-field";
-        String byFieldName2 = "response.time";
+        String byFieldName2;
+        // we should get the friendly advice nomatter which way around the clashing fields are seen
+        if (randomBoolean()) {
+            byFieldName1 = "response";
+            byFieldName2 = "response.time";
+        } else {
+            byFieldName1 = "response.time";
+            byFieldName2 = "response";
+        }
         String jobConfig = String.format(Locale.ROOT, jobTemplate, byFieldName1);
 
         Response response = client().performRequest("put", MachineLearning.BASE_PATH
@@ -354,8 +362,8 @@ public class MlJobIT extends ESRestTestCase {
                         Collections.emptyMap(), new StringEntity(failingJobConfig, ContentType.APPLICATION_JSON)));
 
         assertThat(e.getMessage(),
-                containsString("A field has a different mapping type to an existing field with the same name. " +
+                containsString("This job would cause a mapping clash with existing field [response] - " +
-                        "Use the 'results_index_name' setting to assign the job to another index"));
+                        "avoid the clash by assigning a dedicated results index"));
     }
 
     public void testDeleteJob() throws Exception {

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

@@ -308,7 +308,7 @@ public class AnalysisConfigTests extends AbstractSerializingTestCase<AnalysisCon
         AnalysisConfig.Builder ac = new AnalysisConfig.Builder(Arrays.asList(detector1.build(), detector2.build()));
 
         ElasticsearchException e = expectThrows(ElasticsearchException.class, ac::build);
-        assertThat(e.getMessage(), equalTo("Fields a and a.b cannot both be used in the same analysis_config"));
+        assertThat(e.getMessage(), equalTo("Fields [a] and [a.b] cannot both be used in the same analysis_config"));
     }
 
     public void testBuild_GivenOverlappingNestedFields() {
@@ -319,7 +319,7 @@ public class AnalysisConfigTests extends AbstractSerializingTestCase<AnalysisCon
         ac.setInfluencers(Arrays.asList("a.b", "d"));
 
         ElasticsearchException e = expectThrows(ElasticsearchException.class, ac::build);
-        assertThat(e.getMessage(), equalTo("Fields a.b and a.b.c cannot both be used in the same analysis_config"));
+        assertThat(e.getMessage(), equalTo("Fields [a.b] and [a.b.c] cannot both be used in the same analysis_config"));
     }
 
     public void testBuild_GivenNonOverlappingNestedFields() {