Question : DocumentStore implementation

[TFleury]

@jtkech Is there a reason to prevent registering multiple AfterCommitSuccess/AfterCommitFailure handlers on the same document type ?
Probably to prevent registering the same handler multiple times, but isn't this the service responsibility to do that ?
It could lead to unattended behavior if different services try to register an handler on the same document type.

OrchardCore/src/OrchardCore/OrchardCore.Data.YesSql/Documents/DocumentStore.cs

Lines 112 to 129 in 231cb19

	public void AfterCommitSuccess<T>(DocumentStoreCommitSuccessDelegate afterCommitSuccess)
	{
	if (!_afterCommitsSuccess.Contains(typeof(T)))
	{
	_afterCommitsSuccess.Add(typeof(T));
	_afterCommitSuccess += afterCommitSuccess;
	}
	}
	/// <inheritdoc />
	public void AfterCommitFailure<T>(DocumentStoreCommitFailureDelegate afterCommitFailure)
	{
	if (!_afterCommitsFailure.Contains(typeof(T)))
	{
	_afterCommitsFailure.Add(typeof(T));
	_afterCommitFailure += afterCommitFailure;
	}
	}

[jtkech]

Yes it is used internally by the document manager, e.g. to update the shared cache if the session was committed successfully, but only once even if the UpdateAsync() was called multiple times. We can also pass an afterUpdateAsync delegate but also only one will be executed and here after the cache update.

But yes, we could have allowed to pass distinct but multiple delegates for a given document type.

Hmm, look at VolatileDocumentManager.UpdateAtomicAsync() here we allow to register multiple updateAsync (before cache update) and afterUpdateAsync delegates per document type. To do that I needed to deal with the delegate.Target to differentiate delegates for a given document type.

Open to any suggestion ;)

[TFleury]

OK, it seems quite complicated to retain if a handler is already registered at the service level as service (i.e. DocumentManager) can be singleton and DocumentStore is scoped. There can be failure or cancel...

But things can be quite simpler like this :

public void AfterCommitSuccess<T>(DocumentStoreCommitSuccessDelegate afterCommitSuccess)
{
    if (!_afterCommitSuccess?.GetInvocationList().Contains(afterCommitSuccess) ?? true)
    {
        _afterCommitSuccess += afterCommitSuccess;
    }
}

/// <inheritdoc />
public void AfterCommitFailure<T>(DocumentStoreCommitFailureDelegate afterCommitFailure)
{
    if (!_afterCommitFailure?.GetInvocationList().Contains(afterCommitFailure) ?? true)
    {
        _afterCommitFailure += afterCommitFailure;
    }
}

It removes the need of _afterCommitsSuccess & _afterCommitsFailure lists.
And I think it can simplify things in VolatileDocumentManager

I can make a PR if you think it's correct.

[TFleury]

Is there a reason for AfterCommitSuccess & AfterCommitFailure to be generic as the generic type argument is not used ?

[jtkech]

But things can be quite simpler like this

This is what we first used in UpdateAtomicAsync() but it doesn't work, when the same delegate is enlisted this is still a distinct wrapped object that Contains() doesn't retrieve, see #12545.

Yes, at first IDocumentManager services were scoped services but we made them singletons to reduce the number of scoped services (for perf), but their methods are still intended to be executed in a scope context from which e.g. the default IDocumentStore (and then ISession) may be resolved on the fly.

Is there a reason for AfterCommitSuccess & AfterCommitFailure to be generic as the generic type argument is not used ?

To allow multiple delegates to be registered but still one per document type. Maybe not needed if we check delegate.Target (need to be tried) but I think better for the lookup to still enlist per document type.