Don't Detach() a document already loaded for update

[jtkech]

Fixes #6356

• First, when we get a single document for caching, if it has already been loaded for mutation we do a session detach but the loaded object may have been already flushed and if we save it again a new record is persisted, that's why here we just throw in that case. The opposite is okay, if we first get a single document for caching, we know that it will not be mutated and saved, so we can still detach it allowing to load after an isolated object for mutation.

• This is what happens in a migration recipe where all steps are executed in the same scope and that also uses a Content step. The definition record is first loaded for mutation by a previous migration and then the Content step does a get for caching. This ends up to a duplicate definition record, the first one being incomplete.

• So here the solution is to let as before all regular automatic migrations executed in the same activating scope, which make sense, and that the RecipeMigrator uses a DeferredTask so that the migration recipe will be executed after disposing the scope used to activate the shell and where all other regular migations has been done and committed.

  And each step of a migration recipe is now executed in a new scope (with a new parameter saying that the shell should not be activated as it is in progress) allowing to mix any kind of recipe steps, as with other regular recipes.

[jtkech]

I found a better and simpler way to fix it

So, when we get a document for caching that has already been loaded, currently we detach it before, the problem is that it may have been flushed so we can get and cache uncommitted data, and if we save it again a new record is persisted.

The simpler solution in that case is to just return the loaded document but indicating that it should not be cached. It is still thread safe as the loaded document is not put in a shared cache and then only used in a given scope.

Beautiful it's mainly 2 line of code ;)

The other changes are just because of the signature change of GetForCaching() that not only returns a document but also a bool indicating to the caller if it can be cached or not.

[jtkech]

For infos it fixes the related issue from @ArieGato

Very nice!! I tested a more complex scenario and all works well. I'll try to come up with some more edge cases, but I'm confident it works nice!

[sebastienros]

I think that the "caching" information should not leak at this level. It should always be either cacheable or not. Another solution is to change this method to two:

• CreateContentDefinitionRecord
• GetContentDefinitionRecord

And the user would check if Get returned null before calling Create. Right now it's more like a GetOrCreate() and the cachability of the result will vary.

[jtkech]

Okay, i'm open to any suggestions

That said, this is no more the case in the distributedCache branch, it is done at the IDocumentStore level and only checked at the IDocumentManager<> level, then any service using an IDocumentManager<> don't have to care about this.

/// <inheritdoc />
public Task<ContentDefinitionRecord> LoadContentDefinitionAsync() => _documentManager.GetMutableAsync();

/// <inheritdoc />
public Task<ContentDefinitionRecord> GetContentDefinitionAsync() => _documentManager.GetImmutableAsync();

It should always be either cacheable or not

Yes i agree but this is the singularity i described in my comments. In some rare cases the caller should not create a new instance but should not cache the returned one. But this only happens in a given scope where we can't refresh the cache for a document that has already been loaded for update and that may have been flushed.

it's more like a GetOrCreate() and the cachability of the result will vary.

Yes exactly, before it was already a GetOrCreate() with a factory, but, because of the above, even the result is not null it doesn't mean that it can be cached

I will think about it

[sebastienros]

More like GetOrCreate than Get. That would make the factory usage obvious.

[jtkech]

So here, in the distributedCache branch, it is done at the IDocumentStore (can be an IFileDocumentStore) level, same problem but cacheable is only checked at the IDocumentManager level.

More like GetOrCreate than Get. That would make the factory usage obvious.

Yes i agree, before the change it was already using a factory, would you mean GetOrCreateForCachingAsync? In the distributedCache branch the related method is GetImmutableAsync() that would become GetOrCreateImmutableAsync()

I will think about it, but again i'm open to any suggestions, let me know

[sebastienros]

I am merging but my comments still stand.