Fix/meta fields bad request (elastic#117229)

400 rather a 5xx error is returned when _source / _seq_no / _feature / _nested_path / _field_names is requested, via fields
piergm · Dec 3, 2024 · a514aad · a514aad
1 parent cbb08ba
commit a514aad
Show file tree

Hide file tree

Showing 10 changed files with 92 additions and 12 deletions.
diff --git a/docs/changelog/117229.yaml b/docs/changelog/117229.yaml
@@ -0,0 +1,6 @@
+pr: 117229
+summary: "In this pr, a 400 error is returned when _source / _seq_no / _feature /\
+  \ _nested_path / _field_names is requested, rather a 5xx"
+area: Search
+type: bug
+issues: []
diff --git a/...xtras/src/main/java/org/elasticsearch/index/mapper/extras/RankFeatureMetaFieldMapper.java b/...xtras/src/main/java/org/elasticsearch/index/mapper/extras/RankFeatureMetaFieldMapper.java
@@ -48,7 +48,7 @@ public String typeName() {
 
         @Override
         public ValueFetcher valueFetcher(SearchExecutionContext context, String format) {
-            throw new UnsupportedOperationException("Cannot fetch values for internal field [" + typeName() + "].");
+            throw new IllegalArgumentException("Cannot fetch values for internal field [" + typeName() + "].");
         }
 
         @Override

diff --git a/rest-api-spec/build.gradle b/rest-api-spec/build.gradle
@@ -66,4 +66,5 @@ tasks.named("yamlRestCompatTestTransform").configure ({ task ->
   task.skipTest("logsdb/20_source_mapping/stored _source mode is supported", "no longer serialize source_mode")
   task.skipTest("logsdb/20_source_mapping/include/exclude is supported with stored _source", "no longer serialize source_mode")
   task.skipTest("logsdb/20_source_mapping/synthetic _source is default", "no longer serialize source_mode")
+  task.skipTest("search/520_fetch_fields/fetch _seq_no via fields", "error code is changed from 5xx to 400 in 9.0")
 })
diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/520_fetch_fields.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search/520_fetch_fields.yml
@@ -128,18 +128,88 @@ fetch _seq_no via stored_fields:
 
 ---
 fetch _seq_no via fields:
+  - requires:
+      cluster_features: ["meta_fetch_fields_error_code_changed"]
+      reason: The fields_api returns a 400 instead a 5xx when _seq_no is requested via fields
 
   - do:
-      catch: "request"
+      catch: bad_request
       search:
         index: test
         body:
           fields: [ _seq_no ]
 
-  # This should be `unauthorized` (401) or `forbidden` (403) or at least `bad request` (400)
-  # while instead it is mapped to an `internal_server_error (500)`
-  - match: { status: 500 }
-  - match: { error.root_cause.0.type: unsupported_operation_exception }
+  - match: { status: 400 }
+  - match: { error.root_cause.0.type: illegal_argument_exception }
+  - match: { error.root_cause.0.reason: "error fetching [_seq_no]: Cannot fetch values for internal field [_seq_no]." }
+
+---
+fetch _source via fields:
+  - requires:
+      cluster_features: ["meta_fetch_fields_error_code_changed"]
+      reason: The fields_api returns a 400 instead a 5xx when _seq_no is requested via fields
+
+  - do:
+      catch: bad_request
+      search:
+        index: test
+        body:
+          fields: [ _source ]
+
+  - match: { status: 400 }
+  - match: { error.root_cause.0.type: illegal_argument_exception }
+  - match: { error.root_cause.0.reason: "error fetching [_source]: Cannot fetch values for internal field [_source]." }
+
+---
+fetch _feature via fields:
+  - requires:
+      cluster_features: ["meta_fetch_fields_error_code_changed"]
+      reason: The fields_api returns a 400 instead a 5xx when _seq_no is requested via fields
+
+  - do:
+      catch: bad_request
+      search:
+        index: test
+        body:
+          fields: [ _feature ]
+
+  - match: { status: 400 }
+  - match: { error.root_cause.0.type: illegal_argument_exception }
+  - match: { error.root_cause.0.reason: "error fetching [_feature]: Cannot fetch values for internal field [_feature]." }
+
+---
+fetch _nested_path via fields:
+  - requires:
+      cluster_features: ["meta_fetch_fields_error_code_changed"]
+      reason: The fields_api returns a 400 instead a 5xx when _seq_no is requested via fields
+
+  - do:
+      catch: bad_request
+      search:
+        index: test
+        body:
+          fields: [ _nested_path ]
+
+  - match: { status: 400 }
+  - match: { error.root_cause.0.type: illegal_argument_exception }
+  - match: { error.root_cause.0.reason: "error fetching [_nested_path]: Cannot fetch values for internal field [_nested_path]." }
+
+---
+fetch _field_names via fields:
+  - requires:
+      cluster_features: ["meta_fetch_fields_error_code_changed"]
+      reason: The fields_api returns a 400 instead a 5xx when _seq_no is requested via fields
+
+  - do:
+      catch: bad_request
+      search:
+        index: test
+        body:
+          fields: [ _field_names ]
+
+  - match: { status: 400 }
+  - match: { error.root_cause.0.type: illegal_argument_exception }
+  - match: { error.root_cause.0.reason: "error fetching [_field_names]: Cannot fetch values for internal field [_field_names]." }
 
 ---
 fetch fields with none stored_fields:

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/FieldNamesFieldMapper.java
@@ -135,7 +135,7 @@ public boolean isEnabled() {
 
         @Override
         public ValueFetcher valueFetcher(SearchExecutionContext context, String format) {
-            throw new UnsupportedOperationException("Cannot fetch values for internal field [" + name() + "].");
+            throw new IllegalArgumentException("Cannot fetch values for internal field [" + name() + "].");
         }
 
         @Override

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java b/server/src/main/java/org/elasticsearch/index/mapper/MapperFeatures.java
@@ -61,6 +61,8 @@ public Set<NodeFeature> getFeatures() {
         "mapper.constant_keyword.synthetic_source_write_fix"
     );
 
+    public static final NodeFeature META_FETCH_FIELDS_ERROR_CODE_CHANGED = new NodeFeature("meta_fetch_fields_error_code_changed");
+
     @Override
     public Set<NodeFeature> getTestFeatures() {
         return Set.of(
@@ -71,7 +73,8 @@ public Set<NodeFeature> getTestFeatures() {
             IgnoredSourceFieldMapper.IGNORED_SOURCE_AS_TOP_LEVEL_METADATA_ARRAY_FIELD,
             IgnoredSourceFieldMapper.ALWAYS_STORE_OBJECT_ARRAYS_IN_NESTED_OBJECTS,
             MapperService.LOGSDB_DEFAULT_IGNORE_DYNAMIC_BEYOND_LIMIT,
-            CONSTANT_KEYWORD_SYNTHETIC_SOURCE_WRITE_FIX
+            CONSTANT_KEYWORD_SYNTHETIC_SOURCE_WRITE_FIX,
+            META_FETCH_FIELDS_ERROR_CODE_CHANGED
         );
     }
 }
diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NestedPathFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NestedPathFieldMapper.java
@@ -67,7 +67,7 @@ public Query existsQuery(SearchExecutionContext context) {
 
         @Override
         public ValueFetcher valueFetcher(SearchExecutionContext context, String format) {
-            throw new UnsupportedOperationException("Cannot fetch values for internal field [" + name() + "].");
+            throw new IllegalArgumentException("Cannot fetch values for internal field [" + name() + "].");
         }
 
         @Override

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SeqNoFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/SeqNoFieldMapper.java
@@ -168,7 +168,7 @@ public boolean mayExistInIndex(SearchExecutionContext context) {
 
         @Override
         public ValueFetcher valueFetcher(SearchExecutionContext context, String format) {
-            throw new UnsupportedOperationException("Cannot fetch values for internal field [" + name() + "].");
+            throw new IllegalArgumentException("Cannot fetch values for internal field [" + name() + "].");
         }
 
         @Override

diff --git a/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/SourceFieldMapper.java
@@ -325,7 +325,7 @@ public String typeName() {
 
         @Override
         public ValueFetcher valueFetcher(SearchExecutionContext context, String format) {
-            throw new UnsupportedOperationException("Cannot fetch values for internal field [" + name() + "].");
+            throw new IllegalArgumentException("Cannot fetch values for internal field [" + name() + "].");
         }
 
         @Override

diff --git a/server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java b/server/src/test/java/org/elasticsearch/search/fetch/subphase/FieldFetcherTests.java
@@ -271,7 +271,7 @@ public void testMetadataFields() throws IOException {
             FieldNamesFieldMapper.NAME,
             NestedPathFieldMapper.name(IndexVersion.current())
         )) {
-            expectThrows(UnsupportedOperationException.class, () -> fetchFields(mapperService, source, fieldname));
+            expectThrows(IllegalArgumentException.class, () -> fetchFields(mapperService, source, fieldname));
         }
     }