Add gRPC support for Min and Max metric aggregations by yiyuabc · Pull Request #20794 · opensearch-project/OpenSearch

yiyuabc · 2026-03-07T06:09:58Z

This commit adds complete gRPC support for Min and Max metric aggregations, including request parsing, response conversion, and integration with SearchSourceBuilder.

opensearch-protobufs PR: Add protobuf definitions for min/max aggregation opensearch-protobufs#410

Request-Side Converters (Proto → Java)

Core Infrastructure:

AggregationContainerProtoUtils: Central dispatcher routing aggregation types to specific converters, validates aggregation names
ValuesSourceAggregationProtoUtils: Shared utilities for parsing common ValuesSource fields (field, missing, value_type, format, script)

Min/Max Converters:

MinAggregationProtoUtils: Converts proto MinAggregation → MinAggregationBuilder
MaxAggregationProtoUtils: Converts proto MaxAggregation → MaxAggregationBuilder

SearchSourceBuilder Integration:

Updated SearchSourceBuilderProtoUtils to parse aggregations map from proto SearchRequestBody and add to SearchSourceBuilder

Response-Side Converters (Java → Proto)

Core Infrastructure:

AggregateProtoUtils: Central dispatcher for converting InternalAggregation to proto Aggregate, with metadata and sub-aggregation helpers
SearchResponseSectionsProtoUtils: Put aggregation results back to response

Min/Max Converters:

MinAggregateProtoUtils: Converts InternalMin → proto MinAggregate
MaxAggregateProtoUtils: Converts InternalMax → proto MaxAggregate
Handles special values (infinity, NaN), formatting, and metadata

Server-Side Changes

InternalNumericMetricsAggregation: Added getFormat() getter to expose format information for gRPC converters

Test Coverage

Request-Side Tests (~260 tests):

AggregationContainerProtoUtilsTests: 11 tests
MinAggregationProtoUtilsTests: 108 tests
MaxAggregationProtoUtilsTests: 108 tests
ValuesSourceAggregationProtoUtilsTests: 40+ tests
SearchSourceBuilderProtoUtilsTests: 2 new aggregation tests

Response-Side Tests (~410 tests):

AggregateProtoUtilsTests: 10 tests
SearchResponseSectionsProtoUtilsTests.java: 2 tests
MinAggregateProtoUtilsTests: 190 tests (values, infinity, NaN, formatting)
MaxAggregateProtoUtilsTests: 190 tests (values, infinity, NaN, formatting)
InternalMinTests: 32 tests
InternalMaxTests: 32 tests

Total: ~670 test cases

Design Principles

Mirrors REST API patterns for consistency
Maintains behavioral parity with REST layer
Comprehensive error handling and validation
Special value support (infinity, NaN) matching REST behavior
Metadata handling consistent across all aggregations

Description

Min/Max Aggregation REST vs gRPC Comparison Report

REST API Setup and Examples

Step 1: Create Index

Request:

curl -X PUT "http://localhost:9201/products" -H 'Content-Type: application/json' -d '
{
  "mappings": {
    "properties": {
      "product_name": { "type": "keyword" },
      "price": { "type": "double" },
      "quantity": { "type": "integer" }
    }
  }
}'

Response:

{
  "acknowledged": true,
  "shards_acknowledged": true,
  "index": "products"
}

Step 2: Index Sample Documents

Request:

curl -X POST "http://localhost:9201/products/_bulk" -H 'Content-Type: application/json' -d '
{"index":{}}
{"product_name":"Laptop","price":1299.99,"quantity":5}
{"index":{}}
{"product_name":"Mouse","price":29.99,"quantity":50}
{"index":{}}
{"product_name":"Keyboard","price":79.99,"quantity":30}
{"index":{}}
{"product_name":"Monitor","price":399.99,"quantity":10}
{"index":{}}
{"product_name":"USB Cable","price":9.99,"quantity":100}
'

Quick Test Commands

1. Check Server Health

grpcurl -plaintext localhost:9300 grpc.health.v1.Health/Check

2. List Available Methods

grpcurl -plaintext localhost:9300 describe org.opensearch.protobufs.services.SearchService

Scenario 1: Simple Min Aggregation

 % curl -X POST "http://localhost:9201/products/_search" -H 'Content-Type: application/json' -d '
{
  "size": 0,
  "aggs": {
    "min_price": {
      "min": {
        "field": "price"
      }
    }
  }
}'
{"took":3,"timed_out":false,"terminated_early":true,"_shards":{"total":1,"successful":1,"skipped":0,"failed":0},"hits":{"total":{"value":5,"relation":"eq"},"max_score":null,"hits":[]},"aggregations":{"min_price":{"value":9.99}}}%

 % grpcurl -plaintext \
  -d '{
    "index": ["products"],
    "search_request_body": {
      "size": 0,
      "aggregations": {
        "min_price": {
          "min": {
            "field": "price"
          }
        }
      }
    }
  }' \
  localhost:9300 \
  org.opensearch.protobufs.services.SearchService/Search
{
  "took": "3",
  "xShards": {
    "successful": 1,
    "total": 1,
    "skipped": 0
  },
  "hits": {
    "total": {
      "totalHits": {
        "relation": "TOTAL_HITS_RELATION_EQ",
        "value": "5"
      }
    },
    "maxScore": {
      "nullValue": "NULL_VALUE_NULL"
    }
  },
  "terminatedEarly": true,
  "aggregations": {
    "min_price": {
      "value": {
        "double": 9.99
      }
    }
  }
}

Scenario 2: Simple Max Aggregation

 % curl -X POST "http://localhost:9201/products/_search" -H 'Content-Type: application/json' -d '
{
  "size": 0,
  "aggs": {
    "max_price": {
      "max": {
        "field": "price"
      }
    }
  }
}'
{"took":3,"timed_out":false,"terminated_early":true,"_shards":{"total":1,"successful":1,"skipped":0,"failed":0},"hits":{"total":{"value":5,"relation":"eq"},"max_score":null,"hits":[]},"aggregations":{"max_price":{"value":1299.99}}}%

 % grpcurl -plaintext \
  -d '{
    "index": ["products"],
    "search_request_body": {
      "size": 0,
      "aggregations": {
        "max_price": {
          "max": {
            "field": "price"
          }
        }
      }
    }
  }' \
  localhost:9300 \
  org.opensearch.protobufs.services.SearchService/Search
{
  "took": "3",
  "xShards": {
    "successful": 1,
    "total": 1,
    "skipped": 0
  },
  "hits": {
    "total": {
      "totalHits": {
        "relation": "TOTAL_HITS_RELATION_EQ",
        "value": "5"
      }
    },
    "maxScore": {
      "nullValue": "NULL_VALUE_NULL"
    }
  },
  "terminatedEarly": true,
  "aggregations": {
    "max_price": {
      "value": {
        "double": 1299.99
      }
    }
  }
}

Scenario 3: Multiple Aggregations

Purpose: Verify multiple aggregations in a single request

 curl -X POST "http://localhost:9201/products/_search" -H 'Content-Type: application/json' -d '
{
  "size": 0,
  "aggs": {
    "min_price": {
      "min": {
        "field": "price"
      }
    },
    "max_price": {
      "max": {
        "field": "price"
      }
    },
    "min_quantity": {
      "min": {
        "field": "quantity"
      }
    },
    "max_quantity": {
      "max": {
        "field": "quantity"
      }
    }
  }
}'
{"took":3,"timed_out":false,"terminated_early":true,"_shards":{"total":1,"successful":1,"skipped":0,"failed":0},"hits":{"total":{"value":5,"relation":"eq"},"max_score":null,"hits":[]},"aggregations":{"max_price":{"value":1299.99},"max_quantity":{"value":100.0},"min_price":{"value":9.99},"min_quantity":{"value":5.0}}}%

grpcurl -plaintext \
  -d '{
    "index": ["products"],
    "search_request_body": {
      "size": 0,
      "aggregations": {
        "min_price": {
          "min": {
            "field": "price"
          }
        },
        "max_price": {
          "max": {
            "field": "price"
          }
        },
        "min_quantity": {
          "min": {
            "field": "quantity"
          }
        },
        "max_quantity": {
          "max": {
            "field": "quantity"
          }
        }
      }
    }
  }' \
  localhost:9300 \
  org.opensearch.protobufs.services.SearchService/Search
{
  "took": "4",
  "xShards": {
    "successful": 1,
    "total": 1,
    "skipped": 0
  },
  "hits": {
    "total": {
      "totalHits": {
        "relation": "TOTAL_HITS_RELATION_EQ",
        "value": "5"
      }
    },
    "maxScore": {
      "nullValue": "NULL_VALUE_NULL"
    }
  },
  "terminatedEarly": true,
  "aggregations": {
    "max_price": {
      "value": {
        "double": 1299.99
      }
    },
    "max_quantity": {
      "value": {
        "double": 100
      }
    },
    "min_price": {
      "value": {
        "double": 9.99
      }
    },
    "min_quantity": {
      "value": {
        "double": 5
      }
    }
  }
}

Scenario 4: Invalid Aggregation Name (Validation)

Purpose: Verify aggregation name validation rejects invalid characters

 % curl -X POST "http://localhost:9201/products/_search" -H 'Content-Type: application/json' -d '
{
  "size": 0,
  "aggs": {
    "invalid[name": {
      "min": {
        "field": "price"
      }
    }
  }
}'
{"error":{"root_cause":[{"type":"parsing_exception","reason":"Invalid aggregation name [invalid[name]. Aggregation names can contain any character except '[', ']', and '>'","line":5,"col":5}],"type":"parsing_exception","reason":"Invalid aggregation name [invalid[name]. Aggregation names can contain any character except '[', ']', and '>'","line":5,"col":5},"status":400}%

  % grpcurl -plaintext \
  -d '{
    "index": ["products"],
    "search_request_body": {
      "size": 0,
      "aggregations": {
        "invalid[name": {
          "min": {
            "field": "price"
          }
        }
      }
    }
  }' \
  localhost:9300 \
  org.opensearch.protobufs.services.SearchService/Search
ERROR:
  Code: InvalidArgument
  Message: java.lang.IllegalArgumentException: Invalid aggregation name [invalid[name]. Aggregation names can contain any character except '[', ']', and '>'
        at org.opensearch.transport.grpc.proto.request.search.aggregation.AggregationBuilderProtoConverterSpiRegistry.fromProto(AggregationBuilderProtoConverterSpiRegistry.java:57)
        at org.opensearch.transport.grpc.proto.request.search.aggregation.AggregationBuilderProtoConverterRegistryImpl.fromProto(AggregationBuilderProtoConverterRegistryImpl.java:57)
        at org.opensearch.transport.grpc.proto.request.search.SearchSourceBuilderProtoUtils.parseNonQueryFields(SearchSourceBuilderProtoUtils.java:163)
        at org.opensearch.transport.grpc.proto.request.search.SearchSourceBuilderProtoUtils.parseProto(SearchSourceBuilderProtoUtils.java:62)
        at org.opensearch.transport.grpc.proto.request.search.SearchRequestProtoUtils.parseSearchRequest(SearchRequestProtoUtils.java:121)
        at org.opensearch.transport.grpc.proto.request.search.SearchRequestProtoUtils.prepareRequest(SearchRequestProtoUtils.java:86)
        at org.opensearch.transport.grpc.services.SearchServiceImpl.search(SearchServiceImpl.java:98)
        at org.opensearch.protobufs.services.SearchServiceGrpc$MethodHandlers.invoke(SearchServiceGrpc.java:265)
        at io.grpc.stub.ServerCalls$UnaryServerCallHandler$UnaryServerCallListener.onHalfClose(ServerCalls.java:182)
        at io.grpc.PartialForwardingServerCallListener.onHalfClose(PartialForwardingServerCallListener.java:35)
        at io.grpc.ForwardingServerCallListener.onHalfClose(ForwardingServerCallListener.java:23)
        at io.grpc.ForwardingServerCallListener$SimpleForwardingServerCallListener.onHalfClose(ForwardingServerCallListener.java:40)
        at org.opensearch.transport.grpc.interceptor.GrpcInterceptorChain$3.lambda$onHalfClose$1(GrpcInterceptorChain.java:141)
        at org.opensearch.transport.grpc.interceptor.GrpcInterceptorChain$3.runWithThreadContext(GrpcInterceptorChain.java:130)
        at org.opensearch.transport.grpc.interceptor.GrpcInterceptorChain$3.onHalfClose(GrpcInterceptorChain.java:141)
        at io.grpc.internal.ServerCallImpl$ServerStreamListenerImpl.halfClosed(ServerCallImpl.java:356)
        at io.grpc.internal.ServerImpl$JumpToApplicationThreadServerStreamListener$1HalfClosed.runInContext(ServerImpl.java:861)
        at io.grpc.internal.ContextRunnable.run(ContextRunnable.java:37)
        at io.grpc.internal.SerializingExecutor.run(SerializingExecutor.java:133)
        at org.opensearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:918)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1090)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:614)
        at java.base/java.lang.Thread.run(Thread.java:1474)

Scenario 5: Value Formatting

Purpose: Verify aggregation value formatting with custom format strings

 % curl -X POST "http://localhost:9201/products/_search" -H 'Content-Type: application/json' -d '
{
  "size": 0,
  "aggs": {
    "min_price_formatted": {
      "min": {
        "field": "price",
        "format": "0.00"
      }
    }
  }
}'
{"took":4,"timed_out":false,"terminated_early":true,"_shards":{"total":1,"successful":1,"skipped":0,"failed":0},"hits":{"total":{"value":5,"relation":"eq"},"max_score":null,"hits":[]},"aggregations":{"min_price_formatted":{"value":9.99,"value_as_string":"9.99"}}}%

 % >....                                                                                                                                                                                                         
    "search_request_body": {
      "size": 0,
      "aggregations": {
        "min_price_formatted": {
          "min": {
            "field": "price",
            "format": "0.00"
          }
        }
      }
    }
  }' \
  localhost:9300 \
  org.opensearch.protobufs.services.SearchService/Search
{
  "took": "3",
  "xShards": {
    "successful": 1,
    "total": 1,
    "skipped": 0
  },
  "hits": {
    "total": {
      "totalHits": {
        "relation": "TOTAL_HITS_RELATION_EQ",
        "value": "5"
      }
    },
    "maxScore": {
      "nullValue": "NULL_VALUE_NULL"
    }
  },
  "terminatedEarly": true,
  "aggregations": {
    "min_price_formatted": {
      "value": {
        "double": 9.99
      },
      "valueAsString": "9.99"
    }
  }
}

Scenario 6: Script-Based Aggregation

Purpose: Verify aggregation using Painless script instead of field

 % curl -X POST "http://localhost:9201/products/_search" -H 'Content-Type: application/json' -d '
{
  "size": 0,
  "aggs": {
    "min_discounted_price": {
      "min": {
        "script": {
          "source": "doc['\''price'\''].value * 0.9"
        }
      }
    }
  }
}'
{"took":4,"timed_out":false,"terminated_early":true,"_shards":{"total":1,"successful":1,"skipped":0,"failed":0},"hits":{"total":{"value":5,"relation":"eq"},"max_score":null,"hits":[]},"aggregations":{"min_discounted_price":{"value":8.991}}}%

grpcurl -plaintext \
  -d '{
    "index": ["products"],
    "search_request_body": {
      "size": 0,
      "aggregations": {
        "min_discounted_price": {
          "min": {
            "script": {
              "inline": {
                "source": "doc['\''price'\''].value * 0.9"
              }
            }
          }
        }
      }
    }
  }' \
  localhost:9300 \
  org.opensearch.protobufs.services.SearchService/Search
{
  "took": "4",
  "xShards": {
    "successful": 1,
    "total": 1,
    "skipped": 0
  },
  "hits": {
    "total": {
      "totalHits": {
        "relation": "TOTAL_HITS_RELATION_EQ",
        "value": "5"
      }
    },
    "maxScore": {
      "nullValue": "NULL_VALUE_NULL"
    }
  },
  "terminatedEarly": true,
  "aggregations": {
    "min_discounted_price": {
      "value": {
        "double": 8.991
      }
    }
  }
}

Scenario 7: Missing Value Handling

Purpose: Verify handling of documents missing the aggregation field

Setup: First, add a document without a price field:

 % curl -X POST "http://localhost:9201/products/_doc" -H 'Content-Type: application/json' -d '
{
  "product_name": "Gift Card",
  "quantity": 100
}'
{"_index":"products","_id":"rzTZ6pwB3y5hhgaU1OGj","_version":1,"result":"created","_shards":{"total":2,"successful":1,"failed":0},"_seq_no":9,"_primary_term":1}%

 % curl -X POST "http://localhost:9201/products/_search" -H 'Content-Type: application/json' -d '
{
  "size": 0,
  "aggs": {
    "min_price_with_default": {
      "min": {
        "field": "price",
        "missing": 5.0
      }
    }
  }
}'
{"took":202,"timed_out":false,"terminated_early":true,"_shards":{"total":1,"successful":1,"skipped":0,"failed":0},"hits":{"total":{"value":6,"relation":"eq"},"max_score":null,"hits":[]},"aggregations":{"min_price_with_default":{"value":5.0}}}%

grpcurl -plaintext \
  -d '{
    "index": ["products"],
    "search_request_body": {
      "size": 0,
      "aggregations": {
        "min_price_with_default": {
          "min": {
            "field": "price",
            "missing": {
              "general_number": {
                "double_value": 5.0
              }
            }
          }
        }
      }
    }
  }' \
  localhost:9300 \
  org.opensearch.protobufs.services.SearchService/Search
{
  "took": "3",
  "xShards": {
    "successful": 1,
    "total": 1,
    "skipped": 0
  },
  "hits": {
    "total": {
      "totalHits": {
        "relation": "TOTAL_HITS_RELATION_EQ",
        "value": "6"
      }
    },
    "maxScore": {
      "nullValue": "NULL_VALUE_NULL"
    }
  },
  "terminatedEarly": true,
  "aggregations": {
    "min_price_with_default": {
      "value": {
        "double": 5
      }
    }
  }
}

Scenario 8: Combined Format and Missing

Purpose: Verify combination of value formatting and missing value handling

 % curl -X POST "http://localhost:9201/products/_search" -H 'Content-Type: application/json' -d '
{
  "size": 0,
  "aggs": {
    "min_price_complete": {
      "min": {
        "field": "price",
        "format": "0.00",
        "missing": 5.0
      }
    }
  }
}'
{"took":4,"timed_out":false,"terminated_early":true,"_shards":{"total":1,"successful":1,"skipped":0,"failed":0},"hits":{"total":{"value":6,"relation":"eq"},"max_score":null,"hits":[]},"aggregations":{"min_price_complete":{"value":5.0,"value_as_string":"5.00"}}}%

grpcurl -plaintext \
  -d '{
    "index": ["products"],
    "search_request_body": {
      "size": 0,
      "aggregations": {
        "min_price_complete": {
          "min": {
            "field": "price",
            "format": "0.00",
            "missing": {
              "general_number": {
                "double_value": 5.0
              }
            }
          }
        }
      }
    }
  }' \
  localhost:9300 \
  org.opensearch.protobufs.services.SearchService/Search
{
  "took": "3",
  "xShards": {
    "successful": 1,
    "total": 1,
    "skipped": 0
  },
  "hits": {
    "total": {
      "totalHits": {
        "relation": "TOTAL_HITS_RELATION_EQ",
        "value": "6"
      }
    },
    "maxScore": {
      "nullValue": "NULL_VALUE_NULL"
    }
  },
  "terminatedEarly": true,
  "aggregations": {
    "min_price_complete": {
      "value": {
        "double": 5
      },
      "valueAsString": "5.00"
    }
  }
}

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

Functionality includes testing.
API changes companion pull request created, if applicable.
Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

github-actions · 2026-03-07T06:11:34Z

PR Reviewer Guide 🔍

(Review updated until commit `398d0a8`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
📝 TODO sections modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/aggregation/support/ValuesSourceAggregationProtoUtils.java [106]: Implement parseTimeZone when timezone support is needed modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/aggregation/support/ValuesSourceAggregationProtoUtils.java [224]: Add parseTimeZone() when timezone support is needed modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseSectionsProtoUtils.java [93]: Implement suggest conversion
🔀 Multiple PR themes Sub-PR theme: Add getFormat() getter to InternalNumericMetricsAggregation Relevant files: server/src/main/java/org/opensearch/search/aggregations/metrics/InternalNumericMetricsAggregation.java server/src/test/java/org/opensearch/search/aggregations/metrics/InternalMaxTests.java server/src/test/java/org/opensearch/search/aggregations/metrics/InternalMinTests.java Sub-PR theme: gRPC request-side converters for Min/Max aggregations Relevant files: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/aggregation/AggregationContainerProtoUtils.java modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/aggregation/metrics/MinAggregationProtoUtils.java modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/aggregation/metrics/MaxAggregationProtoUtils.java modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/aggregation/support/ValuesSourceAggregationProtoUtils.java modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/aggregation/AggregationContainerProtoUtilsTests.java modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/aggregation/metrics/MinAggregationProtoUtilsTests.java modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/aggregation/metrics/MaxAggregationProtoUtilsTests.java modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/aggregation/support/ValuesSourceAggregationProtoUtilsTests.java modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtilsTests.java Sub-PR theme: gRPC response-side converters for Min/Max aggregations Relevant files: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/aggregation/AggregateProtoUtils.java modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/aggregation/metrics/MinAggregateProtoUtils.java modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/aggregation/metrics/MaxAggregateProtoUtils.java modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseSectionsProtoUtils.java modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/aggregation/AggregateProtoUtilsTests.java modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/aggregation/metrics/MinAggregateProtoUtilsTests.java modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/aggregation/metrics/MaxAggregateProtoUtilsTests.java modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseSectionsProtoUtilsTests.java
⚡ Recommended focus areas for review Unsafe Cast The code casts `response.getAggregations()` directly to `InternalAggregations` without checking if it is actually an instance of that type. If a non-InternalAggregations implementation is returned, this will throw a ClassCastException at runtime. AggregateProtoUtils.toProtoInternal((InternalAggregations) response.getAggregations(), (name, aggProto) -> builder.putAggregations(name, aggProto)); } Incomplete NaN Handling The `toProto` method in `MinAggregateProtoUtils` and `MaxAggregateProtoUtils` uses `Double.isInfinite()` to detect "no value" cases, but does not handle `Double.isNaN()`. If a NaN value is returned by the aggregation, it will be passed as a double value to the protobuf builder, which may cause unexpected behavior. The PR description mentions handling NaN, but the implementation does not cover it. aggregateBuilder.setMax(MaxAggregateProtoUtils.toProto((InternalMax) aggregation)); } else { TODO Left Unimplemented The `timezoneAware` parameter is accepted but the timezone parsing is not implemented — only a TODO comment is left. If a caller passes `timezoneAware=true` with a timezone value, the timezone will be silently ignored without any warning or error, which could lead to incorrect aggregation results. // Conditional: timezone if (timezoneAware) { // TODO: Implement parseTimeZone when timezone support is needed // Currently no aggregations use this } Unchecked Cast in Loop In `toProtoInternal`, each `Aggregation` from `aggregations.asList()` is cast to `InternalAggregation` without an instanceof check. If a non-InternalAggregation implementation is present in the list, this will throw a ClassCastException at runtime. for (org.opensearch.search.aggregations.Aggregation agg : aggregations.asList()) { Aggregate protoAgg = AggregateProtoUtils.toProto((InternalAggregation) agg); adder.accept(agg.getName(), protoAgg);

github-actions · 2026-03-07T06:12:19Z

PR Code Suggestions ✨

Latest suggestions up to 398d0a8

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Guard against unsafe aggregations cast The cast `(InternalAggregations) response.getAggregations()` will throw a `ClassCastException` at runtime if the aggregations object is not an `InternalAggregations` instance (e.g., in certain test or non-standard contexts). Add an `instanceof` check before casting to avoid this. modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseSectionsProtoUtils.java [76-78] -if (response.getAggregations() != null) { +if (response.getAggregations() instanceof InternalAggregations) { AggregateProtoUtils.toProtoInternal((InternalAggregations) response.getAggregations(), (name, aggProto) -> builder.putAggregations(name, aggProto)); } Suggestion importance[1-10]: 7 __ Why: The direct cast `(InternalAggregations) response.getAggregations()` could throw a `ClassCastException` at runtime if the aggregations object is not an `InternalAggregations` instance. Using `instanceof` before casting is a safer approach that prevents potential runtime failures.	Medium
General	Validate before applying side effects The validation check `!hasField && !hasScript` occurs after `parseScript` is called, but `hasScript` reflects the proto's `hasScript()` state, not whether the script was successfully parsed and set. If `parseScript` throws internally and is caught, the state could be inconsistent. More importantly, the script is parsed before validation, meaning side effects (setting the script on the builder) happen even when validation will subsequently fail. Validation should occur before parsing to avoid partial state. modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/aggregation/support/ValuesSourceAggregationProtoUtils.java [87-102] if (scriptable) { - parseScript(builder, hasScript, scriptProto); - // Required field validation - at least ONE of [field, script] is required + // Validate before applying side effects if (fieldRequired && !hasField && !hasScript) { throw new IllegalArgumentException( "Required one of fields [field, script], but none were specified." ); } + parseScript(builder, hasScript, scriptProto); } else { - // Required field validation - only field is required (script not supported) if (fieldRequired && !hasField) { throw new IllegalArgumentException( "Required field [field] was not specified." ); } } Suggestion importance[1-10]: 4 __ Why: The suggestion to validate before calling `parseScript` is a minor ordering improvement to avoid partial state on the builder. However, since `parseScript` only sets the script if `hasScript` is true, and the validation checks `!hasScript`, the script would not actually be set in the failing case, making this a low-impact issue in practice.	Low
General	Prevent silent metadata loss during conversion If `ObjectMapProtoUtils.toProto(metadata)` returns a value that does not have an `ObjectMap` (e.g., it wraps the map differently), the metadata will be silently dropped without any indication. This could lead to metadata being lost without any error or warning, making debugging difficult. Consider logging a warning or throwing an exception if the conversion unexpectedly produces a non-ObjectMap value. modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/aggregation/AggregateProtoUtils.java [82-92] public static void setMetadataIfPresent( java.util.Map<String, Object> metadata, Consumer<ObjectMap> setter ) { if (metadata != null && !metadata.isEmpty()) { ObjectMap.Value metaValue = ObjectMapProtoUtils.toProto(metadata); if (metaValue.hasObjectMap()) { setter.accept(metaValue.getObjectMap()); + } else { + throw new IllegalStateException( + "Failed to convert aggregation metadata to ObjectMap protobuf: unexpected value type " + metaValue.getKindCase() + ); } } } Suggestion importance[1-10]: 3 __ Why: While the suggestion to avoid silent metadata loss is valid in principle, throwing an `IllegalStateException` could be overly aggressive for metadata that is non-critical. A warning log would be more appropriate, and the suggestion changes the behavior significantly without strong justification from the PR context.	Low

Previous suggestions

Suggestions up to commit e0ade3a

Category	Suggestion	Impact
Possible issue	Add missing mock stubs to prevent NullPointerException The mocked `InternalMin` and `InternalMax` objects are missing stubs for `getFormat()` and `getMetadata()`, which are called by `MinAggregateProtoUtils.toProto` and `MaxAggregateProtoUtils.toProto`. Without these stubs, the mocks will return `null` for `getFormat()`, causing a `NullPointerException` when comparing against `DocValueFormat.RAW`. Add the missing stubs to prevent test failures. modules/transport-grpc/src/test/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseSectionsProtoUtilsTests.java [307-318] org.opensearch.search.aggregations.metrics.InternalMin minAgg = mock( org.opensearch.search.aggregations.metrics.InternalMin.class ); when(minAgg.getName()).thenReturn("min_price"); when(minAgg.getValue()).thenReturn(-50.0); +when(minAgg.getFormat()).thenReturn(DocValueFormat.RAW); +when(minAgg.getMetadata()).thenReturn(null); ... org.opensearch.search.aggregations.metrics.InternalMax maxAgg = mock( org.opensearch.search.aggregations.metrics.InternalMax.class ); when(maxAgg.getName()).thenReturn("max_price"); when(maxAgg.getValue()).thenReturn(9999.0); +when(maxAgg.getFormat()).thenReturn(DocValueFormat.RAW); +when(maxAgg.getMetadata()).thenReturn(null); Suggestion importance[1-10]: 8 __ Why: The mocked `InternalMin` and `InternalMax` objects lack stubs for `getFormat()` and `getMetadata()`, which are called by the proto conversion utilities. Without these stubs, `getFormat()` returns `null`, causing a `NullPointerException` when compared against `DocValueFormat.RAW` in the production code.	Medium
	Guard against unsafe aggregations cast The cast `(InternalAggregations) response.getAggregations()` is unsafe — `response.getAggregations()` returns `Aggregations`, which may not always be an `InternalAggregations` instance. This will throw a `ClassCastException` at runtime if the aggregations object is not of type `InternalAggregations`. Add an `instanceof` check before casting. modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/SearchResponseSectionsProtoUtils.java [76-78] -if (response.getAggregations() != null) { +if (response.getAggregations() instanceof InternalAggregations) { AggregateProtoUtils.toProtoInternal((InternalAggregations) response.getAggregations(), (name, aggProto) -> builder.putAggregations(name, aggProto)); } Suggestion importance[1-10]: 7 __ Why: The direct cast `(InternalAggregations) response.getAggregations()` is unsafe since `getAggregations()` returns `Aggregations` interface. Using `instanceof` before casting prevents potential `ClassCastException` at runtime, which is a valid defensive programming concern.	Medium
	Fix inconsistent method signature for metadata helper The `setMetadataIfPresent` method signature in `AggregateProtoUtils` takes a `Consumer` setter, but the test in `AggregateProtoUtilsTests` calls `AggregateProtoUtils.setMetadataIfPresent(builder, metadata)` with an `Aggregate.Builder` as the first argument. The method signature in the implementation and the test call are inconsistent — the test passes the builder first and metadata second, while the implementation has them reversed. This will cause a compilation error. modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/aggregation/AggregateProtoUtils.java [82-85] public static void setMetadataIfPresent( - java.util.Map<String, Object> metadata, - Consumer<ObjectMap> setter + Aggregate.Builder builder, + java.util.Map<String, Object> metadata ) { + if (metadata != null && !metadata.isEmpty()) { + ObjectMap.Value metaValue = ObjectMapProtoUtils.toProto(metadata); + if (metaValue.hasObjectMap()) { + builder.setMetadata(metaValue.getObjectMap()); + } + } +} Suggestion importance[1-10]: 3 __ Why: Looking at the test in `AggregateProtoUtilsTests`, it calls `AggregateProtoUtils.setMetadataIfPresent(builder, metadata)` with `Aggregate.Builder` first and metadata second, while the implementation has `(metadata, setter)`. However, the actual usage in `MinAggregateProtoUtils` and `MaxAggregateProtoUtils` calls `setMetadataIfPresent(internalMin.getMetadata(), builder::setMeta)` which matches the current implementation signature. The test appears to be calling a different overload. The suggestion's `improved_code` changes the method signature in a way that would break the existing callers in the production code.	Low

Suggestions up to commit 8e58ad1

Category	Suggestion	Impact
Possible issue	Fix mismatched method signature used in tests The test file `AggregateProtoUtilsTests.java` calls `setMetadataIfPresent(builder,` `metadata)` with an `Aggregate.Builder` as the first argument, but the actual implementation signature takes a `Map<String, Object>` and a `Consumer`. This mismatch means the tests won't compile and the API is inconsistent. Consider adding an overload that accepts the builder directly, or update the tests to match the actual signature. modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/aggregation/AggregateProtoUtils.java [82-92] public static void setMetadataIfPresent( java.util.Map<String, Object> metadata, Consumer<ObjectMap> setter ) { if (metadata != null && !metadata.isEmpty()) { ObjectMap.Value metaValue = ObjectMapProtoUtils.toProto(metadata); if (metaValue.hasObjectMap()) { setter.accept(metaValue.getObjectMap()); } } } +public static void setMetadataIfPresent( + Aggregate.Builder builder, + java.util.Map<String, Object> metadata +) { + setMetadataIfPresent(metadata, builder::setMeta); +} + Suggestion importance[1-10]: 7 __ Why: The test file calls `setMetadataIfPresent(builder, metadata)` with an `Aggregate.Builder` as the first argument, but the implementation takes `Map<String, Object>` and `Consumer<ObjectMap>`. This is a real API mismatch that would cause compilation failures in the tests. The suggestion to add an overload is valid, though the tests could also be fixed instead.	Medium
Possible issue	Handle NaN values in aggregation result conversion The `hasValue` check only tests for infinity but not for `NaN`. If `internalMax.getValue()` returns `Double.NaN`, it would be treated as a valid value and set as a double in the protobuf, which is incorrect. The check should also exclude `NaN` values to match the REST API behavior. modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/aggregation/metrics/MaxAggregateProtoUtils.java [61-62] double max = internalMax.getValue(); -boolean hasValue = !Double.isInfinite(max); +boolean hasValue = !Double.isInfinite(max) && !Double.isNaN(max); Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that `NaN` values are not handled, which could lead to incorrect protobuf output. While `NaN` is an edge case for aggregation results, it's a valid concern for robustness. The same issue exists in `MinAggregateProtoUtils` as well.	Low
General	Fix validation to check for non-default script proto The validation checks `!hasScript` (whether the proto has a script field set), but `parseScript` only sets the script when `hasScript` is true. However, `hasScript` reflects whether the proto field is present, not whether the script was successfully parsed and set. If `hasScript` is true but the script proto is empty/default, the validation passes but no meaningful script is set. The validation should also check that the script proto is non-default, or rely on the builder's state after parsing. modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/aggregation/support/ValuesSourceAggregationProtoUtils.java [87-95] if (scriptable) { parseScript(builder, hasScript, scriptProto); // Required field validation - at least ONE of [field, script] is required - if (fieldRequired && !hasField && !hasScript) { + boolean hasValidScript = hasScript && scriptProto != null + && !scriptProto.equals(org.opensearch.protobufs.Script.getDefaultInstance()); + if (fieldRequired && !hasField && !hasValidScript) { throw new IllegalArgumentException( "Required one of fields [field, script], but none were specified." ); } } Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid edge case where `hasScript` is true but the script proto is a default instance. However, in practice, protobuf's `hasScript()` should only return true when the field is explicitly set, making this scenario unlikely. The improvement is minor and adds complexity.	Low

Suggestions up to commit f6698d6

Category	Suggestion	Impact
Possible issue	Fix method signature mismatch between implementation and tests The test file `AggregateProtoUtilsTests.java` calls `AggregateProtoUtils.setMetadataIfPresent(builder, metadata)` with two arguments (an `Aggregate.Builder` and a `Map`), but the actual implementation only accepts a `Map` and a `Consumer`. This signature mismatch will cause a compilation error. The method signature needs to match how it's called in tests, or the tests need to be updated to pass a lambda setter. modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/aggregation/AggregateProtoUtils.java [82-92] public static void setMetadataIfPresent( - java.util.Map<String, Object> metadata, - Consumer<ObjectMap> setter + Aggregate.Builder aggregateBuilder, + java.util.Map<String, Object> metadata ) { if (metadata != null && !metadata.isEmpty()) { ObjectMap.Value metaValue = ObjectMapProtoUtils.toProto(metadata); if (metaValue.hasObjectMap()) { - setter.accept(metaValue.getObjectMap()); + aggregateBuilder.setMetadata(metaValue.getObjectMap()); } } } Suggestion importance[1-10]: 7 __ Why: The test file calls `setMetadataIfPresent(builder, metadata)` with an `Aggregate.Builder` and a `Map`, but the implementation accepts a `Map` and a `Consumer<ObjectMap>`. This is a real signature mismatch that would cause compilation errors. However, the actual callers in production code (e.g., `MinAggregateProtoUtils`, `MaxAggregateProtoUtils`) use the lambda form `builder::setMeta`, so the tests may be wrong rather than the implementation.	Medium
General	Handle NaN values in addition to infinity checks The `hasValue` check only tests for infinity but not for `NaN`. If `internalMax.getValue()` returns `NaN`, it would be treated as a valid value and set as a double in the protobuf, which is likely incorrect behavior. The check should also exclude `NaN` values, consistent with how the REST API handles this. modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/aggregation/metrics/MaxAggregateProtoUtils.java [61-62] double max = internalMax.getValue(); -boolean hasValue = !Double.isInfinite(max); +boolean hasValue = !Double.isInfinite(max) && !Double.isNaN(max); Suggestion importance[1-10]: 5 __ Why: The `hasValue` check only handles infinity but not `NaN`, which could result in incorrect protobuf output if `getValue()` returns `NaN`. The same issue exists in `MinAggregateProtoUtils`. Adding a `NaN` check would make the code more robust and consistent with proper numeric handling.	Low
General	Handle null return from enum string conversion explicitly When `hasValueType` is `true` but `ProtobufEnumUtils.convertToString` returns `null` (e.g., for `VALUE_TYPE_UNSPECIFIED`), the method silently skips setting the value type without any indication. This could mask misconfigured proto messages. Consider throwing an exception or logging a warning when `hasValueType` is true but the string conversion returns null. modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/aggregation/support/ValuesSourceAggregationProtoUtils.java [161-177] public static void parseValueType( ValuesSourceAggregationBuilder<?> builder, boolean hasValueType, Enum<?> valueTypeProto ) { if (hasValueType) { String valueTypeStr = ProtobufEnumUtils.convertToString(valueTypeProto); - if (valueTypeStr != null) { - ValueType valueType = ValueType.lenientParse(valueTypeStr); - if (valueType == null) { - // Match REST behavior: throw exception for unknown value type - throw new IllegalArgumentException("Unknown value type [" + valueTypeStr + "]"); - } - builder.userValueTypeHint(valueType); + if (valueTypeStr == null) { + throw new IllegalArgumentException("Cannot convert value type proto enum to string: " + valueTypeProto); } + ValueType valueType = ValueType.lenientParse(valueTypeStr); + if (valueType == null) { + // Match REST behavior: throw exception for unknown value type + throw new IllegalArgumentException("Unknown value type [" + valueTypeStr + "]"); + } + builder.userValueTypeHint(valueType); } } Suggestion importance[1-10]: 4 __ Why: When `hasValueType` is true but `ProtobufEnumUtils.convertToString` returns null (e.g., for `VALUE_TYPE_UNSPECIFIED`), the method silently skips setting the value type. While this could mask misconfigured protos, the current behavior may be intentional for unspecified enum values, making this a minor improvement rather than a critical fix.	Low

Suggestions up to commit 13a01fb

Category	Suggestion	Impact
General	Handle null value type string from enum conversion When `hasValueType` is true but `ProtobufEnumUtils.convertToString(valueTypeProto)` returns null (e.g., for `VALUE_TYPE_UNSPECIFIED`), the method silently does nothing. This could mask cases where a client explicitly sets an unrecognized value type. Consider throwing an exception or logging a warning when `valueTypeStr` is null but `hasValueType` is true, to match the REST behavior of failing on unknown value types. modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/aggregation/support/ValuesSourceAggregationProtoUtils.java [161-177] public static void parseValueType( ValuesSourceAggregationBuilder<?> builder, boolean hasValueType, Enum<?> valueTypeProto ) { if (hasValueType) { String valueTypeStr = ProtobufEnumUtils.convertToString(valueTypeProto); - if (valueTypeStr != null) { - ValueType valueType = ValueType.lenientParse(valueTypeStr); - if (valueType == null) { - // Match REST behavior: throw exception for unknown value type - throw new IllegalArgumentException("Unknown value type [" + valueTypeStr + "]"); - } - builder.userValueTypeHint(valueType); + if (valueTypeStr == null) { + throw new IllegalArgumentException("Unknown or unspecified value type: " + valueTypeProto); } + ValueType valueType = ValueType.lenientParse(valueTypeStr); + if (valueType == null) { + // Match REST behavior: throw exception for unknown value type + throw new IllegalArgumentException("Unknown value type [" + valueTypeStr + "]"); + } + builder.userValueTypeHint(valueType); } } Suggestion importance[1-10]: 4 __ Why: The suggestion raises a valid concern about silently ignoring unrecognized value types when `convertToString` returns null. However, `VALUE_TYPE_UNSPECIFIED` is a legitimate protobuf default value that clients may send when they don't intend to set a value type, so throwing an exception for it could break valid use cases. The current behavior of silently skipping may be intentional.	Low
General	Fix field/script validation to check actual builder state The validation checks `!hasScript` but `hasScript` is the parameter passed in, which reflects whether the proto has a script set. However, the script may have been parsed and set on the builder even if `hasScript` is true but the script proto is empty/default. The validation should be consistent — if `parseScript` was called and the builder now has a script, that should satisfy the requirement. Consider checking `builder.script() != null` instead of relying solely on the `hasScript` parameter for validation. modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/aggregation/support/ValuesSourceAggregationProtoUtils.java [89-93] -if (fieldRequired && !hasField && !hasScript) { +if (fieldRequired && !hasField && (!hasScript \|\| builder.script() == null)) { throw new IllegalArgumentException( "Required one of fields [field, script], but none were specified." ); } Suggestion importance[1-10]: 3 __ Why: The suggestion to check `builder.script() != null` instead of `hasScript` has some merit for robustness, but the current approach of using the `hasScript` parameter is consistent with how other parameters (`hasField`, `hasFormat`) are handled throughout the class. The validation logic is called after `parseScript`, so `hasScript` accurately reflects whether a script was provided.	Low
Possible issue	Align method signature with test usage The test `AggregateProtoUtilsTests.testSetMetadataIfPresentWithValidMetadata` calls `AggregateProtoUtils.setMetadataIfPresent(builder, metadata)` with an `Aggregate.Builder` as the first argument, but the actual method signature takes a `Map<String, Object>` and a `Consumer`. This mismatch indicates the method signature in the production code doesn't match how it's being called in the tests, suggesting the method should accept a builder directly or the tests are wrong. Verify the intended API and align the method signature with its usage. modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/aggregation/AggregateProtoUtils.java [82-92] -public static void setMetadataIfPresent( +public static <B> void setMetadataIfPresent( java.util.Map<String, Object> metadata, Consumer<ObjectMap> setter ) { if (metadata != null && !metadata.isEmpty()) { ObjectMap.Value metaValue = ObjectMapProtoUtils.toProto(metadata); if (metaValue.hasObjectMap()) { setter.accept(metaValue.getObjectMap()); } } } Suggestion importance[1-10]: 2 __ Why: The test file calls `setMetadataIfPresent(builder, metadata)` but looking at the actual test code more carefully (lines 115-120 in `AggregateProtoUtilsTests.java`), it appears the tests may be using a different overload or the tests themselves are incorrect. The `improved_code` adds a generic type parameter `<B>` but doesn't actually change the method signature in a meaningful way, making this suggestion inaccurate and unhelpful.	Low

github-actions · 2026-03-07T06:17:42Z

Persistent review updated to latest commit f6698d6

github-actions · 2026-03-07T06:24:59Z

❌ Gradle check result for f6698d6: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-03-07T06:33:15Z

Persistent review updated to latest commit 8e58ad1

github-actions · 2026-03-07T06:40:45Z

❌ Gradle check result for 8e58ad1: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-03-07T18:43:28Z

Persistent review updated to latest commit e0ade3a

github-actions · 2026-03-12T02:07:29Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit b90d2c6.

Path	Line	Severity	Description
gradle/libs.versions.toml	27	medium	The opensearchprotobufs dependency was changed from a pinned release version (1.2.0) to a mutable SNAPSHOT version (1.4.0-SNAPSHOT). SNAPSHOT artifacts are not immutable — they can be silently overwritten in the artifact repository without a version change, creating a supply chain risk where unreviewed code could be pulled into the build. This is anomalous for a production dependency update and warrants verification that the SNAPSHOT repository is controlled and the artifact content is trusted.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 1 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-03-13T21:44:20Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit b6b95c3.

Path	Line	Severity	Description
buildSrc/build.gradle	97	high	mavenLocal() is added as the FIRST repository, taking precedence over mavenCentral(). This is a well-known supply chain attack vector: any artifact placed in the local Maven cache (~/.m2/repository) on the build machine will silently override the legitimate artifact from Maven Central. This is unrelated to the stated PR purpose of adding gRPC aggregation support and should not be committed to a shared codebase. Combined with the simultaneous switch to a SNAPSHOT dependency, this could allow injection of a malicious local protobufs library during the build.
gradle/libs.versions.toml	27	medium	The opensearchprotobufs dependency is changed from a stable pinned release (1.2.0) to a SNAPSHOT version (1.4.0-SNAPSHOT). SNAPSHOT artifacts are mutable by design and their content can change at any time in the remote repository. When combined with mavenLocal() being placed first in the build repositories, this creates a path for dependency injection: a malicious actor with access to the build environment could place a tampered protobufs-1.4.0-SNAPSHOT.jar in the local Maven cache and it would be consumed by the build without integrity verification against a fixed version.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 1 | Medium: 1 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-03-13T22:01:58Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit c31f59c.

Path	Line	Severity	Description
buildSrc/build.gradle	97	high	mavenLocal() is added as the FIRST repository, before mavenCentral(). This is a well-known supply chain attack vector: any artifact present in the developer's local Maven cache (~/.m2/repository) will shadow the legitimate version from Maven Central. Combined with the SNAPSHOT dependency introduced in libs.versions.toml, this allows an attacker with write access to the local Maven cache to silently inject a malicious version of 'opensearchprotobufs' into the build, potentially compromising all gRPC message serialization/deserialization code.
gradle/libs.versions.toml	27	medium	The 'opensearchprotobufs' version is changed from the pinned stable release '1.2.0' to '1.4.0-SNAPSHOT'. SNAPSHOT versions are mutable — they can be overwritten in a Maven repository without changing the version string. This is particularly dangerous in conjunction with the mavenLocal() change: an attacker who can write to ~/.m2/repository/org/opensearch/protobufs/opensearch-protobufs/1.4.0-SNAPSHOT/ can replace the protobuf dependency with a malicious artifact that would be resolved silently. The combination of mavenLocal() priority + SNAPSHOT mutability creates a reproducible supply chain injection path.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 1 | Medium: 1 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-03-13T22:04:47Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit e37f084.

Path	Line	Severity	Description
buildSrc/build.gradle	97	medium	mavenLocal() is added as the first repository before mavenCentral(). This is a known supply chain attack vector: if a malicious artifact matching any dependency coordinates exists in the local ~/.m2 repository, it will be resolved before the legitimate artifact from Maven Central. This is especially risky in CI/CD environments where local repos may be pre-seeded.
gradle/libs.versions.toml	27	medium	The opensearchprotobufs dependency version was changed from a pinned release (1.2.0) to a mutable SNAPSHOT version (1.4.0-SNAPSHOT). SNAPSHOT artifacts are non-deterministic and can be overwritten silently. Combined with the mavenLocal() change, this creates a supply chain risk: a malicious 1.4.0-SNAPSHOT artifact placed in a local or internal Maven repository could be used in place of the intended protobuf library, potentially affecting all gRPC serialization/deserialization logic introduced in this PR.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 2 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-03-13T22:12:28Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 7e48b84.

Path	Line	Severity	Description
buildSrc/build.gradle	97	high	mavenLocal() is added as the FIRST repository in buildSrc, meaning the local Maven cache (~/.m2) is consulted before mavenCentral(). Combined with the simultaneous change to a SNAPSHOT dependency, this is a classic supply chain attack setup: an attacker with local machine access could plant a malicious artifact under the SNAPSHOT coordinates and it would be resolved before any trusted repository. Adding mavenLocal() to production build scripts—especially buildSrc which controls the build toolchain itself—is a well-known supply chain attack vector with no stated justification in this PR.
gradle/libs.versions.toml	27	medium	The opensearchprotobufs dependency is changed from the stable release '1.2.0' to '1.4.0-SNAPSHOT'. SNAPSHOT versions are mutable: the same version string can resolve to entirely different artifacts across builds. This makes the build non-deterministic and, when combined with the mavenLocal() change above, creates a concrete mechanism for injecting a malicious replacement: a poisoned 1.4.0-SNAPSHOT jar placed in the local Maven cache would silently substitute for the real dependency with no version mismatch detected.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 1 | Medium: 1 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-03-13T22:14:22Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 73b303c.

Path	Line	Severity	Description
buildSrc/build.gradle	97	high	mavenLocal() was added as the first (highest-priority) repository in the buildSrc build configuration. This is a well-known supply chain attack vector: any artifact placed in the developer's local Maven cache (~/.m2/repository) will shadow artifacts from mavenCentral(). Combined with the simultaneous switch to a SNAPSHOT dependency in the same PR, this creates a pathway for a malicious local artifact to be injected into the build without verification. SNAPSHOT artifacts are never published to Maven Central, so they can only be resolved from mavenLocal() or a private registry — making this dependency unresolvable without trusting local state.
gradle/libs.versions.toml	27	medium	The opensearchprotobufs dependency was changed from the stable release '1.2.0' to '1.4.0-SNAPSHOT'. SNAPSHOT versions are mutable, unversioned development builds that bypass reproducible build guarantees. In the context of the mavenLocal() addition in the same PR, this SNAPSHOT version can only be resolved from the local Maven repository, meaning whoever builds this project must have this artifact installed locally. This is a classic dependency confusion or build poisoning setup: a malicious actor could pre-install a crafted 1.4.0-SNAPSHOT jar that would be consumed during compilation of the entire transport-grpc module.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 1 | Medium: 1 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-03-13T22:18:48Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 3a4d4b7.

Path	Line	Severity	Description
buildSrc/build.gradle	97	medium	mavenLocal() is added as the first repository, giving locally installed Maven artifacts precedence over mavenCentral(). In a CI/CD environment, this means any artifact with a matching group/artifact/version placed in the local ~/.m2 repository would shadow the legitimate remote artifact. When combined with the simultaneous introduction of a SNAPSHOT dependency, this creates a potential supply chain attack vector where a malicious local artifact could be substituted for the real one.
gradle/libs.versions.toml	27	medium	opensearchprotobufs version changed from stable '1.2.0' to mutable '1.4.0-SNAPSHOT'. SNAPSHOT versions are non-deterministic — the same version identifier can resolve to different artifacts over time or across build environments. Combined with mavenLocal() being added to the repository list, a build machine with a malicious 1.4.0-SNAPSHOT artifact in its local Maven cache would silently use that artifact instead of the expected one. These two changes together (mavenLocal + SNAPSHOT) are a recognized supply chain attack pattern.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 2 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-03-13T23:39:25Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit e52cd6c.

Path	Line	Severity	Description
buildSrc/build.gradle	97	medium	mavenLocal() is added as the first repository, giving it precedence over mavenCentral(). This means any artifact present in the developer's local Maven cache (~/.m2) will shadow the official published version. Combined with the SNAPSHOT dependency change, this forces resolution from the local cache and creates a supply chain risk if the local repository is compromised or manipulated.
gradle/libs.versions.toml	27	medium	opensearchprotobufs dependency downgraded from stable release 1.2.0 to mutable SNAPSHOT version 1.4.0-SNAPSHOT. SNAPSHOT artifacts are not immutable and, combined with the mavenLocal() addition, will resolve exclusively from the local Maven repository. This pairing is a classic supply chain attack setup: a developer's local ~/.m2 cache could contain a malicious build of opensearchprotobufs that gets silently picked up by every build.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 2 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-03-13T23:47:57Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 29bed9d.

Path	Line	Severity	Description
buildSrc/build.gradle	97	medium	mavenLocal() added as the first (highest-priority) repository in the build script. This causes the build to prefer artifacts from the developer's local ~/.m2 cache over official repositories. In a CI/CD pipeline this could allow a compromised local Maven cache to inject malicious artifacts, but is also a common development workflow pattern.
gradle/libs.versions.toml	27	medium	The opensearchprotobufs dependency was changed from a pinned release (1.2.0) to a mutable SNAPSHOT version (1.4.0-SNAPSHOT). Combined with the mavenLocal() change, the build will first resolve this artifact from the local Maven cache, where the content is not version-locked and can change silently. This pairing creates a supply chain risk if the local environment is not trusted, though it is also a standard pattern during active development of an unreleased dependency.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 2 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-03-14T05:32:01Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 3aec589.

Path	Line	Severity	Description
buildSrc/build.gradle	97	high	mavenLocal() added to the build repository list. When combined with the simultaneous change to a SNAPSHOT version of opensearchprotobufs (the gRPC serialization library), this creates a supply chain attack vector: the build can be silently redirected to load a locally-installed, potentially malicious version of the protobuf library that controls all gRPC data serialization and deserialization. This combination is particularly dangerous in CI/CD environments where repository poisoning could go undetected.
gradle/libs.versions.toml	27	medium	opensearchprotobufs dependency changed from the stable, pinned release '1.2.0' to a mutable SNAPSHOT version '1.4.0-SNAPSHOT'. SNAPSHOT versions are not content-addressable and can be silently replaced after the initial resolution. Because this library handles all gRPC protocol buffer serialization, a substituted SNAPSHOT could intercept, manipulate, or exfiltrate all data passing through the gRPC transport layer without modifying any application-level code.
modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/response/search/aggregation/AggregateProtoUtils.java	33	low	Static mutable field 'registry' with a public setRegistry() method allows runtime replacement of the entire aggregation converter registry from any code path with access to this class. While documented as for testing, this pattern acts as a global hook point: a malicious plugin or injected component could replace the registry to intercept, modify, or exfiltrate all aggregation results processed through the gRPC response path.

The table above displays the top 10 most important findings.

Total: 3 | Critical: 0 | High: 1 | Medium: 1 | Low: 1

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-03-14T05:55:30Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 7252dcb.

Path	Line	Severity	Description
buildSrc/build.gradle	97	high	mavenLocal() is added as the FIRST repository before mavenCentral(). Combined with the simultaneous change to a SNAPSHOT dependency version, this creates a supply chain attack vector: any artifact placed in the local Maven repository (~/.m2) takes precedence over the central repository. An attacker with write access to the local Maven cache (e.g., via a compromised build agent or prior malware) could inject a malicious opensearchprotobufs artifact that would be silently resolved before the legitimate one.
gradle/libs.versions.toml	27	medium	The opensearchprotobufs dependency version is changed from a fixed release (1.2.0) to a mutable SNAPSHOT (1.4.0-SNAPSHOT). SNAPSHOT versions are non-deterministic — the resolved artifact can change between builds without any change to the version string. When combined with mavenLocal() being added first in the repository list, this creates a window for a local dependency substitution attack. Even in isolation, shipping SNAPSHOT dependencies in production code introduces uncontrolled build inputs.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 1 | Medium: 1 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-03-14T06:00:00Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit c174727.

Path	Line	Severity	Description
gradle/libs.versions.toml	27	high	The 'opensearchprotobufs' dependency version is changed from the stable '1.2.0' to '1.4.0-SNAPSHOT'. Combined with the addition of mavenLocal() in buildSrc/build.gradle, this forms a classic dependency confusion / supply chain attack setup. SNAPSHOT versions are not fixed artifacts—if an attacker can place a malicious opensearchprotobufs-1.4.0-SNAPSHOT.jar in the build machine's local Maven repository (~/.m2), it will be resolved preferentially due to mavenLocal() being listed first. The protobuf library controls serialization/deserialization of all gRPC traffic, making a tampered version a high-impact injection point.
buildSrc/build.gradle	97	medium	mavenLocal() is added as the first entry in the buildSrc repositories block, causing Gradle to resolve dependencies from the local Maven cache (~/.m2) before checking mavenCentral or gradlePluginPortal. This creates a supply chain attack surface: any dependency can be shadowed by a locally planted artifact. While sometimes used legitimately during development, adding this to a shared build configuration in a PR to a production open-source project is anomalous and significantly increases the risk of dependency substitution attacks on CI/CD build machines.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 1 | Medium: 1 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-03-14T21:42:18Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit c9295da.

Path	Line	Severity	Description
buildSrc/build.gradle	97	medium	mavenLocal() is added as the first repository ahead of mavenCentral(). This means the local Maven cache (~/.m2) is checked before any remote repository, making the build vulnerable to local dependency confusion: anyone with write access to the build environment's home directory can inject a malicious JAR that shadows a legitimate dependency. This is especially risky in CI/CD pipelines where the home directory may be shared or writable by multiple jobs.
gradle/libs.versions.toml	27	medium	The opensearchprotobufs dependency version is changed from the stable release '1.2.0' to the mutable SNAPSHOT '1.4.0-SNAPSHOT'. SNAPSHOT artifacts are not immutable — their content can change without a version bump. Combined with the simultaneous addition of mavenLocal() in buildSrc/build.gradle, this creates a concrete supply chain attack path: an attacker who can write to ~/.m2/repository on the build machine can place an arbitrary opensearch-protobufs-1.4.0-SNAPSHOT.jar that would be resolved first, injecting malicious protobuf conversion logic into request/response handling for the gRPC transport layer.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 2 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

github-actions · 2026-03-14T22:02:33Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 4251a15.

Path	Line	Severity	Description
buildSrc/build.gradle	97	high	mavenLocal() is inserted as the first repository in buildSrc, which controls the Gradle build toolchain itself. Combined with the simultaneous change to a SNAPSHOT dependency, this creates a supply chain attack vector: any actor who can place a crafted opensearch-protobufs-1.4.0-SNAPSHOT JAR in the local Maven cache on build/CI machines will have that artifact silently used instead of a published release. buildSrc code runs with full build-system privileges and is an especially sensitive injection point.
gradle/libs.versions.toml	27	medium	The opensearchprotobufs dependency version was changed from the stable release 1.2.0 to 1.4.0-SNAPSHOT. SNAPSHOT versions are mutable and non-reproducible; their content can change between resolutions without any version-string change. When paired with the mavenLocal() addition in buildSrc, this means a locally-planted artifact of the same coordinate would be preferred over any remote copy, enabling silent substitution of a dependency that is deeply embedded in gRPC request/response serialization paths.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 1 | Medium: 1 | Low: 0

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

This commit adds complete gRPC support for Min and Max metric aggregations, including request parsing, response conversion, and integration with the search pipeline. **Request Conversion (OpenSearch ← gRPC):** - AggregationContainerProtoUtils: Central dispatcher routing aggregation types to specific converters, validates aggregation names - ValuesSourceAggregationProtoUtils: Shared utilities for parsing common ValuesSource fields (field, missing, value_type, format, script) - MinAggregationProtoUtils: Converts proto MinAggregation → MinAggregationBuilder - MaxAggregationProtoUtils: Converts proto MaxAggregation → MaxAggregationBuilder - Updated SearchSourceBuilderProtoUtils to parse aggregations map from proto SearchRequestBody and add to SearchSourceBuilder **Response Conversion (gRPC ← OpenSearch):** - SearchResponseSectionsProtoUtils: **CRITICAL FIX** - Added aggregation response conversion to search response pipeline, enabling aggregation results to be returned via gRPC. This connects the Min/Max aggregate converters to the search response builder. - AggregateProtoUtils: Central dispatcher for converting InternalAggregation to proto Aggregate, with metadata and sub-aggregation helpers - MinAggregateProtoUtils: Converts InternalMin → proto MinAggregate - MaxAggregateProtoUtils: Converts InternalMax → proto MaxAggregate - Handles special values (infinity, NaN), formatting, and metadata **OpenSearch Core Changes:** - InternalNumericMetricsAggregation: Added getFormat() getter to expose format information for gRPC converters **Request Conversion Tests:** - AggregationContainerProtoUtilsTests: 11 tests - MinAggregationProtoUtilsTests: 108 tests - MaxAggregationProtoUtilsTests: 108 tests - ValuesSourceAggregationProtoUtilsTests: 40+ tests - SearchSourceBuilderProtoUtilsTests: 2 new aggregation tests **Response Conversion Tests:** - AggregateProtoUtilsTests: 10 tests - MinAggregateProtoUtilsTests: 190 tests (values, infinity, NaN, formatting) - MaxAggregateProtoUtilsTests: 190 tests (values, infinity, NaN, formatting) - SearchResponseSectionsProtoUtilsTests: 2 new aggregation tests (null/present) - InternalMinTests: 32 tests - InternalMaxTests: 32 tests **Total: ~680 test cases** **Design Principles:** - Mirrors REST API patterns for consistency - Maintains behavioral parity with REST layer - Comprehensive error handling and validation - Special value support (infinity, NaN) matching REST behavior - Metadata handling consistent across all aggregations **Implementation Summary:** - **New Implementation Files**: 11 (converters + infrastructure) - **New Test Files**: 8 (comprehensive test coverage) - **Modified Files**: 4 (SearchSourceBuilder + SearchResponseSections + server changes) - **Package Documentation**: 5 package-info.java files - **Total Lines**: ~1,850 (implementation + tests) **Testing & Validation:** - Comprehensive integration testing via REST and gRPC - Verified behavioral parity (see min_max_aggregation_comparison_report.md) - All core functionality confirmed working correctly - Values match exactly between REST and gRPC responses **API Differences (By Design):** - REST uses simple JSON values; gRPC uses typed protobuf structures - Parameter formats differ (e.g., missing parameter requires FieldValue in gRPC) - Field naming follows protobuf conventions (camelCase vs snake_case) Co-Authored-By: Claude (claude-sonnet-4-5) <noreply@anthropic.com> Signed-off-by: Yiyu Pan <yypan14@gmail.com>

This commit implements a comprehensive converter pattern for gRPC aggregations that mirrors the REST API structure, enabling min/max aggregations support with proper request parsing and response serialization. Request-side (proto → OpenSearch): - Add AggregationBuilderProtoConverter SPI for extensible converter registration - Add AggregationBuilderProtoConverterRegistry for converter lookup and dispatch - Add ObjectParserProtoUtils to mirror REST's ObjectParser pattern - Implement MinAggregationBuilderProtoConverter and MaxAggregationBuilderProtoConverter - Add ValuesSourceProtoFields wrapper to reduce parameter verbosity - Support metadata and field validation matching REST behavior Response-side (OpenSearch → proto): - Add AggregationsProtoUtils to mirror Aggregations.java (iterate collection) - Update AggregateProtoUtils to mirror InternalAggregation.java (metadata + dispatch) - Update MinAggregateProtoUtils and MaxAggregateProtoUtils to mirror doXContentBody() - Complete REST-to-gRPC pattern mapping for aggregation results The implementation follows OpenSearch's REST pattern exactly: - Request: AggregationContainer → ObjectParser → AggregationBuilder - Response: InternalAggregation → Aggregations → Aggregate proto Tests verify behavioral parity with REST API including edge cases, null handling, metadata support, and proper field validation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Yiyu Pan <yypan14@gmail.com>

Remove the sub-packages list from aggregation package-info.java as it was already outdated (missing the support package) and violates separation of concerns - outer packages should not document implementation details of inner packages. This avoids maintenance burden when sub-packages are added or modified. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Yiyu Pan <yypan14@gmail.com>

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> Signed-off-by: Yiyu Pan <yypan14@gmail.com>

The testToProtoWithAggregations test had incorrect expected values: - minAgg mock returns -50.0 but assertion expected 10.0 - maxAgg mock returns 9999.0 but assertion expected 100.0 Updated assertions to match the actual mock values. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Yiyu Pan <yypan14@gmail.com>

This commit implements an extensible registry pattern for both response-side and request-side gRPC converters, improving consistency and maintainability. Response-side changes: - Add AggregateProtoConverter and AggregateProtoConverterRegistry SPI interfaces - Implement AggregateProtoConverterSpiRegistry with class hierarchy lookup - Create AggregateProtoConverterRegistryImpl as public facade - Add MinAggregateProtoConverter and MaxAggregateProtoConverter implementations - Update AggregateProtoUtils to delegate to registry instead of instanceof checks - Add comprehensive test suite (AggregateProtoConverterRegistryTests) Request-side changes: - Inline ObjectParserProtoUtils logic into ValuesSourceAggregationProtoUtils - Remove unnecessary ObjectParserProtoUtils abstraction layer - Simplify field parsing by directly calling builder methods Benefits: - Extensible: New aggregation types can be added without modifying central dispatcher - Consistent: Response-side now mirrors request-side registry pattern - Type-safe: Class-based dispatch with support for subclasses and mocks - Simplified: Reduced complexity by removing one abstraction layer All 1,280+ transport-grpc tests passing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Yiyu Pan <yypan14@gmail.com>

- Add testMinAggregation() and testMaxAggregation() to SearchServiceIT - Verify min/max aggregations work end-to-end with live cluster via gRPC - Add missing javadocs to AggregationBuilderProtoConverter* classes - Add missing javadocs to AggregateProtoConverter* classes Integration tests validate: - Min aggregation returns correct minimum value (5.2 from [10.5, 25.0, 5.2]) - Max aggregation returns correct maximum value (25.0 from [10.5, 25.0, 5.2]) - gRPC aggregation results match expected values All integration tests passing (8 tests total). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Yiyu Pan <yypan14@gmail.com>

github-actions · 2026-03-14T22:25:26Z

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 32770fc.

Path	Line	Severity	Description
buildSrc/build.gradle	97	medium	mavenLocal() added as the first repository before mavenCentral(). This prioritizes artifacts from the local Maven cache (~/.m2) over trusted central repositories, creating a dependency confusion attack vector. Any attacker who can write to the local Maven repository on the build machine can inject malicious artifacts that would shadow legitimate dependencies. This is a well-documented supply chain attack technique.
gradle/libs.versions.toml	27	low	The opensearchprotobufs version was changed from the pinned stable release '1.2.0' to '1.4.0-SNAPSHOT'. SNAPSHOT versions are mutable and unpinned — the resolved artifact can change between builds without any version bump. When combined with the mavenLocal() addition in buildSrc/build.gradle, a locally-cached SNAPSHOT artifact could silently replace the expected dependency. In isolation this is common in development, but the pairing raises the risk profile.

The table above displays the top 10 most important findings.

Total: 2 | Critical: 0 | High: 0 | Medium: 1 | Low: 1

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

yiyuabc requested review from a team, Bukhtawar, CEHENKLE, Rishikesh1159, anasalkouz, andrross, ashking94, cwperks, dbwiddis, gbbafna, jed326, kotwanikunal, mch2, msfroh, owaiskazi19, reta, sachinpkale, saratvemulapalli, shwetathareja and sohami as code owners March 7, 2026 06:09

yiyuabc mentioned this pull request Mar 7, 2026

Add protobuf definitions for min/max aggregation opensearch-project/opensearch-protobufs#410

Open

yiyuabc force-pushed the feature/min-max-aggregations branch from f6698d6 to 8e58ad1 Compare March 7, 2026 06:31

yiyuabc force-pushed the feature/min-max-aggregations branch from 8e58ad1 to e0ade3a Compare March 7, 2026 18:41

yiyuabc force-pushed the feature/min-max-aggregations branch from 0ccdcde to b90d2c6 Compare March 12, 2026 02:06

yiyuabc force-pushed the feature/min-max-aggregations branch from b6b95c3 to c31f59c Compare March 13, 2026 22:00

yiyuabc force-pushed the feature/min-max-aggregations branch from 7e48b84 to 73b303c Compare March 13, 2026 22:13

yiyuabc force-pushed the feature/min-max-aggregations branch from 73b303c to 3a4d4b7 Compare March 13, 2026 22:17

yiyuabc force-pushed the feature/min-max-aggregations branch from e52cd6c to 29bed9d Compare March 13, 2026 23:47

yiyuabc force-pushed the feature/min-max-aggregations branch from 7252dcb to c174727 Compare March 14, 2026 05:59

yiyuabc force-pushed the feature/min-max-aggregations branch from c174727 to c9295da Compare March 14, 2026 21:41

yiyuabc and others added 7 commits March 14, 2026 22:24

yiyuabc force-pushed the feature/min-max-aggregations branch from 4251a15 to 32770fc Compare March 14, 2026 22:24

Conversation

yiyuabc commented Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Request-Side Converters (Proto → Java)

Core Infrastructure:

Min/Max Converters:

SearchSourceBuilder Integration:

Response-Side Converters (Java → Proto)

Core Infrastructure:

Min/Max Converters:

Server-Side Changes

Test Coverage

Request-Side Tests (~260 tests):

Response-Side Tests (~410 tests):

Design Principles

Description

Min/Max Aggregation REST vs gRPC Comparison Report

REST API Setup and Examples

Step 1: Create Index

Step 2: Index Sample Documents

Quick Test Commands

Scenario 3: Multiple Aggregations

Scenario 4: Invalid Aggregation Name (Validation)

Scenario 5: Value Formatting

Scenario 6: Script-Based Aggregation

Scenario 7: Missing Value Handling

Scenario 8: Combined Format and Missing

Related Issues

Check List

Uh oh!

github-actions bot commented Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 398d0a8)

Uh oh!

github-actions bot commented Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions bot commented Mar 7, 2026

Uh oh!

github-actions bot commented Mar 7, 2026

Uh oh!

github-actions bot commented Mar 7, 2026

Uh oh!

github-actions bot commented Mar 7, 2026

Uh oh!

github-actions bot commented Mar 7, 2026

Uh oh!

github-actions bot commented Mar 12, 2026

PR Code Analyzer ❗

Uh oh!

github-actions bot commented Mar 13, 2026

PR Code Analyzer ❗

Uh oh!

github-actions bot commented Mar 13, 2026

PR Code Analyzer ❗

Uh oh!

github-actions bot commented Mar 13, 2026

PR Code Analyzer ❗

Uh oh!

github-actions bot commented Mar 13, 2026

PR Code Analyzer ❗

Uh oh!

github-actions bot commented Mar 13, 2026

PR Code Analyzer ❗

Uh oh!

github-actions bot commented Mar 13, 2026

PR Code Analyzer ❗

Uh oh!

github-actions bot commented Mar 13, 2026

PR Code Analyzer ❗

Uh oh!

github-actions bot commented Mar 13, 2026

PR Code Analyzer ❗

Uh oh!

github-actions bot commented Mar 14, 2026

PR Code Analyzer ❗

Uh oh!

yiyuabc commented Mar 7, 2026 •

edited

Loading

github-actions bot commented Mar 7, 2026 •

edited

Loading

(Review updated until commit `398d0a8`)

github-actions bot commented Mar 7, 2026 •

edited

Loading