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-16957 Embeddable cannot have only one @generated field #8543

Closed
wants to merge 2 commits into from

Conversation

dreab8
Copy link
Contributor

@dreab8 dreab8 commented Jun 11, 2024

scope.inTransaction(
session -> {
TestEntity testEntity = new TestEntity( 1l, "prop1", "prop2" );
session.save( testEntity );

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note generated test

Invoking
Session.save
should be avoided because it has been deprecated.
if ( subtype instanceof BasicType ) {
final boolean insertable;
final boolean updateable;
if ( generatedWithNoParameter( onExecutionGenerator ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please help me understand, why would we want to set insertable/updatable of a property to false when it has a generator. Doesn't this depend on the generation types? Or should we "care" about the config at all i.e. rely on the use configuring this correctly? Also, how does this interact with the writable flag of @org.hibernate.annotations.Generated?


if ( generatedWithNoParameter( onExecutionGenerator ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question as above about insertability/updatability.

Comment on lines +865 to +866
dirtinessChecker != null ?
dirtinessChecker.isDirty( attributeIndex, attributeMapping ).isDirty() :
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit concerned that dirtiness checking is so high level i.e. just for root attributes. Shouldn't this be part of the AttributeAnalysis? Also, can that maybe check nested dirtiness automatically when using dynamic updates? I think that would make the whole process a bit simpler as we could pass along the AttributeAnalysis info into the decomposition.

Copy link
Contributor

Choose a reason for hiding this comment

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

My idea is that while building UpdateValuesAnalysisImpl and the respective AttributeAnalysis, we could compute e.g. a bit set that says if a selectable index is to be included or not. That way, we could have a very simple check in the decompose step and push the logic with generators into the analysis phase where it IMO belongs. We can even pre-compute some model based info to only do checks at runtime that really rely on the value/dirtiness.

jdbcValueBindings,
tableMapping,
(valueIndex, bindings, table, jdbcValue, jdbcMapping) -> {
if ( jdbcMapping instanceof EmbeddedAttributeMapping ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since EmbeddedAttributeMapping does not extend SelectableMapping, this branch does not make much sense, unless I am missing something.

@dreab8
Copy link
Contributor Author

dreab8 commented Nov 18, 2024

closed in favour of #9280

@dreab8 dreab8 closed this Nov 18, 2024
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.

2 participants