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.

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

Simplify JavaIntegerConstraint structure to align with decimal constraint improvements #1156

Merged
merged 5 commits into from
Mar 5, 2025

Conversation

Seol-JY
Copy link
Contributor

@Seol-JY Seol-JY commented Feb 25, 2025

Summary

This PR simplifies the JavaIntegerConstraint structure by removing the separate positive and negative range handling, aligning with the recent decimal constraint implementation improvements in #1131. Additionally, it improves the handling of @Digits annotation for integer types.

Related issues:
Fixes #1153

Description

This PR implements a simplification of the integer constraint structure and improves digit validation:

1. Simplified JavaIntegerConstraint Structure

  • Removed redundant fields by eliminating separate positive/negative range handling
  • Replaced with a single min/max range representation:
    JavaIntegerConstraint(
        @Nullable BigInteger min,
        @Nullable BigInteger max
    )
  • Ensures uniform distribution across valid ranges

2. Improved @Digits Annotation Processing for Integers

  • Aligned @Digits validation behavior with Jakarta Validation's actual implementation
  • Previous implementation was overly restrictive, forcing exact digit counts
  • New implementation matches actual validation rules:
    • For @Digits(integer=3), correctly allows any number up to 999 (≤ 3 digits)
    • Example: Both 5 and 999 are now valid where previously only exact 3-digit numbers might be generated

3. Consolidated Constraint Processing

4. Reduced Code Complexity

  • Simplified constraint logic and improved maintainability
  • Ensured consistent integer constraint handling with our decimal constraint implementation
  • Maintained full compatibility with Jakarta Validation expectations

How Has This Been Tested?

Updated @Digits annotation tests for integer types to verify correct validation behavior.

Is the Document updated?

No documentation update is required as this is an internal implementation improvement that maintains the same behavior from the user's perspective.

Comment on lines 245 to 246
max = max.min(BIG_INTEGER_MAX_BYTE);
min = min.max(BIG_INTEGER_MIN_BYTE);
Copy link
Contributor Author

@Seol-JY Seol-JY Feb 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this part may be a bit ambiguous. Should we change the variable name?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

negativeMin = defaultIfNull(negativeMin, () -> BigInteger.valueOf(DEFAULT_MIN_NUMBER_VALUE));
negativeMax = defaultIfNull(negativeMax, () -> BigInteger.ZERO);
}
BigInteger min = negativeMinNumberValue != null
Copy link
Contributor

@seongahjo seongahjo Mar 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the fields in SimpleValueJqwikPlugin should be also simplified.

	@Nullable
	private Long positiveMinNumberValue = null;
	@Nullable
	private Long positiveMaxNumberValue = null;
	@Nullable
	private Long negativeMinNumberValue = null;
	@Nullable
	private Long negativeMaxNumberValue = null;

@Seol-JY Seol-JY requested a review from seongahjo March 3, 2025 11:29
}
}

Optional<Digits> digitsAnn = context.findAnnotation(Digits.class);
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 better not to use the abbreviation to avoid confusion.

digitsAnn -> digitsAnnotation

}
}

Optional<Digits> digitsAnn = context.findAnnotation(Digits.class);
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 better not to use the abbreviation to avoid confusion as well.

@Seol-JY Seol-JY requested a review from seongahjo March 4, 2025 13:53
@Seol-JY
Copy link
Contributor Author

Seol-JY commented Mar 4, 2025

@seongahjo
I noticed that the supplierReturnsNew test failed during the CI process for my changes:

FAILED
303 java.lang.AssertionError at FixtureMonkeyTest.java:1044

This appears to be an intermittent issue. To investigate further, I increased the test execution from the original 10 attempts to 100,000 attempts locally by modifying the annotation to @Property(tries = 100000). During this extended testing, I observed a single failure.
I'm wondering if this very rare failure is related to my changes or if it's an existing intermittent issue in the test itself.

@seongahjo
Copy link
Contributor

I'm wondering if this very rare failure is related to my changes or if it's an existing intermittent issue in the test itself.

👍 It's a known issue, don't worry about it.

Copy link
Contributor

@seongahjo seongahjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@seongahjo seongahjo added this to the 1.1.10 milestone Mar 5, 2025
@seongahjo seongahjo merged commit d5ba0a2 into naver:main Mar 5, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simplify JavaIntegerConstraint structure to align with decimal constraint improvements
2 participants