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

HHH-13959 - Add nullability and uniqueness for @OneToOne with @JoinTable #3358

Closed
wants to merge 2 commits into from

Conversation

dreab8
Copy link
Member

@dreab8 dreab8 commented Apr 20, 2020

@pdinc-oss
Copy link

much, much nicer than our mess from 4AM.

}
else {
propertyBinder.makePropertyAndBind();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand you wanna avoid a temporary variable for makePropertyAndBind() result, but it is still a little bit confusing why propertyBinder.makePropertyAndBind() remains the same across the two if blocks.

Maybe the following refactoring is a little bit better in terms of code readability

final Property boundProperty = propertyBinder.makePropertyAndBind();
if ( property.isAnnotationPresent( JoinTable.class ) ) {
    boundProperty.setOptional( optional );
}

Not a big deal and feel free to ignore my comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @NathanQingyangXu , I have applied your suggestion.

propertyBinder.makePropertyAndBind().setOptional( optional );
}
else {
propertyBinder.makePropertyAndBind();
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason why this clause differs from the case with the JoinTable? It would seem only sensible that the optionality is always relevant for the bound property. I trust that it is not, but then I am somewhat surprised that this is an appropriate fix 😉

Choose a reason for hiding this comment

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

This patch is a bit different than the one I wrote, i'll look at it.

Choose a reason for hiding this comment

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

A quick look at it, the bindManyToOne method may be used outside of the JoinTable situation, so setting optional would be disallowed. Also not all usage of JoinTable gets you to this code, there has to be a OneToOne.

Copy link
Member Author

@dreab8 dreab8 May 26, 2020

Choose a reason for hiding this comment

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

Hi @jwgmeligmeyling I'm looking at the issue again, and I tend to agree with you.

The problem with just setting propertyBinder.makePropertyAndBind().setOptional( optional ); is for cases like the one in NonNullableCircularDependencyCascadeTest where we have

@Entity
public class Child
{
   ...

   @ManyToOne(cascade=CascadeType.PERSIST)
   @JoinColumn(name="parentId", nullable=false)
	Parent parent; 
    ...
}

I pushed a slightly different solution.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @jwgmeligmeyling for the review

@dreab8
Copy link
Member Author

dreab8 commented May 28, 2020

applied upstream

@dreab8 dreab8 closed this May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants