hibernate-orm icon indicating copy to clipboard operation
hibernate-orm copied to clipboard

add a method for use by quarkus extension

Open gavinking opened this issue 1 year ago • 7 comments

[Please describe here what your change is about]


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.


gavinking avatar Aug 14 '24 20:08 gavinking

Thanks for your pull request!

This pull request does not follow the contribution rules. Could you have a look?

❌ All commit messages should start with a JIRA issue key matching pattern HHH-\d+     ↳ Offending commits: [a7043419d4cc3407f0561b09313a1cf01ca65415, 6969e647775ef9d4555c2008a9b40d689fc6df20]

› This message was automatically generated.

@yrodiere @Sanne This is really, really fragile.

Would it be better to add a clone() method to MetadataImpl and have Quarkus call that instead?

gavinking avatar Aug 14 '24 20:08 gavinking

Would it be better to add a clone() method to MetadataImpl and have Quarkus call that instead?

Looks like it should be called trim().

WDYT?

gavinking avatar Aug 14 '24 20:08 gavinking

As discussed over Zulip, this is just one of many places where the integration between Quarkus and Hibernate ORM is very tight, to the point of being fragile, and that's the reason we only support running Quarkus with the exact version of Hibernate ORM it references in its BOM.

We can merge this if you want, but IMO it only makes sense if we also add a test to check that trim does what Quarkus expects (which is not immediately clear to me here, I'd have to dig into Quarkus code to remember -- might be worth a comment?).

yrodiere avatar Aug 26 '24 11:08 yrodiere

Also FWIW MetadataImpl is internal, so unless you move trim to some SPI, there's a decent chance it'll just get removed in a later commit by someone not aware of what it's for.

yrodiere avatar Aug 26 '24 11:08 yrodiere

Also FWIW MetadataImpl is internal, so unless you move trim to some SPI, there's a decent chance it'll just get removed in a later commit by someone not aware of what it's for.

Which is why I added a comment.

The main issue is that the constructor that Quarkus was calling was not even commented as such.

gavinking avatar Aug 26 '24 12:08 gavinking

Also FWIW MetadataImpl is internal, so unless you move trim to some SPI, there's a decent chance it'll just get removed in a later commit by someone not aware of what it's for.

Which is why I added a comment.

Fair enough.

LGTM, though I think changes that impact code we ship need a Jira issue, so we'll need to add that before merging.

yrodiere avatar Aug 26 '24 12:08 yrodiere