Hi Steve, I'm circling back to this. Please see below... On Sat, Sep 2, 2017 at 8:47 AM, Steve Ebersole <steve(a)hibernate.org&gt; wrote: > I don't have the original email to reply to. So I'll reply here. > > Overall, I had not really considered the primitive attribute case, but yeah > that's clearly an issue. My mistake. > > A) I agree that the con here is huge. Not the best option > > B) Is close to better. We could certainly check this and throw an error. Throwing an exception is option A.

> A better logical option is to do similar to what we do for ids and versions... > on startup instantiate one of these and grab/store its initial state when > freshly instantiated. We can later use those values to perform the empty > check. This is more logical, but not sure how "practical" it is given that > we do not really have a good place to keep this relative to an embeddable, > nor relative to an embedded, aside from CompositeType, but that does not > feel right. Is this what you refer to as (B) (expanded)?

My first email in the thread (which I told you to disregard) basically suggests doing this. I told you to disregard it because I realized that it would have serious issues.

an ID: https://github.com/hibernate/hibernate-orm/pull/1993/commits/329354bfa4bd...

If I'm understanding what you mean by (B) (expanded), then I don't think that's is a good idea.

that can be done).

> > 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 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.

Currently the property is named `hibernate.create_empty_composites.enabled`. Are you suggesting we rename the property in 6.0+?

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

My one concern with doing this in 5.x is that we'd almost certainly change the signature of EmptyCompositeStrategy as we transition to 6. 6.0 has a real encapsulation of the embeddable in addition to the embedded. 5.x only really keeps the embedded (CompositeType). Anotger aspect that we have not decided is whether this empty-creation applies to the embeddable or the embedded. The checks would all be embeddable specific - the state initialization is part of the embeddable (the Class) so it would not change between embedded usages. We would have to decide if it makes sense to allow different empty-creation decisions for each embedded usage of that embeddable. On Tue, Oct 17, 2017, 9:42 AM Steve Ebersole <steve(a)hibernate.org&gt; wrote: > > On Tue, Oct 17, 2017 at 12:55 AM Gail Badner <gbadner(a)redhat.com&gt; 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. > >