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-5757 - OneToOne SQL missing parameter #1833

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@dreab8
Member

dreab8 commented Mar 6, 2017

@vladmihalcea

This comment has been minimized.

Show comment
Hide comment
@vladmihalcea

vladmihalcea Mar 8, 2017

Member

@gbadner would you like to review this PR? @dreab8 and I think it's fine, but we'd love if you could take a look on it and let us know if there's something we might miss.

Member

vladmihalcea commented Mar 8, 2017

@gbadner would you like to review this PR? @dreab8 and I think it's fine, but we'd love if you could take a look on it and let us know if there's something we might miss.

@@ -262,6 +263,22 @@ public final Object nullSafeGet(
return resolve( hydrate( rs, names, session, owner ), session, owner );
}

This comment has been minimized.

@gbadner

gbadner Mar 16, 2017

Member

I don't remember the details for SpecialOneToOneType (which extends OneToOneType), so I'm not sure if the following changes are safe for SpecialOneToOneType.

I believe the reason why OneToOneType#nullSafeSet did nothing is because in the unidirectional (non-mappedBy) case, the parent entity ID itself is the foreign key. The entity ID is already bound to the query elsewhere, so it shouldn't be bound again. We need to make sure that adding this block does not break this case.

I would need to see more tests of these other cases to be sure it doesn't break anything.

@gbadner

gbadner Mar 16, 2017

Member

I don't remember the details for SpecialOneToOneType (which extends OneToOneType), so I'm not sure if the following changes are safe for SpecialOneToOneType.

I believe the reason why OneToOneType#nullSafeSet did nothing is because in the unidirectional (non-mappedBy) case, the parent entity ID itself is the foreign key. The entity ID is already bound to the query elsewhere, so it shouldn't be bound again. We need to make sure that adding this block does not break this case.

I would need to see more tests of these other cases to be sure it doesn't break anything.

@@ -114,11 +114,6 @@ public void nullSafeSet(PreparedStatement st, Object value, int index, boolean[]
}

This comment has been minimized.

@gbadner

gbadner Oct 18, 2017

Member

The lines that precede the following deletion is:

 	@Override
 	public void nullSafeSet(PreparedStatement st, Object value, int index, boolean[] settable, SharedSessionContractImplementor session) {
 		//nothing to do
  	}

IOW, when OneToOneType#nullSafeSet(PreparedStatement, Object, int, boolean[], SharedSessionContractImplementor) is called, nothing is done.

When OneToOneType#nullSafeSet(PreparedStatement st, Object, int, SharedSessionContractImplementor) is called, EntityType#nullSafeSet(PreparedStatement st, Object, int, SharedSessionContractImplementor) will actually be called, and a value will be bound.

I don't think it makes sense to bind a value in one case, but not another. I would guess that something will break somewhere.

I'll try to come up with a test case.

@gbadner

gbadner Oct 18, 2017

Member

The lines that precede the following deletion is:

 	@Override
 	public void nullSafeSet(PreparedStatement st, Object value, int index, boolean[] settable, SharedSessionContractImplementor session) {
 		//nothing to do
  	}

IOW, when OneToOneType#nullSafeSet(PreparedStatement, Object, int, boolean[], SharedSessionContractImplementor) is called, nothing is done.

When OneToOneType#nullSafeSet(PreparedStatement st, Object, int, SharedSessionContractImplementor) is called, EntityType#nullSafeSet(PreparedStatement st, Object, int, SharedSessionContractImplementor) will actually be called, and a value will be bound.

I don't think it makes sense to bind a value in one case, but not another. I would guess that something will break somewhere.

I'll try to come up with a test case.

This comment has been minimized.

@gbadner

gbadner Oct 19, 2017

Member

I think this is OK.

OneToOneType#nullSafeSet(PreparedStatement, Object, int, boolean[], SharedSessionContractImplementor) is used when inserting, updating, or deleting the entity with the one-to-one association. In those cases, OneToOneType#nullSafeSet should do nothing.

@gbadner

gbadner Oct 19, 2017

Member

I think this is OK.

OneToOneType#nullSafeSet(PreparedStatement, Object, int, boolean[], SharedSessionContractImplementor) is used when inserting, updating, or deleting the entity with the one-to-one association. In those cases, OneToOneType#nullSafeSet should do nothing.

This comment has been minimized.

@gbadner

gbadner Oct 19, 2017

Member

I added a comment that I checked this out and it was OK. I think you can see the comment when you look at the "Files Changed" tab.

@gbadner

gbadner Oct 19, 2017

Member

I added a comment that I checked this out and it was OK. I think you can see the comment when you look at the "Files Changed" tab.

@@ -708,4 +725,15 @@ public Object loadByUniqueKey(
return result == null ? null : persistenceContext.proxyFor( result );
}

This comment has been minimized.

@gbadner

gbadner Oct 19, 2017

Member

The error message below refers to "many-to-one", and I don't think it makes sense for a OneToOneType attribute.

Also, EntityType#getLHSPropertyName returns null, so it's not informative.

I'm not sure of the point of having #requireIdentifierOrUniqueKeyType, since #getIdentifierOrUniqueKeyType is supposed to throw MappingException if the Type can't be resolved. Also, There are lots of usages of #getIdentifierOrUniqueKeyType that assume the return value is not null. If #getIdentifierOrUniqueKeyType returned null, I think we'd be seeing NullPointerException.

I'm ok with removing #requireIdentifierOrUniqueKeyType and changing references to that method to #getIdentifierOrUniqueKeyType, but please check with @sebersole in case I'm missing something here.

@gbadner

gbadner Oct 19, 2017

Member

The error message below refers to "many-to-one", and I don't think it makes sense for a OneToOneType attribute.

Also, EntityType#getLHSPropertyName returns null, so it's not informative.

I'm not sure of the point of having #requireIdentifierOrUniqueKeyType, since #getIdentifierOrUniqueKeyType is supposed to throw MappingException if the Type can't be resolved. Also, There are lots of usages of #getIdentifierOrUniqueKeyType that assume the return value is not null. If #getIdentifierOrUniqueKeyType returned null, I think we'd be seeing NullPointerException.

I'm ok with removing #requireIdentifierOrUniqueKeyType and changing references to that method to #getIdentifierOrUniqueKeyType, but please check with @sebersole in case I'm missing something here.

@gbadner

This looks good. It would be nice to get rid of EntityType#requireIdentifierOrUniqueKeyType if it's not needed.

@vladmihalcea

This comment has been minimized.

Show comment
Hide comment
@vladmihalcea

vladmihalcea Jan 17, 2018

Member

Applied upstream, thanks

Member

vladmihalcea commented Jan 17, 2018

Applied upstream, thanks

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