Copilot commented on code in PR #4621:
URL: https://github.com/apache/polaris/pull/4621#discussion_r3363383238
##########
server-templates/pojo.mustache:
##########
@@ -90,7 +97,21 @@ It is updated to remove all swagger annotations and support
builders and immutab
return new Builder();
}
{{#hasRequired}}
- public static Builder builder({{#requiredVars}}{{{datatypeWithEnum}}}
{{name}}{{^-last}}, {{/-last}}{{/requiredVars}}) {
+ public static Builder
builder({{#requiredVars}}{{#isContainer}}{{{baseType}}}<{{{items.baseType}}}>{{/isContainer}}{{^isContainer}}{{{datatypeWithEnum}}}{{/isContainer}}
{{name}}{{^-last}}, {{/-last}}{{/requiredVars}}) {
+ {{#useBeanValidation}}
+ {{#requiredVars}}
+ {{#isContainer}}
+ if ({{name}} != null) {
+ {{name}}.forEach(item -> {
+ var violations = VALIDATOR.validate(item);
Review Comment:
`var` requires Java 10+, but many OpenAPI/Swagger Java POJO templates
typically target Java 8/11 compatibility depending on generator settings. As
written, this can cause generated code to fail compilation on Java 8/9. Prefer
an explicit type (e.g., `Set<ConstraintViolation<...>> violations = ...`) that
compiles across the supported Java versions.
##########
server-templates/pojo.mustache:
##########
@@ -90,7 +97,21 @@ It is updated to remove all swagger annotations and support
builders and immutab
return new Builder();
}
{{#hasRequired}}
- public static Builder builder({{#requiredVars}}{{{datatypeWithEnum}}}
{{name}}{{^-last}}, {{/-last}}{{/requiredVars}}) {
+ public static Builder
builder({{#requiredVars}}{{#isContainer}}{{{baseType}}}<{{{items.baseType}}}>{{/isContainer}}{{^isContainer}}{{{datatypeWithEnum}}}{{/isContainer}}
{{name}}{{^-last}}, {{/-last}}{{/requiredVars}}) {
+ {{#useBeanValidation}}
+ {{#requiredVars}}
+ {{#isContainer}}
+ if ({{name}} != null) {
+ {{name}}.forEach(item -> {
+ var violations = VALIDATOR.validate(item);
+ if (!violations.isEmpty()) {
+ throw new ConstraintViolationException(violations);
+ }
+ });
Review Comment:
`Validator#validate(...)` throws `IllegalArgumentException` when the object
to validate is `null`. If a required collection contains null elements (which
can happen even if the collection itself is non-null), the generated code will
throw an unexpected exception type. Add an explicit `item != null` guard (or
convert to a deterministic `ConstraintViolationException` / custom message)
before calling `VALIDATOR.validate(item)`.
##########
server-templates/pojo.mustache:
##########
@@ -90,7 +97,21 @@ It is updated to remove all swagger annotations and support
builders and immutab
return new Builder();
}
{{#hasRequired}}
- public static Builder builder({{#requiredVars}}{{{datatypeWithEnum}}}
{{name}}{{^-last}}, {{/-last}}{{/requiredVars}}) {
+ public static Builder
builder({{#requiredVars}}{{#isContainer}}{{{baseType}}}<{{{items.baseType}}}>{{/isContainer}}{{^isContainer}}{{{datatypeWithEnum}}}{{/isContainer}}
{{name}}{{^-last}}, {{/-last}}{{/requiredVars}}) {
Review Comment:
The new container type rendering `{{{baseType}}}<{{{items.baseType}}}>` is
likely invalid for some container shapes (notably maps, which typically need
two generic parameters like `Map<String, V>`). Using `datatypeWithEnum` (or an
existing template variable that already represents the fully-qualified generic
type) avoids producing uncompilable signatures such as `Map<Foo>`.
##########
server-templates/pojo.mustache:
##########
@@ -15,6 +18,10 @@ It is updated to remove all swagger annotations and support
builders and immutab
{{#serializableModel}}
private static final long serialVersionUID = 1L;
{{/serializableModel}}
+{{#useBeanValidation}}
+ private static final Validator VALIDATOR =
+ Validation.buildDefaultValidatorFactory().getValidator();
Review Comment:
`Validation.buildDefaultValidatorFactory()` creates a `ValidatorFactory`
that is typically expected to be closed when no longer needed. Storing only the
`Validator` makes it hard to close the factory, which can lead to leaked
resources depending on the provider. Consider keeping a static
`ValidatorFactory` field and closing it via an application lifecycle hook, or
avoid building a factory per generated model and instead rely on
framework-managed validation (e.g., passing a `Validator` in from the calling
layer).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
