Improve consistency of how we call implicitNamingStrategy.determineBasicColumnName with element collections

Description

This is a followup of and a proposal to fix .

In OGM, our implicit naming strategy rely on the fact that ImplicitNamingStrategy#determineBasicColumnName is called with a propertyName containing collection&&element. for collection elements.

This was the case before ORM 5, but following HHH-6005, the behavior of Ejb3Column has changed in this regard and ImplicitNamingStrategy#determineBasicColumnName is sometimes called with a propertyName cleaned up from collection&&element. and sometimes with the full propertyName containing collection&&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
-> determineBasicColumnName is called with items.name

while in https://github.com/gsmet/hibernate-orm/blob/master/hibernate-core/src/main/java/org/hibernate/cfg/Ejb3Column.java#L375
-> determineBasicColumnName is called with the full propertyName items.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 to ImplicitNamingStrategy#determineBasicColumnName and restore the old behavior in this regard.

It doesn't break the expectations of people having implemented their own implicit naming strategy as currently determineBasicColumnName is already sometimes called with collection&&element. in the property name.

What I propose is to change the code in Ejb3Column to always call ImplicitNamingStrategy#determineBasicColumnName with "collection&&element." included in the propertyName and only remove it before passing it to the physical strategy.

This would allow the calls to ImplicitNamingStrategy#determineBasicColumnName to 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.

Environment

None

Activity

Show:
Emmanuel Bernard
June 21, 2016, 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.

Gunnar Morling
June 23, 2016, 7:49 AM

Is such flag really needed though? As per 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.

Gunnar Morling
June 23, 2016, 8:02 AM
Edited

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.

Steve Ebersole
June 29, 2016, 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?

Steve Ebersole
June 30, 2016, 4:12 PM

Applied to master (5.2) and 5.1 branches

Assignee

Guillaume Smet

Reporter

Guillaume Smet

Fix versions

Labels

None

backPortable

None

Suitable for new contributors

None

Requires Release Note

None

Pull Request

None

backportDecision

None

Components

Affects versions

Priority

Major
Configure