Sorry I could not look at it until now. I looked at what's on master and it looks accurate to me. I know you find the logic spread over too much. But the way it is handled is consistent and in the same spirit of similar-ish concepts (AttributeOverride, PropertyHolder, property navigation etc).
Emmanuel On 6 sept. 2013, at 05:35, Steve Ebersole wrote: > https://github.com/hibernate/hibernate-orm/pull/591 > > > On Thu 05 Sep 2013 10:06:14 PM CDT, Shaozhuang Liu wrote: >> I can give it a try >> ------------------------- >> Best Regards, >> >> Strong Liu <stliu at hibernate.org> >> http://about.me/stliu/bio >> >> On 2013Sep 6, at 8:15 AM, Steve Ebersole <st...@hibernate.org> wrote: >> >>> I have something mostly working and not causing regressions. Its quite >>> fugly, but I blame that on binders and hcann :) >>> >>> I'd really like to do a pull request for this one and have y'all take a >>> look. But at the same time I'd also really like to do the 4.3 Beta4 >>> release tomorrow. Anyone familiar with those bits of code willing to look >>> this over in the next 12 hours if I go through the steps of creating the PR? >>> >>> >>> On 09/05/2013 12:10 PM, Steve Ebersole wrote: >>>> On Thu 05 Sep 2013 11:52:22 AM CDT, Shaozhuang Liu wrote: >>>>>> Also, I am not sure that iterating properties yet again is a great idea. >>>>>> >>>>>> One alternative I had considered was to hook this in with >>>>>> PropertyBinder, where it calls SimpleValueBinder. That needs to happen >>>>>> anyway; the plan was to delegate the call to determine the Convert to >>>>>> use PropertyHolder and to pass that Convert into SimpleValueBinder. >>>>>> That code would need to pass in the XProperty anyway. So currently >>>>>> PropertyBinder does: >>>>>> >>>>>> simpleValueBinder.setType( property, returnedClass, containerClassName ); >>>>>> >>>>>> My plan was to instead have it do: >>>>>> >>>>>> simpleValueBinder.setType( property, returnedClass, containerClassName, >>>>>> holder.resolveAttributeConverter( property ) ); >>>>>> >>>>>> I *think* that when resolveAttributeConverter() would be called, we'd >>>>>> have enough info to fully resolve the converter to use properly. >>>>>> >>>>>> There are similar concerns in ComponentPropertyHolder, but maybe a >>>>>> discussion of the above will shed light on those concerns too. >>>>> >>>>> doesn't this work? >>>> >>>> What exactly? >>>> >>>> >>>>> at this time, the PropertyHolder is resolved, so it knows the convert it >>>>> holds, >>>>> then the convert defined on the attribute get applied first, then the one >>>>> in PropertyHolder >>>> >>>> I am not sure what you mean by "resolved" here. At no time that is useful >>>> in this process does PropertyHolder have full access to all the properties >>>> the thing holds. >>>> >>>> If by "the convert it holds" you mean the converts specifically defined on >>>> the @Entity or @Embedded (and therefore @Embeddable), then yes. >>>> >>>> But "then the convert defined on the attribute get applied first" is >>>> actually wrong for composite paths. For composites, you actually want the >>>> one higher up to have higher precedence. In the Person.homeAddress.city >>>> example, a convert defined on Address#city should actually have the >>>> *lowest* precedence. >>>> >>>> I think what we'll have to end up doing is to not normalize the composite >>>> paths to one place unfortunately. Then we'd have to pass the XProperty >>>> for which we are trying to resolve the converter to use into the >>>> PropertyHolder method. For composite paths this will just have to mean >>>> walking multiple levels (one per path-part). The down side to this is >>>> that at no time do we have an overview of the overall converts for a >>>> property path (aside from walking the composite path to actually resolve >>>> the converter to use). >>>> >>>> >>>>> >>>>> any problem with this approach ? >>>>> >>>>>> >>>>>> >>>>>> >>>>>> On Wed 04 Sep 2013 02:44:40 PM CDT, Steve Ebersole wrote: >>>>>>> >>>>>>> I am still a bit confused on how to apply the normalization to make >>>>>>> sure it happens in the proper order. >>>>>>> >>>>>>> Let's look at: >>>>>>> >>>>>>> @Entity >>>>>>> class Person { >>>>>>> ... >>>>>>> @Embedded >>>>>>> @Convert( attributeName="city", converter=Converter2.class ) >>>>>>> public Address homeAddress; >>>>>>> } >>>>>>> >>>>>>> @Embeddable >>>>>>> class Address { >>>>>>> ... >>>>>>> @Convert( converter=Converter1.class ) >>>>>>> public String city; >>>>>>> } >>>>>>> >>>>>>> >>>>>>> So here, the Converter2 class ought to be the one applied to >>>>>>> "homeAddress.city" path. >>>>>>> >>>>>>> Now granted there are a few different ways to skin this cat, but the >>>>>>> plan I had was to normalize these all on the root of the path. So >>>>>>> here, both of these conversions would get stored on >>>>>>> ClassPropertyHolder<Person> under the "homeAddress.city" path. When I >>>>>>> go to build the SimpleValue for Person.homeAddress.city, I'd ask the >>>>>>> CompositePropertyHolder for Person.homeAddress to resolve the explicit >>>>>>> (non-autoApply) conversion for its city attribute (the simple value). >>>>>>> The trouble I have though is applying the conversions in the proper >>>>>>> order. For example, here, I'd want to apply the conversions defined >>>>>>> directly on the Embeddable first (the Embeddable conversions should >>>>>>> always act as the baseline), then the conversions at its Embedded >>>>>>> point (via XML or annotations). >>>>>>> >>>>>>> One idea was to hook into org.hibernate.cfg.PropertyHolder#addProperty >>>>>>> in terms of normalizing the paths. I am just not sure of the timing >>>>>>> between these PropertyHolder#addProperty (and how populated the passed >>>>>>> Property objects are at that point) and the calls to >>>>>>> SimpleValueBinder/PropertyBinder. >>>>>>> >>>>>>> Interestingly, I still am not sure we have enough here to report an >>>>>>> error in cases like: >>>>>>> >>>>>>> @Entity >>>>>>> @Convert( attributeName="homeAddress.city", converter=Converter3.class ) >>>>>>> class Person { >>>>>>> ... >>>>>>> @Embedded >>>>>>> @Convert( attributeName="city", converter=Converter2.class ) >>>>>>> public Address homeAddress; >>>>>>> } >>>>>>> >>>>>>> Unless we somehow kept "proximity info" or "location info" about the >>>>>>> conversion in PropertyHolder. >>>>>>> >>>>>>> >>>>>>> On Wed 04 Sep 2013 01:27:21 AM CDT, Emmanuel Bernard wrote: >>>>>>>> >>>>>>>> >>>>>>>> On Tue 2013-09-03 17:22, Steve Ebersole wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> 2.a) It seems like there are times when >>>>>>>>> org.hibernate.cfg.AbstractPropertyHolder#parent would be useful for >>>>>>>>> what I need to do. But there appears to be times when this is null. >>>>>>>>> For entity mappings (ClassPropertyHolder) thats fine. But for the >>>>>>>>> others, that would be problematic. Going back to the >>>>>>>>> Person#homeAddress example, I'd really need the >>>>>>>>> ComponentPropertyHolder for Person#homeAddress to register the >>>>>>>>> converts with ClassPropertyHolder<Person> under the "homeAddress" >>>>>>>>> base key ("homeAddress.city" for example). Is there a time here >>>>>>>>> where AbstractPropertyHolder#parent would be null for >>>>>>>>> ComponentPropertyHolder/CollectionPropertyHolder? >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> I looked around the code base and the only case I could find is for >>>>>>>> ClassPropertyHolder whose parent is indeed null. >>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> 2.b) Is this AbstractPropertyHolder#parent the best way you see to >>>>>>>>> handle path-based converts? Or do you see a better option? >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Yes I still think it's the best aproach and frankly I don't quite see >>>>>>>> alternatives. >>>>>> _______________________________________________ >>>>>> hibernate-dev mailing list >>>>>> hibernate-dev@lists.jboss.org >>>>>> https://lists.jboss.org/mailman/listinfo/hibernate-dev >>>>> >>> >> _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev
