HHH-19542 embeddable property order

[itsmoonrack]

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

The change is about using either the secondary table of a component, by looking at the first @column available (to avoid nested component to be picked up), or fall-back on the property holder table if no secondary table is defined.

At first i tried to change the property order, which worked, but when doing the test against java record, it messed up the constructor order, so i had to find another strategy. Since we cannot change the property orders, the idea is to set the table directly on the component so we avoid changing it back in the ComponentPropertyHolder.

See jira for detailed analysis.

I am a bit concerned about naming convention in the lookup method getSecondaryTable if we have prefix, will it still work here ? Also for the lookup of secondary table best I could find was from the Column definition itself line 1011 of the EmbeddableBinder. Maybe there is a more direct route ?

I did not run the full test suit but only my two tests validate, I will let jenkins do the full suit.

---

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.

---

[itsmoonrack]

ok there is lot of tests failing due to this change, i'm working on fixing them

[itsmoonrack]

i am not sure about the direct access to Column annotation here, is there a better way to access the table here ? Also with the naming convention the table name can be altered, i am covering all use cases here ? @gavinking

[mbellade]

Accessing the @Column annotation is fine, but this logic is changing the behavior: previously, a component's columns all had to belong to the same table and, if that was not the owner's table, you had to explicitly define it in the component (i.e. all properties having the same @Column(table="...").

With your change, as soon as 1 column has a different table, we inherit that one. Not saying the change is incorrect, especially since now the behavior depends on the order of columns, but we must take that into account. Also, this logic now lives in ComponentPropertyHolder.addProperty, is there any reason you couldn't change it there?

[gavinking]

as soon as 1 column has a different table, we inherit that one. Not saying the change is incorrect, especially since now the behavior depends on the order of columns

Wait, what?

That doesn't sound right. One @Column annotation should not side-effect other column mappings.

[mbellade]

Hey @gavinking, if you look at the Jira or the test: right now, if one component has a property with a single @Column mapping and another sub-component, depending on the property's order (for records) or names (for classes), it affects the component's table.

What @itsmoonrack is trying to do with his change, is derive the table information as soon as one @Column points to a secondary table. That is indeed not the best, I'd rather see something similar to ComponentPropertyHolder.addProperty where we require that all of the component's properties point to the same table.

[gavinking]

if one component has a property with a single @column mapping and another sub-component, depending on the property's order (for records) or names (for classes), it affects the component's table.

Well, I mean, that sounds like a bug.

derive the table information as soon as one @Column points to a secondary table

To me this would quite simply violate the intent of the spec.

If I understand the use case correctly, you're supposed to use an explicit @AttributeOverride for this.

[itsmoonrack]

@gavinking yes that's normal because the actual code only take the firstColumn in the AnnotatedColumns to decide which explicit table to take:

public Join getJoin() {
		final AnnotatedColumn firstColumn = columns.get( 0 );
		final String explicitTableName = firstColumn.getExplicitTableName();

https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/java/org/hibernate/boot/model/internal/AnnotatedColumns.java#L86

And I don't see how I should change that logic or not.

If you comment the first override the test should fail as expected.

How do you think we could fix this ? By doing an extra checking on the ComponentPropertyHolder or trying to fix upstream to get the columns object (not null) instead ?

[gavinking]

Well, no, it's not normal, it's a bug. In that code you just linked me, there's a comment saying:

//note: checkPropertyConsistency() is responsible for ensuring they all have the same table name

And then checkPropertyConsistency() has this code:

if ( !current.getExplicitTableName().equals( previous.getExplicitTableName() ) ) {
        throw new AnnotationException(
		        "Column mappings for property '" + propertyName + "' mix distinct secondary tables"
        );
}

which is clearly intended to detect such conflicts.

So the question is: why doesn't it work?

[itsmoonrack]

From what I see the checkPropertyConsistency() is called from the make() method.

But because in this case of a nested component, the columns are null so we never actually call the makePropertyValueAndBind() but instead the makePropertyAndBind() since the value of the Embedded is the nested Embeddable on the PropertyBinder :

//used when value is provided
public Property makePropertyAndBind() {
	return bind( makeProperty() );
}

//used to build everything from scratch
private Property makePropertyValueAndBind() {
	return bind( makePropertyAndValue() );
}

Only on the line 277 we actually calle make on the basicValueBinder which calls the checkPropertyConsitency():

https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/java/org/hibernate/boot/model/internal/PropertyBinder.java#L277

[itsmoonrack]

Trying to scratch my head around, when I remove the @AttributeOverride from the lastName like you did, the last name still have the "Person" table.

In fact, going step by step, when the property JdkFieldDetails(firstName) is being makePropertyValueAndBind() from scratch, the annotated columns contains a single mapping column Column(firstName) which have the explicitTableName = 'Person'. The method then delegate to the basicValueBinder.make() logic in which is will resolve the table from the single columns. Here the isSecondaryTable() is true since we have the explicitTableName set.

Now the table of the BasicValueBinder is Table(Person). So far so good. When the Property(firstName) is added to the ComponentPropertyHolder, the component Component[FullName] table is replaced from Table(UserEntity) to Table(Person) after checking that it is the first proparty being added otherwise the AnnotationException is thrown. We just bound firstName.

Now we process the element annotations of lastName, columnsBuilder gets only one column as well, a Column(lastName) with explicitTableName = null. We then bindBasicOrComposite again, which makePropertyValueAndBind() from scratch, which delegate to basicValueBinder.make() again. Just like for firstName, the columns.checkPropertyConsistency() is called again, and since only 1 column (the lastName) is here, is goes through without issue. Now isSecondary() is not true anymore since explicitTableName is null, so we get the propertyHolder ComponentPropertyHolder(person.fullName) table which has been set previously when adding the property firstName. So the table here is Table(Person).

It is correct but implicitly set just because the former property changed the component's table when binding it.

Maybe it is at this precise point that we could act but I see potential issues working on the BasicValueBinder. The code is not local to Embeddable or Component logic, unlike the ComponentPropertyHolder, so we could have side effects. On the ComponentPropertyHolder.addProperty its also not a good entry point since the properties we are adding does not knows each others, and when we are adding the Component(FullName) its state is already inferred and looks like this:

• Property[fullName]
• • Component[FullName]
• • • properties (2)
• • • • Property[firstName] -> BasicValue([Column(firstName)])
• • • • Property[lastName] -> BasicValue([Column(lastName)])

So at this point we can't do anything either, since the values already resolve to the secondary table and we don't have the information anymore that the explicitTable is not set on the lastName property.

On the mapping side, lastName is correctly mapped to the Person table even if not specified, but inherited by the ComponentPropertyHolder.addProperty logic, which is not ideal, but working by accident.

I know its not ideal but I am not seeing how to improve that. Maybe we could check through a second pass that all the properties have explicit mapping ? But again, I think it will be too late since the properties are bound already and we lose the raw information, also it would add complexity for the binding sequence overall ?

Or a last idea would be to rethink the makeProperty strategy to have columns and kick in the columns.checkPropertyConsistency() but in our case the selected strategy was to not make from scratch since a component was already passed.

[itsmoonrack]

@mbellade @gavinking what do you think if we create another jira to be more strict on the checking of columns ?

Right now as I see it the original jira was to fix the issue when the embeddable properties were in an order that was buggy. If we had other fields that do not specify the secondary table it would still map them to the secondary table.

So in that regard this PR fixes the original issue.

Now I understand we want to make it cleaner, but i start to believe this would involve more refactoring and i'm out of ideas at the moment. You are definitely pointing me into good directions but my lack of historical knowledge on the current design disallow me to make a more larger proposition.

Is that something you would agree on ? I can create a follow-up jira and work on it, but I feel this one is also fixing an issue on its own even if not perfect it does work ok here.

Or you want to improve the product in this ticket and I go for the broader refactoring, just let me know

[itsmoonrack]

all tests are passing on local, would a maintainer accept the process in jenkins so it can run ?

[itsmoonrack]

What is the process now (this is my first contribution). Should I wait for a design review ? Do we just merge on master ? What if I want to backport this on the 6.x release lifecycle branches ?

Thank you for your time

[mbellade]

Hello @itsmoonrack, thank you very much for your contribution, left an initial comment.

[itsmoonrack]

since we are not using columns at all i wonder if in term of design i should keep the logic in this method at all here ? let me know

[mbellade]

Hey @itsmoonrack, this solution looks much better!

There's one last thing that makes me wonder: if the property.getValue().getTable() method returns the expected result (i.e. the @AttributeOverride's table for nested embeddables), why was it not the case in the AnnotatedColumns?

Could you please have a look at the binders logic and check if there might be something missing there? It would be great for consistency to understand why this is happening as well.

[itsmoonrack]

Hey @mbellade, thanks !

That's indeed a good question. From what I have seen earlier the property is already constructed from those columns (higher in the hierarchy call). So in effect the property contains the same informations as the AnnotatedColumns.

What I found however, is that in the special event of an embeddable in an embeddable (our nested use case), that the columns were missing (columns = null), effectively skipping the whole logic of replacing a table. I have ran the full test suit to ensure non regression and it passed. So conceptually I am a bit bugged about this AnnotatedColumns versus Property, for me, after analysing, the later contains the former;

I thought about doing the following,

columns != null ? columns.getTable() : property.getValue().getTable()

But eventually rolled back as I did not find any value of doing so.

In both cases the full test suit passed.

Please correct me if I'm wrong or if I missed something obvious. We can put back the proposed line in this comment if we are more confident with it.

[itsmoonrack]

Oh, you mean why the columns were empty in the first place ?

That's a good question, I will investigate this evening.

[itsmoonrack]

@mbellade I tried to pass the columns to the EmbeddableBinder.createEmbeddedProperty to see what happen. Now the AnnotatedColumns are correctly passed, however the getTable() resolve to the component.getTable(), at this stage, the Person component getTable() returns UserEntity, while the property.getValue().getTable() returns Person. The later being correct.

So we cannot blatantly passes the columns to the PropertyBinder.

I am also reading that when we have an underlying value we use the makePropertyAndBind(), instead of the makePropertyValueAndBind() //used when the value is provided and the binding is done elsewhere

[itsmoonrack]

Hello @mbellade, the fact that the columns are null is because in the method createEmbeddedProperty() from the EmbeddableBinder simple don't set columns at all before makePropertyAndBind() is called. But it looks like this is by design since "the value is provided and the binding is done elsewhere".

From now since the value is provided, I feel that the code proposed in this PR does solve our issue.

Do you think its acceptable or you want to dig further ? I feel that the changeset would be larger if we go this other route and my (limited) knowledge is impeding me from finding a satisfactory solution other than that.

[itsmoonrack]

Please follow this discussion for the last proposition @gavinking

[gavinking]

What happens when the column mappings for different fields disagree on the mapped table (via @AttributeOverride or otherwise) ?

[itsmoonrack]

@gavinking from what i've seen, when I remove the first column @AttributeOverride mapping, to have a disagrement, we have the exception saying all properties of the component must map to the same table.

The propert.getValue().getTable() already resolve to the correct table because the overrides are already made at this point.

[itsmoonrack]

I tried to understand the binding logic upstream but just ended not knowing what to do to solve the issue of having columns null here.

[gavinking]

Suggested change

	+ "' has properties mapped to two different tables"
	+ " (all properties of the embeddable class must map to the same table)"
	);
	+ "' has properties mapped to two different tables"
	+ " '" + table.getName() + "' and '" + getTable().getName() + "'"
	+ " (all properties of the embeddable class must map to the same table)"
	);