Skip to content

Conversation

@NathanQingyangXu
Copy link
Contributor

https://hibernate.atlassian.net/browse/HHH-19320

Found this defect when working on Hibernate integration project in MongoDB.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.


@hibernate-github-bot
Copy link

hibernate-github-bot bot commented Apr 5, 2025

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

@NathanQingyangXu NathanQingyangXu changed the title fix defect that currentValue parameter is still null in BeforeExecutionGenerator#generate() when id allowed to be assigned and was assigned Fix defect that currentValue parameter is still null in BeforeExecutionGenerator#generate() when id allowed to be assigned and was assigned Apr 5, 2025
…oreExecutionGenerator#generate() when id allowed to be assigned and was assigned
generatedId = null;
}
else if ( generatedBeforeExecution ) {
else if ( generatedBeforeExecution && ( ! generator.allowAssignedIdentifiers() || persister.getIdentifier( entity, source ) == null ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

While I agree with the scope of the change, I don't believe this is the correct way to do it. This way, when allowAssignedIdentifier returns true and there is a non-null value we do not call generate at all.

I would say we want to still invoke your custom BeforeExecutionGenerator though, and pass the existing identifier through the currentValue parameter.

Copy link
Member

Choose a reason for hiding this comment

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

I would say we want to still invoke your custom BeforeExecutionGenerator though, and pass the existing identifier through the currentValue parameter.

Yes, I agree. I think the bug is that currentValue is null when it shouldn't be.

Copy link
Member

Choose a reason for hiding this comment

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

I think the bug is that currentValue is null when it shouldn't be.

[That's what I was trying to say in the comment I left on the issue, though now I realize my comment wasn't very clear.]

@mbellade
Copy link
Member

mbellade commented Apr 7, 2025

I've opened #9969 which tests all cases (including non-id properties) as handles assigned ids via currentValue.

@NathanQingyangXu
Copy link
Contributor Author

I've opened #9969 which tests all cases (including non-id properties) as handles assigned ids via currentValue.

Thanks. Now I closed this PR.

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.

3 participants