Improve consistency of how we call implicitNamingStrategy.determineBasicColumnName with element collections
Description
causes
follows up on
Activity
Steve EbersoleJune 30, 2016 at 4:12 PM
Applied to master (5.2) and 5.1 branches
Steve EbersoleJune 29, 2016 at 3:31 PM
I think I agree with Gunnar about AttributePath. I think that is a nicer solution.
Longer term I do see further changes to ImplicitNamingStrategy, especially in terms of how they are called. Personally I'd like to put off those changes until jandex-binding work and do it all at once. Maybe we should start a ImplicitNamingStrategy design wiki?
Gunnar MorlingJune 23, 2016 at 8:02 AMEdited
Taking a step back, what would also work is that "collection&&element" is always filtered prior to invoking ImplicitNamingStrategy (and not only in some invocations as today) and then expose the information whether that path represents a collection element via a separate member on the passed AttributePath. Much nicer than String fiddling on the implementor side.
For the sake of compatibility, we could also leave the magic value in there where it's passed today and pass that info additionally via that new member on AttributePath. In 6.0, we'd remove any passing of the magic String value to implicit naming strategies.
Gunnar MorlingJune 23, 2016 at 7:49 AM
Is such flag really needed though? As per @Guillaume Smet description, implementors of ImplicitNamingStrategy were already facing the weird magic value "collection&&element" in some cases, so it seems just logical to me to make this consistent and do all invocations of that method in the same way.
But if we cannot agree on that, +1 for having this hidden option. Also +1 for finding a saner approach than passing this magic value in the loger term.
Emmanuel BernardJune 21, 2016 at 2:06 PM
One smoother approach is to hie this change behind a (non documented) flag that OGM could enable. This can give time to ensure proper compatibility before enabling this as the default.
This is a followup of https://hibernate.atlassian.net/browse/HHH-6005#icft=HHH-6005 and a proposal to fix https://hibernate.atlassian.net/browse/OGM-893#icft=OGM-893 .
In OGM, our implicit naming strategy rely on the fact that
ImplicitNamingStrategy#determineBasicColumnNameis called with a propertyName containingcollection&&element.for collection elements.This was the case before ORM 5, but following HHH-6005, the behavior of
Ejb3Columnhas changed in this regard andImplicitNamingStrategy#determineBasicColumnNameis sometimes called with a propertyName cleaned up fromcollection&&element.and sometimes with the full propertyName containingcollection&&element..Let's admit, we have the following propertyName
items.collection&&element.name.in https://github.com/gsmet/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/cfg/Ejb3Column.java#L272
->
determineBasicColumnNameis called withitems.namewhile in https://github.com/gsmet/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/cfg/Ejb3Column.java#L375
->
determineBasicColumnNameis called with the full propertyNameitems.collection&&element.name.While we should not expose
collection&&element.to the physical naming strategy as we don't want it to be included in the column name, I think we should be consistent regarding the parameter we pass toImplicitNamingStrategy#determineBasicColumnNameand restore the old behavior in this regard.It doesn't break the expectations of people having implemented their own implicit naming strategy as currently
determineBasicColumnNameis already sometimes called withcollection&&element.in the property name.What I propose is to change the code in
Ejb3Columnto always callImplicitNamingStrategy#determineBasicColumnNamewith "collection&&element." included in the propertyName and only remove it before passing it to the physical strategy.This would allow the calls to
ImplicitNamingStrategy#determineBasicColumnNameto be consistent and would fix the aforementioned OGM bug which has been silently introduced when we migrated to ORM 5 (and yes, we will add a unit test in OGM).It would be nice if it could be backported also to 5.1 as OGM will move to 5.1 soon and we would like to fix this bug sooner rather than later.