diff --git a/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/common/FetchSourceContextProtoUtilsTests.java b/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/common/FetchSourceContextProtoUtilsTests.java index 4547be75fe2fd..7efaa1f11e21b 100644 --- a/modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/common/FetchSourceContextProtoUtilsTests.java +++ b/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()); + } } diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search/10_source_filtering.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search/10_source_filtering.yml index 4de2e8142f6ec..de807ef8f1a09 100644 --- a/rest-api-spec/src/main/resources/rest-api-spec/test/search/10_source_filtering.yml +++ b/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: {} } } } } diff --git a/server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java b/server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java index 3d1f42b7e1eb7..fda7c41a9a268 100644 --- a/server/src/main/java/org/opensearch/search/fetch/subphase/FetchSourceContext.java +++ b/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 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 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 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 includes = Collections.emptySet(); + Set 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,14 +289,30 @@ 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 parseSourceArray(XContentParser parser) throws IOException { - List sourceArr = new ArrayList<>(); + private static Set parseSourceFieldArray(XContentParser parser, ParseField parseField, Set opposite) + throws IOException { + Set 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(), @@ -236,6 +320,12 @@ private static List parseSourceArray(XContentParser parser) throws IOExc ); } } + if (sourceArr.isEmpty()) { + deprecationLogger.deprecate( + "empty_source_" + parseField.getPreferredName(), + "Expected at least one value for an array of [" + parseField.getPreferredName() + "]" + ); + } return sourceArr; } diff --git a/server/src/test/java/org/opensearch/search/fetch/subphase/FetchSourceContextTests.java b/server/src/test/java/org/opensearch/search/fetch/subphase/FetchSourceContextTests.java new file mode 100644 index 0000000000000..db7fd5b11c618 --- /dev/null +++ b/server/src/test/java/org/opensearch/search/fetch/subphase/FetchSourceContextTests.java @@ -0,0 +1,280 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +/* + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ + +package org.opensearch.search.fetch.subphase; + +import org.opensearch.common.xcontent.XContentFactory; +import org.opensearch.core.common.ParsingException; +import org.opensearch.core.xcontent.XContentBuilder; +import org.opensearch.core.xcontent.XContentParser; +import org.opensearch.test.OpenSearchTestCase; + +import java.io.IOException; +import java.util.Arrays; + +public class FetchSourceContextTests extends OpenSearchTestCase { + + private XContentParser createSourceParser(XContentBuilder source) throws IOException { + XContentParser parser = createParser(source); + parser.nextToken(); // move to start object + parser.nextToken(); // move to field name "_source" + parser.nextToken(); // move to _source value to parse + return parser; + } + + public void testFetchSource() throws IOException { + final XContentBuilder source = XContentFactory.jsonBuilder().startObject().field("_source", true).endObject(); + final XContentParser parser = createSourceParser(source); + + FetchSourceContext result = FetchSourceContext.fromXContent(parser); + assertEquals(FetchSourceContext.FETCH_SOURCE, result); + } + + public void testDoNotFetchSource() throws IOException { + final XContentBuilder source = XContentFactory.jsonBuilder().startObject().field("_source", false).endObject(); + final XContentParser parser = createSourceParser(source); + + FetchSourceContext result = FetchSourceContext.fromXContent(parser); + assertEquals(FetchSourceContext.DO_NOT_FETCH_SOURCE, result); + } + + public void testFetchSourceString() throws IOException { + final XContentBuilder source = XContentFactory.jsonBuilder().startObject().field("_source", "include1").endObject(); + final XContentParser parser = createSourceParser(source); + + FetchSourceContext result = FetchSourceContext.fromXContent(parser); + assertTrue(result.fetchSource()); // fetch source + assertArrayEquals(new String[] { "include1" }, result.includes()); // single include + assertEquals(0, result.excludes().length); // no excludes + } + + public void testFetchSourceArray() throws IOException { + final XContentBuilder source = XContentFactory.jsonBuilder() + .startObject() + .field("_source") + .startArray() + .value("include1") + .value("include2") + .value("include2") + .endArray() + .endObject(); + final XContentParser parser = createSourceParser(source); + + FetchSourceContext result = FetchSourceContext.fromXContent(parser); + assertTrue(result.fetchSource()); // fetch source + // validate includes + assertEquals(2, result.includes().length); // no duplicates + assertTrue(Arrays.asList(result.includes()).containsAll(Arrays.asList("include1", "include2"))); + // validate no excludes + assertEquals(0, result.excludes().length); + } + + public void testFetchSourceAsArrayAssertWarningExplicitEmptyArray() throws IOException { + final XContentBuilder source = XContentFactory.jsonBuilder().startObject().field("_source").startArray().endArray().endObject(); + final XContentParser parser = createSourceParser(source); + + FetchSourceContext result = FetchSourceContext.fromXContent(parser); + assertTrue(result.fetchSource()); + assertWarnings( + "An empty array was provided as [_source]. Provide at least one field pattern or use `_source: true` to fetch the entire source." + ); + } + + public void testFetchSourceAsObject() throws IOException { + final XContentBuilder source = XContentFactory.jsonBuilder() + .startObject() + .field("_source") + .startObject() + .field("includes", "include1") + .field("excludes", "exclude1") + .endObject() + .endObject(); + final XContentParser parser = createSourceParser(source); + + FetchSourceContext result = FetchSourceContext.fromXContent(parser); + assertTrue(result.fetchSource()); // fetch source + assertArrayEquals(new String[] { "include1" }, result.includes()); // single include + assertArrayEquals(new String[] { "exclude1" }, result.excludes()); // single exclude + } + + public void testFetchSourceAsObjectBothIncludeAndExcludeArrays() throws IOException { + final XContentBuilder source = XContentFactory.jsonBuilder() + .startObject() + .field("_source") + .startObject() + .field("includes") + .startArray() + .value("iii") + .endArray() + .field("excludes") + .startArray() + .value("aaa") + .value("bbb") + .endArray() + .endObject() + .endObject(); + final XContentParser parser = createSourceParser(source); + + FetchSourceContext result = FetchSourceContext.fromXContent(parser); + assertTrue(result.fetchSource()); + // validate includes + assertArrayEquals(new String[] { "iii" }, result.includes()); + // validate excludes + assertEquals(2, result.excludes().length); // no duplicates + assertTrue(Arrays.asList(result.excludes()).containsAll(Arrays.asList("aaa", "bbb"))); + } + + public void testFetchSourceAssertWarningEmptyObject() throws IOException { + final XContentBuilder source = XContentFactory.jsonBuilder().startObject().field("_source").startObject().endObject().endObject(); + final XContentParser parser = createSourceParser(source); + + FetchSourceContext result = FetchSourceContext.fromXContent(parser); + assertTrue(result.fetchSource()); + assertWarnings( + "An empty object was provided as [_source]. Provide at least one of [" + + FetchSourceContext.INCLUDES_FIELD.getPreferredName() + + "] or [" + + FetchSourceContext.EXCLUDES_FIELD.getPreferredName() + + "] or use `_source: true` to fetch the entire source." + ); + } + + public void testFetchSourceObjectAssertWarningExplicitEmptyExcludes() throws IOException { + final XContentBuilder source = XContentFactory.jsonBuilder() + .startObject() + .field("_source") + .startObject() + .field("includes", "include1") + .field("excludes") + .startArray() + .endArray() + .endObject() + .endObject(); + final XContentParser parser = createSourceParser(source); + + FetchSourceContext result = FetchSourceContext.fromXContent(parser); + assertTrue(result.fetchSource()); + assertWarnings("Expected at least one value for an array of [" + FetchSourceContext.EXCLUDES_FIELD.getPreferredName() + "]"); + } + + public void testFetchSourceObjectAssertWarningExplicitEmptyIncludes() throws IOException { + final XContentBuilder source = XContentFactory.jsonBuilder() + .startObject() + .field("_source") + .startObject() + .field("excludes") + .startArray() + .value("exclude1") + .endArray() + .field("includes") + .startArray() + .endArray() + .endObject() + .endObject(); + final XContentParser parser = createSourceParser(source); + + FetchSourceContext result = FetchSourceContext.fromXContent(parser); + assertTrue(result.fetchSource()); + assertWarnings("Expected at least one value for an array of [" + FetchSourceContext.INCLUDES_FIELD.getPreferredName() + "]"); + } + + public void testFetchSourceAsObjectConflictingEntries() throws IOException { + { + final XContentBuilder source = XContentFactory.jsonBuilder() + .startObject() + .field("_source") + .startObject() + .field("includes") + .value("AAA") + .field("excludes") + .value("AAA") + .endObject() + .endObject(); + final XContentParser parser = createSourceParser(source); + + ParsingException result = expectThrows(ParsingException.class, () -> FetchSourceContext.fromXContent(parser)); + assertEquals("The same entry [AAA] cannot be both included and excluded in _source.", result.getMessage()); + } + { + final XContentBuilder source = XContentFactory.jsonBuilder() + .startObject() + .field("_source") + .startObject() + .field("includes") + .value("AAA") + .field("excludes") + .startArray() + .value("AAA") + .value("BBB") + .endArray() + .endObject() + .endObject(); + final XContentParser parser = createSourceParser(source); + + ParsingException result = expectThrows(ParsingException.class, () -> FetchSourceContext.fromXContent(parser)); + assertEquals("The same entry [AAA] cannot be both included and excluded in _source.", result.getMessage()); + } + { + final XContentBuilder source = XContentFactory.jsonBuilder() + .startObject() + .field("_source") + .startObject() + .field("includes") + .startArray() + .value("AAA") + .value("BBB") + .endArray() + .field("excludes") + .value("AAA") + .endObject() + .endObject(); + final XContentParser parser = createSourceParser(source); + + ParsingException result = expectThrows(ParsingException.class, () -> FetchSourceContext.fromXContent(parser)); + assertEquals("The same entry [AAA] cannot be both included and excluded in _source.", result.getMessage()); + } + { + final XContentBuilder source = XContentFactory.jsonBuilder() + .startObject() + .field("_source") + .startObject() + .field("includes") + .startArray() + .value("AAA") + .value("BBB") + .endArray() + .field("excludes") + .startArray() + .value("BBB") + .value("CCC") + .endArray() + .endObject() + .endObject(); + final XContentParser parser = createSourceParser(source); + + ParsingException result = expectThrows(ParsingException.class, () -> FetchSourceContext.fromXContent(parser)); + assertEquals("The same entry [BBB] cannot be both included and excluded in _source.", result.getMessage()); + } + } + + public void testParseSourceObjectInvalidInput() throws IOException { + final XContentBuilder source = XContentFactory.jsonBuilder().startObject().field("_source", true).endObject(); + final XContentParser parser = createSourceParser(source); + + ParsingException invalidObject = expectThrows(ParsingException.class, () -> FetchSourceContext.parseSourceObject(parser)); + assertEquals("Expected a START_OBJECT but got a VALUE_BOOLEAN in [_source].", invalidObject.getMessage()); + + ParsingException invalidArray = expectThrows(ParsingException.class, () -> FetchSourceContext.parseSourceArray(parser)); + assertEquals("Expected a START_ARRAY but got a VALUE_BOOLEAN in [_source].", invalidArray.getMessage()); + } +}