Skip to content
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.

Sign up for GitHub

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
+216 −272
Draft

Enhance decimal validation to match actual Jakarta Validation behavior and fix related issues #1131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2dcd754
Introduce repeatable annotation value collection for fields
Seol-JY Dec 23, 2024
396ef8f
Enhance decimal validation constraints handling with accurate behavior
Seol-JY Dec 23, 2024
9598de3
Fix operator line wrapping in validation constraint generators
Seol-JY Dec 23, 2024
952364b
Fix wildcard import in ArbitraryGeneratorContext
Seol-JY Dec 23, 2024
eb987a5
Fix wildcard import in ValidationFixtureMonkeyTest
Seol-JY Dec 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions ...pi/src/main/java/com/navercorp/fixturemonkey/api/generator/ArbitraryGeneratorContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,16 @@

package com.navercorp.fixturemonkey.api.generator;

import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toMap;

import java.lang.annotation.Annotation;
import java.lang.annotation.Repeatable;
import java.lang.reflect.AnnotatedType;
import java.lang.reflect.Method;
import java.lang.reflect.Type;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.List;
Expand Down Expand Up @@ -107,6 +111,30 @@ public <T extends Annotation> Optional<T> findAnnotation(Class<T> annotationClas
return this.getResolvedProperty().getAnnotation(annotationClass);
}

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;
}
Comment on lines +114 to +136
Copy link
Contributor

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.

Copy link
Author

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?

Copy link
Contributor

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?

Copy link
Author

@Seol-JY Seol-JY Dec 23, 2024 •
edited
Loading

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.

Copy link
Contributor

@seongahjo seongahjo Dec 24, 2024 •
edited
Loading

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

Copy link
Author

@Seol-JY Seol-JY Dec 24, 2024 •
edited
Loading

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.


public List<ArbitraryProperty> getChildren() {
return Collections.unmodifiableList(this.children);
}
Expand Down
220 changes: 84 additions & 136 deletions ...p/fixturemonkey/jakarta/validation/introspector/JakartaValidationConstraintGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,178 +292,126 @@ public JavaIntegerConstraint generateIntegerConstraint(ArbitraryGeneratorContext
@Override
@Nullable
public JavaDecimalConstraint generateDecimalConstraint(ArbitraryGeneratorContext context) {
BigDecimal positiveMin = null;
Boolean positiveMinInclusive = null;
BigDecimal positiveMax = null;
Boolean positiveMaxInclusive = null;
BigDecimal negativeMin = null;
Boolean negativeMinInclusive = null;
BigDecimal negativeMax = null;
boolean negativeMaxInclusive = false;
BigDecimal min = null;
Boolean minInclusive = null;
BigDecimal max = null;
Boolean maxInclusive = null;
Integer scale = null;

Optional<Digits> digits = context.findAnnotation(Digits.class);
if (digits.isPresent()) {
BigDecimal value = BigDecimal.ONE;
int integer = digits.get().integer();
if (integer > 1) {
value = BigDecimal.TEN.pow(integer - 1);
for (Min minAnn : context.findAnnotations(Min.class)) {
BigDecimal newMin = BigDecimal.valueOf(minAnn.value());
if (min == null || newMin.compareTo(min) > 0) {
min = newMin;
minInclusive = true;
}
positiveMax = value.multiply(BigDecimal.TEN).subtract(BigDecimal.ONE);
positiveMin = value;
negativeMax = positiveMin.negate();
negativeMin = positiveMax.negate();
positiveMinInclusive = false;
negativeMinInclusive = false;
scale = digits.get().fraction();
}

Optional<Min> minAnnotation = context.findAnnotation(Min.class);
if (minAnnotation.isPresent()) {
BigDecimal minValue = minAnnotation.map(Min::value).map(BigDecimal::valueOf).get();
if (minValue.compareTo(BigDecimal.ZERO) >= 0) {
if (positiveMin == null) {
positiveMin = minValue;
} else {
positiveMin = positiveMin.min(minValue);
}
negativeMax = null;
negativeMin = null;
} else {
if (negativeMin == null) {
negativeMin = minValue;
} else {
negativeMin = negativeMin.min(minValue);
}
negativeMinInclusive = true;
for (DecimalMin decimalMin : context.findAnnotations(DecimalMin.class)) {
BigDecimal newMin = new BigDecimal(decimalMin.value());
if (min == null || newMin.compareTo(min) > 0
|| (newMin.compareTo(min) == 0 && !decimalMin.inclusive() && minInclusive)) {
min = newMin;
minInclusive = decimalMin.inclusive();
}
}

Optional<DecimalMin> decimalMinAnnotation = context.findAnnotation(DecimalMin.class);
if (decimalMinAnnotation.isPresent()) {
BigDecimal decimalMin = new BigDecimal(
decimalMinAnnotation
.get()
.value()
);

if (decimalMin.compareTo(BigDecimal.ZERO) >= 0) {
if (positiveMin == null) {
positiveMin = decimalMin;
} else {
positiveMin = positiveMin.min(decimalMin);
}
if (!decimalMinAnnotation.map(DecimalMin::inclusive).get()) {
positiveMinInclusive = false;
}
negativeMax = null;
negativeMin = null;
} else {
if (negativeMin == null) {
negativeMin = decimalMin;
} else {
negativeMin = negativeMin.min(negativeMin);
}
if (!decimalMinAnnotation.map(DecimalMin::inclusive).get()) {
negativeMinInclusive = false;
}
for (Max maxAnn : context.findAnnotations(Max.class)) {
BigDecimal newMax = BigDecimal.valueOf(maxAnn.value());
if (max == null || newMax.compareTo(max) < 0) {
max = newMax;
maxInclusive = true;
}
}

Optional<Max> maxAnnotation = context.findAnnotation(Max.class);
if (maxAnnotation.isPresent()) {
BigDecimal maxValue = maxAnnotation.map(Max::value).map(BigDecimal::valueOf).get();
if (maxValue.compareTo(BigDecimal.ZERO) > 0) {
if (positiveMax == null) {
positiveMax = maxValue;
} else {
positiveMax = positiveMax.max(maxValue);
}
} else {
if (negativeMax == null) {
negativeMax = maxValue;
} else {
negativeMax = negativeMax.max(maxValue);
}
for (DecimalMax decimalMax : context.findAnnotations(DecimalMax.class)) {
BigDecimal newMax = new BigDecimal(decimalMax.value());
if (max == null || newMax.compareTo(max) < 0
|| (newMax.compareTo(max) == 0 && !decimalMax.inclusive() && maxInclusive)) {
max = newMax;
maxInclusive = decimalMax.inclusive();
}
}

Optional<DecimalMax> decimalMaxAnnotation = context.findAnnotation(DecimalMax.class);
if (decimalMaxAnnotation.isPresent()) {
BigDecimal decimalMax = new BigDecimal(
decimalMaxAnnotation
.get()
.value()
);

if (decimalMax.compareTo(BigDecimal.ZERO) > 0) {
if (positiveMax == null) {
positiveMax = decimalMax;
} else {
positiveMax = positiveMax.max(decimalMax);
}
positiveMaxInclusive = decimalMaxAnnotation.map(DecimalMax::inclusive).get();
} else {
if (negativeMax == null) {
negativeMax = decimalMax;
} else {
negativeMax = negativeMax.max(decimalMax);
}
negativeMaxInclusive = decimalMaxAnnotation.map(DecimalMax::inclusive).get();
}

if (!decimalMaxAnnotation.map(DecimalMax::inclusive).get()) {
positiveMaxInclusive = false;
if (context.findAnnotation(Positive.class).isPresent()) {
if (min == null || BigDecimal.ZERO.compareTo(min) > 0
|| (BigDecimal.ZERO.compareTo(min) == 0 && minInclusive)) {
min = BigDecimal.ZERO;
minInclusive = false;
}
}

if (positiveMax == null) {
positiveMax = decimalMax;
} else if (positiveMax.compareTo(decimalMax) > 0) {
positiveMax = decimalMax;
if (context.findAnnotation(PositiveOrZero.class).isPresent()) {
if (min == null || BigDecimal.ZERO.compareTo(min) > 0) {
min = BigDecimal.ZERO;
minInclusive = true;
}
}

if (context.findAnnotation(Negative.class).isPresent()) {
if (negativeMax == null || negativeMax.compareTo(BigDecimal.ZERO) > 0) {
negativeMax = BigDecimal.ZERO;
negativeMaxInclusive = false;
if (max == null || BigDecimal.ZERO.compareTo(max) < 0
|| (BigDecimal.ZERO.compareTo(max) == 0 && maxInclusive)) {
max = BigDecimal.ZERO;
maxInclusive = false;
}
}

if (context.findAnnotation(NegativeOrZero.class).isPresent()) {
if (negativeMax == null || negativeMax.compareTo(BigDecimal.ZERO) > 0) {
negativeMax = BigDecimal.ZERO;
negativeMaxInclusive = true;
if (max == null || BigDecimal.ZERO.compareTo(max) < 0) {
max = BigDecimal.ZERO;
maxInclusive = true;
}
}

if (context.findAnnotation(Positive.class).isPresent()) {
if (positiveMin == null || positiveMin.compareTo(BigDecimal.ZERO) < 0) {
positiveMin = BigDecimal.ZERO;
positiveMinInclusive = false;
Optional<Digits> digitsAnn = context.findAnnotation(Digits.class);
if (digitsAnn.isPresent()) {
Digits digits = digitsAnn.get();
int integerDigits = digits.integer();
int fractionDigits = digits.fraction();

StringBuilder maxBuilder = new StringBuilder();
for (int i = 0; i < integerDigits; i++) {
maxBuilder.append('9');
}
}
if (fractionDigits > 0) {
maxBuilder.append('.');
for (int i = 0; i < fractionDigits; i++) {
maxBuilder.append('9');
}
}
BigDecimal digitsMax = new BigDecimal(maxBuilder.toString());
BigDecimal digitsMin = digitsMax.negate();

if (context.findAnnotation(PositiveOrZero.class).isPresent()) {
if (positiveMin == null || positiveMin.compareTo(BigDecimal.ZERO) < 0) {
positiveMin = BigDecimal.ZERO;
positiveMinInclusive = true;
if (max == null || digitsMax.compareTo(max) < 0) {
max = digitsMax;
maxInclusive = true;
}
if (min == null || digitsMin.compareTo(min) > 0) {
min = digitsMin;
minInclusive = true;
}

scale = digits.fraction();
}

if (positiveMin == null && positiveMax == null && negativeMin == null && negativeMax == null && scale == null) {
if (min == null && max == null) {
return null;
}

boolean isPositiveMin = min != null && min.compareTo(BigDecimal.ZERO) >= 0;
boolean isPositiveMax = max != null && max.compareTo(BigDecimal.ZERO) >= 0;
boolean isNegativeMin = min != null && min.compareTo(BigDecimal.ZERO) < 0;
boolean isNegativeMax = max != null && max.compareTo(BigDecimal.ZERO) < 0;

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
);
Comment on lines 405 to 416
Copy link
Author

@Seol-JY Seol-JY Dec 24, 2024 •
edited
Loading

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?

Copy link
Contributor

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.

  1. 0 <= value <= 999.99
  2. -999.99 <= value <= 0

Copy link
Author

@Seol-JY Seol-JY Dec 29, 2024 •
edited
Loading

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.

}
Expand Down
2 changes: 2 additions & 0 deletions ...va/com/navercorp/fixturemonkey/jakarta/validation/JakartaValidationFixtureMonkeyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,8 @@ void sampleBigDecimal() {
then(actual.getNegativeOrZero()).isLessThanOrEqualTo(BigDecimal.ZERO);
then(actual.getPositive()).isPositive();
then(actual.getPositiveOrZero()).isGreaterThanOrEqualTo(BigDecimal.ZERO);
then(actual.getDecimalEqual()).isEqualByComparingTo(BigDecimal.valueOf(100.1));
then(actual.getIntegerEqual()).isEqualByComparingTo(BigDecimal.valueOf(100));
}

@Property(tries = 100)
Expand Down
8 changes: 8 additions & 0 deletions .../java/com/navercorp/fixturemonkey/jakarta/validation/spec/BigDecimalIntrospectorSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,12 @@ public class BigDecimalIntrospectorSpec {

@PositiveOrZero
private BigDecimal positiveOrZero;

@DecimalMin(value = "100.1")
@DecimalMax(value = "100.1")
private BigDecimal decimalEqual;

@Max(value = 100)
@Min(value = 100)
private BigDecimal integerEqual;
}
Loading
Loading