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 <st...@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 <st...@hibernate.org> wrote:
>>
>> On Tue, Oct 17, 2017 at 12:55 AM Gail Badner <gbad...@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.
>>
>>
_______________________________________________
hibernate-dev mailing list
hibernate-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev
