Document null handling behavior in declareField to match REST

Add comprehensive javadoc explaining null handling behavior:

1. If value is null:
   - Parser and consumer NOT called (no-op)
   - Matches REST behavior when field not present in JSON

2. If value is not null:
   - Parser transforms the value
   - Consumer called with transformed value

3. If parser returns null:
   - Consumer STILL called with null
   - Setter is responsible for null validation (matches REST)

This clarifies that the null-check behavior perfectly mirrors REST's
ObjectParser pattern, where absent fields result in no parser/consumer
invocation.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: yiyuabc

modules/transport-grpc/min_max_aggregation_comparison_report.md

@@ -16,33 +16,6 @@
  * <p>This class provides declarative field parsing methods that explicitly mirror REST's ObjectParser pattern,
  * making it easy to compare gRPC and REST implementations side-by-side.
  *
- * <p>Key principles:
- * <ul>
- *   <li>Proto uses hasField() booleans instead of checking for field presence in JSON</li>
- *   <li>Validation happens in the setter/consumer, just like REST (setters throw with aggregation name)</li>
- *   <li>ObjectParserProtoUtils just dispatches - it doesn't validate</li>
- * </ul>
- *
- * <p>Example usage mirroring REST:
- * <pre>
- * // REST:
- * objectParser.declareField(
- *     ValuesSourceAggregationBuilder::format,
- *     XContentParser::text,
- *     ParseField.CommonFields.FORMAT,
- *     ObjectParser.ValueType.STRING
- * );
- *
- * // gRPC equivalent:
- * ObjectParserProtoUtils.declareField(
- *     builder,
- *     ValuesSourceAggregationBuilder::format,  // Setter validates!
- *     format,                                   // null if not set
- *     Function.identity(),                      // No transformation needed
- *     "format"
- * );
- * </pre>
- *
  * @see org.opensearch.core.xcontent.ObjectParser
  */
 public class ObjectParserProtoUtils {
@@ -61,6 +34,15 @@ private ObjectParserProtoUtils() {
      * extracted from proto, so the parser is used for transformation only. For simple fields that
      * need no transformation, use {@link Function#identity()}.
      *
+     * <p><b>Null handling (matches REST):</b>
+     * <ul>
+     *   <li>If value is null: parser and consumer are NOT called (no-op), matching REST behavior
+     *       when a field is not present in JSON</li>
+     *   <li>If value is not null: parser transforms it, then consumer is called with the result</li>
+     *   <li>If parser returns null: consumer is STILL called - the setter is responsible for
+     *       null validation, just like REST</li>
+     * </ul>
+     *
      * @param builder The builder to set the field on
      * @param consumer The consumer to set the field value (e.g., Builder::field, Builder::missing)
      * @param value The proto value (if null, the field is not set)
@@ -87,12 +69,15 @@ public static <T, P, V> void declareField(
         if (parser == null) {
             throw new IllegalArgumentException("[parser] is required");
         }
+        // If value is null, do nothing - matches REST behavior when field not present in JSON
         if (value != null) {
             try {
                 V transformedValue = parser.apply(value);
-                consumer.accept(builder, transformedValue);  // Setter validates
+                // Note: Even if transformedValue is null, we still call consumer
+                // The consumer is responsible for null validation, just like REST
+                consumer.accept(builder, transformedValue);
             } catch (IllegalArgumentException e) {
-                throw e; // Re-throw validation exceptions
+                throw e;
             } catch (Exception e) {
                 throw new IllegalArgumentException(
                     "Failed to parse [" + fieldName + "]",

modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/common/ObjectParserProtoUtils.java

@@ -30,24 +30,20 @@ public class AggregationBuilderProtoConverterRegistryImpl implements Aggregation
 
     @Inject
     public AggregationBuilderProtoConverterRegistryImpl() {
-        // Create the SPI registry which will hold all converters
         this.delegate = new AggregationBuilderProtoConverterSpiRegistry();
 
-        // Register built-in converters for this module
         registerBuiltInConverters();
     }
 
     protected void registerBuiltInConverters() {
         // Register metric aggregation converters
-        // These are simple aggregations that compute a single metric value
         delegate.registerConverter(new MinAggregationBuilderProtoConverter());
         delegate.registerConverter(new MaxAggregationBuilderProtoConverter());
 
         // Future: Register bucket aggregation converters here
         // Example: delegate.registerConverter(new TermsAggregationBuilderProtoConverter());
 
         // Set the registry on all converters so they can access each other
-        // This is critical for bucket aggregations that need to parse nested aggregations
         delegate.setRegistryOnAllConverters(this);
 
         logger.info("Registered {} built-in aggregation converter(s)", delegate.size());

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

@@ -51,7 +51,6 @@ public AggregationBuilder fromProto(String name, AggregationContainer container)
             throw new IllegalArgumentException("Aggregation name cannot be null or empty");
         }
 
-        // Use direct map lookup for better performance
         AggregationContainer.AggregationContainerCase aggregationCase = container.getAggregationContainerCase();
         AggregationBuilderProtoConverter converter = converterMap.get(aggregationCase);
 

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

@@ -14,8 +14,6 @@
 /**
  * Converter for Max aggregation. Delegates to {@link MaxAggregationProtoUtils}.
  * Mirrors REST parsing via {@link org.opensearch.search.aggregations.metrics.MaxAggregationBuilder#PARSER}.
- *
- * @see org.opensearch.search.aggregations.metrics.MaxAggregationBuilder
  */
 public class MaxAggregationBuilderProtoConverter implements AggregationBuilderProtoConverter {
 

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

@@ -37,7 +37,6 @@ private MaxAggregationProtoUtils() {
      * @param maxAggProto The Protocol Buffer MaxAggregation to convert
      * @return A configured MaxAggregationBuilder
      * @throws IllegalArgumentException if required fields are missing or validation fails
-     * @see MaxAggregationBuilder#PARSER
      */
     public static MaxAggregationBuilder fromProto(String name, MaxAggregation maxAggProto) {
         if (maxAggProto == null) {

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

@@ -14,8 +14,6 @@
 /**
  * Converter for Min aggregation. Delegates to {@link MinAggregationProtoUtils}.
  * Mirrors REST parsing via {@link org.opensearch.search.aggregations.metrics.MinAggregationBuilder#PARSER}.
- *
- * @see org.opensearch.search.aggregations.metrics.MinAggregationBuilder
  */
 public class MinAggregationBuilderProtoConverter implements AggregationBuilderProtoConverter {
 

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

@@ -37,7 +37,6 @@ private MinAggregationProtoUtils() {
      * @param minAggProto The Protocol Buffer MinAggregation to convert
      * @return A configured MinAggregationBuilder
      * @throws IllegalArgumentException if required fields are missing or validation fails
-     * @see MinAggregationBuilder#PARSER
      */
     public static MinAggregationBuilder fromProto(String name, MinAggregation minAggProto) {
         if (minAggProto == null) {

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

@@ -69,7 +69,6 @@ public static void declareFields(
         boolean timezoneAware,
         boolean fieldRequired
     ) {
-        // Field - mirrors: objectParser.declareField(Builder::field, XContentParser::text, FIELD, STRING)
         ObjectParserProtoUtils.declareField(
             builder,
             ValuesSourceAggregationBuilder::field,
@@ -78,7 +77,6 @@ public static void declareFields(
             ParseField.CommonFields.FIELD.getPreferredName()
         );
 
-        // Missing - mirrors: objectParser.declareField(Builder::missing, XContentParser::objectText, MISSING, VALUE)
         ObjectParserProtoUtils.declareField(
             builder,
             ValuesSourceAggregationBuilder::missing,
@@ -87,7 +85,6 @@ public static void declareFields(
             ParseField.CommonFields.MISSING.getPreferredName()
         );
 
-        // Value type - mirrors: objectParser.declareField(Builder::userValueTypeHint, parser, VALUE_TYPE, STRING)
         ObjectParserProtoUtils.declareField(
             builder,
             ValuesSourceAggregationBuilder::userValueTypeHint,
@@ -106,7 +103,6 @@ public static void declareFields(
             ValueType.VALUE_TYPE.getPreferredName()
         );
 
-        // Format - mirrors: objectParser.declareField(Builder::format, XContentParser::text, FORMAT, STRING)
         if (formattable) {
             ObjectParserProtoUtils.declareField(
                 builder,
@@ -117,7 +113,6 @@ public static void declareFields(
             );
         }
 
-        // Script - mirrors: objectParser.declareField(Builder::script, Script::parse, SCRIPT, OBJECT_OR_STRING)
         if (scriptable) {
             ObjectParserProtoUtils.declareField(
                 builder,
@@ -127,7 +122,6 @@ public static void declareFields(
                 Script.SCRIPT_PARSE_FIELD.getPreferredName()
             );
 
-            // Field requirement validation - mirrors: objectParser.declareRequiredFieldSet(...)
             if (fieldRequired) {
                 if (fields.getField() == null && fields.getScript() == null) {
                     throw new IllegalArgumentException(
@@ -140,7 +134,6 @@ public static void declareFields(
                 }
             }
         } else {
-            // Non-scriptable aggregations
             if (fieldRequired) {
                 if (fields.getField() == null) {
                     throw new IllegalArgumentException(
@@ -150,7 +143,6 @@ public static void declareFields(
             }
         }
 
-        // Timezone - not yet supported
         if (timezoneAware) {
             throw new UnsupportedOperationException(
                 "Timezone field is not yet supported in gRPC aggregations"