From 354f9743e6524be706b4f57a86c0d3278ea3edae Mon Sep 17 00:00:00 2001 From: Meta Vens Date: Tue, 29 Sep 2026 08:28:24 +0200 Subject: [PATCH] Refuse a plain write that shares a variant's indexed value; a flush sheds overrides the type lacks The index rule is about the commit's result, so it now runs for every commit that changes an indexed value: a plain record landing on a value a variant of the post-commit set holds is refused naming both keys and the index (the held index is never trusted; a plain write had already overwritten it). Plain-vs-plain is untouched. A variant sheds overrides of slots its type lacks when it is flushed, as a plain record sheds the member, and the load's report names the slot when the schema itself never had it. Co-Authored-By: Claude Sonnet 5 --- src/GameCult.Caching/CultCache.cs | 98 +++++++++++- tests/GameCult.Caching.Tests/VariantTests.cs | 150 +++++++++++++++++++ 2 files changed, 245 insertions(+), 3 deletions(-) diff --git a/src/GameCult.Caching/CultCache.cs b/src/GameCult.Caching/CultCache.cs index f17d5b93..2d4e7137 100644 --- a/src/GameCult.Caching/CultCache.cs +++ b/src/GameCult.Caching/CultCache.cs @@ -2520,7 +2520,11 @@ namespace GameCult.Caching } if (toResolve.Count == 0) - return new VariantPlan(admitted); + { + var unchanged = new VariantPlan(admitted); + RefusePlainSharingWithVariant(unchanged, incoming, gone); + return unchanged; + } var pending = new HashSet(toResolve, StringComparer.Ordinal); var done = new Dictionary(StringComparer.Ordinal); @@ -2532,6 +2536,7 @@ namespace GameCult.Caching var plan = new VariantPlan(admitted.Select(stored => done.TryGetValue(stored.Key.Value, out var resolved) ? resolved : stored).ToList()); foreach (var key in toResolve.Where(key => !incoming.ContainsKey(key))) plan.Dependents.Add(done[key]); + RefusePlainSharingWithVariant(plan, incoming, gone); return plan; CultStoredDocument ResolveOne(string key) @@ -2672,6 +2677,61 @@ namespace GameCult.Caching } } + // The index rule is about the commit's result, so it holds for every commit that changes an indexed value, in either + // order: a variant resolving onto another record's value is refused in ResolveOne; a plain record landing on a + // variant's value is refused here, against the variants of the post-commit set (never the held index, which the plain + // write would already have overwritten). Plain-vs-plain sharing is not this rule's business. + private void RefusePlainSharingWithVariant( + VariantPlan plan, + IReadOnlyDictionary incoming, + HashSet gone) + { + Dictionary<(Type Type, string Alias), Dictionary>? variantValues = null; + foreach (var stored in plan.Admitted) + { + if (stored.Variant != null || stored.Descriptor.IndexAccessors.Count == 0) + continue; + foreach (var pair in stored.Descriptor.IndexAccessors) + { + var value = pair.Value(stored.Document); + if (string.IsNullOrWhiteSpace(value)) + continue; + variantValues ??= HeldByVariants(plan, incoming, gone); + if (variantValues.TryGetValue((stored.Descriptor.DocumentType, pair.Key), out var values) && + values.TryGetValue(value, out var variantKey) && variantKey != stored.Key.Value) + throw new InvalidOperationException( + $"Record {stored.Key.Value} and variant {variantKey} both hold index {pair.Key} '{value}'; " + + "a variant may not share an indexed value with another record."); + } + } + } + + // The indexed values the variants of the post-commit set hold: those this commit resolved, and those it left alone. + private Dictionary<(Type Type, string Alias), Dictionary> HeldByVariants( + VariantPlan plan, + IReadOnlyDictionary incoming, + HashSet gone) + { + var variants = new Dictionary(StringComparer.Ordinal); + foreach (var variantKey in _variantKeys.Where(variantKey => !incoming.ContainsKey(variantKey) && !gone.Contains(variantKey))) + variants[variantKey] = _entries[variantKey]; + foreach (var stored in plan.Admitted.Concat(plan.Dependents).Where(stored => stored.Variant != null)) + variants[stored.Key.Value] = stored; + + var held = new Dictionary<(Type Type, string Alias), Dictionary>(); + foreach (var stored in variants.Values) + { + foreach (var pair in stored.Descriptor.IndexAccessors) + { + var value = pair.Value(stored.Document); + if (!string.IsNullOrWhiteSpace(value)) + MapOf(held, (stored.Descriptor.DocumentType, pair.Key))[value] = stored.Key.Value; + } + } + + return held; + } + private static InvalidOperationException CycleRefusal(IEnumerable keys) => new($"Variant bases form a cycle: {string.Join(" -> ", keys)}."); @@ -2917,10 +2977,41 @@ namespace GameCult.Caching SchemaId = entry.Descriptor.SchemaId, StoredAt = entry.StoredAt, Payload = entry.Variant != null ? Array.Empty() : serializePayload(entry.Document), - Variant = entry.Variant == null ? null : new CultVariantDelta(entry.Variant.BaseKey, entry.Variant.Overrides) + Variant = entry.Variant == null ? null : new CultVariantDelta(entry.Variant.BaseKey, OverridesTheTypeHas(entry).ToArray()) }; } + // A plain record sheds a member its type dropped when it is flushed; a variant sheds the override of that slot, so + // a later type reusing the slot number cannot resurrect it. The load's report names the shed slots. + private static IEnumerable OverridesTheTypeHas(CultStoredDocument entry) => + entry.Variant!.Overrides.Where(entryOverride => + entryOverride.Path.Count != 1 || entry.Descriptor.RichMembers.Any(member => member.Slot == entryOverride.Path[0].Slot)); + + // A variant's override of a slot its type lacks is ignored like an extra slot on a plain record. Schema resolution + // reports slots the persisted schema had and the type dropped; an override the schema never had is added here. + protected static CultSchemaMigrationReport ReportVariantSheds(CultSchemaMigrationReport report, CultStoredDocument stored) + { + if (stored.Variant == null) + return report; + var shed = stored.Variant.Overrides + .Where(entryOverride => entryOverride.Path.Count == 1 && + stored.Descriptor.RichMembers.All(member => member.Slot != entryOverride.Path[0].Slot)) + .Select(entryOverride => entryOverride.Path[0].Slot) + .Except(report.IgnoredExtraSlots) + .Distinct() + .OrderBy(slot => slot) + .ToArray(); + if (shed.Length == 0) + return report; + report.IgnoredExtraSlots = report.IgnoredExtraSlots.Concat(shed).OrderBy(slot => slot).ToArray(); + report.Warnings = report.Warnings.Concat(shed.Select(slot => new CultSchemaMigrationWarning + { + Code = "ignored_extra_slot", + Message = $"Variant {stored.Key.Value} overrides slot {slot}, which {stored.Descriptor.DocumentType.Name} does not have; the override is ignored and shed on flush." + })).ToArray(); + return report; + } + protected CultStoredDocument ToStoredDocument( CultPersistedRecord record, IReadOnlyCollection catalog, @@ -2993,8 +3084,9 @@ namespace GameCult.Caching var persisted = new Dictionary(StringComparer.Ordinal); foreach (var record in snapshot.Records) { - reports.Add(Registry.ResolvePersistedSchemaReport(record.SchemaId, snapshot.SchemaCatalog)); + var report = Registry.ResolvePersistedSchemaReport(record.SchemaId, snapshot.SchemaCatalog); var stored = ToStoredDocument(record, snapshot.SchemaCatalog, DeserializePayload); + reports.Add(ReportVariantSheds(report, stored)); persisted[stored.Key.Value] = stored; } diff --git a/tests/GameCult.Caching.Tests/VariantTests.cs b/tests/GameCult.Caching.Tests/VariantTests.cs index 2cd7cde2..c81e892b 100644 --- a/tests/GameCult.Caching.Tests/VariantTests.cs +++ b/tests/GameCult.Caching.Tests/VariantTests.cs @@ -587,6 +587,140 @@ namespace GameCult.Caching.Tests Assert.That(refused.Message, Does.Contain("hand-variant").And.Contain("hand-base").And.Contain("code")); } + // ---- the index rule holds for the commit's result, in either write order, and the store always reopens ---- + + private static VariantGear Other(string code) => new() { Name = "other", Power = 1, Code = code }; + + [Test] + public async Task APlainWriteThatWouldShareAVariantsIndexedValueIsRefusedNamingBothKeysAndTheStoreReopens() + { + var path = PathOf("gear.cc"); + var other = new CultRecordKey("other"); + using (var cache = Open(path)) + { + SeedBase(cache); + SeedBig(cache); + + var refused = Refused(() => cache.Commit(batch => batch.Upsert(typeof(VariantGear), Other("laser big"), other))); + Assert.That(refused.Message, Does.Contain(other.Value).And.Contain(BigKey.Value).And.Contain("code").And.Contain("laser big")); + + var single = Assert.ThrowsAsync( + () => cache.UpsertAsync(Other("laser big"), new CultRecordHandle(other)))!; + Assert.That(single.Message, Does.Contain(other.Value).And.Contain(BigKey.Value)); + + Assert.That(cache.Get(other), Is.Null, "nothing landed"); + Assert.That(cache.GetByIndex("code", "laser big")!.Name, Is.EqualTo("laser big"), "the variant is still found by its value"); + EditBase(cache, gear => gear.Power = 11); + Assert.That(cache.Get(BigKey)!.Power, Is.EqualTo(25), "the variant is still editable through its base"); + await Task.CompletedTask; + } + + using var reopened = Open(path); + Assert.That(reopened.GetByIndex("code", "laser big")!.Name, Is.EqualTo("laser big")); + Assert.That(reopened.Get(other), Is.Null); + } + + [Test] + public void ABaseEditOrANewPlainRecordFirstThenAVariantOnTheSameValueIsRefusedAndTheStoreReopens() + { + var path = PathOf("gear.cc"); + var other = new CultRecordKey("other"); + using (var cache = Open(path)) + { + SeedBase(cache); + cache.Commit(batch => batch.Upsert(typeof(VariantGear), Other("laser big"), other)); + + var refused = Refused(() => SeedBig(cache)); + Assert.That(refused.Message, Does.Contain(BigKey.Value).And.Contain(other.Value).And.Contain("code").And.Contain("laser big")); + Assert.That(cache.Get(BigKey), Is.Null, "nothing landed"); + + // The same batch, either staging order: the result is what is judged. + var sameBatchPlainFirst = Refused(() => cache.Commit(batch => + { + batch.Upsert(typeof(VariantGear), Other("taken"), new CultRecordKey("taker")); + batch.UpsertVariant(BigKey, BaseKey, new[] + { + cache.Override(nameof(VariantGear.Name), "laser big"), + cache.Override(nameof(VariantGear.Code), "taken") + }); + })); + Assert.That(sameBatchPlainFirst.Message, Does.Contain(BigKey.Value).And.Contain("taker")); + var sameBatchVariantFirst = Refused(() => cache.Commit(batch => + { + batch.UpsertVariant(BigKey, BaseKey, new[] + { + cache.Override(nameof(VariantGear.Name), "laser big"), + cache.Override(nameof(VariantGear.Code), "taken") + }); + batch.Upsert(typeof(VariantGear), Other("taken"), new CultRecordKey("taker")); + })); + Assert.That(sameBatchVariantFirst.Message, Does.Contain(BigKey.Value).And.Contain("taker")); + + // Plain-vs-plain sharing is not this rule's business. + Assert.DoesNotThrow(() => cache.Commit(batch => batch.Upsert(typeof(VariantGear), Other("l1"), new CultRecordKey("twin")))); + } + + using var reopened = Open(path); + Assert.That(reopened.Get(BigKey), Is.Null); + Assert.That(reopened.Get(other), Is.Not.Null); + } + + // ---- a variant sheds overrides its type lacks when it is flushed, as a plain record sheds the member ---- + + [Test] + public void AFlushShedsAnOverrideOfADroppedSlotSoALaterTypeReusingTheSlotDoesNotResurrectIt() + { + var path = PathOf("drift.cc"); + var wide = CultDocumentRegistry.ForTypes(new[] { typeof(DriftWide) }); + using (var writer = Open(path, registry: wide)) + { + writer.Commit(batch => + { + batch.Upsert(typeof(DriftWide), new DriftWide { Name = "drift", Power = 1, Wing = "base wing" }, new CultRecordKey("drift-base")); + batch.UpsertVariant(new CultRecordKey("drift-variant"), new CultRecordKey("drift-base"), new[] + { + writer.Override(nameof(DriftWide.Name), "drift variant"), + writer.Override(nameof(DriftWide.Wing), "stale wing") + }); + }); + } + + using (var narrow = Open(path, registry: CultDocumentRegistry.ForTypes(new[] { typeof(DriftNarrow) }))) + narrow.BackingStores[0].PushAll(); + + Assert.That(OverriddenSlotsOn(path, "drift-variant"), Is.EqualTo(new[] { 0 }), "the dropped slot's override is gone from the flushed record"); + + using var reused = Open(path, registry: CultDocumentRegistry.ForTypes(new[] { typeof(DriftReused) })); + Assert.That(reused.Get(new CultRecordKey("drift-base"))!.Colour, Is.Empty); + Assert.That(reused.Get(new CultRecordKey("drift-variant"))!.Colour, Is.Empty, "the variant inherits its base's colour, not the old wing"); + } + + [Test] + public void AnOverrideOfASlotTheTypeNeverHadIsReportedOnLoadAndShedOnTheNextFlush() + { + var path = WriteStore(PlainGear("hand-base"), OverridingRecord("hand-variant", "hand-base", + CultVariantOverride.Set(99, new byte[] { 0x01 }), + TextOverride(0, "hand variant"), + TextOverride(2, "hand-code"))); + using (var cache = Open(path)) + { + var variantReport = cache.BackingStores[0].LastSchemaMigrationReports.Single(report => report.IgnoredExtraSlots.Length > 0); + Assert.That(variantReport.IgnoredExtraSlots, Is.EqualTo(new[] { 99 })); + Assert.That(variantReport.Warnings.Select(warning => warning.Code), Has.Some.EqualTo("ignored_extra_slot")); + Assert.That(variantReport.Warnings.Select(warning => warning.Message), Has.Some.Contain("hand-variant")); + cache.BackingStores[0].PushAll(); + } + + Assert.That(OverriddenSlotsOn(path, "hand-variant").OrderBy(slot => slot), Is.EqualTo(new[] { 0, 2 })); + using var reopened = Open(path); + Assert.That(reopened.BackingStores[0].LastSchemaMigrationReports.SelectMany(report => report.IgnoredExtraSlots), Is.Empty); + Assert.That(reopened.Get(new CultRecordKey("hand-variant"))!.Name, Is.EqualTo("hand variant")); + } + + private static int[] OverriddenSlotsOn(string path, string key) => + CultDocumentMessagePackSerialization.DeserializeSnapshot(File.ReadAllBytes(path)).Records + .Single(record => record.Key == key).Variant!.Overrides.Select(entry => entry.Path[0].Slot).ToArray(); + // ---- flatten resolves against the same batch ---- [Test] @@ -950,6 +1084,22 @@ namespace GameCult.Caching.Tests public string Wing = string.Empty; } + // A later type that reuses the slot Narrow dropped, for another member. + [CultDocument("tests.variant_drift", "tests.variant_drift.v3")] + [MessagePackObject(AllowPrivate = true)] + internal sealed class DriftReused + { + [Key(0)] + [CultName] + public string Name = string.Empty; + + [Key(1)] + public int Power; + + [Key(2)] + public string Colour = string.Empty; + } + [CultDocument("tests.variant_drift", "tests.variant_drift.v2")] [MessagePackObject(AllowPrivate = true)] internal sealed class DriftNarrow