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

Jump to bottom

HHH-18724 Take constraint composition type into account when determining nullable state of a Column #9092

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

HHH-18724 Take constraint composition type into account when determining nullable state of a Column #9092

wants to merge 1 commit into from
+489 −35

Conversation

wytzevanderploeg
Copy link

@wytzevanderploeg wytzevanderploeg commented Oct 15, 2024 •
edited by hibernate-github-bot bot
Loading

gavinking
gavinking requested changes Oct 15, 2024
View reviewed changes
Copy link
Member

@gavinking gavinking left a comment

Choose a reason for hiding this comment

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

The pull request should target main.

Also see my comments.

hibernate-core/src/main/java/org/hibernate/boot/beanvalidation/TypeSafeActivator.java Outdated Show resolved Hide resolved
hibernate-core/src/main/java/org/hibernate/boot/beanvalidation/TypeSafeActivator.java Outdated Show resolved Hide resolved
@wytzevanderploeg wytzevanderploeg changed the base branch from 6.6 to main October 15, 2024 10:16
@hibernate-github-bot
Copy link

hibernate-github-bot bot commented Oct 15, 2024 •
edited
Loading

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

@wytzevanderploeg wytzevanderploeg force-pushed the HHH-18724 branch 2 times, most recently from 4cbef2e to 0e343dc Compare October 15, 2024 12:57
@wytzevanderploeg
Copy link
Author

Found an edge case that needs fixing, will mark as draft for now.

@wytzevanderploeg wytzevanderploeg marked this pull request as draft October 16, 2024 09:57
@wytzevanderploeg wytzevanderploeg marked this pull request as ready for review October 16, 2024 18:07
@wytzevanderploeg
Copy link
Author

Fixed edge case. Added a few more tests to ensure better test coverage.
Had to change logic a bit to ensure that initial state of the nullable boolean was set based on the first constraint in the collection.

@wytzevanderploeg wytzevanderploeg force-pushed the HHH-18724 branch 2 times, most recently from 8e93c3d to 14827c5 Compare October 25, 2024 18:00
@gavinking
Copy link
Member

Thanks, looking good now.

beikov
beikov reviewed Jan 20, 2025
View reviewed changes
hibernate-core/src/main/java/org/hibernate/boot/beanvalidation/TypeSafeActivator.java
@@ -72,6 +75,9 @@ class TypeSafeActivator {

private static final CoreMessageLogger LOG = Logger.getMessageLogger( MethodHandles.lookup(), CoreMessageLogger.class, TypeSafeActivator.class.getName() );

private static final Map<Class<? extends Annotation>, Boolean>
Copy link
Member

Choose a reason for hiding this comment

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

Storing class references to application classes in a static field is unfortunately going to be a problem, unless you clean up the storage properly. Also, concurrent deployments could potentially run into race conditions, so I would suggest you use a ThreadLocal<Map<Class<? extends Annotation>, Boolean>> instead and also make sure you clear it in a finally block within the activate method.

beikov
beikov reviewed Jan 20, 2025
View reviewed changes
hibernate-core/src/main/java/org/hibernate/boot/beanvalidation/TypeSafeActivator.java
Comment on lines +270 to +271
boolean firstItem = true;
boolean composedResultHasNotNull = false;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
boolean firstItem = true;
boolean composedResultHasNotNull = false;
boolean composedResultHasNotNull = !useOrLogicForComposedConstraint;

Copy link
Author

@wytzevanderploeg wytzevanderploeg Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is what I initially had but it leads to an edge case when the first property is nullable in a composed constraint of type OR. (See test case I added for the Address.line3 property) In this case the result should be that the column should be considered nullable (nullable OR not-nullable => nullable) This is partly due to the fact that this function is applied recursively for constraints and I did not feel comfortable enough to completely change this. Thus resulting in this head/tail approach.

beikov
beikov reviewed Jan 20, 2025
View reviewed changes
hibernate-core/src/main/java/org/hibernate/boot/beanvalidation/TypeSafeActivator.java
Comment on lines +305 to +309
if ( firstItem ) {
composedResultHasNotNull = hasNotNull;
firstItem = false;
}
else if ( !useOrLogicForComposedConstraint ) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ( firstItem ) {
composedResultHasNotNull = hasNotNull;
firstItem = false;
}
else if ( !useOrLogicForComposedConstraint ) {
if ( !useOrLogicForComposedConstraint ) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@beikov beikov beikov left review comments

@gavinking gavinking Awaiting requested review from gavinking

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@wytzevanderploeg @gavinking @beikov