Skip to content

Conversation

@dreab8
Copy link
Member

@dreab8 dreab8 commented Nov 7, 2024

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

the main change is to use Fetchable instead of the simple attribute name as key for FetchBuilders, this should fix the problem with subclasses having properties with the same name but different columns.


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.


* Locate an explicit fetch definition for the named fetchable
*/
FetchBuilder findFetchBuilder(String fetchableName);
FetchBuilder findFetchBuilder(Fetchable fetchable);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we consider this to be "API", but wanted to mention the backwards compatibility concern.

Copy link
Member

Choose a reason for hiding this comment

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

It seems like all these methods here are in fact user-facing, so maybe we should keep the string variants?

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 it's incredibly unlikely that any user code depends on DynamicFetchBuilderContainer since there are no SPIs which return or accept this type.

And therefore I don't think there's any need to worry about breaking it in a new major version.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@sebersole sebersole Nov 15, 2024

Choose a reason for hiding this comment

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

The work I've done @ #9257 addresses this specific concern.

However, a natural continuation there is that everything else in the org.hibernate.query.results package should really be an spi. I started down this path, but it affects a few things that are in the same boat imo. E.g. this has ties with the org.hibernate.query.namedpackage, which itself ought to be spi imo. I'm going to follow up with a separate pr that follows that rabbit hole so we can see the full impact.

@DavideD wrt ^^, as far as I could see this change would have a minimal impact on HR so wanted to get your take. HR has an implementation of the main contract there, but like I said I think the impact for HR is minimal.

Copy link
Member

@sebersole sebersole Nov 15, 2024

Choose a reason for hiding this comment

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

However, a natural continuation there is that everything else in the org.hibernate.query.results package should really be an spi

Well, unless we forsee actually adding some form of support for an application creating these directly. A sort of improved approach to NativeQuery#addReturn, etc. That was the original plan, but we've not done it and no one has asked for it so...

Copy link
Member

Choose a reason for hiding this comment

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

@DavideD wrt ^^, as far as I could see this change would have a minimal impact on HR so wanted to get your take. HR has an implementation of the main contract there, but like I said I think the impact for HR is minimal.

Yes, the impact should be minimal.

Copy link
Member

Choose a reason for hiding this comment

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

I can try tomorrow to update the code in Hibernate Reactive and make sure that there are not surprises.

@dreab8
Copy link
Member Author

dreab8 commented Nov 20, 2024

close if favour of #9291

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

5 participants