[hibernate-dev] Question..

[Steve Ebersole]

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.