Re: [hibernate-dev] Question..

On Fri, Oct 13, 2017, 3:35 PM Gail Badner <gbadner(a)redhat.com&gt; wrote: 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. 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 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. 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 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/s...