From c1d14089f5a987688d03cf09511e62317ace7dd7 Mon Sep 17 00:00:00 2001 From: Mikhail Urmich Date: Fri, 20 Feb 2026 14:11:51 +0100 Subject: [PATCH 1/5] Refactor Signed-off-by: Mikhail Urmich Signed-off-by: Mikhail Urmich --- .../FetchSourceContextProtoUtilsTests.java | 1 + .../fetch/subphase/FetchSourceContext.java | 78 +++++++++++-------- 2 files changed, 47 insertions(+), 32 deletions(-) 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..cfa306bbcc26a 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 @@ -18,6 +18,7 @@ import org.opensearch.search.fetch.subphase.FetchSourceContext; import org.opensearch.test.OpenSearchTestCase; +// ISSUE-20612 Source Validation public class FetchSourceContextProtoUtilsTests extends OpenSearchTestCase { public void testParseFromProtoRequestWithBoolValue() { 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..3f1b3f3ec95ce 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 @@ -47,12 +47,13 @@ import org.opensearch.rest.RestRequest; import java.io.IOException; -import java.util.ArrayList; import java.util.Arrays; -import java.util.List; +import java.util.HashSet; import java.util.Map; +import java.util.Set; import java.util.function.Function; +// ISSUE-20612 Source Validation /** * Context used to fetch the {@code _source}. * @@ -142,36 +143,37 @@ public static FetchSourceContext fromXContent(XContentParser parser) throws IOEx XContentParser.Token token = parser.currentToken(); switch (token) { case XContentParser.Token.VALUE_BOOLEAN -> { - return parser.booleanValue() ? FETCH_SOURCE : DO_NOT_FETCH_SOURCE; + return new FetchSourceContext(parser.booleanValue()); } case XContentParser.Token.VALUE_STRING -> { String[] includes = new String[] { parser.text() }; 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]); + String[] includes = parseSourceArray(parser, INCLUDES_FIELD).toArray(new String[0]); return new FetchSourceContext(true, includes, null); } case XContentParser.Token.START_OBJECT -> { return parseSourceObject(parser); } - default -> { + default -> throw new ParsingException( parser.getTokenLocation(), "Expected one of [" + XContentParser.Token.VALUE_BOOLEAN + ", " + + XContentParser.Token.VALUE_STRING + + ", " + + XContentParser.Token.START_ARRAY + + ", " + XContentParser.Token.START_OBJECT + "] but found [" + token - + "]" + + "]", + parser.getTokenLocation() ); - } } + // MUST never reach here } private static FetchSourceContext parseSourceObject(XContentParser parser) throws IOException { @@ -179,12 +181,6 @@ private static FetchSourceContext parseSourceObject(XContentParser parser) throw String[] includes = Strings.EMPTY_ARRAY; String[] excludes = Strings.EMPTY_ARRAY; String currentFieldName = null; - if (token != XContentParser.Token.START_OBJECT) { - throw new ParsingException( - parser.getTokenLocation(), - "Expected a " + XContentParser.Token.START_OBJECT + " but got a " + token + " in [" + parser.currentName() + "]." - ); - } while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); @@ -194,48 +190,66 @@ private static FetchSourceContext parseSourceObject(XContentParser parser) throw switch (token) { case XContentParser.Token.START_ARRAY -> { if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - includes = parseSourceArray(parser).toArray(new String[0]); - } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - excludes = parseSourceArray(parser).toArray(new String[0]); - } else { + includes = parseSourceArray(parser, INCLUDES_FIELD).toArray(new String[0]); + } + else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { + excludes = parseSourceArray(parser, EXCLUDES_FIELD).toArray(new String[0]); + } + else { throw new ParsingException( parser.getTokenLocation(), - "Unknown key for a " + token + " in [" + currentFieldName + "]." + "Unknown key for a " + token + " in [" + currentFieldName + "].", + parser.getTokenLocation() ); } } case XContentParser.Token.VALUE_STRING -> { if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - includes = new String[] { parser.text() }; - } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - excludes = new String[] { parser.text() }; - } else { + includes = new String[]{parser.text()}; + } + else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { + excludes = new String[]{parser.text()}; + } + else { throw new ParsingException( parser.getTokenLocation(), "Unknown key for a " + token + " in [" + currentFieldName + "]." ); } } - default -> { - throw new ParsingException(parser.getTokenLocation(), "Unknown key for a " + token + " in [" + currentFieldName + "]."); + default -> { + throw new ParsingException( + parser.getTokenLocation(), + "Unknown key for a " + token + " in [" + currentFieldName + "].", + parser.getTokenLocation() + ); } } } return new FetchSourceContext(true, includes, excludes); } - private static List parseSourceArray(XContentParser parser) throws IOException { - List sourceArr = new ArrayList<>(); + private static Set parseSourceArray(XContentParser parser, ParseField parseField) throws IOException { + Set sourceArr = new HashSet<>(); // include or exclude lists while (parser.nextToken() != XContentParser.Token.END_ARRAY) { if (parser.currentToken() == XContentParser.Token.VALUE_STRING) { sourceArr.add(parser.text()); - } else { + } + else { throw new ParsingException( parser.getTokenLocation(), - "Unknown key for a " + parser.currentToken() + " in [" + parser.currentName() + "]." + "Unknown key for a " + parser.currentToken() + " in [" + parser.currentName() + "].", + parser.getTokenLocation() ); } } + if (sourceArr.isEmpty()) { + throw new ParsingException( + parser.getTokenLocation(), + "Expected at least one value for an array of [" + parseField.getPreferredName() + "]", + parser.getTokenLocation() + ); + } return sourceArr; } From 55c63603f9a74945b5a6b41e2ba400d5728b7bd3 Mon Sep 17 00:00:00 2001 From: Mikhail Urmich Date: Sun, 22 Feb 2026 10:46:51 +0100 Subject: [PATCH 2/5] Intersection Validation include and exclude collections must not contain the same entries Signed-off-by: Mikhail Urmich Signed-off-by: Mikhail Urmich --- .../fetch/subphase/FetchSourceContext.java | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) 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 3f1b3f3ec95ce..77c35f6e54802 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 @@ -178,8 +178,8 @@ public static FetchSourceContext fromXContent(XContentParser parser) throws IOEx private static FetchSourceContext parseSourceObject(XContentParser parser) throws IOException { XContentParser.Token token = parser.currentToken(); - String[] includes = Strings.EMPTY_ARRAY; - String[] excludes = Strings.EMPTY_ARRAY; + Set includes = new HashSet<>(); + Set excludes = new HashSet<>(); String currentFieldName = null; while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { @@ -190,10 +190,10 @@ private static FetchSourceContext parseSourceObject(XContentParser parser) throw switch (token) { case XContentParser.Token.START_ARRAY -> { if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - includes = parseSourceArray(parser, INCLUDES_FIELD).toArray(new String[0]); + includes = parseSourceArray(parser, INCLUDES_FIELD); } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - excludes = parseSourceArray(parser, EXCLUDES_FIELD).toArray(new String[0]); + excludes = parseSourceArray(parser, EXCLUDES_FIELD); } else { throw new ParsingException( @@ -205,10 +205,12 @@ else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) } case XContentParser.Token.VALUE_STRING -> { if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - includes = new String[]{parser.text()}; + // safe, field names are unique in objects + includes.add(parser.text()); } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - excludes = new String[]{parser.text()}; + // safe, field names are unique in objects + excludes.add(parser.text()); } else { throw new ParsingException( @@ -226,7 +228,8 @@ else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) } } } - return new FetchSourceContext(true, includes, excludes); + // TODO: validate no intersections: retainAll vs manual loop + return new FetchSourceContext(true, includes.toArray(new String[0]), excludes.toArray(new String[0])); } private static Set parseSourceArray(XContentParser parser, ParseField parseField) throws IOException { From 34f3548e178fbf386145c6e9f23caa7fe7e61fda Mon Sep 17 00:00:00 2001 From: Mikhail Urmich Date: Sun, 22 Feb 2026 16:56:59 +0100 Subject: [PATCH 3/5] Validation of includes / excludes arrays The same entry MUST NOT be present in both inludes AND excludes arrays. This leads to ambiguity. Signed-off-by: Mikhail Urmich --- .../fetch/subphase/FetchSourceContext.java | 75 +++++++++++++++---- .../subphase/FetchSourceContextTests.java | 43 +++++++++++ 2 files changed, 103 insertions(+), 15 deletions(-) create mode 100644 server/src/test/java/org/opensearch/search/fetch/subphase/FetchSourceContextTests.java 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 77c35f6e54802..56acdc95065e7 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,6 +32,7 @@ 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.xcontent.support.XContentMapValues; @@ -48,6 +49,7 @@ import java.io.IOException; import java.util.Arrays; +import java.util.Collections; import java.util.HashSet; import java.util.Map; import java.util.Set; @@ -67,6 +69,9 @@ public class FetchSourceContext implements Writeable, ToXContentObject { public static final FetchSourceContext FETCH_SOURCE = new FetchSourceContext(true); public static final FetchSourceContext DO_NOT_FETCH_SOURCE = new FetchSourceContext(false); + + 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; @@ -76,6 +81,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) { @@ -86,6 +92,20 @@ public FetchSourceContext(StreamInput in) throws IOException { fetchSource = in.readBoolean(); includes = in.readStringArray(); excludes = in.readStringArray(); + validateAmbiguousFields(); + } + + /** + * 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 @@ -150,13 +170,13 @@ public static FetchSourceContext fromXContent(XContentParser parser) throws IOEx return new FetchSourceContext(true, includes, null); } case XContentParser.Token.START_ARRAY -> { - String[] includes = parseSourceArray(parser, INCLUDES_FIELD).toArray(new String[0]); + String[] includes = parseSourceArray(parser, INCLUDES_FIELD, null).toArray(new String[0]); return new FetchSourceContext(true, includes, null); } case XContentParser.Token.START_OBJECT -> { return parseSourceObject(parser); } - default -> + default -> { throw new ParsingException( parser.getTokenLocation(), "Expected one of [" @@ -172,28 +192,36 @@ public static FetchSourceContext fromXContent(XContentParser parser) throws IOEx + "]", parser.getTokenLocation() ); + } } // MUST never reach here } - private static FetchSourceContext parseSourceObject(XContentParser parser) throws IOException { + public static FetchSourceContext parseSourceObject(XContentParser parser) throws IOException { XContentParser.Token token = parser.currentToken(); - Set includes = new HashSet<>(); - Set excludes = new HashSet<>(); + Set includes = Collections.emptySet(); + Set excludes = Collections.emptySet(); String currentFieldName = null; while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { 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() + "].", + parser.getTokenLocation() + ); + } // process field value switch (token) { case XContentParser.Token.START_ARRAY -> { if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - includes = parseSourceArray(parser, INCLUDES_FIELD); + includes = parseSourceArray(parser, INCLUDES_FIELD, excludes); } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - excludes = parseSourceArray(parser, EXCLUDES_FIELD); + excludes = parseSourceArray(parser, EXCLUDES_FIELD, includes); } else { throw new ParsingException( @@ -205,12 +233,18 @@ else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) } case XContentParser.Token.VALUE_STRING -> { if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - // safe, field names are unique in objects - includes.add(parser.text()); + String includeEntry = parser.text(); + if (excludes.contains(includeEntry)) { + throw new OpenSearchException(AMBIGUOUS_FIELD_MESSAGE, includeEntry); + } + includes = Collections.singleton(includeEntry); } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - // safe, field names are unique in objects - excludes.add(parser.text()); + String excludeEntry = parser.text(); + if (includes.contains(excludeEntry)) { + throw new OpenSearchException(AMBIGUOUS_FIELD_MESSAGE, excludeEntry); + } + excludes = Collections.singleton(excludeEntry); } else { throw new ParsingException( @@ -219,7 +253,7 @@ else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) ); } } - default -> { + default -> { throw new ParsingException( parser.getTokenLocation(), "Unknown key for a " + token + " in [" + currentFieldName + "].", @@ -228,15 +262,26 @@ else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) } } } - // TODO: validate no intersections: retainAll vs manual loop + if (currentFieldName == null) { + // no field names -> empty object; empty object is not allowed + throw new ParsingException( + parser.getTokenLocation(), + "Expected at least one of [" + INCLUDES_FIELD.getPreferredName() + "] or [" + EXCLUDES_FIELD.getPreferredName() + "]", + parser.getTokenLocation() + ); + } return new FetchSourceContext(true, includes.toArray(new String[0]), excludes.toArray(new String[0])); } - private static Set parseSourceArray(XContentParser parser, ParseField parseField) throws IOException { + private static Set parseSourceArray(XContentParser parser, ParseField parseField, Set opposite) throws IOException { Set sourceArr = new HashSet<>(); // include or exclude lists 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 OpenSearchException(AMBIGUOUS_FIELD_MESSAGE, entry); + } + sourceArr.add(entry); } else { throw new ParsingException( 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..970de8f6ab885 --- /dev/null +++ b/server/src/test/java/org/opensearch/search/fetch/subphase/FetchSourceContextTests.java @@ -0,0 +1,43 @@ +/* + * 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.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.index.memory.MemoryIndex; +import org.opensearch.common.xcontent.XContentFactory; +import org.opensearch.core.common.Strings; +import org.opensearch.core.common.bytes.BytesReference; +import org.opensearch.core.xcontent.XContentBuilder; +import org.opensearch.search.SearchHit; +import org.opensearch.search.fetch.FetchContext; +import org.opensearch.search.fetch.FetchSubPhase.HitContext; +import org.opensearch.search.fetch.FetchSubPhaseProcessor; +import org.opensearch.search.lookup.SourceLookup; +import org.opensearch.test.OpenSearchTestCase; + +import java.io.IOException; +import java.util.Collections; +import java.util.Map; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class FetchSourceContextTests extends OpenSearchTestCase { + + public void testFetchSource() throws IOException { + XContentBuilder source = XContentFactory.jsonBuilder().startObject().field("field", "value").endObject(); + FetchSourceContext.fromXContent(source); + } + +} From e236fef93aa438d8e1742a77717e86c2a0c35fa6 Mon Sep 17 00:00:00 2001 From: Mikhail Urmich Date: Thu, 26 Feb 2026 08:43:57 +0100 Subject: [PATCH 4/5] unit and yml test Signed-off-by: Mikhail Urmich --- .../FetchSourceContextProtoUtilsTests.java | 21 +- .../test/search/10_source_filtering.yml | 58 ++++ .../fetch/subphase/FetchSourceContext.java | 90 +++--- .../subphase/FetchSourceContextTests.java | 271 ++++++++++++++++-- 4 files changed, 379 insertions(+), 61 deletions(-) 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 cfa306bbcc26a..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; @@ -18,7 +19,6 @@ import org.opensearch.search.fetch.subphase.FetchSourceContext; import org.opensearch.test.OpenSearchTestCase; -// ISSUE-20612 Source Validation public class FetchSourceContextProtoUtilsTests extends OpenSearchTestCase { public void testParseFromProtoRequestWithBoolValue() { @@ -353,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..84ca8d3327190 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,64 @@ 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" + - do: + search: { body: { _source: { }, query: { match_all: {} } } } + catch: bad_request + - match: { status: 400 } + - match: { error.type: parsing_exception } + - match: { error.reason: "Expected at least one of [includes] or [excludes]" } + +--- +"_source as an empty includes array": + - skip: + version: " - 3.6.99" + reason: "validation was added later" + - do: + search: { body: { _source: [], query: { match_all: {} } } } + catch: bad_request + - match: { status: 400 } + - match: { error.type: parsing_exception } + - match: { error.reason: "Expected at least one value for an array of [includes]" } + +--- +"_source with an empty excludes array": + - skip: + version: " - 3.6.99" + reason: "validation was added later" + - do: + search: + body: + _source: + includes: [ include.field1, include.field2 ] + excludes: [] + query: { match_all: {} } + catch: bad_request + - match: { status: 400 } + - match: { error.type: parsing_exception } + - match: { error.reason: "Expected at least one value for an array of [excludes]" } + +--- +"_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 56acdc95065e7..32d554bc549ba 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 @@ -51,11 +51,11 @@ import java.util.Arrays; 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; -// ISSUE-20612 Source Validation /** * Context used to fetch the {@code _source}. * @@ -92,7 +92,6 @@ public FetchSourceContext(StreamInput in) throws IOException { fetchSource = in.readBoolean(); includes = in.readStringArray(); excludes = in.readStringArray(); - validateAmbiguousFields(); } /** @@ -170,7 +169,7 @@ public static FetchSourceContext fromXContent(XContentParser parser) throws IOEx return new FetchSourceContext(true, includes, null); } case XContentParser.Token.START_ARRAY -> { - String[] includes = parseSourceArray(parser, INCLUDES_FIELD, null).toArray(new String[0]); + String[] includes = parseSourceFieldArray(parser, INCLUDES_FIELD, null).toArray(new String[0]); return new FetchSourceContext(true, includes, null); } case XContentParser.Token.START_OBJECT -> { @@ -189,8 +188,7 @@ public static FetchSourceContext fromXContent(XContentParser parser) throws IOEx + XContentParser.Token.START_OBJECT + "] but found [" + token - + "]", - parser.getTokenLocation() + + "]" ); } } @@ -202,6 +200,12 @@ public static FetchSourceContext parseSourceObject(XContentParser parser) throws Set includes = Collections.emptySet(); Set excludes = Collections.emptySet(); String currentFieldName = null; + if (token != XContentParser.Token.START_OBJECT) { + throw new ParsingException( + parser.getTokenLocation(), + "Expected a " + XContentParser.Token.START_OBJECT + " but got a " + token + " in [" + parser.currentName() + "]." + ); + } while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); @@ -210,24 +214,20 @@ public static FetchSourceContext parseSourceObject(XContentParser parser) throws if (currentFieldName == null) { throw new ParsingException( parser.getTokenLocation(), - "Expected a field name but got a " + token + " in [" + parser.currentName() + "].", - 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, INCLUDES_FIELD, excludes); - } - else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - excludes = parseSourceArray(parser, EXCLUDES_FIELD, includes); - } - else { + includes = parseSourceFieldArray(parser, INCLUDES_FIELD, excludes); + } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { + excludes = parseSourceFieldArray(parser, EXCLUDES_FIELD, includes); + } else { throw new ParsingException( parser.getTokenLocation(), - "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation() + "Unknown key for a " + token + " in [" + currentFieldName + "]." ); } } @@ -235,18 +235,16 @@ else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { String includeEntry = parser.text(); if (excludes.contains(includeEntry)) { - throw new OpenSearchException(AMBIGUOUS_FIELD_MESSAGE, includeEntry); + throw new ParsingException(parser.getTokenLocation(), AMBIGUOUS_FIELD_MESSAGE, includeEntry); } includes = Collections.singleton(includeEntry); - } - else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { + } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { String excludeEntry = parser.text(); if (includes.contains(excludeEntry)) { - throw new OpenSearchException(AMBIGUOUS_FIELD_MESSAGE, excludeEntry); + throw new ParsingException(parser.getTokenLocation(), AMBIGUOUS_FIELD_MESSAGE, excludeEntry); } excludes = Collections.singleton(excludeEntry); - } - else { + } else { throw new ParsingException( parser.getTokenLocation(), "Unknown key for a " + token + " in [" + currentFieldName + "]." @@ -254,48 +252,41 @@ else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) } } default -> { - throw new ParsingException( - parser.getTokenLocation(), - "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation() - ); + throw new ParsingException(parser.getTokenLocation(), "Unknown key for a " + token + " in [" + currentFieldName + "]."); } } } - if (currentFieldName == null) { - // no field names -> empty object; empty object is not allowed + if (includes.isEmpty() && excludes.isEmpty()) { + // no valid field names -> empty or unrecognized fields; not allowed throw new ParsingException( parser.getTokenLocation(), - "Expected at least one of [" + INCLUDES_FIELD.getPreferredName() + "] or [" + EXCLUDES_FIELD.getPreferredName() + "]", - parser.getTokenLocation() + "Expected at least one of [" + INCLUDES_FIELD.getPreferredName() + "] or [" + EXCLUDES_FIELD.getPreferredName() + "]" ); } return new FetchSourceContext(true, includes.toArray(new String[0]), excludes.toArray(new String[0])); } - private static Set parseSourceArray(XContentParser parser, ParseField parseField, Set opposite) throws IOException { - Set sourceArr = new HashSet<>(); // include or exclude lists + 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) { String entry = parser.text(); if (opposite != null && opposite.contains(entry)) { - throw new OpenSearchException(AMBIGUOUS_FIELD_MESSAGE, entry); + throw new ParsingException(parser.getTokenLocation(), AMBIGUOUS_FIELD_MESSAGE, entry); } sourceArr.add(entry); - } - else { + } else { throw new ParsingException( parser.getTokenLocation(), - "Unknown key for a " + parser.currentToken() + " in [" + parser.currentName() + "].", - parser.getTokenLocation() + "Unknown key for a " + parser.currentToken() + " in [" + parser.currentName() + "]." ); } } if (sourceArr.isEmpty()) { throw new ParsingException( parser.getTokenLocation(), - "Expected at least one value for an array of [" + parseField.getPreferredName() + "]", - parser.getTokenLocation() + "Expected at least one value for an array of [" + parseField.getPreferredName() + "]" ); } return sourceArr; @@ -303,14 +294,25 @@ private static Set parseSourceArray(XContentParser parser, ParseField pa @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - if (fetchSource) { - builder.startObject(); + if (!fetchSource) { + // do not fetch source + builder.value(false); + return builder; + } + if (includes.length == 0 && excludes.length == 0) { + // no empty arrays + builder.value(true); + return builder; + } + + builder.startObject(); + if (includes.length > 0) { builder.array(INCLUDES_FIELD.getPreferredName(), includes); + } + if (excludes.length > 0) { builder.array(EXCLUDES_FIELD.getPreferredName(), excludes); - builder.endObject(); - } else { - builder.value(false); } + builder.endObject(); return builder; } 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 index 970de8f6ab885..17f50494e58c0 100644 --- a/server/src/test/java/org/opensearch/search/fetch/subphase/FetchSourceContextTests.java +++ b/server/src/test/java/org/opensearch/search/fetch/subphase/FetchSourceContextTests.java @@ -13,31 +13,270 @@ package org.opensearch.search.fetch.subphase; -import org.apache.lucene.index.LeafReaderContext; -import org.apache.lucene.index.memory.MemoryIndex; import org.opensearch.common.xcontent.XContentFactory; -import org.opensearch.core.common.Strings; -import org.opensearch.core.common.bytes.BytesReference; +import org.opensearch.core.common.ParsingException; import org.opensearch.core.xcontent.XContentBuilder; -import org.opensearch.search.SearchHit; -import org.opensearch.search.fetch.FetchContext; -import org.opensearch.search.fetch.FetchSubPhase.HitContext; -import org.opensearch.search.fetch.FetchSubPhaseProcessor; -import org.opensearch.search.lookup.SourceLookup; +import org.opensearch.core.xcontent.XContentParser; import org.opensearch.test.OpenSearchTestCase; import java.io.IOException; -import java.util.Collections; -import java.util.Map; - -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; +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 { - XContentBuilder source = XContentFactory.jsonBuilder().startObject().field("field", "value").endObject(); - FetchSourceContext.fromXContent(source); + 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 testFetchSourceExplicitEmptyArrayNotAllowed() throws IOException { + final XContentBuilder source = XContentFactory.jsonBuilder().startObject().field("_source").startArray().endArray().endObject(); + final XContentParser parser = createSourceParser(source); + + ParsingException result = expectThrows(ParsingException.class, () -> FetchSourceContext.fromXContent(parser)); + assertEquals( + "Expected at least one value for an array of [" + FetchSourceContext.INCLUDES_FIELD.getPreferredName() + "]", + result.getMessage() + ); + } + + 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 testFetchSourceObjectEmptyObjectNotAllowed() throws IOException { + final XContentBuilder source = XContentFactory.jsonBuilder().startObject().field("_source").startObject().endObject().endObject(); + final XContentParser parser = createSourceParser(source); + + ParsingException result = expectThrows(ParsingException.class, () -> FetchSourceContext.fromXContent(parser)); + assertEquals( + "Expected at least one of [" + + FetchSourceContext.INCLUDES_FIELD.getPreferredName() + + "] or [" + + FetchSourceContext.EXCLUDES_FIELD.getPreferredName() + + "]", + result.getMessage() + ); + } + + public void testFetchSourceObjectExplicitEmptyArraysNotAllowed() 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); + + ParsingException result = expectThrows(ParsingException.class, () -> FetchSourceContext.fromXContent(parser)); + assertEquals( + "Expected at least one value for an array of [" + FetchSourceContext.EXCLUDES_FIELD.getPreferredName() + "]", + result.getMessage() + ); + } + { + 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); + + ParsingException result = expectThrows(ParsingException.class, () -> FetchSourceContext.fromXContent(parser)); + assertEquals( + "Expected at least one value for an array of [" + FetchSourceContext.INCLUDES_FIELD.getPreferredName() + "]", + result.getMessage() + ); + } + } + + 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 result = expectThrows(ParsingException.class, () -> FetchSourceContext.parseSourceObject(parser)); + assertEquals("Expected a START_OBJECT but got a VALUE_BOOLEAN in [_source].", result.getMessage()); + } } From 884af1cb2a82b4b4737de892a75e38ebf3460ce6 Mon Sep 17 00:00:00 2001 From: Mikhail Urmich Date: Wed, 8 Apr 2026 22:25:13 +0200 Subject: [PATCH 5/5] Updates and tests Signed-off-by: Mikhail Urmich Signed-off-by: Mikhail Urmich --- .../test/search/10_source_filtering.yml | 53 ++++++--- .../fetch/subphase/FetchSourceContext.java | 84 +++++++++----- .../subphase/FetchSourceContextTests.java | 104 +++++++++--------- 3 files changed, 145 insertions(+), 96 deletions(-) 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 84ca8d3327190..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 @@ -39,41 +39,66 @@ setup: - 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: {} } } } - catch: bad_request - - match: { status: 400 } - - match: { error.type: parsing_exception } - - match: { error.reason: "Expected at least one of [includes] or [excludes]" } + - length: { hits.hits: 1 } + - match: { hits.hits.0._source.count: 1 } --- -"_source as an empty includes array": +"_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: {} } } } - catch: bad_request - - match: { status: 400 } - - match: { error.type: parsing_exception } - - match: { error.reason: "Expected at least one value for an array of [includes]" } + - length: { hits.hits: 1 } + - match: { hits.hits.0._source.count: 1 } --- -"_source with an empty excludes array": +"_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: {} } - catch: bad_request - - match: { status: 400 } - - match: { error.type: parsing_exception } - - match: { error.reason: "Expected at least one value for an array of [excludes]" } + - 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": 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 32d554bc549ba..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 @@ -35,6 +35,7 @@ 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; @@ -63,12 +64,13 @@ */ @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."; @@ -162,15 +164,14 @@ public static FetchSourceContext fromXContent(XContentParser parser) throws IOEx XContentParser.Token token = parser.currentToken(); switch (token) { case XContentParser.Token.VALUE_BOOLEAN -> { - return new FetchSourceContext(parser.booleanValue()); + return parser.booleanValue() ? FETCH_SOURCE : DO_NOT_FETCH_SOURCE; } case XContentParser.Token.VALUE_STRING -> { String[] includes = new String[] { parser.text() }; return new FetchSourceContext(true, includes, null); } case XContentParser.Token.START_ARRAY -> { - String[] includes = parseSourceFieldArray(parser, INCLUDES_FIELD, null).toArray(new String[0]); - return new FetchSourceContext(true, includes, null); + return parseSourceArray(parser); } case XContentParser.Token.START_OBJECT -> { return parseSourceObject(parser); @@ -192,10 +193,42 @@ public static FetchSourceContext fromXContent(XContentParser parser) throws IOEx ); } } - // MUST never reach here } - public 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(); Set includes = Collections.emptySet(); Set excludes = Collections.emptySet(); @@ -257,10 +290,14 @@ public static FetchSourceContext parseSourceObject(XContentParser parser) throws } } if (includes.isEmpty() && excludes.isEmpty()) { - // no valid field names -> empty or unrecognized fields; not allowed - throw new ParsingException( - parser.getTokenLocation(), - "Expected at least one of [" + INCLUDES_FIELD.getPreferredName() + "] or [" + EXCLUDES_FIELD.getPreferredName() + "]" + // 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])); @@ -284,8 +321,8 @@ private static Set parseSourceFieldArray(XContentParser parser, ParseFie } } if (sourceArr.isEmpty()) { - throw new ParsingException( - parser.getTokenLocation(), + deprecationLogger.deprecate( + "empty_source_" + parseField.getPreferredName(), "Expected at least one value for an array of [" + parseField.getPreferredName() + "]" ); } @@ -294,25 +331,14 @@ private static Set parseSourceFieldArray(XContentParser parser, ParseFie @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - if (!fetchSource) { - // do not fetch source - builder.value(false); - return builder; - } - if (includes.length == 0 && excludes.length == 0) { - // no empty arrays - builder.value(true); - return builder; - } - - builder.startObject(); - if (includes.length > 0) { + if (fetchSource) { + builder.startObject(); builder.array(INCLUDES_FIELD.getPreferredName(), includes); - } - if (excludes.length > 0) { builder.array(EXCLUDES_FIELD.getPreferredName(), excludes); + builder.endObject(); + } else { + builder.value(false); } - builder.endObject(); return builder; } 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 index 17f50494e58c0..db7fd5b11c618 100644 --- a/server/src/test/java/org/opensearch/search/fetch/subphase/FetchSourceContextTests.java +++ b/server/src/test/java/org/opensearch/search/fetch/subphase/FetchSourceContextTests.java @@ -79,14 +79,14 @@ public void testFetchSourceArray() throws IOException { assertEquals(0, result.excludes().length); } - public void testFetchSourceExplicitEmptyArrayNotAllowed() throws IOException { + public void testFetchSourceAsArrayAssertWarningExplicitEmptyArray() throws IOException { final XContentBuilder source = XContentFactory.jsonBuilder().startObject().field("_source").startArray().endArray().endObject(); final XContentParser parser = createSourceParser(source); - ParsingException result = expectThrows(ParsingException.class, () -> FetchSourceContext.fromXContent(parser)); - assertEquals( - "Expected at least one value for an array of [" + FetchSourceContext.INCLUDES_FIELD.getPreferredName() + "]", - result.getMessage() + 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." ); } @@ -134,63 +134,58 @@ public void testFetchSourceAsObjectBothIncludeAndExcludeArrays() throws IOExcept assertTrue(Arrays.asList(result.excludes()).containsAll(Arrays.asList("aaa", "bbb"))); } - public void testFetchSourceObjectEmptyObjectNotAllowed() throws IOException { + public void testFetchSourceAssertWarningEmptyObject() throws IOException { final XContentBuilder source = XContentFactory.jsonBuilder().startObject().field("_source").startObject().endObject().endObject(); final XContentParser parser = createSourceParser(source); - ParsingException result = expectThrows(ParsingException.class, () -> FetchSourceContext.fromXContent(parser)); - assertEquals( - "Expected at least one of [" + 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() - + "]", - result.getMessage() + + "] or use `_source: true` to fetch the entire source." ); } - public void testFetchSourceObjectExplicitEmptyArraysNotAllowed() 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); + 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); - ParsingException result = expectThrows(ParsingException.class, () -> FetchSourceContext.fromXContent(parser)); - assertEquals( - "Expected at least one value for an array of [" + FetchSourceContext.EXCLUDES_FIELD.getPreferredName() + "]", - result.getMessage() - ); - } - { - 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.EXCLUDES_FIELD.getPreferredName() + "]"); + } - ParsingException result = expectThrows(ParsingException.class, () -> FetchSourceContext.fromXContent(parser)); - assertEquals( - "Expected at least one value for an array of [" + FetchSourceContext.INCLUDES_FIELD.getPreferredName() + "]", - result.getMessage() - ); - } + 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 { @@ -276,7 +271,10 @@ public void testParseSourceObjectInvalidInput() throws IOException { final XContentBuilder source = XContentFactory.jsonBuilder().startObject().field("_source", true).endObject(); final XContentParser parser = createSourceParser(source); - ParsingException result = expectThrows(ParsingException.class, () -> FetchSourceContext.parseSourceObject(parser)); - assertEquals("Expected a START_OBJECT but got a VALUE_BOOLEAN in [_source].", result.getMessage()); + 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()); } }