MiniLcmApiValidationWrapper.CreateEntry skips entry validation

[myieye]

Summary

Entry validation is silently not run for CreateEntry calls made through the user-facing MiniLcm wrapper stack. UpdateEntry, CreateSense, etc. are unaffected.

Verified behavior

Creating an invalid Entry (e.g. with DeletedAt set, which EntryValidator requires to be null) through a validation-wrapped api does not throw ValidationException:

var validatedApi = sp.GetRequiredService<MiniLcmApiValidationWrapperFactory>().Create(api);
var invalid = new Entry { Id = Guid.NewGuid(), LexemeForm = { { "en", "test" } }, DeletedAt = DateTimeOffset.UtcNow };
await validatedApi.CreateEntry(invalid, ...);   // expected: throws; actual: succeeds

Likely cause (not fully confirmed)

MiniLcmApiValidationWrapper declares a hand-written one-arg override:

public async Task<Entry> CreateEntry(Entry entry)              // validates, then _api.CreateEntry(entry)

but the interface member is two-arg CreateEntry(Entry, CreateEntryOptions? = null). The [BeaKona.AutoInterface(MemberMatch = Any)] generator appears to emit a two-arg forwarder (_api.CreateEntry(entry, options)) with no ValidateAndThrow call, and all interface dispatch hits that forwarder — so the validating one-arg method is never reached. The sibling wrappers (MiniLcmApiWriteNormalizationWrapper, MiniLcmApiNotifyWrapper) use the two-arg signature and forward correctly.

Suggested fix

Give the override the full signature so it both validates and forwards options (matching the siblings):

public async Task<Entry> CreateEntry(Entry entry, CreateEntryOptions? options = null)
{
    await validators.ValidateAndThrow(entry);
    return await _api.CreateEntry(entry, options);
}

Caution

Enabling validation may begin rejecting entries that were previously accepted via this path — the fix needs its own review and a test run across CreateEntry callers (interactive create, GraphQL/route, hub, JS-invokable).

Discovered during review of #2341 (the Publication.IsMain work); pre-existing and unrelated, so filed separately rather than fixed there.

[myieye]

Worth zooming out: the reason this slipped through is that our *TestsBase classes and fixtures run against a normalization-only wrapper stack, whereas production wraps with validation too (MiniLcmApiUserFacingWrappers = read-norm → validation → write-norm). No conformance test ever exercises the validation layer, so a wrapper that quietly skips validation looks fine in CI. The high-value follow-up is to point the tests at the real production stack — likely its own PR.

Wins

• MiniLcmTestBase.Api is a single chokepoint: ~17 conformance base classes (CreateEntry, Publications, WritingSystem, Sense, …) flow through it, for both the CRDT and FwData implementations. Swapping its hand-listed [read-norm, write-norm] for MiniLcmApiUserFacingWrappers.Apply(...) makes every test mirror production in one edit, and auto-tracks future wrapper changes instead of drifting.
• It closes the whole class of "wrapper silently drops behavior" bugs. Today an "invalid input is rejected" test would pass against the unvalidated stack regardless; once tests go through validation, such a test actually means something — and the fix for this bug should add exactly one such regression test through the wrapped Api (not the raw api), or the blind spot stays.
• No new plumbing for the bypass cases: BaseApi (raw, unwrapped) already sits beside Api. The tests that must skip validation — convergence tests driving DataModel directly, impl-specific casts like (FwDataMiniLcmApi)BaseApi, deliberately-invalid staging — already use BaseApi. So it's opt-out-where-needed, not opt-in-everywhere.

Decisions / watch-outs

• It's a migration with fallout, so do it as its own pass with a full conformance-suite run to triage — don't bundle it. Any test that feeds the validators data they reject (empty multistring values, DeletedAt set, invalid writing-system ids, a second main, …) will start failing and needs either valid data or an explicit BaseApi opt-out. Error-path tests that currently assert a deeper failure may need to expect a ValidationException instead.
• The blast radius is probably smaller than it looks: AutoFakerDefault already shapes generated data toward validity (bounded WS ids, IsMain=false, Predefined=false, no Unknown morph types), so most faked round-trips should pass. The real work is the handful of minimal-object / error-path tests.
• Wiring: both the CRDT and FwData test fixtures expose a service provider and their kernels call AddMiniLcmValidators (which also registers MiniLcmApiUserFacingWrappers), so the base can pull the production stack from the fixture rather than hand-constructing validators. Threading that provider into MiniLcmTestBase is the main mechanical task.

[myieye]

Cross-linking: the structural groundwork is in #2362 / PR #2363. The validation wrapper now hand-writes every write (so a write can't silently skip validation again), with tests pinning today's behavior — including that CreateEntry deliberately does not validate. That PR preserves current behavior on purpose.

This issue stays open as the actual behavior decision: should user-facing CreateEntry validate? The catch we confirmed while doing #2362: sync/import drives CreateEntry (and CreatePublication/CreateMorphType) with empty FLEx MultiStrings that EntryValidator.NoEmptyValues rejects — so just enabling validation breaks import. So it needs a real design call rather than the one-line fix suggested above: validate the user-facing path only / a sync-specific (laxer) validation profile / strip empties on import / reconsider whether NoEmptyValues should reject at all (empties legitimately round-trip from FLEx).

[hahn-kev]

I think we should make a style change to prevent this class of error. If we change our declaration of overrides.
Before:

public async Task<Entry> CreateEntry(Entry entry)

After:

async Task<Entry> IMiniLcmWriteApi.CreateEntry(Entry entry)

then we get a compile error, because there's no method with that signature. This should prevent this class of error (silently bypassing validation due to signature changes).