[hibernate-dev] Question..

[Steve Ebersole]

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 at hibernate.org> wrote:

> On Tue, Oct 17, 2017 at 12:55 AM Gail Badner <gbadner at 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.
>
>
>