[hibernate-dev] Question..

[Gail Badner]

Won't there be many breaking changes moving to 6.0 anyhow? I can see
keeping the signatures the same in 5.x, but I, personally, am not as
concerned about keeping them the same moving to 6.0. I would certainly
try to make the migration as painless as possible.

I'm on the fence about whether empty-creation configuration should
apply per embedded vs embeddable.

I'm going to create a gist to summarize what we've discussed, and we
can discuss further there.

On Tue, Oct 17, 2017 at 8:04 AM, Steve Ebersole <steve at hibernate.org> wrote:
> 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.
>>
>>