NH-3925 - Entity with version not detected as transient when calling SaveOrUpdate and using custom Id generator class

[nhibernate-bot]

codekaizen created an issue — 8th December 2016, 0:03:31:

When an entity has a Version property and uses a custom Id generator which allows assigned ids to transient instances and SaveOrUpdate is called on the entity, a StaleObjectStateException exception is thrown during Session.Flush().

The reason for this is that the fix for NH-3505 short circuits the version check and only decides if the entity is transient in AbstractEntityPersister.IsTransient if it is using the built in "Assigned" generator. This breaks previously supported scenarios allowing assigned ids with custom generators.

---

codekaizen added a comment — 8th December 2016, 3:26:38:

This can be resolved by adding unsaved-value="undefined" to the id property in the mapping. It would be nice to have some validation for this if using a generator which permits assigned values, but that looks too complex for this corner case.

---

Oskar Berggren added a comment — 10th December 2016, 16:05:38:

Can you elaborate on the use case for such a generator?

I was going to suggest that perhaps there could be an interface "IAssigned" instead of checking the exact type, but then... it's not really that "the custom generator allows assigned ids" is it? Just to confirm, would "an entity type where some instances are added with an application-assigned identifier, while other instances should be assigned an identifier automatically using the configured identity generator" be a more accurate description? So it's not really about the generator being of any specific kind, more in how it's used? (And preferably about making sure automatic values cannot coincide with manually assigned values.)

If the latter description above is more accurate, then this really has nothing to do with custom id generators, but just about trying to assign some identifiers despite having an automatic identifier of any kind configured. Then it seems like the solution would be setting the unsaved-value in the mappings as you mention. Any idea for how to implement the "validation" you mention? I might be missing something but I don't see what parameters NH can look at to determine the suspicious case.

[fredericDelaporte]

NH-3505 has effectively broken a documented behavior:

if the object is versioned (version or timestamp), then the version will take precedence to identifier check, unless the versions unsaved-value="undefined"

With NH-3505 (3.2.0.GA), the id unsaved-value is always checked first.
(The shortcut for the Assigned case is a bug introduced by NH-1914 which has to be reverted. It breaks unsaved-value="none" just for "fixing" the case of an user having left an useless unsaved-value="0" in its mapping...)

There is no way to do NH-3505 otherwise I think.

I am for acknowledging the behavioral change it has introduced, and update the documentation. (I am in the process of reviewing all the mapping chapters, and I am documenting the current state, fixing this kind of discrepancies.)

The solution for the user with its custom generator is to explicitly map its identifier with unsaved-value="undefined". (The assigned generator already has it defaulting to undefined. This is hard-coded into NHibernate mapping logic, and it applies to the composite-id case too.)

[fredericDelaporte]

Reading again NH-3505, it seems it could have been implemented another way: since the trouble was related to proxies, why have they not just checked if the entity was an un-initialized proxy? An un-initialized proxy must be already persistent, so they could have added a shortcut before version check, yielding false if NHibernateUtil.IsInitialized(entity) is false.
(Moreover it would have handled the case of proxies of a versioned entity with undefined id unsaved value, which is certainly still broken with the current NH-3505 fix.)

But now the transient/persistent semantic is changed since quite long, and reverting back to previous behavior could be a breaking change for many, while the current semantic makes sens too.

After all, why should checking the version take precedence over checking the id by default? This was maybe a logic done for NH < 2.1.0, where unsaved-value="undefined" was not yet available on id. But now that it is supported, a user willing to have the check done on version rather than on id can ask it explicitly, with unsaved-value="undefined" on the id. Instead of requiring from users putting a version in their mapping to deactivate the check on version if they want it to stay on id.
(By the way, for assigned identifiers (including composite id), unsaved-value="undefined" is the default, so the version check still applies by default for them.)

[hazzik]

In some cases there is a check if something is a proxy before going to IsTransient method