Skip to content

HHH-19036 Process all filters before going through classes #9930

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

Conversation

marko-bekhta
Copy link
Member

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

This already works fine in 7.0 since the switch to models and having this:

public void processFilterDefinitions() {
final Map<String, FilterDefRegistration> filterDefRegistrations = domainModelSource.getGlobalRegistrations().getFilterDefRegistrations();
for ( Map.Entry<String, FilterDefRegistration> filterDefRegistrationEntry : filterDefRegistrations.entrySet() ) {
final FilterDefRegistration filterDefRegistration = filterDefRegistrationEntry.getValue();
final FilterDefinition filterDefinition = filterDefRegistration.toFilterDefinition( rootMetadataBuildingContext );
rootMetadataBuildingContext.getMetadataCollector().addFilterDefinition( filterDefinition );
}
}

I did consider doing the pass over classes in the processFilterDefinitions method as well for 6.6, but it feels a bit redundant to generate the orderedClasses = orderAndFillHierarchy( xClasses ) two times, or alternatively going through xClasses + their super types... so this seems like a good compromise, no ?


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.


Copy link
Member

@mbladel mbladel left a comment

Choose a reason for hiding this comment

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

This already works fine in 7.0 since the switch to models

Would be great to add the same test there, so we can ensure no regression are introduced in the future.

I did consider doing the pass over classes in the processFilterDefinitions method as well for 6.6, but it feels a bit redundant to generate the orderedClasses = orderAndFillHierarchy( xClasses ) two times

We could cache the orderedClasses in a local property, and initialize them lazily the first time they're needed:

private List<XClass> getOrderedClasses() {
	if ( orderedClasses == null ) {
		orderedClasses = orderAndFillHierarchy( xClasses );
	}
	return orderedClasses;
}

You're approach looks fine anyway, this would just be a way of making processFilterDefinitions actually do the thing 😄

@marko-bekhta marko-bekhta force-pushed the fix/HHH-19036-find-filters-before-binding-classes branch from 34110f5 to 48634d3 Compare March 31, 2025 09:56
@marko-bekhta marko-bekhta force-pushed the fix/HHH-19036-find-filters-before-binding-classes branch from 48634d3 to 3e192eb Compare March 31, 2025 09:56
@marko-bekhta
Copy link
Member Author

added a test for main here #9943

@beikov beikov merged commit 37ee87a into hibernate:6.6 Apr 11, 2025
31 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants