Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughValidator API now requires protobuf Messages; adds PGV-based validator and a SPI-registered StrictXdsValidatorIndex that runs PGV then supported-field checks; replaces per-field boolean supported options with numeric message-level options and annotates many Envoy protos with the new armeria.xds.supported options. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant StrictIndex as StrictXdsValidatorIndex
participant PGV as PgvValidator
participant Supported as SupportedFieldValidator
participant Msg as "protobuf Message"
Client->>StrictIndex: assertValid(Msg)
StrictIndex->>PGV: assertValid(Msg)
PGV->>PGV: run PGV structural validation
alt PGV fails
PGV-->>StrictIndex: throws XdsValidationException
StrictIndex-->>Client: propagate XdsValidationException
else PGV passes
PGV-->>StrictIndex: success
StrictIndex->>Supported: validate(Msg, collector)
Supported->>Supported: read supported.field/oneof/enum lists
Supported->>Supported: collect unsupported items
alt violations found
Supported-->>StrictIndex: violations list
StrictIndex-->>Client: throw XdsValidationException(aggregated)
else no violations
StrictIndex-->>Client: validation success
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
xds-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.java (1)
150-171:⚠️ Potential issue | 🟠 MajorUnannotated envoy/xds enums will be treated as entirely unsupported.
unsupportedEnumValue()returnstruewhenever the enum'ssupported.enum_valuelist does not contain the value's number. For any envoy/xds enum type that has not been annotated at all (empty list), every value — including the default 0 — will be flagged as unsupported, andvalidateFieldValuewill invoke the handler for it.Example:
DnsLookupFamilyinenvoy/config/cluster/v3/cluster.protohas no(armeria.xds.supported.enum_value)annotations on any of its values, yet it is used in the supported fielddns_lookup_family = 17. This will cause spurious warnings (or failures underStrictXdsValidatorIndex) for all enum values.Consider treating "no
supported.enum_valueannotations on the enum" as "no opinion" (skip enum value validation) and only enforce when at least one value is annotated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.java` around lines 150 - 171, validateFieldValue is flagging every enum value as unsupported when the enum has no supported.enum_value annotations because unsupportedEnumValue returns true if the list does not contain the value; change unsupportedEnumValue(EnumValueDescriptor ev) to treat an empty supported-values list as "no opinion" by returning false when the retrieved List<Integer> supportedValues is null or empty, and only perform contains-check when the list has entries (use ev.getType().getOptions().getExtension(SupportedFieldProto.supported.enumValue) and check for null/empty before contains); this will ensure validateFieldValue only invokes handler for enums where at least one enum_value is explicitly annotated.xds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/common.proto (1)
216-223:⚠️ Potential issue | 🟠 MajorMove
(armeria.xds.supported.field)options to the message level.The
(armeria.xds.supported.field)options are message-level options but are currently placed between field documentation comments and field declarations. Protobuf tooling associates leading comments with the next declaration, so these comments no longer document the fields when options sit between them. Relocate all supported-field options to the top of each message, immediately after other message options.Example refactor pattern
message TlsCertificate { option (udpa.annotations.versioning).previous_message_type = "envoy.api.v2.auth.TlsCertificate"; + option (armeria.xds.supported.field) = 1; + option (armeria.xds.supported.field) = 2; + option (armeria.xds.supported.field) = 7; + option (armeria.xds.supported.field) = 3; // The TLS certificate chain. // // If ``certificate_chain`` is a filesystem path, a watch will be added to the // parent directory for any file moves to support rotation. This currently // only applies to dynamic secrets, when the ``TlsCertificate`` is delivered via // SDS. - option (armeria.xds.supported.field) = 1; config.core.v3.DataSource certificate_chain = 1;Apply to all 13 occurrences across
TlsCertificate,SubjectAltNameMatcher, andCertificateValidationContextmessages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/common.proto` around lines 216 - 223, The (armeria.xds.supported.field) option is placed between field comments and field declarations (e.g., before the certificate_chain field), which breaks the association of leading comments with the fields; move every occurrence of the (armeria.xds.supported.field) option into the corresponding message header (immediately after other message-level options) for the TlsCertificate, SubjectAltNameMatcher, and CertificateValidationContext messages (all 13 occurrences) so the field documentation remains attached to the fields.
🧹 Nitpick comments (3)
xds-api/src/main/java/com/linecorp/armeria/xds/api/PgvValidator.java (2)
49-55: Consider tightening parameter toMessagefor consistency.The rest of this PR narrows validator APIs from
Objecttocom.google.protobuf.Message.PgvValidator.assertValidstill takesObject; since the only sensible input is a protobuf message (PGV'svalidatorForwill otherwise return a no-op/unsupported validator), tightening toMessagewould align withXdsValidatorIndex.assertValid(Message)and prevent accidental misuse.Proposed change
- public void assertValid(Object message) { + public void assertValid(com.google.protobuf.Message message) { try { delegate.validatorFor(message).assertValid(message); } catch (ValidationException e) { throw new IllegalArgumentException(e); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds-api/src/main/java/com/linecorp/armeria/xds/api/PgvValidator.java` around lines 49 - 55, Change PgvValidator.assertValid to accept com.google.protobuf.Message instead of Object: update the method signature of PgvValidator.assertValid(Message message), adjust the call to delegate.validatorFor(message) and subsequent validator invocation to use the Message type, and keep the existing try/catch that converts ValidationException into IllegalArgumentException; this aligns the API with XdsValidatorIndex.assertValid(Message) and prevents passing non-protobuf objects to delegate.validatorFor.
49-55: Preserve the original message in the thrown exception.
new IllegalArgumentException(e)uses only the cause; the resulting top-level message isio.envoyproxy.pgv.ValidationException: ..., which is noisier than needed and loses context about which xDS resource failed. Consider including pgv's message and (optionally) the descriptor name so the error is actionable in logs.Proposed change
- } catch (ValidationException e) { - throw new IllegalArgumentException(e); - } + } catch (ValidationException e) { + throw new IllegalArgumentException( + "PGV validation failed for " + message.getClass().getName() + ": " + e.getMessage(), e); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds-api/src/main/java/com/linecorp/armeria/xds/api/PgvValidator.java` around lines 49 - 55, The catch in PgvValidator.assertValid currently rethrows ValidationException as new IllegalArgumentException(e) which loses a clear top-level message; update the catch to build a descriptive message that includes pgv's error text and the protobuf descriptor name for the failing resource, e.g. retrieve the descriptor via ((com.google.protobuf.Message) message).getDescriptorForType().getFullName() when message is a protobuf and format a message like "Invalid xDS resource <DescriptorName>: <pgv message>" then throw new IllegalArgumentException(formattedMessage, e) so the original cause is preserved.xds-api/src/main/proto/envoy/config/core/v3/base.proto (1)
407-434: Nit: inconsistent placement of supported-field option relative to doc comment.For
HeaderValue.key, theoption (armeria.xds.supported.field) = 1;at line 410 is placed before the// Header name.comment, whereas forvaluethe option at line 423 is placed after the doc comment. Placement has no semantic effect on protobuf, but keeping it consistently immediately above the field (after its doc comment) improves readability and matches the convention used elsewhere in this PR.Proposed tweak
- option (armeria.xds.supported.field) = 1; - // Header name. - string key = 1 + // Header name. + option (armeria.xds.supported.field) = 1; + string key = 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds-api/src/main/proto/envoy/config/core/v3/base.proto` around lines 407 - 434, Move the option placement for HeaderValue.key so it sits immediately above the field declaration and immediately below the `// Header name.` doc comment (i.e., place `option (armeria.xds.supported.field) = 1;` after the comment and just before `string key = 1 ...`) to match the placement used for `value` and `raw_value` and keep `HeaderValue`'s field-option ordering consistent across the message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@xds-api/src/main/java/com/linecorp/armeria/xds/api/DefaultXdsValidatorIndex.java`:
- Around line 44-45: Ensure DefaultXdsValidatorIndex.assertValid performs an
explicit null-check on the incoming Message before delegating to
pgvValidator.assertValid: use Objects.requireNonNull(message, "message") at the
start of assertValid in DefaultXdsValidatorIndex so the public API enforces the
null contract rather than relying on PgvValidator/SupportedFieldValidator to
catch it; keep the subsequent call to pgvValidator.assertValid(message).
In
`@xds-api/src/main/java/com/linecorp/armeria/xds/api/StrictXdsValidatorIndex.java`:
- Around line 43-44: Add an explicit null-check at the public API boundary of
StrictXdsValidatorIndex.assertValid by calling Objects.requireNonNull(message,
...) before delegating to pgvValidator.assertValid; this ensures the method
enforces its contract and prevents null from being handled later by
PgvValidator/SupportedFieldValidator implementation details.
In
`@xds-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.java`:
- Around line 39-51: The Javadoc incorrectly names and shows the option; update
the class-level Javadoc in SupportedFieldValidator to reference the correct
proto option `(armeria.xds.supported.field)` and adjust the example to show the
option used inside the message-body via the supported wrapper message as defined
in SupportedFieldProto.supported.field (i.e., demonstrate a message with option
(armeria.xds.supported.field) = { field: 1 };), and remove the mistaken
`(armeria.xds.supported_fields)` and the combined field/option example so the
docs match the actual SupportedFieldProto.supported.field extension and the
validator's behavior.
In `@xds-validator/build.gradle`:
- Line 2: The build uses implementation for protobuf but
XdsValidatorIndex#assertValid(Message) exposes com.google.protobuf.Message to
consumers; change the dependency for protobuf (currently referenced as
libs.protobuf.java in build.gradle) from implementation to api so downstream
projects compiling against the XdsValidatorIndex interface will have the
protobuf types on their compile classpath.
---
Outside diff comments:
In
`@xds-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.java`:
- Around line 150-171: validateFieldValue is flagging every enum value as
unsupported when the enum has no supported.enum_value annotations because
unsupportedEnumValue returns true if the list does not contain the value; change
unsupportedEnumValue(EnumValueDescriptor ev) to treat an empty supported-values
list as "no opinion" by returning false when the retrieved List<Integer>
supportedValues is null or empty, and only perform contains-check when the list
has entries (use
ev.getType().getOptions().getExtension(SupportedFieldProto.supported.enumValue)
and check for null/empty before contains); this will ensure validateFieldValue
only invokes handler for enums where at least one enum_value is explicitly
annotated.
In
`@xds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/common.proto`:
- Around line 216-223: The (armeria.xds.supported.field) option is placed
between field comments and field declarations (e.g., before the
certificate_chain field), which breaks the association of leading comments with
the fields; move every occurrence of the (armeria.xds.supported.field) option
into the corresponding message header (immediately after other message-level
options) for the TlsCertificate, SubjectAltNameMatcher, and
CertificateValidationContext messages (all 13 occurrences) so the field
documentation remains attached to the fields.
---
Nitpick comments:
In `@xds-api/src/main/java/com/linecorp/armeria/xds/api/PgvValidator.java`:
- Around line 49-55: Change PgvValidator.assertValid to accept
com.google.protobuf.Message instead of Object: update the method signature of
PgvValidator.assertValid(Message message), adjust the call to
delegate.validatorFor(message) and subsequent validator invocation to use the
Message type, and keep the existing try/catch that converts ValidationException
into IllegalArgumentException; this aligns the API with
XdsValidatorIndex.assertValid(Message) and prevents passing non-protobuf objects
to delegate.validatorFor.
- Around line 49-55: The catch in PgvValidator.assertValid currently rethrows
ValidationException as new IllegalArgumentException(e) which loses a clear
top-level message; update the catch to build a descriptive message that includes
pgv's error text and the protobuf descriptor name for the failing resource, e.g.
retrieve the descriptor via ((com.google.protobuf.Message)
message).getDescriptorForType().getFullName() when message is a protobuf and
format a message like "Invalid xDS resource <DescriptorName>: <pgv message>"
then throw new IllegalArgumentException(formattedMessage, e) so the original
cause is preserved.
In `@xds-api/src/main/proto/envoy/config/core/v3/base.proto`:
- Around line 407-434: Move the option placement for HeaderValue.key so it sits
immediately above the field declaration and immediately below the `// Header
name.` doc comment (i.e., place `option (armeria.xds.supported.field) = 1;`
after the comment and just before `string key = 1 ...`) to match the placement
used for `value` and `raw_value` and keep `HeaderValue`'s field-option ordering
consistent across the message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7d6b55ea-c25b-42b8-8684-d29110d2fa7f
📒 Files selected for processing (32)
it/xds-client/src/test/resources/META-INF/services/com.linecorp.armeria.xds.validator.XdsValidatorIndexit/xds-no-validation/src/test/java/com/linecorp/armeria/xds/it/NoopXdsValidatorIndex.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/DefaultXdsValidatorIndex.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/PgvValidator.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/StrictXdsValidatorIndex.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.javaxds-api/src/main/proto/armeria/xds/supported.protoxds-api/src/main/proto/envoy/config/bootstrap/v3/bootstrap.protoxds-api/src/main/proto/envoy/config/cluster/v3/cluster.protoxds-api/src/main/proto/envoy/config/core/v3/address.protoxds-api/src/main/proto/envoy/config/core/v3/base.protoxds-api/src/main/proto/envoy/config/core/v3/config_source.protoxds-api/src/main/proto/envoy/config/core/v3/grpc_service.protoxds-api/src/main/proto/envoy/config/core/v3/health_check.protoxds-api/src/main/proto/envoy/config/endpoint/v3/endpoint.protoxds-api/src/main/proto/envoy/config/endpoint/v3/endpoint_components.protoxds-api/src/main/proto/envoy/config/listener/v3/api_listener.protoxds-api/src/main/proto/envoy/config/listener/v3/listener.protoxds-api/src/main/proto/envoy/config/route/v3/route.protoxds-api/src/main/proto/envoy/config/route/v3/route_components.protoxds-api/src/main/proto/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.protoxds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/common.protoxds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/secret.protoxds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/tls.protoxds-api/src/main/proto/envoy/type/matcher/v3/regex.protoxds-api/src/main/proto/envoy/type/matcher/v3/string.protoxds-api/src/main/proto/envoy/type/v3/percent.protoxds-api/src/main/proto/envoy/type/v3/range.protoxds-api/src/test/proto/armeria/xds/testing/test_supported.protoxds-validator/build.gradlexds-validator/src/main/java/com/linecorp/armeria/xds/validator/XdsValidatorIndex.javaxds/src/main/java/com/linecorp/armeria/xds/XdsValidatorIndexRegistry.java
Contributor
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
xds-api/src/main/java/com/linecorp/armeria/xds/api/StrictXdsValidatorIndex.java (2)
44-57: Consider documenting the pgv-first short-circuit behavior.When
pgvValidator.assertValid(message)fails, any unsupported-field violations present in the same message are never collected or surfaced. For a strict/test validator whose value proposition is "report everything at once," it would be worth a short Javadoc note clarifying that structural PGV errors take precedence and short-circuit supported-field reporting. No functional change required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds-api/src/main/java/com/linecorp/armeria/xds/api/StrictXdsValidatorIndex.java` around lines 44 - 57, Add Javadoc to the StrictXdsValidatorIndex.assertValid method explaining that pgvValidator.assertValid(message) runs first and short-circuits further supported-field validation: if PGV structural validation fails, supported-field violations will not be collected or reported; mention this is intentional for prioritizing structural errors and that no functional changes are made. Reference the pgvValidator field and the SupportedFieldValidator-based check in the doc comment so readers know which checks run and in what order.
48-56: Minor: consider including the offending value / a stable ordering in the aggregated message.The violations list uses
descriptorName + ": " + fieldPath, dropping thevaluepassed to the handler. For a test-mode validator, including a truncatedvalue(or at least the type/oneof case) in the error message would make failures easier to diagnose when the same field path appears in multiple sub-messages. As-is, duplicates collapse visually into identical entries. Optional.Illustrative tweak
- SupportedFieldValidator.of((descriptorName, fieldPath, value) -> - violations.add(descriptorName + ": " + fieldPath)); + SupportedFieldValidator.of((descriptorName, fieldPath, value) -> + violations.add(descriptorName + ": " + fieldPath + + (value != null ? " (=" + value + ')' : "")));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds-api/src/main/java/com/linecorp/armeria/xds/api/StrictXdsValidatorIndex.java` around lines 48 - 56, The aggregated error message in StrictXdsValidatorIndex collects only descriptorName and fieldPath into the violations list, making duplicate entries hard to distinguish; update the SupportedFieldValidator handler (the lambda passed to SupportedFieldValidator.of) to include a concise representation of the offending value (e.g., value.getClass().getSimpleName() or a truncated toString/first N chars) when adding to violations, and after validator.validate(message) sort and deduplicate the violations list to produce a stable, readable order before throwing the IllegalArgumentException that currently contains "Unsupported xDS fields detected: " + String.join(", ", violations); reference SupportedFieldValidator, the violations variable, validator.validate(message), and the IllegalArgumentException in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@xds-api/src/main/java/com/linecorp/armeria/xds/api/StrictXdsValidatorIndex.java`:
- Around line 44-57: Add Javadoc to the StrictXdsValidatorIndex.assertValid
method explaining that pgvValidator.assertValid(message) runs first and
short-circuits further supported-field validation: if PGV structural validation
fails, supported-field violations will not be collected or reported; mention
this is intentional for prioritizing structural errors and that no functional
changes are made. Reference the pgvValidator field and the
SupportedFieldValidator-based check in the doc comment so readers know which
checks run and in what order.
- Around line 48-56: The aggregated error message in StrictXdsValidatorIndex
collects only descriptorName and fieldPath into the violations list, making
duplicate entries hard to distinguish; update the SupportedFieldValidator
handler (the lambda passed to SupportedFieldValidator.of) to include a concise
representation of the offending value (e.g., value.getClass().getSimpleName() or
a truncated toString/first N chars) when adding to violations, and after
validator.validate(message) sort and deduplicate the violations list to produce
a stable, readable order before throwing the IllegalArgumentException that
currently contains "Unsupported xDS fields detected: " + String.join(", ",
violations); reference SupportedFieldValidator, the violations variable,
validator.validate(message), and the IllegalArgumentException in your change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ebbdc4d7-046d-490a-bcae-a08fb4b6eb1e
📒 Files selected for processing (17)
xds-api/src/main/java/com/linecorp/armeria/xds/api/DefaultXdsValidatorIndex.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/StrictXdsValidatorIndex.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.javaxds-api/src/main/proto/armeria/xds/supported.protoxds-api/src/main/proto/envoy/config/cluster/v3/cluster.protoxds-api/src/main/proto/envoy/config/core/v3/address.protoxds-api/src/main/proto/envoy/config/core/v3/base.protoxds-api/src/main/proto/envoy/config/core/v3/config_source.protoxds-api/src/main/proto/envoy/config/core/v3/grpc_service.protoxds-api/src/main/proto/envoy/config/core/v3/health_check.protoxds-api/src/main/proto/envoy/config/endpoint/v3/endpoint_components.protoxds-api/src/main/proto/envoy/config/route/v3/route_components.protoxds-api/src/main/proto/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.protoxds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/secret.protoxds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/tls.protoxds-api/src/main/proto/envoy/type/matcher/v3/string.protoxds-validator/build.gradle
✅ Files skipped from review due to trivial changes (7)
- xds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/secret.proto
- xds-api/src/main/proto/envoy/config/core/v3/address.proto
- xds-api/src/main/proto/envoy/extensions/filters/network/http_connection_manager/v3/http_connection_manager.proto
- xds-api/src/main/proto/envoy/type/matcher/v3/string.proto
- xds-api/src/main/proto/envoy/config/core/v3/config_source.proto
- xds-api/src/main/proto/envoy/config/endpoint/v3/endpoint_components.proto
- xds-api/src/main/proto/envoy/config/route/v3/route_components.proto
🚧 Files skipped from review as they are similar to previous changes (6)
- xds-validator/build.gradle
- xds-api/src/main/java/com/linecorp/armeria/xds/api/SupportedFieldValidator.java
- xds-api/src/main/proto/armeria/xds/supported.proto
- xds-api/src/main/proto/envoy/config/core/v3/base.proto
- xds-api/src/main/proto/envoy/extensions/transport_sockets/tls/v3/tls.proto
- xds-api/src/main/proto/envoy/config/core/v3/health_check.proto
ikhoon
approved these changes
Apr 27, 2026
| /** | ||
| * A strict {@link XdsValidatorIndex} that composes pgv (protoc-gen-validate) structural validation | ||
| * with supported-field validation. All unsupported field violations are collected and reported | ||
| * together in a single {@link IllegalArgumentException}. |
Contributor
There was a problem hiding this comment.
Optional) What do you think of introducing a dedicated exception type to identify the validation error more easily.
Contributor
Author
There was a problem hiding this comment.
Note that exceptions don't really impact actually request flow, so I don't really expect users to react based on the exceptions. Updated anyways.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@xds-validator/src/main/java/com/linecorp/armeria/xds/validator/XdsValidationException.java`:
- Around line 34-53: Add explicit null checks to the public factory methods to
make NPEs deterministic: in XdsValidationException.of(Message, Throwable) call
Objects.requireNonNull(resource, "resource") and Objects.requireNonNull(cause,
"cause") before using them and use their values to build the message; in
XdsValidationException.of(Message, String) call Objects.requireNonNull(resource,
"resource") and Objects.requireNonNull(message, "message"); and in
XdsValidationException.of(String) call Objects.requireNonNull(message,
"message"). Ensure the exception messages use the parameter name strings exactly
as shown so the errors are deterministic and user-facing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 609121c9-2b4c-4a54-a18a-920ae77787c4
📒 Files selected for processing (5)
xds-api/src/main/java/com/linecorp/armeria/xds/api/PgvValidator.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/StrictXdsValidatorIndex.javaxds-api/src/main/java/com/linecorp/armeria/xds/api/UnsupportedFieldHandler.javaxds-validator/src/main/java/com/linecorp/armeria/xds/validator/XdsValidationException.javaxds/src/test/java/com/linecorp/armeria/xds/XdsValidatorIndexRegistryTest.java
✅ Files skipped from review due to trivial changes (1)
- xds/src/test/java/com/linecorp/armeria/xds/XdsValidatorIndexRegistryTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- xds-api/src/main/java/com/linecorp/armeria/xds/api/PgvValidator.java
- xds-api/src/main/java/com/linecorp/armeria/xds/api/StrictXdsValidatorIndex.java
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
xds-validator/src/main/java/com/linecorp/armeria/xds/validator/XdsValidationException.java (1)
36-40:⚠️ Potential issue | 🟡 MinorMake
of(Message, Throwable)robust when cause message is null.If
cause.getMessage()isnull, the resulting text becomes<type>: null, which is not very actionable. Fall back to the cause class name.Suggested patch
public static XdsValidationException of(Message resource, Throwable cause) { requireNonNull(resource, "resource"); requireNonNull(cause, "cause"); + final String causeMessage = cause.getMessage(); return new XdsValidationException( - resource.getDescriptorForType().getFullName() + ": " + cause.getMessage(), cause); + resource.getDescriptorForType().getFullName() + ": " + + (causeMessage != null ? causeMessage : cause.getClass().getName()), cause); }As per coding guidelines, "Exceptions: when throwing, use meaningful messages that include the offending value/expectation; use validation-style messages like
"value: %s (expected: ...)"."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@xds-validator/src/main/java/com/linecorp/armeria/xds/validator/XdsValidationException.java` around lines 36 - 40, The factory method XdsValidationException.of(Message, Throwable) currently concatenates cause.getMessage() which can be null; update this method so it uses a non-null message by falling back to the cause class name when cause.getMessage() is null (e.g., use cause.getMessage() != null ? cause.getMessage() : cause.getClass().getName()), then build the exception message as resource.getDescriptorForType().getFullName() + ": " + resolvedCauseMessage and pass the original cause through to the XdsValidationException constructor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@xds-validator/src/main/java/com/linecorp/armeria/xds/validator/XdsValidationException.java`:
- Around line 36-40: The factory method XdsValidationException.of(Message,
Throwable) currently concatenates cause.getMessage() which can be null; update
this method so it uses a non-null message by falling back to the cause class
name when cause.getMessage() is null (e.g., use cause.getMessage() != null ?
cause.getMessage() : cause.getClass().getName()), then build the exception
message as resource.getDescriptorForType().getFullName() + ": " +
resolvedCauseMessage and pass the original cause through to the
XdsValidationException constructor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68d99b7a-f209-47e1-b881-61bbcd4dfecb
📒 Files selected for processing (1)
xds-validator/src/main/java/com/linecorp/armeria/xds/validator/XdsValidationException.java
minwoox
approved these changes
Apr 29, 2026
|
|
||
| private static final PgvValidator INSTANCE = new PgvValidator(); | ||
|
|
||
| private final ValidatorIndex delegate = new ReflectiveValidatorIndex(); |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6736 +/- ##
============================================
- Coverage 74.46% 0 -74.47%
============================================
Files 1963 0 -1963
Lines 82437 0 -82437
Branches 10764 0 -10764
============================================
- Hits 61385 0 -61385
+ Misses 15918 0 -15918
+ Partials 5134 0 -5134 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
The
(armeria.xds.supported)proto annotation was introduced in #6722 to detect unsupported xDS field usage, but the annotation was not yet applied to the proto field definitions. Additionally,XdsValidatorIndex.assertValid()acceptedObjectinstead of the more type-safeMessage, and aStrictXdsValidatorIndex(which collects all violations and throws a single exception) was missing for use in test environments.Modifications:
XdsValidatorIndex.assertValid(Object)toassertValid(Message)for type safety, and updated all call sites (XdsValidatorIndexRegistry,XdsResourceValidator,XdsExtensionRegistry,DefaultXdsValidatorIndex).protobuf-javadependency toxds-validatormodule to support theMessageparameter type.DefaultXdsValidatorIndexinto a dedicatedPgvValidatorclass.DefaultXdsValidatorIndexto composePgvValidatorandSupportedFieldValidator(withwarn()handler).StrictXdsValidatorIndexwith priority1for test environments, which collects all unsupported field violations and reports them in a singleIllegalArgumentException.(armeria.xds.supported) = trueacross all xDS proto files.Result:
DefaultXdsValidatorIndexlogs warnings for unsupported xDS fields viaSupportedFieldValidator.StrictXdsValidatorIndexis on the classpath via SPI), unsupported field usage fails fast with a clear error listing all violations.