Validation of the _source to reject contradicting and ambiguous requests

modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/common/FetchSourceContextProtoUtilsTests.java

@@ -8,6 +8,7 @@
 
 package org.opensearch.transport.grpc.proto.request.common;
 
+import org.opensearch.OpenSearchException;
 import org.opensearch.core.common.Strings;
 import org.opensearch.protobufs.BulkRequest;
 import org.opensearch.protobufs.SearchRequest;
@@ -352,4 +353,23 @@ public void testFromProtoWithSourceConfigFilterBothIncludesAndExcludes() {
         assertArrayEquals("includes should match", new String[] { "include1", "include2" }, context.includes());
         assertArrayEquals("excludes should match", new String[] { "exclude1", "exclude2" }, context.excludes());
     }
+
+    public void testFromProtoWithSourceConfigFilterAmbiguousIncludesAndExcludes() {
+        // Create a SourceConfig with filter includes and excludes
+        final SourceConfig sourceConfig = SourceConfig.newBuilder()
+            .setFilter(
+                SourceFilter.newBuilder()
+                    .addIncludes("theSameEntry")
+                    .addIncludes("include2")
+                    .addExcludes("theSameEntry")
+                    .addExcludes("exclude2")
+                    .build()
+            )
+            .build();
+
+        // Exception when attempting to convert to FetchSourceContext
+        final OpenSearchException e = expectThrows(OpenSearchException.class, () -> FetchSourceContextProtoUtils.fromProto(sourceConfig));
+
+        assertEquals("The same entry [theSameEntry] cannot be both included and excluded in _source.", e.getMessage());
+    }
 }

rest-api-spec/src/main/resources/rest-api-spec/test/search/10_source_filtering.yml

@@ -34,6 +34,89 @@ setup:
   - length:   { hits.hits: 1  }
   - is_false: hits.hits.0._source
 
+---
+"_source as an empty object":
+  - skip:
+      version: " - 3.6.99"
+      reason: "validation was added later"
+      features: "warnings"
+  - do:
+      warnings:
+        - 'An empty object was provided as [_source]. Provide at least one of [includes] or [excludes] or use `_source: true` to fetch the entire source.'
+      search: { body: { _source: {  }, query: { match_all: {} } } }
+  - length:   { hits.hits: 1  }
+  - match: { hits.hits.0._source.count: 1 }
+
+---
+"_source as an empty array":
+  - skip:
+      version: " - 3.6.99"
+      reason: "validation was added later"
+      features: "warnings"
+  - do:
+      warnings:
+        - 'An empty array was provided as [_source]. Provide at least one field pattern or use `_source: true` to fetch the entire source.'
+      search: { body: { _source: [], query: { match_all: {} } } }
+  - length:   { hits.hits: 1  }
+  - match: { hits.hits.0._source.count: 1 }
+
+---
+"_source as object with an empty excludes array":
+  - skip:
+      version: " - 3.6.99"
+      reason: "validation was added later"
+      features: "warnings"
+  - do:
+      warnings:
+        - 'Expected at least one value for an array of [excludes]'
+      search:
+        body:
+          _source:
+            includes: [ include.field1, include.field2 ]
+            excludes: []
+          query: { match_all: {} }
+  - length:   { hits.hits: 1  }
+  - match:  { hits.hits.0._source.include.field1: v1 }
+  - match:  { hits.hits.0._source.include.field2: v2 }
+  - is_false: hits.hits.0._source.count
+
+---
+"_source as object with an empty includes array":
+  - skip:
+      version: " - 3.6.99"
+      reason: "validation was added later"
+      features: "warnings"
+  - do:
+      warnings:
+        - 'Expected at least one value for an array of [includes]'
+      search:
+        body:
+          _source:
+            includes: []
+            excludes: [ include.field1 ]
+          query: { match_all: {} }
+  - length:   { hits.hits: 1  }
+  - is_false: hits.hits.0._source.include.field1
+  - match:  { hits.hits.0._source.include.field2: v2 }
+  - match: { hits.hits.0._source.count: 1 }
+
+---
+"_source with an ambiguous field":
+  - skip:
+      version: " - 3.6.99"
+      reason: "validation was added later"
+  - do:
+      search:
+        body:
+          _source:
+            includes: [ include.field1, include.field2 ]
+            excludes: [ include.field1 ]
+          query: { match_all: {} }
+      catch: bad_request
+  - match: { status: 400 }
+  - match: { error.type: parsing_exception }
+  - match: { error.reason: "The same entry [include.field1] cannot be both included and excluded in _source." }
+
 ---
 "no filtering":
   - do: { search: { body: { query: { match_all: {} } } } }

server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java

@@ -32,8 +32,10 @@
 
 package org.opensearch.search.fetch.subphase;
 
+import org.opensearch.OpenSearchException;
 import org.opensearch.common.Booleans;
 import org.opensearch.common.annotation.PublicApi;
+import org.opensearch.common.logging.DeprecationLogger;
 import org.opensearch.common.xcontent.support.XContentMapValues;
 import org.opensearch.core.ParseField;
 import org.opensearch.core.common.ParsingException;
@@ -47,10 +49,12 @@
 import org.opensearch.rest.RestRequest;
 
 import java.io.IOException;
-import java.util.ArrayList;
 import java.util.Arrays;
-import java.util.List;
+import java.util.Collections;
+import java.util.HashSet;
+import java.util.LinkedHashSet;
 import java.util.Map;
+import java.util.Set;
 import java.util.function.Function;
 
 /**
@@ -60,12 +64,16 @@
  */
 @PublicApi(since = "1.0.0")
 public class FetchSourceContext implements Writeable, ToXContentObject {
+    private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(FetchSourceContext.class);
 
     public static final ParseField INCLUDES_FIELD = new ParseField("includes", "include");
     public static final ParseField EXCLUDES_FIELD = new ParseField("excludes", "exclude");
 
-    public static final FetchSourceContext FETCH_SOURCE = new FetchSourceContext(true);
-    public static final FetchSourceContext DO_NOT_FETCH_SOURCE = new FetchSourceContext(false);
+    public static final FetchSourceContext FETCH_SOURCE = new FetchSourceContext(true, null, null);
+    public static final FetchSourceContext DO_NOT_FETCH_SOURCE = new FetchSourceContext(false, null, null);
+
+    private static final String AMBIGUOUS_FIELD_MESSAGE = "The same entry [{}] cannot be both included and excluded in _source.";
+
     private final boolean fetchSource;
     private final String[] includes;
     private final String[] excludes;
@@ -75,6 +83,7 @@ public FetchSourceContext(boolean fetchSource, String[] includes, String[] exclu
         this.fetchSource = fetchSource;
         this.includes = includes == null ? Strings.EMPTY_ARRAY : includes;
         this.excludes = excludes == null ? Strings.EMPTY_ARRAY : excludes;
+        validateAmbiguousFields();
     }
 
     public FetchSourceContext(boolean fetchSource) {
@@ -87,6 +96,19 @@ public FetchSourceContext(StreamInput in) throws IOException {
         excludes = in.readStringArray();
     }
 
+    /**
+     * The same entry cannot be both included and excluded in _source.
+     * Since the constructors are public, this validation is required to be called in the constructor.
+     * */
+    private void validateAmbiguousFields() {
+        Set<String> includeSet = new HashSet<>(Arrays.asList(this.includes));
+        for (String exclude : this.excludes) {
+            if (includeSet.contains(exclude)) {
+                throw new OpenSearchException(AMBIGUOUS_FIELD_MESSAGE, exclude);
+            }
+        }
+    }
+
     @Override
     public void writeTo(StreamOutput out) throws IOException {
         out.writeBoolean(fetchSource);
@@ -149,12 +171,7 @@ public static FetchSourceContext fromXContent(XContentParser parser) throws IOEx
                 return new FetchSourceContext(true, includes, null);
             }
             case XContentParser.Token.START_ARRAY -> {
-                ArrayList<String> list = new ArrayList<>();
-                while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) {
-                    list.add(parser.text());
-                }
-                String[] includes = list.toArray(new String[0]);
-                return new FetchSourceContext(true, includes, null);
+                return parseSourceArray(parser);
             }
             case XContentParser.Token.START_OBJECT -> {
                 return parseSourceObject(parser);
@@ -165,6 +182,10 @@ public static FetchSourceContext fromXContent(XContentParser parser) throws IOEx
                     "Expected one of ["
                         + XContentParser.Token.VALUE_BOOLEAN
                         + ", "
+                        + XContentParser.Token.VALUE_STRING
+                        + ", "
+                        + XContentParser.Token.START_ARRAY
+                        + ", "
                         + XContentParser.Token.START_OBJECT
                         + "] but found ["
                         + token
@@ -174,10 +195,43 @@ public static FetchSourceContext fromXContent(XContentParser parser) throws IOEx
         }
     }
 
-    private static FetchSourceContext parseSourceObject(XContentParser parser) throws IOException {
+    static FetchSourceContext parseSourceArray(XContentParser parser) throws IOException {
+        Set<String> includes = new LinkedHashSet<>();
+        if (parser.currentToken() != XContentParser.Token.START_ARRAY) {
+            throw new ParsingException(
+                parser.getTokenLocation(),
+                "Expected a "
+                    + XContentParser.Token.START_ARRAY
+                    + " but got a "
+                    + parser.currentToken()
+                    + " in ["
+                    + parser.currentName()
+                    + "]."
+            );
+        }
+        while (parser.nextToken() != XContentParser.Token.END_ARRAY) {
+            if (parser.currentToken() == XContentParser.Token.VALUE_STRING) {
+                includes.add(parser.text());
+            } else {
+                throw new ParsingException(
+                    parser.getTokenLocation(),
+                    "Unknown key for a " + parser.currentToken() + " in [" + parser.currentName() + "]."
+                );
+            }
+        }
+        if (includes.isEmpty()) {
+            deprecationLogger.deprecate(
+                "empty_source_array",
+                "An empty array was provided as [_source]. Provide at least one field pattern or use `_source: true` to fetch the entire source."
+            );
+        }
+        return new FetchSourceContext(true, includes.toArray(new String[0]), null);
+    }
+
+    static FetchSourceContext parseSourceObject(XContentParser parser) throws IOException {
         XContentParser.Token token = parser.currentToken();
-        String[] includes = Strings.EMPTY_ARRAY;
-        String[] excludes = Strings.EMPTY_ARRAY;
+        Set<String> includes = Collections.emptySet();
+        Set<String> excludes = Collections.emptySet();
         String currentFieldName = null;
         if (token != XContentParser.Token.START_OBJECT) {
             throw new ParsingException(
@@ -190,13 +244,19 @@ private static FetchSourceContext parseSourceObject(XContentParser parser) throw
                 currentFieldName = parser.currentName();
                 continue; // only field name is required in this iteration
             }
+            if (currentFieldName == null) {
+                throw new ParsingException(
+                    parser.getTokenLocation(),
+                    "Expected a field name but got a " + token + " in [" + parser.currentName() + "]."
+                );
+            }
             // process field value
             switch (token) {
                 case XContentParser.Token.START_ARRAY -> {
                     if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                        includes = parseSourceArray(parser).toArray(new String[0]);
+                        includes = parseSourceFieldArray(parser, INCLUDES_FIELD, excludes);
                     } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                        excludes = parseSourceArray(parser).toArray(new String[0]);
+                        excludes = parseSourceFieldArray(parser, EXCLUDES_FIELD, includes);
                     } else {
                         throw new ParsingException(
                             parser.getTokenLocation(),
@@ -206,9 +266,17 @@ private static FetchSourceContext parseSourceObject(XContentParser parser) throw
                 }
                 case XContentParser.Token.VALUE_STRING -> {
                     if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                        includes = new String[] { parser.text() };
+                        String includeEntry = parser.text();
+                        if (excludes.contains(includeEntry)) {
+                            throw new ParsingException(parser.getTokenLocation(), AMBIGUOUS_FIELD_MESSAGE, includeEntry);
+                        }
+                        includes = Collections.singleton(includeEntry);
                     } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) {
-                        excludes = new String[] { parser.text() };
+                        String excludeEntry = parser.text();
+                        if (includes.contains(excludeEntry)) {
+                            throw new ParsingException(parser.getTokenLocation(), AMBIGUOUS_FIELD_MESSAGE, excludeEntry);
+                        }
+                        excludes = Collections.singleton(excludeEntry);
                     } else {
                         throw new ParsingException(
                             parser.getTokenLocation(),
@@ -221,21 +289,43 @@ private static FetchSourceContext parseSourceObject(XContentParser parser) throw
                 }
             }
         }
-        return new FetchSourceContext(true, includes, excludes);
+        if (includes.isEmpty() && excludes.isEmpty()) {
+            // no valid field names -> empty or unrecognized fields; deprecated
+            deprecationLogger.deprecate(
+                "empty_source_object",
+                "An empty object was provided as [_source]. Provide at least one of ["
+                    + INCLUDES_FIELD.getPreferredName()
+                    + "] or ["
+                    + EXCLUDES_FIELD.getPreferredName()
+                    + "] or use `_source: true` to fetch the entire source."
+            );
+        }
+        return new FetchSourceContext(true, includes.toArray(new String[0]), excludes.toArray(new String[0]));
     }
 
-    private static List<String> parseSourceArray(XContentParser parser) throws IOException {
-        List<String> sourceArr = new ArrayList<>();
+    private static Set<String> parseSourceFieldArray(XContentParser parser, ParseField parseField, Set<String> opposite)
+        throws IOException {
+        Set<String> sourceArr = new LinkedHashSet<>(); // include or exclude lists, LinkedHashSet preserves the order of fields
         while (parser.nextToken() != XContentParser.Token.END_ARRAY) {
             if (parser.currentToken() == XContentParser.Token.VALUE_STRING) {
-                sourceArr.add(parser.text());
+                String entry = parser.text();
+                if (opposite != null && opposite.contains(entry)) {
+                    throw new ParsingException(parser.getTokenLocation(), AMBIGUOUS_FIELD_MESSAGE, entry);
+                }
+                sourceArr.add(entry);
             } else {
                 throw new ParsingException(
                     parser.getTokenLocation(),
                     "Unknown key for a " + parser.currentToken() + " in [" + parser.currentName() + "]."
                 );
             }
         }
+        if (sourceArr.isEmpty()) {
+            deprecationLogger.deprecate(
+                "empty_source_" + parseField.getPreferredName(),
+                "Expected at least one value for an array of [" + parseField.getPreferredName() + "]"
+            );
+        }
         return sourceArr;
     }