-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhance decimal validation to match actual Jakarta Validation behavior and fix related issues #1131
base: main
Are you sure you want to change the base?
Enhance decimal validation to match actual Jakarta Validation behavior and fix related issues #1131
Conversation
@DecimalMax(value = "100.1") | ||
@DecimalMin(value = "100.1") | ||
private BigDecimal decimalEqual; | ||
|
||
@Max(value = 100) | ||
@Min(value = 100) | ||
private BigDecimal integerEqual; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot infer your intent, could you create a new DTO and make a new concise and clear test to reveal your intent?
public <T extends Annotation> List<T> findAnnotations(Class<T> annotationClass) { | ||
List<T> results = this.getResolvedProperty().getAnnotations().stream() | ||
.filter(it -> annotationClass.isAssignableFrom(it.annotationType())) | ||
.map(annotationClass::cast).collect(toList()); | ||
|
||
Repeatable repeatable = annotationClass.getAnnotation(Repeatable.class); | ||
if (repeatable != null) { | ||
Class<? extends Annotation> containerClass = repeatable.value(); | ||
this.getResolvedProperty().getAnnotations().stream() | ||
.filter(it -> containerClass.isAssignableFrom(it.annotationType())) | ||
.findFirst() | ||
.ifPresent(container -> { | ||
try { | ||
Method valueMethod = container.annotationType().getDeclaredMethod("value"); | ||
T[] values = (T[])valueMethod.invoke(container); | ||
results.addAll(Arrays.asList(values)); | ||
} catch (Exception ignored) { | ||
} | ||
}); | ||
} | ||
|
||
return results; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not for common case.
You can not confirm that user uses the annotation @Repeatable
, the name of property value
.
I think it should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seongahjo
Ah, I understand! Instead of defining a new findAnnotations
in ArbitraryGeneratorContext
, should I handle duplicate annotations within ArbitraryGeneratorContext
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this.getResolvedProperty().getAnnotations()
not return the duplicate annotations?
Could you make a test to show when the duplicate annotation is an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this.getResolvedProperty().getAnnotations() not return the duplicate annotations?
Yes.
@Getter
@Setter
public class TestSpec {
@Max(value = 50) // interface jakarta.validation.constraints.Max
private BigDecimal a;
@Max(value = 100) // multiple @Max annotations - internally processed as container annotation
@Max(value = 50)
private BigDecimal b;
@Max.List({ // interface jakarta.validation.constraints.Max$List
@Max(value = 100),
@Max(value = 50)
})
private BigDecimal c;
}
Duplicate annotations appear to be automatically processed in the same way as the annotations in field c.
Therefore, I implemented findAnnotations
to process the contents within Max$List
in the same way as regular duplicate annotations, as shown above.
Without the Repeatable
processing in findAnnotations
, if we only use this.getResolvedProperty().getAnnotations()
, we would need to search twice using getAnnotations(Max.class)
and getAnnotations(Max.List.class)
. This is because we need to consider both cases where there is a single annotation and multiple annotations.
Since getAnnotationsByType
cannot be used in the current implementation, I chose to implement findAnnotations
to handle both single annotations and container annotations in a unified way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not clear to me how it works when there are multiple constraints on the same property.
Does the JSR-303 support multiple annotations on the same property within the same group?
I only found the applying multiple constraints within the different group.
https://beanvalidation.org/1.0/spec/#constraintsdefinitionimplementation-multipleconstraints
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the @Repeatable
annotation allows multiple annotations to be applied, it was confirmed during testing that all validation annotations (including duplicates) are processed as expected.
However, JSR-303 does not define a specific approach for handling duplicate annotations, meaning implementations can vary.
Given this, rather than implementing complex logic to handle duplicates, it may be more practical to revert to the original approach of simply checking for the presence of annotations.
return new JavaDecimalConstraint( | ||
positiveMin, | ||
positiveMinInclusive, | ||
positiveMax, | ||
positiveMaxInclusive, | ||
negativeMin, | ||
negativeMinInclusive, | ||
negativeMax, | ||
negativeMaxInclusive, | ||
isPositiveMin ? min : null, | ||
isPositiveMin ? minInclusive : null, | ||
isPositiveMax ? max : null, | ||
isPositiveMax ? maxInclusive : null, | ||
|
||
isNegativeMin ? min : null, | ||
isNegativeMin ? minInclusive : null, | ||
isNegativeMax ? max : null, | ||
isNegativeMax ? maxInclusive : null, | ||
scale | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why the JavaDecimalConstraint class needs to manage separate min/max values for negative and positive numbers. In the previous implementation with the @Digits
annotation, this was necessary, but with the "3. Improved @Digits
Annotation Processing," the method for enforcing maximum digit limits has changed. In this case, wouldn't it be sufficient to just manage the min/max values and their inclusiveness to generate random numbers? What do you think about changing this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better idea for addressing both positive and negative values in the case of @Digits
?
For example, in the case of @Digits(integer=3, fraction=2)
, it should be the value that satisfies the following constraints.
- 0 <= value <= 999.99
- -999.99 <= value <= 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at your example with @Digits(integer=3, fraction=2)
, I believe we could satisfy these constraints simply using a minimum (-999.99) and maximum (999.99) value. Is there any specific case where we need to manage positive and negative ranges separately in the current implementation?
Moreover, simplifying to a single range would actually help prevent potential distribution issues. For instance, with the current implementation, when we have constraints like:
@Digits(integer=3, fraction=2)
@Min(-2)
@Max(100)
private BigDecimal b;
The resolveArbitrary
method in JqwikJavaArbitraryResolver
splits the generation between negative and positive ranges with equal probability, leading to disproportionate distribution where values between -2 and 0 have the same total probability as values between 0 and 100. By using a single range approach, we could ensure truly uniform distribution across the entire valid range.
If I'm missing any important considerations, please let me know.
Summary
This PR addresses several issues and enhances the decimal validation handling in Fixture Monkey, primarily focused on fixing #1126 and improving the overall validation behavior for numeric types.
Related issues:
Fixes #1126, #1127
Description
This PR implements comprehensive improvements to the decimal validation system:
1. Fixed
Inclusive
Property Handlinginclusive
property in validation annotationsBigDecimal
,float
, anddouble
types2. Enhanced Multiple Validation Handling
(e.g., when
@DecimalMax("11.5") @Max(10)
is applied to a field, values should be less than or equal to 10.0, as it satisfies both constraints)3. Improved
@Digits
Annotation Processing@Digits
validation behavior with Jakarta Validation's actual implementation@Digits(integer=3, fraction=2)
, strictly generated numbers inxxx.xx
format@Digits(integer=3, fraction=2)
, correctly allows:xxx.xx
format was generated4.
Added Repeatable Annotation SupportImplemented collection of all instances from container annotations marked with@Repeatable
Ensures comprehensive validation when multiple constraints are present5. Code Refactoring
How Has This Been Tested?
Added new test cases for:
Manually verified functionality for other changes through direct testing:
@Digits
annotation's new behavior with various combinations (e.g., integer=3,fraction=2 properly allowing values up to 999.99)@Min
/@Max
with@DecimalMin
/@DecimalMax
)Is the Document updated?
No documentation update is required as this is a bug fix for internal implementation.