[hibernate-dev] Question..

[Steve Ebersole]

On Fri, Oct 13, 2017, 3:35 PM Gail Badner <gbadner at redhat.com> wrote:

> Hi Steve,
>
> I'm circling back to this. Please see below...
>
> On Sat, Sep 2, 2017 at 8:47 AM, Steve Ebersole <steve at hibernate.org>
> 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.
>

Your (A) and (B) are the exact same thing (same underlying condition check)
except in (A) you throw an exception and in (B) you log a warning and
disregard an explicit user setting.

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.

The downside I was trying to point out (and it is the same for issue for
some of the other parts of the discussion) is that we currently do not have
a singular place where we keep "embeddable" metadata - again, CompositeType
describes the embedded, not the embeddable.  But maybe it is ok to
duplicate this "instantiate and check" for each CompositeType.  A
alternative is to keep a mapping of this information as part of the
bootstrap data structures to help do that just once for every embedded of
that embeddable.

Further, in discussing these various options it is important to make the
distinction between the best solution in terms of

   1. short-term - how do we handle this for 5.x
   2. longer-term - how do we address this in 6.0+ because here we have
   some more flexibility.

Longer term, I ultimately think the following is the best solution:

   1. correct the condition as discussed above
   2. slightly modified version of (C) - see below
   3. slightly modified version of (D) - see below

The change I'd make to `EmptyCompositeStrategy` is specifically the
signatures and (slightly) the intent to work with (D).  Best case scenario,
again for 6.0+) I'd pass in
`org.hibernate.metamodel.model.domain.spi.EmbeddedTypeDescriptor` whose
intent is kind of, sort of the same as `ComponentMetamodel`[1].  Worst case
we'd pass in this `org.hibernate.metamodel.model.domain.NavigableRole` you
can see exposed here.  NavigableRoles can be resolved to their Navigable
via the TypeConfiguration (the Navigable here would be the
EmbeddedTypeDescriptor).  Additionally, these roles are nice in terms of
configuration as we will see below.

In terms of (D), I think configuration here is a good idea especially in
combination with (C).  In fact I can see the following settings:


   1. hibernate.empty_composite_creation
   1. enable - enable creation & throw exceptions when we deem we cannot
      create them (condition check discussion).  This is basically (A)
      2. warn - same as "enable", but warn when we cannot create them -
      basically (B)
      3. disable (default) - "opt out"
      4. strategy - consult the configured EmptyCompositeStrategy
   2. hibernate.empty_composite_strategy - names a `EmptyCompositeStrategy`
   to use


Additionally, I think it would be awesome to allow configuring (1) per
role, e.g. (assuming Person#name as a composite):

   1. hibernate.empty_composite_creation = strategy
   2.  hibernate.empty_composite_creation.com.acme.Person.name = disable

So for all composites we will use the configured strategy, except for
Person#name, which we have explicitly disabled / opted-out-of.





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

I'm not seeing where that first email suggests checking based on the actual
initialized value rather than the "primitive type default".  But if it did,
then yes that is a better check as discussed above.



Please see an example of the consequences when the embedded represents
> an ID:
> https://github.com/hibernate/hibernate-orm/pull/1993/commits/329354bfa4bd86190252f1d5a7019f8b06c21b17


Ids should be excluded from this completely.  Hibernate does not support
any part of a PK to be nullable - ergo this entire discussion is completely
irrelevant for composite PKs.




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

As I said in my initial reply and above, I am not a fan of (B) - I think it
is, generally speaking, a bad option to ignore settings that the user has
explicitly set.  But again, we need to keep short/long term in mind because
short-term I think every other option has some significant drawbacks.

And long-term it comes down to what we allow for configuration.  E.g. if we
do not allow the full breath of config options I mentioned, then that
changes things.



I think C is a good starting point in H5, preferably using role (if
> that can be done).
>

For 5.x, (C) may be the best option aside from the argument discussion.
Like I said, in terms of 6 the best argument to pass would be
EmbeddedTypeDescriptor.  Or, depending on the timing (boot model versus
runtime model) `org.hibernate.boot.model.domain.EmbeddedMapping` /
`org.hibernate.boot.model.domain.EmbeddedValueMapping`.

We *could* pass in this role as a String, but I really hate passing
Strings.  Another option would be to back-port NavigableRole and expose
these from EntityPersister/EntityType, CollectionPersister/CollectionType
and ComponentType.

I'm trying to get something that will work in both 5 and 6 to minimize
changes for users as they migrate.  Assuming we back-port NavigableRole,
something like:


public interface EmptyCompositeStrategy {
    /**
     * Should a composite/embeddable be instantiated all of
     * its composed attribute values are null?
     */
    boolean supportsEmptyComposite(NavigableRole compositeRole);

    /**
     * Is the given composite value considered "empty"?
     */
    boolean isEmptyComposite(
            NavigableRole compositeRole,
            Object value,
            Object component,
            SessionFactoryImplementor factory);
}

Although, tbh I am not really understanding the intent of your second
method.  Are you asking the strategy to check each individual sub-attribute
for emptiness?  Also, are you thinking this gets called when reading the
values?  Or when writing the values?


[1]
https://github.com/sebersole/hibernate-core/blob/wip/6.0/hibernate-core/src/main/java/org/hibernate/metamodel/model/domain/spi/EmbeddedTypeDescriptor.java