On Tue, Oct 17, 2017 at 12:55 AM Gail Badner <gbad...@redhat.com> wrote:
> > > > WRT to the condition check, you suggest to "compare a primitive value in > a > > composite with the default for the primitive type (e.g, comparing an int > > value with 0 instead of null)". That's not the best condition check. I > was > > pointing out that we already have better handling for this for ids and > > versions and that we ought to follow that paradigm, mainly on startup we > > would instantiate an instance of said component and check the initial > values > > for its attributes. E.g., consider: > > > > @Embeddable > > public class MyEmbeddable { > > private int someInt; > > private int anotherInt = -1; > > } > > > > Here, comparison of `#anotherInt` to 0 is not really correct. The proper > > comparison is to -1. Again, this is exactly what we do for ids and > > versions. > > > > So because your (A) and (B) check the same condition I was pointing out > that > > they actually are based on the incorrect condition check (0 versus -1), > so > > we should correct that assumption. But even then we have and further > that > > we should update the check and that we still have the different outcome > > between (A) and (B) to decide between. > > I have already implemented something that does the check that you > suggested, and I decided against it for a couple of reasons: > 1) Any initialized values can be "real" values. > 2) Suppose an instantiated embedded value includes something like: > Date created = new Date(); > double random = Math.random(); > For 2), an embedded value would never be considered empty, and its > values would always be persisted instead of null. > I'm sorry but I completely disagree here. The Date one is interesting but ultimately can be handled somewhat easily. I'll discuss why/how below. And the "random" is completely a fabricated case I'm guessing. Can you show me a user reporting that as a real use case? Regardless I think you'll agree that this is very less-than-common use-case. And anyway, it too can be mitigated by the same handling I hinted at with Date. Consider: @Embeddable class MyEmbeddable { Date created = new Date(); double random = Math.random(); } ... final MyEmbeddable e1 = new MyEmbeddable(); final MyEmbeddable e2 = new MyEmbeddable(); if ( theCompositeType.isEqual( e1, e2 ) ) { // we can recognize and leverage initial values ... } else { // we can't (your date/random examples) // - this branch requires that either: // - a strategy is supplied, or // - creation of empty is disabled ... } These kind of decisions are all about covering common cases and then allowing config or SPI to cover the less-common ones. It boils down to what you think is more common: double someNumber = -1; or: double someNumber = Math.random(); Clearly initializing to a set (magic) number is far more common. Also, I think you have to ask yourself about the likelihood that such a component with its values initialized as such will ever be empty. At least for those specific values, I think it is HIGHLY unlikely. > I think that it should be up to the `EmptyCompositeStrategy` to decide > if a primitive that has the default value, or something like `created` > or `random` should be ignored when determining if an embedded value is > empty. > While I do not disagree that EmptyCompositeStrategy is the most flexible solution (which is why I included it as part of my "ideal" solution), the majority of users do not want flexibility when that flexibility requires writing some code to achieve it. They want something that JustWorks. Most will be ok with configuration settings as well. For the most part, it is the "power users" who are going to implement such an SPI. So while EmptyCompositeStrategy is certainly a good idea as part of a complete solution, it is just that - a part. I like what you describe above. > > Currently the property is named > `hibernate.create_empty_composites.enabled`. > > Are you suggesting we rename the property in 6.0+? For 5.x, I guess we'd have something like this to opt out: > > hibernate.create_empty_composites.enabled=strategy > hibernate.create_empty_composites.enabled.com.acme.Person.name=false > > Should we also allow to disable in general, then opt-in for Person.name, > as in: > > hibernate.create_empty_composites.enabled=false > hibernate.create_empty_composites.enabled.com.acme.Person.name=true > (this would fail like case (A) if the newly instantiated value has > initialized values) > > Also, allow a strategy to be used for the opted-in value: > > hibernate.create_empty_composites.enabled=false > hibernate.create_empty_composites.enabled.com.acme.Person.name=strategy > I would do what I described in the earlier email for 5.x. This is a case where the "long term solution" causes no API/SPI conflicts, and so is perfectly fine for the short term as well. And not only that, but the only SPI change here is what you already propose - adding a new SPI extension contract (EmptyCompositeStrategy). In other words, you are already proposing the only thing that would normally be a blocker to doing something in a maintenance branch - adding new settings is cake compared to that. The new settings have different names so there is no collision. We'll consider `hibernate.create_empty_composites.enabled` deprecated. We will use it when no `hibernate.create_empty_composites.enabled` has been specified to determine the value for `hibernate.empty_composite_creation` (just like we do for other deprecated settings). For existing apps, the old setting (`hibernate.create_empty_composites.enabled`) would continue to have the same effect; these apps would not have the new settings, so `hibernate.create_empty_composites.enabled` would define the default for `hibernate.empty_composite_creation`. BTW, I am thinking we may do a 5.3 specifically for the new JPA MR. So we could decide to do this work there rather than 5.2 - but honestly, adding new settings is perfectly fine for 5.2 as already discussed. _______________________________________________ hibernate-dev mailing list hibernate-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/hibernate-dev