Skip to content

Commit 7e48b84

Browse files
yiyuabcclaude
andcommitted
Add aggregation name validation matching REST API behavior
Apply the same aggregation name validation as REST API using AggregatorFactories.VALID_AGG_NAME pattern in the gRPC registry. This ensures aggregation names cannot contain '[', ']', or '>' characters, matching REST validation behavior exactly. Changes: - Add name validation in AggregationBuilderProtoConverterSpiRegistry.fromProto() - Remove outdated javadoc stating "Name validation is handled by the registry" from MinAggregationProtoUtils and MaxAggregationProtoUtils - Add comprehensive tests for invalid aggregation name validation Mirrors REST-side validation from AggregatorFactories#parseAggregators. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent e37f084 commit 7e48b84

File tree

4 files changed

+63
-6
lines changed

4 files changed

+63
-6
lines changed

modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/aggregation/AggregationBuilderProtoConverterSpiRegistry.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import java.util.HashMap;
2222
import java.util.Map;
23+
import java.util.regex.Matcher;
2324

2425
/**
2526
* SPI registry for AggregationBuilderProtoConverter implementations.
@@ -51,6 +52,15 @@ public AggregationBuilder fromProto(String name, AggregationContainer container)
5152
throw new IllegalArgumentException("Aggregation name cannot be null or empty");
5253
}
5354

55+
// Validate aggregation name using the same pattern as REST
56+
// See AggregatorFactories#parseAggregators
57+
Matcher validAggMatcher = AggregatorFactories.VALID_AGG_NAME.matcher(name);
58+
if (!validAggMatcher.matches()) {
59+
throw new IllegalArgumentException(
60+
"Invalid aggregation name [" + name + "]. Aggregation names can contain any character except '[', ']', and '>'"
61+
);
62+
}
63+
5464
AggregationContainer.AggregationContainerCase aggregationCase = container.getAggregationContainerCase();
5565
AggregationBuilderProtoConverter converter = converterMap.get(aggregationCase);
5666

modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/aggregation/metrics/MaxAggregationProtoUtils.java

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,7 @@ private MaxAggregationProtoUtils() {
3131
* <p>This method parallels the REST parsing logic in {@link MaxAggregationBuilder#PARSER},
3232
* processing fields in the exact same sequence to ensure consistent validation and behavior.
3333
*
34-
* <p>Name validation is handled by the registry before this method is called.
35-
*
36-
* @param name The name of the aggregation (validated by registry)
34+
* @param name The name of the aggregation
3735
* @param maxAggProto The Protocol Buffer MaxAggregation to convert
3836
* @return A configured MaxAggregationBuilder
3937
* @throws IllegalArgumentException if required fields are missing or validation fails

modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/aggregation/metrics/MinAggregationProtoUtils.java

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,9 +31,7 @@ private MinAggregationProtoUtils() {
3131
* <p>This method parallels the REST parsing logic in {@link MinAggregationBuilder#PARSER},
3232
* processing fields in the exact same sequence to ensure consistent validation and behavior.
3333
*
34-
* <p>Name validation is handled by the registry before this method is called.
35-
*
36-
* @param name The name of the aggregation (validated by registry)
34+
* @param name The name of the aggregation
3735
* @param minAggProto The Protocol Buffer MinAggregation to convert
3836
* @return A configured MinAggregationBuilder
3937
* @throws IllegalArgumentException if required fields are missing or validation fails

modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/aggregation/AggregationBuilderProtoConverterRegistryTests.java

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -288,4 +288,55 @@ public void testNestedAggregationsInfrastructure() {
288288
// exception.getMessage().contains("cannot accept sub-aggregations")
289289
// );
290290
}
291+
292+
/**
293+
* Test that invalid aggregation names are rejected.
294+
* Verifies that aggregation name validation matches REST API behavior.
295+
* See {@link org.opensearch.search.aggregations.AggregatorFactories#parseAggregators}.
296+
*/
297+
public void testInvalidAggregationNames() {
298+
AggregationBuilderProtoConverterRegistryImpl registry = new AggregationBuilderProtoConverterRegistryImpl();
299+
AggregationContainer container = AggregationContainer.newBuilder()
300+
.setMin(MinAggregation.newBuilder().setField("price").build())
301+
.build();
302+
303+
// Test name with '[' character
304+
IllegalArgumentException ex1 = expectThrows(
305+
IllegalArgumentException.class,
306+
() -> registry.fromProto("invalid[name", container)
307+
);
308+
assertTrue(
309+
"Exception should mention invalid aggregation name",
310+
ex1.getMessage().contains("Invalid aggregation name")
311+
);
312+
assertTrue(
313+
"Exception should mention forbidden characters",
314+
ex1.getMessage().contains("'[', ']', and '>'")
315+
);
316+
317+
// Test name with ']' character
318+
IllegalArgumentException ex2 = expectThrows(
319+
IllegalArgumentException.class,
320+
() -> registry.fromProto("invalid]name", container)
321+
);
322+
assertTrue(
323+
"Exception should mention invalid aggregation name",
324+
ex2.getMessage().contains("Invalid aggregation name")
325+
);
326+
327+
// Test name with '>' character
328+
IllegalArgumentException ex3 = expectThrows(
329+
IllegalArgumentException.class,
330+
() -> registry.fromProto("invalid>name", container)
331+
);
332+
assertTrue(
333+
"Exception should mention invalid aggregation name",
334+
ex3.getMessage().contains("Invalid aggregation name")
335+
);
336+
337+
// Test that valid names work fine
338+
AggregationBuilder validBuilder = registry.fromProto("valid_name-with.dots:and_more", container);
339+
assertNotNull("Valid aggregation name should be accepted", validBuilder);
340+
assertEquals("valid_name-with.dots:and_more", validBuilder.getName());
341+
}
291342
}

0 commit comments

Comments
 (0)