UseOptimisticConcurrency and UseNumericRevisions are asymmetric: call order decides whether the config works or throws

[jeremydmiller]

Summary

UseOptimisticConcurrency(true) and UseNumericRevisions(true) are not symmetric in how they clear the other flavor's metadata, so the order the two fluent calls are made in decides whether the configuration silently works or throws at bootstrap.

Behavior today (master, 77efabc0f)

// Order A — last-wins, works.
opts.Schema.For<Foo>().UseOptimisticConcurrency(true);
opts.Schema.For<Foo>().UseNumericRevisions(true);

// Order B — InvalidDocumentException at bootstrap.
opts.Schema.For<Foo>().UseNumericRevisions(true);
opts.Schema.For<Foo>().UseOptimisticConcurrency(true);

Root cause

MartenRegistry.DocumentMappingExpression<T> (src/Marten/MartenRegistry.cs:708-742):

public DocumentMappingExpression<T> UseOptimisticConcurrency(bool enabled)
{
    _builder.Alter = m =>
    {
        m.UseOptimisticConcurrency = enabled;
        if (enabled)
        {
            m.UseNumericRevisions = false;
            m.Metadata.Version.Enabled = true;
            // NOTE: does NOT clear m.Metadata.Revision.Enabled
        }
    };
    return this;
}

public DocumentMappingExpression<T> UseNumericRevisions(bool enabled)
{
    _builder.Alter = m =>
    {
        m.UseNumericRevisions = enabled;
        if (enabled)
        {
            m.UseOptimisticConcurrency = false;
            m.Metadata.Revision.Enabled = true;
            m.Metadata.Version.Enabled = false;   // <-- clears the other flavor
        }
    };
    return this;
}

UseNumericRevisions clears Metadata.Version.Enabled; UseOptimisticConcurrency never clears Metadata.Revision.Enabled. Both flavors then read as enabled, and #5121's new guard in DocumentMapping.CompileAndValidate throws.

Note that VersionedPolicy.Apply already reconciles the pair correctly:

if (mapping.UseOptimisticConcurrency) { Version.Enabled = true;  Revision.Enabled = false; }
if (mapping.UseNumericRevisions)      { Version.Enabled = false; Revision.Enabled = true;  }

So the invalid state is only reachable through a fluent Alter, which runs after the policies.

Why this matters beyond call ordering

The same asymmetry means UseOptimisticConcurrency(true) throws on any type that already has numeric revisions from a policy rather than from an explicit call — it implements IRevisioned/ILongVersioned, carries a [Version] member, or is an aggregate-projection target. In those cases nothing in the user's configuration says "numeric revisions", so the error reads as though Marten invented a conflict. (Covered by interface_driven_revisions_plus_optimistic_concurrency_fails_fast in #5157.)

Proposed direction

Two changes that compose:

1. Make UseOptimisticConcurrency(true) clear Metadata.Revision.Enabled, mirroring UseNumericRevisions. Both orders then become last-wins for ordinary documents, which is what the fluent API reads like it should do.
2. Narrow the Derive the StreamOne ETag from the numeric revision for revisioned documents #5121 guard so it fires on the case that genuinely cannot work rather than on "both flags happen to be set" — an aggregate-projection target being pushed onto Guid concurrency, detectable via StoreOptions.Projections.TryFindAggregate(DocumentType, out _). That is the actual Do not allow projected documents to be configured as requiring optimistic concurrency #2978 rule, and it keeps the actionable error where it belongs while letting plain documents override freely.

Worth deciding explicitly whether (1) should also apply to interface-driven revisions (IRevisioned + an explicit UseOptimisticConcurrency(true)), or whether that should keep failing fast — the interface's Version member would be left unmapped, so continuing to throw there is defensible.

Context

Not a regression: before #5121 both orders reached the database as DDL with two mt_version columns and failed with MartenSchemaException: DDL Execution for 'All Configured Changes' Failed!. Verified against master at de63fc5ed. #5121 turned that into an actionable InvalidDocumentException; this issue is about the underlying asymmetry it surfaced.

Raised while reviewing #5121, deliberately left out of #5157 because it changes document-configuration behavior.

[arnelirobles]

Had a look at implementing (1) and hit something the write-up does not mention: it also inverts a test merged in #5157 (77efabc0f), not just the interface-driven case.

optimistic_concurrency_on_projection_target_fails_fast currently asserts that Order B throws for a plain document:

[Fact]
public void switching_a_plain_document_from_numeric_revisions_to_optimistic_concurrency_also_fails_fast()
{
    StoreOptions(opts =>
    {
        opts.Schema.For<Target>().UseNumericRevisions(true);
        opts.Schema.For<Target>().UseOptimisticConcurrency(true);
    });

    Should.Throw<InvalidDocumentException>(
        () => theStore.Options.Storage.MappingFor(typeof(Target)));
}

That is exactly the "ordinary documents become last-wins" case, so (1) turns this test from passing to failing and it would have to be inverted rather than kept.

The other thing worth flagging: (1) cannot be scoped by looking at whether the revision metadata is member-mapped. ProjectionDocumentPolicy enables Metadata.Revision without ever assigning Metadata.Revision.Member (nor does any ConfigureAggregateMapping implementation), while VersionedPolicy does assign it for IRevisioned/ILongVersioned. So a Revision.Member == null heuristic would clear the revision for projection targets too and take use_optimistic_concurrency_on_projected_document_throws_invalid_document_exception with it. Preserving that guard needs (2) and its Projections.TryFindAggregate check, so (1) and (2) really do have to land together.

That leaves three current behaviours and a decision on each:

Case	Today	Under (1) + (2)
Plain document, both fluent calls	throws	last-wins, test inverted
Projection target + UseOptimisticConcurrency(true)	throws	still throws, via TryFindAggregate
IRevisioned + UseOptimisticConcurrency(true)	throws	open question in this issue

Happy to put up a PR for (1) + (2) with the plain-document test inverted and the other two guards kept, if that is the intent. Wanted to confirm the direction first rather than delete a test from last week on my own reading.