From e4730f80b20360b3b4c5fb09c6a426d022897ede Mon Sep 17 00:00:00 2001 From: Mikhail Urmich Date: Fri, 20 Feb 2026 14:11:51 +0100 Subject: [PATCH 01/11] Init marks for ISSUE-20612 Signed-off-by: Mikhail Urmich --- .../proto/request/common/FetchSourceContextProtoUtilsTests.java | 1 + .../org/opensearch/search/fetch/subphase/FetchSourceContext.java | 1 + 2 files changed, 2 insertions(+) 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 2bdb311c0003f..1172a76d17a7b 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 @@ -53,6 +53,7 @@ import java.util.Map; import java.util.function.Function; +// ISSUE-20612 Source Validation /** * Context used to fetch the {@code _source}. * From a563582c78ec139dadfad396de580748c11b523a Mon Sep 17 00:00:00 2001 From: Mikhail Urmich Date: Fri, 20 Feb 2026 16:10:16 +0100 Subject: [PATCH 02/11] simplification i Signed-off-by: Mikhail Urmich --- .../fetch/subphase/FetchSourceContext.java | 175 ++++++++++-------- 1 file changed, 100 insertions(+), 75 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 1172a76d17a7b..ad46fef7c019f 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 @@ -134,79 +134,98 @@ public static FetchSourceContext parseFromRestRequest(RestRequest request) { } if (fetchSource != null || sourceIncludes != null || sourceExcludes != null) { - return new FetchSourceContext(fetchSource == null ? true : fetchSource, sourceIncludes, sourceExcludes); + return new FetchSourceContext(fetchSource == null || fetchSource, sourceIncludes, sourceExcludes); } return null; } public static FetchSourceContext fromXContent(XContentParser parser) throws IOException { XContentParser.Token token = parser.currentToken(); - boolean fetchSource = true; + String[] emptyExcludes = Strings.EMPTY_ARRAY; + switch (token) { + case XContentParser.Token.VALUE_BOOLEAN -> { + return new FetchSourceContext(parser.booleanValue()); + } + case XContentParser.Token.VALUE_STRING -> { + String[] includes = new String[]{parser.text()}; + return new FetchSourceContext(true, includes, emptyExcludes); + } + case XContentParser.Token.START_ARRAY -> { + ArrayList list = new ArrayList<>(); + while (parser.nextToken() != XContentParser.Token.END_ARRAY) { + list.add(parser.text()); + } + if (list.isEmpty()) { + throw new ParsingException( + parser.getTokenLocation(), + "Expected at least one value for an array of [" + INCLUDES_FIELD.getPreferredName() + "]", + parser.getTokenLocation() + ); + } + String[] includes = list.toArray(new String[0]); + return new FetchSourceContext(true, includes, emptyExcludes); + } + case XContentParser.Token.START_OBJECT -> { + return parseSourceObject(parser); + } + 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 { + XContentParser.Token token = parser.currentToken(); String[] includes = Strings.EMPTY_ARRAY; String[] excludes = Strings.EMPTY_ARRAY; - if (token == XContentParser.Token.VALUE_BOOLEAN) { - fetchSource = parser.booleanValue(); - } else if (token == XContentParser.Token.VALUE_STRING) { - includes = new String[] { parser.text() }; - } else if (token == XContentParser.Token.START_ARRAY) { - ArrayList list = new ArrayList<>(); - while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { - list.add(parser.text()); - } - includes = list.toArray(new String[0]); - } else if (token == XContentParser.Token.START_OBJECT) { - String currentFieldName = null; - while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { - if (token == XContentParser.Token.FIELD_NAME) { - currentFieldName = parser.currentName(); - } else if (token == XContentParser.Token.START_ARRAY) { - if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - List includesList = new ArrayList<>(); - while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { - if (token == XContentParser.Token.VALUE_STRING) { - includesList.add(parser.text()); - } else { - throw new ParsingException( - parser.getTokenLocation(), - "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation() - ); - } - } - includes = includesList.toArray(new String[0]); - } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - List excludesList = new ArrayList<>(); - while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { - if (token == XContentParser.Token.VALUE_STRING) { - excludesList.add(parser.text()); - } else { - throw new ParsingException( - parser.getTokenLocation(), - "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation() - ); - } + + String currentFieldName = null; + while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { + if (token == XContentParser.Token.FIELD_NAME) { + currentFieldName = parser.currentName(); + } else if (token == XContentParser.Token.START_ARRAY) { + if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { + List includesList = new ArrayList<>(); + while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { + if (token == XContentParser.Token.VALUE_STRING) { + includesList.add(parser.text()); + } else { + throw new ParsingException( + parser.getTokenLocation(), + "Unknown key for a " + token + " in [" + currentFieldName + "].", + parser.getTokenLocation() + ); } - excludes = excludesList.toArray(new String[0]); - } else { - throw new ParsingException( - parser.getTokenLocation(), - "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation() - ); } - } else if (token == 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 { - throw new ParsingException( - parser.getTokenLocation(), - "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation() - ); + includes = includesList.toArray(new String[0]); + } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { + List excludesList = new ArrayList<>(); + while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { + if (token == XContentParser.Token.VALUE_STRING) { + excludesList.add(parser.text()); + } else { + throw new ParsingException( + parser.getTokenLocation(), + "Unknown key for a " + token + " in [" + currentFieldName + "].", + parser.getTokenLocation() + ); + } } + excludes = excludesList.toArray(new String[0]); } else { throw new ParsingException( parser.getTokenLocation(), @@ -214,21 +233,27 @@ public static FetchSourceContext fromXContent(XContentParser parser) throws IOEx parser.getTokenLocation() ); } + } else if (token == 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 { + throw new ParsingException( + parser.getTokenLocation(), + "Unknown key for a " + token + " in [" + currentFieldName + "].", + parser.getTokenLocation() + ); + } + } else { + throw new ParsingException( + parser.getTokenLocation(), + "Unknown key for a " + token + " in [" + currentFieldName + "].", + parser.getTokenLocation() + ); } - } else { - throw new ParsingException( - parser.getTokenLocation(), - "Expected one of [" - + XContentParser.Token.VALUE_BOOLEAN - + ", " - + XContentParser.Token.START_OBJECT - + "] but found [" - + token - + "]", - parser.getTokenLocation() - ); } - return new FetchSourceContext(fetchSource, includes, excludes); + return new FetchSourceContext(true, includes, excludes); } @Override From 4de63be563f0a5a3a090c32597cff7cdefe3648b Mon Sep 17 00:00:00 2001 From: Mikhail Urmich Date: Sat, 21 Feb 2026 10:27:45 +0100 Subject: [PATCH 03/11] switch case in favour of if-else-if Signed-off-by: Mikhail Urmich --- .../fetch/subphase/FetchSourceContext.java | 101 ++++++++++-------- 1 file changed, 56 insertions(+), 45 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 ad46fef7c019f..aac03c1744c85 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 @@ -195,62 +195,73 @@ private static FetchSourceContext parseSourceObject(XContentParser parser) throw String currentFieldName = null; while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { - if (token == XContentParser.Token.FIELD_NAME) { - currentFieldName = parser.currentName(); - } else if (token == XContentParser.Token.START_ARRAY) { - if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - List includesList = new ArrayList<>(); - while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { - if (token == XContentParser.Token.VALUE_STRING) { - includesList.add(parser.text()); - } else { - throw new ParsingException( - parser.getTokenLocation(), - "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation() - ); + switch (token) { + case XContentParser.Token.FIELD_NAME -> { + currentFieldName = parser.currentName(); + } + case XContentParser.Token.START_ARRAY -> { + if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { + List includesList = new ArrayList<>(); + while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { + if (token == XContentParser.Token.VALUE_STRING) { + includesList.add(parser.text()); + } + else { + throw new ParsingException( + parser.getTokenLocation(), + "Unknown key for a " + token + " in [" + currentFieldName + "].", + parser.getTokenLocation() + ); + } } + includes = includesList.toArray(new String[0]); } - includes = includesList.toArray(new String[0]); - } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - List excludesList = new ArrayList<>(); - while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { - if (token == XContentParser.Token.VALUE_STRING) { - excludesList.add(parser.text()); - } else { - throw new ParsingException( - parser.getTokenLocation(), - "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation() - ); + else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { + List excludesList = new ArrayList<>(); + while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { + if (token == XContentParser.Token.VALUE_STRING) { + excludesList.add(parser.text()); + } + else { + throw new ParsingException( + parser.getTokenLocation(), + "Unknown key for a " + token + " in [" + currentFieldName + "].", + parser.getTokenLocation() + ); + } } + excludes = excludesList.toArray(new String[0]); + } + else { + throw new ParsingException( + parser.getTokenLocation(), + "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 { + throw new ParsingException( + parser.getTokenLocation(), + "Unknown key for a " + token + " in [" + currentFieldName + "].", + parser.getTokenLocation() + ); } - excludes = excludesList.toArray(new String[0]); - } else { - throw new ParsingException( - parser.getTokenLocation(), - "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation() - ); } - } else if (token == 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 { + default -> { throw new ParsingException( parser.getTokenLocation(), "Unknown key for a " + token + " in [" + currentFieldName + "].", parser.getTokenLocation() ); } - } else { - throw new ParsingException( - parser.getTokenLocation(), - "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation() - ); } } return new FetchSourceContext(true, includes, excludes); From 0df65ae5cef71fe9fe581ca90cf2159765a087c1 Mon Sep 17 00:00:00 2001 From: Mikhail Urmich Date: Sat, 21 Feb 2026 10:38:43 +0100 Subject: [PATCH 04/11] minor refactor extract array parsing as its own function Signed-off-by: Mikhail Urmich --- .../fetch/subphase/FetchSourceContext.java | 46 ++++++++----------- 1 file changed, 19 insertions(+), 27 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 aac03c1744c85..e8ea11d3864a9 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 @@ -192,7 +192,6 @@ private static FetchSourceContext parseSourceObject(XContentParser parser) throw XContentParser.Token token = parser.currentToken(); String[] includes = Strings.EMPTY_ARRAY; String[] excludes = Strings.EMPTY_ARRAY; - String currentFieldName = null; while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { switch (token) { @@ -201,35 +200,11 @@ private static FetchSourceContext parseSourceObject(XContentParser parser) throw } case XContentParser.Token.START_ARRAY -> { if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - List includesList = new ArrayList<>(); - while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { - if (token == XContentParser.Token.VALUE_STRING) { - includesList.add(parser.text()); - } - else { - throw new ParsingException( - parser.getTokenLocation(), - "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation() - ); - } - } + List includesList = parseStringArray(parser); includes = includesList.toArray(new String[0]); } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - List excludesList = new ArrayList<>(); - while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { - if (token == XContentParser.Token.VALUE_STRING) { - excludesList.add(parser.text()); - } - else { - throw new ParsingException( - parser.getTokenLocation(), - "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation() - ); - } - } + List excludesList = parseStringArray(parser); excludes = excludesList.toArray(new String[0]); } else { @@ -267,6 +242,23 @@ else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) return new FetchSourceContext(true, includes, excludes); } + private static List parseStringArray(XContentParser parser) throws IOException { + List list = new ArrayList<>(); + while (parser.nextToken() != XContentParser.Token.END_ARRAY) { + if (parser.currentToken() == XContentParser.Token.VALUE_STRING) { + list.add(parser.text()); + } + else { + throw new ParsingException( + parser.getTokenLocation(), + "Unknown key for a " + parser.currentToken() + " in [" + parser.currentName() + "].", + parser.getTokenLocation() + ); + } + } + return list; + } + @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { if (fetchSource) { From 5521d6858c790daa4c10d52a32f81c5855c1f3bc Mon Sep 17 00:00:00 2001 From: Mikhail Urmich Date: Sun, 22 Feb 2026 08:43:27 +0100 Subject: [PATCH 05/11] Refactor parseSourceObject: split key-value process into different code-blocks Signed-off-by: Mikhail Urmich --- .../fetch/subphase/FetchSourceContext.java | 53 +++++++++---------- 1 file changed, 24 insertions(+), 29 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 e8ea11d3864a9..0bf4047892a8b 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,10 +47,10 @@ 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 @@ -141,29 +141,17 @@ public static FetchSourceContext parseFromRestRequest(RestRequest request) { public static FetchSourceContext fromXContent(XContentParser parser) throws IOException { XContentParser.Token token = parser.currentToken(); - String[] emptyExcludes = Strings.EMPTY_ARRAY; switch (token) { case XContentParser.Token.VALUE_BOOLEAN -> { return new FetchSourceContext(parser.booleanValue()); } case XContentParser.Token.VALUE_STRING -> { - String[] includes = new String[]{parser.text()}; - return new FetchSourceContext(true, includes, emptyExcludes); + String[] includes = new String[] { parser.text() }; + return new FetchSourceContext(true, includes, null); } case XContentParser.Token.START_ARRAY -> { - ArrayList list = new ArrayList<>(); - while (parser.nextToken() != XContentParser.Token.END_ARRAY) { - list.add(parser.text()); - } - if (list.isEmpty()) { - throw new ParsingException( - parser.getTokenLocation(), - "Expected at least one value for an array of [" + INCLUDES_FIELD.getPreferredName() + "]", - parser.getTokenLocation() - ); - } - String[] includes = list.toArray(new String[0]); - return new FetchSourceContext(true, includes, emptyExcludes); + String[] includes = parseSourceArray(parser, INCLUDES_FIELD).toArray(new String[0]); + return new FetchSourceContext(true, includes, null); } case XContentParser.Token.START_OBJECT -> { return parseSourceObject(parser); @@ -194,18 +182,18 @@ private static FetchSourceContext parseSourceObject(XContentParser parser) throw String[] excludes = Strings.EMPTY_ARRAY; 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 + } + // process field value switch (token) { - case XContentParser.Token.FIELD_NAME -> { - currentFieldName = parser.currentName(); - } case XContentParser.Token.START_ARRAY -> { if (INCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - List includesList = parseStringArray(parser); - includes = includesList.toArray(new String[0]); + includes = parseSourceArray(parser, INCLUDES_FIELD).toArray(new String[0]); } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - List excludesList = parseStringArray(parser); - excludes = excludesList.toArray(new String[0]); + excludes = parseSourceArray(parser, EXCLUDES_FIELD).toArray(new String[0]); } else { throw new ParsingException( @@ -242,11 +230,11 @@ else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) return new FetchSourceContext(true, includes, excludes); } - private static List parseStringArray(XContentParser parser) throws IOException { - List list = 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) { - list.add(parser.text()); + sourceArr.add(parser.text()); } else { throw new ParsingException( @@ -256,7 +244,14 @@ private static List parseStringArray(XContentParser parser) throws IOExc ); } } - return list; + if (sourceArr.isEmpty()) { + throw new ParsingException( + parser.getTokenLocation(), + "Expected at least one value for an array of [" + parseField.getPreferredName() + "]", + parser.getTokenLocation() + ); + } + return sourceArr; } @Override From 0c91bd12fee5b7035f458e5181db2b45ea72134e Mon Sep 17 00:00:00 2001 From: Mikhail Urmich Date: Thu, 2 Apr 2026 14:40:55 +0200 Subject: [PATCH 06/11] Refactoring only Signed-off-by: Mikhail Urmich --- .../FetchSourceContextProtoUtilsTests.java | 1 - .../fetch/subphase/FetchSourceContext.java | 48 ++++++++----------- 2 files changed, 21 insertions(+), 28 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..4547be75fe2fd 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,7 +18,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() { 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 0bf4047892a8b..7cc81cc285b39 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,13 +47,12 @@ import org.opensearch.rest.RestRequest; import java.io.IOException; +import java.util.ArrayList; import java.util.Arrays; -import java.util.HashSet; +import java.util.List; import java.util.Map; -import java.util.Set; import java.util.function.Function; -// ISSUE-20612 Source Validation /** * Context used to fetch the {@code _source}. * @@ -143,20 +142,20 @@ 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 = parseSourceArray(parser, INCLUDES_FIELD).toArray(new String[0]); + String[] includes = parseSourceArray(parser).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 [" @@ -169,9 +168,9 @@ public static FetchSourceContext fromXContent(XContentParser parser) throws IOEx + XContentParser.Token.START_OBJECT + "] but found [" + token - + "]", - parser.getTokenLocation() + + "]" ); + } } // MUST never reach here } @@ -181,6 +180,12 @@ 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(); @@ -190,16 +195,15 @@ 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).toArray(new String[0]); } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - excludes = parseSourceArray(parser, EXCLUDES_FIELD).toArray(new String[0]); + excludes = parseSourceArray(parser).toArray(new String[0]); } else { throw new ParsingException( parser.getTokenLocation(), - "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation() + "Unknown key for a " + token + " in [" + currentFieldName + "]." ); } } @@ -213,16 +217,14 @@ else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) else { throw new ParsingException( parser.getTokenLocation(), - "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation() + "Unknown key for a " + token + " in [" + currentFieldName + "]." ); } } default -> { throw new ParsingException( parser.getTokenLocation(), - "Unknown key for a " + token + " in [" + currentFieldName + "].", - parser.getTokenLocation() + "Unknown key for a " + token + " in [" + currentFieldName + "]." ); } } @@ -230,8 +232,8 @@ else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) return new FetchSourceContext(true, includes, excludes); } - private static Set parseSourceArray(XContentParser parser, ParseField parseField) throws IOException { - Set sourceArr = new HashSet<>(); // include or exclude lists + private static List parseSourceArray(XContentParser parser) throws IOException { + List sourceArr = new ArrayList<>(); while (parser.nextToken() != XContentParser.Token.END_ARRAY) { if (parser.currentToken() == XContentParser.Token.VALUE_STRING) { sourceArr.add(parser.text()); @@ -239,18 +241,10 @@ private static Set parseSourceArray(XContentParser parser, ParseField pa 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() - ); - } return sourceArr; } From 0a2b0ab087e26754985c0ac7ba359eff4f3ff4f5 Mon Sep 17 00:00:00 2001 From: Mikhail Urmich Date: Thu, 2 Apr 2026 19:53:10 +0200 Subject: [PATCH 07/11] changelog and spotless Signed-off-by: Mikhail Urmich --- CHANGELOG.md | 1 + .../fetch/subphase/FetchSourceContext.java | 26 +++++++------------ 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 891c36ee715a8..fd40ad37966f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,7 @@ As of the 3.6 release [the CHANGELOG is no longer used][1] to generate release n - Lazy init stored field reader in SourceLookup ([#20827](https://github.com/opensearch-project/OpenSearch/pull/20827)) * Improved error message when trying to open an index originally created with Elasticsearch on OpenSearch ([#20512](https://github.com/opensearch-project/OpenSearch/pull/20512)) - Updated MMapDirectory to use ReadAdviseByContext rather than default readadvise of Lucene([#21031](https://github.com/opensearch-project/OpenSearch/pull/21031)) +- Refactoring and simplification of `FetchSourceContext` class ([#20612](https://github.com/opensearch-project/OpenSearch/issues/20612)) ### Fixed - Relax index template pattern overlap check to use minimum-string heuristic, allowing distinguishable multi-wildcard patterns at the same priority ([#20702](https://github.com/opensearch-project/OpenSearch/pull/20702)) 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 7cc81cc285b39..aef7eecd6b0fc 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 @@ -196,11 +196,9 @@ private static FetchSourceContext parseSourceObject(XContentParser parser) throw 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())) { + } else if (EXCLUDES_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { excludes = parseSourceArray(parser).toArray(new String[0]); - } - else { + } else { throw new ParsingException( parser.getTokenLocation(), "Unknown key for a " + token + " in [" + currentFieldName + "]." @@ -209,23 +207,18 @@ 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()}; - } - 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 + "]."); } } } @@ -237,8 +230,7 @@ private static List parseSourceArray(XContentParser parser) throws IOExc 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() + "]." From 1a6b21382a44d4eaac735b09d730c88b4a31f805 Mon Sep 17 00:00:00 2001 From: Mikhail Urmich Date: Thu, 2 Apr 2026 22:15:41 +0200 Subject: [PATCH 08/11] error message revert to original Signed-off-by: Mikhail Urmich --- .../opensearch/search/fetch/subphase/FetchSourceContext.java | 5 ----- 1 file changed, 5 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 aef7eecd6b0fc..be920d107fee5 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 @@ -161,10 +161,6 @@ 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 @@ -172,7 +168,6 @@ public static FetchSourceContext fromXContent(XContentParser parser) throws IOEx ); } } - // MUST never reach here } private static FetchSourceContext parseSourceObject(XContentParser parser) throws IOException { From 840881358f84021d711d77821ccdc843b1e767fa Mon Sep 17 00:00:00 2001 From: Mikhail Urmich Date: Thu, 2 Apr 2026 22:21:02 +0200 Subject: [PATCH 09/11] parsing array had no validation Signed-off-by: Mikhail Urmich --- .../search/fetch/subphase/FetchSourceContext.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 be920d107fee5..cb6a467ef6078 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 @@ -149,7 +149,11 @@ public static FetchSourceContext fromXContent(XContentParser parser) throws IOEx return new FetchSourceContext(true, includes, null); } case XContentParser.Token.START_ARRAY -> { - String[] includes = parseSourceArray(parser).toArray(new String[0]); + 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); } case XContentParser.Token.START_OBJECT -> { From c629facdb18ba414038f0c53cb7b01bd5176c34a Mon Sep 17 00:00:00 2001 From: Mikhail Urmich Date: Thu, 2 Apr 2026 22:34:24 +0200 Subject: [PATCH 10/11] minor revert, to simplify the PR Signed-off-by: Mikhail Urmich --- .../opensearch/search/fetch/subphase/FetchSourceContext.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 cb6a467ef6078..3d1f42b7e1eb7 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 @@ -133,7 +133,7 @@ public static FetchSourceContext parseFromRestRequest(RestRequest request) { } if (fetchSource != null || sourceIncludes != null || sourceExcludes != null) { - return new FetchSourceContext(fetchSource == null || fetchSource, sourceIncludes, sourceExcludes); + return new FetchSourceContext(fetchSource == null ? true : fetchSource, sourceIncludes, sourceExcludes); } return null; } From 1fa538d3169a0c5b96d5ffe3fa7e8eafa3e66cb5 Mon Sep 17 00:00:00 2001 From: Andrew Ross Date: Tue, 7 Apr 2026 15:22:26 +0000 Subject: [PATCH 11/11] Rebase and remove changelog entry Signed-off-by: Andrew Ross --- CHANGELOG.md | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fd40ad37966f2..891c36ee715a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,7 +44,6 @@ As of the 3.6 release [the CHANGELOG is no longer used][1] to generate release n - Lazy init stored field reader in SourceLookup ([#20827](https://github.com/opensearch-project/OpenSearch/pull/20827)) * Improved error message when trying to open an index originally created with Elasticsearch on OpenSearch ([#20512](https://github.com/opensearch-project/OpenSearch/pull/20512)) - Updated MMapDirectory to use ReadAdviseByContext rather than default readadvise of Lucene([#21031](https://github.com/opensearch-project/OpenSearch/pull/21031)) -- Refactoring and simplification of `FetchSourceContext` class ([#20612](https://github.com/opensearch-project/OpenSearch/issues/20612)) ### Fixed - Relax index template pattern overlap check to use minimum-string heuristic, allowing distinguishable multi-wildcard patterns at the same priority ([#20702](https://github.com/opensearch-project/OpenSearch/pull/20702))