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]

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]

the full test suit passes, and i also make this work:

@Embeddable
public record Person(
	FullName fullName,
	@Column(table = "Person")
	Integer age) {

}

@Embeddable
public record FullName(
	@Column(table = "Person")
	String firstName,
	@Column(table = "Person")
	String lastName) {

}

[itsmoonrack]

while this work, i feel i should find a way to pass columns directly, the solution proposed here is actually messy.

My idea is - just like the AnnotatedJoinColumns works - create a "virtual column parent AnnotatedColumns for the component. That way the checkPropertyConsistency will do its work

[itsmoonrack]

I have changed the approach completely using my idea but improve to check on the fly and i am pretty happy with the results now.

	@Entity
	@Table(name = "UserEntity")
	@SecondaryTable(name = "Person")
	public static class UserEntity {
		@Id
		@GeneratedValue
		private Integer id;
		@AttributeOverride(name = "fullName.firstName", column = @Column(table = "Person"))
		@AttributeOverride(name = "fullName.lastName", column = @Column(table = "Person"))
		@AttributeOverride(name = "age", column = @Column(table = "Person"))
		private Person person;

		public UserEntity(
				final Person person) {
			this.person = person;
		}

		protected UserEntity() {

		}
	}

	@Embeddable
	public record Person(
			FullName fullName,
			Integer age) {

	}

	@Embeddable
	public record FullName(
			String firstName,
			String lastName) {

	}

Checked that every combination of mapping works, and when remove one override or one mapping for 1 or 2 fields, it fails as excepted.

[itsmoonrack]

@gavinking, @mbellade, its in your hands now :)

[itsmoonrack]

@beikov you were right, I found out that the last version did not threw an exception when the columns did not matched. its now resolved

[beikov]

Can you please add a negative test similar to org.hibernate.orm.test.mapping.collections.SortAndOrderTests that verifies some mapping variants that use multiple tables in an embeddable really fail?

[gavinking]

Would you mind reverting this whitespace change (yes, I know IntelliJ makes this change since a recent release, but I'm not keen on it, and I wish there was a way to revert to the old behavior to stay consistent with the existing code.

[itsmoonrack]

@gavinking which whitespace ? in the EmbeddableBinder ?

those ?

binder.setEmbedded( isComponentEmbedded );
		binder.setHolder( propertyHolder );
		binder.setId( isId );
		binder.setEntityBinder( entityBinder );
		binder.setInheritanceStatePerClass( inheritanceStatePerClass );
		binder.setBuildingContext( context );

[gavinking]

No, look at the quoted lines, which removed two tabs.

[itsmoonrack]

@gavinking its actually an optical illusion, the AnnotatedException is threw twice here (i could not really find a better way of doing this) and git says there is a change in whitespace but if you pull the PR there is actually none.

For the squash, I create a new PR based on main correct ? And close this one ? Or there is a way to squash inside this branch ?

[itsmoonrack]

I created the new PR here: #10447

[gavinking]

This looks OK now, based on a superficial review.

@itsmoonrack Would you please squash the commits and maybe fix up that minor whitespace change at the same time.

[beikov]

Can't you just use component.getColumns() instead?