On Sun, Oct 15, 2017 at 10:46 AM, Steve Ebersole <st...@hibernate.org> wrote:
> On Fri, Oct 13, 2017, 3:35 PM Gail Badner <gbad...@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 <st...@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.
Correct.
>
> 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.
>
> 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
>
> short-term - how do we handle this for 5.x
> 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:
>
> correct the condition as discussed above
As I mention, I don't think correcting the condition is the right thing to do.
> slightly modified version of (C) - see below
> 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:
>
> hibernate.empty_composite_creation
>
> enable - enable creation & throw exceptions when we deem we cannot create
> them (condition check discussion). This is basically (A)
> warn - same as "enable", but warn when we cannot create them - basically (B)
> disable (default) - "opt out"
> strategy - consult the configured EmptyCompositeStrategy
>
> hibernate.empty_composite_strategy - names a `EmptyCompositeStrategy` to use
>
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, these would be options for hibernate.create_empty_composites.enabled:
true - enable creation & throw exceptions when we deem we cannot
create them (condition check discussion). This is basically (A)
warn - same as "enable", but warn when we cannot create them - basically (B)
false - disable (default) - "opt out"
strategy - consult the configured EmptyCompositeStrategy
>
> Additionally, I think it would be awesome to allow configuring (1) per role,
> e.g. (assuming Person#name as a composite):
>
> hibernate.empty_composite_creation = strategy
> 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.
>
I like this as well.
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
>
>
>
>>
>> > 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.
>
My example involves a nullable property that happens to be a composite ID.
Please take a look at the mapping in my example:
https://github.com/hibernate/hibernate-orm/pull/1993/commits/329354bfa4bd86190252f1d5a7019f8b06c21b17#diff-a16e0cd57dc44a1cabeed67869babacfR162
IMO, it's a really bad idea for Hibernate to assume, by default, that
initialized values in a newly instantiated embedded value would be
considered empty.
>
>
>>
>> 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'll have to look at NavigableRole to see what you mean. We are hoping
to backport to 5.1. I don't think I can make changes to
EntityPersister/CollectionPersister in 5.1, since we can't use default
methods.
>
> 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);
> }
I mentioned in my previous email that there was an extra argument and
corrected it there. With your change it should be:
> boolean isEmptyComposite(
> NavigableRole compositeRole,
> Object value,
> 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?
>
It is intended to determine if a particular composite value should be
considered empty. It will be needed by ComponentType#isEqual,
#compare, #isDirty, #isModified to determine if 2 values are both
considered empty.
In the case that an embeddable has an indeterminate sub-attribute, the
2 values could have a different sub-attribute value, but
EmptyCompositeStrategy#isEmptyComposite could ignore that
sub-attribute in its determination.
>
> [1]
> https://github.com/sebersole/hibernate-core/blob/wip/6.0/hibernate-core/src/main/java/org/hibernate/metamodel/model/domain/spi/EmbeddedTypeDescriptor.java
>
>
_______________________________________________
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
