migrate from Translation to Translations#2000
Conversation
📝 WalkthroughWalkthroughThis PR replaces ExampleSentence.Translation with a Translations collection across backend, CRDT, sync, and frontend, adds translation lifecycle APIs (add/update/remove), migrates storage (EF migration), updates JSON (deserialization/resolvers), introduces CRDT changes for translations, adjusts tests/fixtures extensively, bumps SIL.LCModel packages, and updates viewer types/UI/localizations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
💻 FW Lite
abeca73 to
fe94d47
Compare
|
one thing I realized based on a comment you left is that it's possible in CRDT to define a translation with the same Guid in different examples, but that would fail when syncing to FLEx. That's not likely to happen, but right now we don't and can't really prevent it unless we check all translation ids in Apply when adding a new translation. |
|
@hahn-kev Yup 😬 |
169dea2 to
98f2a73
Compare
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/viewer/src/locales/id.po (1)
716-721: Drop the stray period from “Never” translation.The source string doesn’t include punctuation. Keeping the trailing period will render an unexpected “Tidak pernah.” in the UI.
msgid "Never" -msgstr "Tidak pernah." +msgstr "Tidak pernah"
🧹 Nitpick comments (50)
backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/MultiStringOverride.cs (3)
74-83: Deep-copy Tags and ObjData to avoid shared mutable references.Assigning
existing.Tagsandexisting.ObjDataby reference can lead to aliasing/mutation issues in tests. Mirror the pattern used inRichSpan.Copy()to clone these members.Apply this diff:
var minimalSpan = new RichSpan { Text = existing.Text, Ws = existing.Ws, - Tags = existing.Tags, + Tags = existing.Tags is null ? null : (Guid[])existing.Tags.Clone(), Bold = existing.Bold, FontSize = existing.FontSize, ForeColor = existing.ForeColor, - ObjData = existing.ObjData, + ObjData = existing.ObjData is null ? null : existing.ObjData with { }, };This aligns with the copy semantics used elsewhere (see RichString.cs Copy method). Based on learnings.
71-71: Simplify the guard; the type check is likely redundant.Given this override targets
RichSpanandPreinitializeis true,context.Instanceshould already be aRichSpan. You can simplify to an early return on!minimaland cast directly.If
Soenneker.Utils.AutoBoguscan ever supply a non-RichSpaninstance here, keep the check. Otherwise:- if (!minimal || context.Instance is not RichSpan existing) return; + if (!minimal) return; + var existing = (RichSpan)context.Instance;
67-67: Confirm intentional divergence on Preinitialize behavior.Other overrides in this file use
Preinitialize => false, while this one setstrue. If the goal is to base the minimal instance on faker-populated values and then prune noise, this is fine. Otherwise, considerfalseand constructing minimal spans from scratch for consistency.backend/FwLite/MiniLcm.Tests/AutoFakerHelpers/AutoFakerDefault.cs (4)
54-57: Initialize JsonPatchDocument<> earlier to avoid downstream generation.Pre-initializing prevents default generation from touching internals that could cascade into generating JsonSerializerOptions.
Apply:
- new SimpleGenericOverride(typeof(JsonPatchDocument<>), context => - { - context.Instance = Activator.CreateInstance(context.GenerateType.Type!)!; - }, false), + new SimpleGenericOverride(typeof(JsonPatchDocument<>), context => + { + context.Instance = Activator.CreateInstance(context.GenerateType.Type!)!; + }, true),
58-63: Fix minor grammar and add null-safe type display in exception message.Apply:
- new SimpleOverride<JsonSerializerOptions>(context => - { - var typeName = context.GenerateType.Type?.FullName; - throw new InvalidOperationException( - $"You should not be generating JsonSerializerOptions. You're probably didn't intend to generate an instance of the current type: {typeName}."); - }, false) + new SimpleOverride<JsonSerializerOptions>(context => + { + var typeName = context.GenerateType.Type?.FullName ?? "<unknown>"; + throw new InvalidOperationException( + $"You should not be generating JsonSerializerOptions. You probably didn't intend to generate an instance of the current type: {typeName}."); + }, false)
68-81: Guard against potential infinite loop in PredicateOverride.If the predicate can’t be satisfied, this will loop forever. Add a cap with graceful fallback.
Apply:
- public override void Generate(AutoFakerOverrideContext context) - { - var value = context.Instance; - while (value is not T instance || !predicate(instance)) - { - value = context.AutoFaker.Generate<T>(); - } - context.Instance = value; - } + public override void Generate(AutoFakerOverrideContext context) + { + var value = context.Instance; + var attempts = 0; + while ((value is not T instance || !predicate(instance)) && attempts++ < 100) + { + value = context.AutoFaker.Generate<T>(); + } + context.Instance = value; + }
94-101: Use the underlying Type for generic checks to improve clarity and null-safety.Apply:
- public override bool CanOverride(AutoFakerContext context) - { - return context.GenerateType.IsGenericType && context.GenerateType.GetGenericTypeDefinition() == genericTypeDefinition; - } + public override bool CanOverride(AutoFakerContext context) + { + var type = context.GenerateType.Type; + return type is not null && type.IsGenericType && type.GetGenericTypeDefinition() == genericTypeDefinition; + }backend/FwLite/FwLiteProjectSync.Tests/Import/ImportTests.cs (1)
46-53: Assert translation identity is set and round-trips.Translations are now first‑class with IDs. Strengthen the test by asserting the translation Id is non‑empty and stable after import.
You can add after Line 61:
var origTr = entry.Senses.Single().ExampleSentences.Single().Translations.Single(); var importedTr = importedEntry.Senses.Single().ExampleSentences.Single().Translations.Single(); origTr.Id.Should().NotBe(Guid.Empty); importedTr.Id.Should().Be(origTr.Id);backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs (1)
137-140: Explicit IAsyncDisposable.DisposeAsync forwards to DisposeAsync.This avoids code duplication. Consider sealing the class or documenting disposal order to prevent double-dispose in derived types, if any.
backend/FwLite/MiniLcm.Tests/ExampleSentenceTestsBase.cs (2)
25-34: Verify expectations around generated Ids (ExampleSentence and Translation).The test constructs an ExampleSentence and a Translation without Ids but asserts full equivalence. If the API assigns Ids on create (likely), BeEquivalentTo will fail on Id mismatches. Either set explicit Ids on the expected objects or exclude Ids from the assertion.
Example tweak:
-actualSentence.Should().BeEquivalentTo(expectedExampleSentence); +actualSentence.Should().BeEquivalentTo(expectedExampleSentence, opts => opts + .Excluding(e => e.Id) + .Excluding(e => e.Translations[0].Id));
47-73: Add a guard for duplicate Translation Ids across examples (risk noted in PR).Given the CRDT/FLEx sync risk, add a negative test that attempts to add the same translation Id to two different examples and verifies the API rejects it.
I can draft the test and/or API guard if desired.
Also applies to: 106-131
backend/FwLite/LcmCrdt.Tests/Changes/ChangeDeserializationRegressionData.latest.verified.txt (1)
686-696: Non-standard JSON Patch ops in UpdateTranslationChange."op": "green" and "path": "HTTP" are not valid JSON Patch semantics. If this payload is intentionally treated as opaque JSON for deserialization coverage, fine. Otherwise, prefer valid ops ("add", "remove", "replace", "move", "copy", "test") and RFC6901 paths to avoid confusion.
backend/FwLite/LcmCrdt/Migrations/20250911065934_ChangeTranslationColumn.cs (1)
11-17: Column rename alone may not handle legacy JSON shape.Renaming Translation -> Translations preserves existing JSON content but changes the target type to a list. Without a tolerant converter, reading old rows will fail. Verify that deserialization handles an object (legacy MultiString) by wrapping it into a single Translation with a sentinel Id (e.g., MissingTranslationId).
If the read path is not tolerant, add a data migration to wrap legacy JSON:
protected override void Up(MigrationBuilder migrationBuilder) { migrationBuilder.RenameColumn( name: "Translation", table: "ExampleSentence", newName: "Translations"); + // Normalize legacy object -> array-of-translation with sentinel Id + migrationBuilder.Sql(""" +UPDATE "ExampleSentence" +SET "Translations" = jsonb_build_array( + jsonb_build_object( + 'Id', '3dce1982-8e93-44f1-b92c-e9c7bdf72801', -- MissingTranslationId + 'Text', "Translations" + ) +) +WHERE jsonb_typeof("Translations") = 'object'; +"""); }frontend/viewer/src/lib/DictionaryEntry.svelte (1)
79-81: Prefer first translation with text in the target WS (not just index 0).Selecting translations[0] can hide translations when the first item lacks that WS. Pick the first translation containing the WS instead.
Apply:
- text: asString(example.translations[0]?.text?.[ws.wsId]), + text: asString( + example.translations.find(t => t.text?.[ws.wsId])?.text?.[ws.wsId] + ),frontend/viewer/src/lib/writing-system-service.svelte.ts (1)
180-187: Defensive guard for possibly missing translations.If older payloads ever omit translations, iterating example.translations can throw. Safe-guard with nullish coalescing.
Apply:
- for (const translation of example.translations) { + for (const translation of (example.translations ?? [])) {backend/FwLite/FwLiteProjectSync.Tests/UpdateDiffTests.cs (1)
65-75: Nice: standalone Translation diff test added.Covers patching Text while excluding Id. Consider adding a “no-op diff returns null” test for both ExampleSentence and Translation to guard regressions in DiffToUpdate().
frontend/viewer/src/locales/ms.po (1)
1114-1117: Fill in the new parameterized string.Provide a Malay translation to avoid English fallback.
msgid "Translation {0}" -msgstr "" +msgstr "Terjemahan {0}"frontend/viewer/src/locales/fr.po (1)
1114-1117: Add the missing translation for the parameterized label.Prevents inconsistent UI text.
msgid "Translation {0}" -msgstr "" +msgstr "Traduction {0}"frontend/viewer/src/locales/ko.po (2)
716-721: Consider a more natural label for “Never” in UI.“절대로” reads adverbial; “없음” or “기록 없음” fits status timestamps better. Please confirm context.
msgid "Never" -msgstr "절대로" +msgstr "없음"Alternative: "기록 없음"
1114-1117: Add the missing translation for the parameterized label.Improves consistency in the example editor.
msgid "Translation {0}" -msgstr "" +msgstr "번역 {0}"backend/FwLite/FwLiteProjectSync/CrdtFwdataProjectSyncService.cs (1)
31-33: Type-check is fine; also set activity status before throwing.For consistent telemetry, set ActivityStatusCode.Error on type mismatch too. Optionally, consider making the parameter strongly typed (CrdtMiniLcmApi) in a future PR.
- if (crdtApi is not CrdtMiniLcmApi crdt) // maybe the argument type should be changed? - throw new InvalidOperationException("CrdtApi must be of type CrdtMiniLcmApi to sync."); + if (crdtApi is not CrdtMiniLcmApi crdt) // maybe the argument type should be changed? + { + activity?.SetStatus(ActivityStatusCode.Error, "CrdtApi must be CrdtMiniLcmApi."); + throw new InvalidOperationException("CrdtApi must be of type CrdtMiniLcmApi to sync."); + }backend/FwLite/LcmCrdt.Tests/TestJsonOptions.cs (1)
7-15: Confirm legacy “Translation” and casing are covered in External().External() sets a custom resolver; ensure it includes the ExampleSentence legacy “Translation” mapping and case-insensitive property matching, given the legacy fixtures mix casing.
If needed, set PropertyNameCaseInsensitive and (if not already included) add the legacy modifier:
public static JsonSerializerOptions External() { var config = new CrdtConfig(); LcmCrdtKernel.ConfigureCrdt(config); - return new JsonSerializerOptions(JsonSerializerDefaults.General) + return new JsonSerializerOptions(JsonSerializerDefaults.General) { TypeInfoResolver = config.MakeLcmCrdtExternalJsonTypeResolver(), + PropertyNameCaseInsensitive = true, }; }And verify that MakeLcmCrdtExternalJsonTypeResolver includes the ExampleSentenceTranslationModifier; otherwise we can combine with it.
backend/FwLite/LcmCrdt.Tests/Data/BaseSerializationTest.cs (1)
74-84: DRY up “+” unescaping in JSON normalization.Both serializers perform the same string replace. Extract a helper to avoid duplication and future drift.
- .Replace("\\u002B", "+"); + .NormalizeEscapedPlus();- .Replace("\\u002B", "+"); + .NormalizeEscapedPlus();Add this helper (outside the shown ranges):
private static string NormalizeEscapedPlus(this string s) => s.Replace("\\u002B", "+");Also applies to: 100-103
backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs (1)
280-300: Include patch summary in UpdateTranslation dry‑run log.Other update methods log update.Summarize(); mirror that here for consistency.
- DryRunRecords.Add(new DryRunRecord(nameof(UpdateTranslation), $"Update translation {translationId} in example sentence {exampleSentenceId}")); + DryRunRecords.Add(new DryRunRecord( + nameof(UpdateTranslation), + $"Update translation {translationId} in example sentence {exampleSentenceId}, changes: {update.Summarize()}"));backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/UpdateEntryTests.cs (1)
51-51: Nit: fix test name grammar.Use “Exist” (plural subject).
- public async Task UpdateEntry_CanUpdateExampleSentenceTranslations_WhenNoTranslationsExists() + public async Task UpdateEntry_CanUpdateExampleSentenceTranslations_WhenNoTranslationsExist()frontend/viewer/src/lib/entry-editor/object-editors/ExampleEditorPrimitive.svelte (1)
50-51: Keyed each may collide for draft itemsWhen no translations exist, draftTranslation(example) must provide a stable id. If it doesn’t, keyed by translation.id can duplicate/shift unexpectedly.
Consider a fallback key:
- {#each (example.translations.length ? example.translations : [draftTranslation(example)]) as translation, i (translation.id)} + {#each (example.translations.length ? example.translations : [draftTranslation(example)]) as translation, i (translation.id ?? `draft-${i}`)}Please confirm draftTranslation(example) guarantees a non-empty, stable id across renders.
backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs (1)
110-113: API ergonomics: consider returning created/updated resourcesThese methods return Task. Returning the created Translation (Add) and updated ExampleSentence or Translation (Update/Remove) can reduce callers’ fetches and improve ergonomics. Add XML docs for these new methods to match surrounding API.
Would you like a follow-up commit adjusting return types and adding docs?
backend/FwLite/LcmCrdt/Changes/ExampleSentences/SetFirstTranslationIdChange.cs (1)
20-26: Safer apply: only retag the single legacy translationLimit to the intended repair case to avoid mis-setting IDs when multiple translations exist or when the first already has a proper ID.
Apply:
- var translation = entity.Translations.FirstOrDefault(); - if (translation == null) return ValueTask.CompletedTask; - translation.Id = TranslationId; + var translation = entity.Translations.FirstOrDefault(); + if (translation == null) return ValueTask.CompletedTask; +#pragma warning disable CS0618 + if (entity.Translations.Count != 1 || translation.Id != Translation.MissingTranslationId) + return ValueTask.CompletedTask; +#pragma warning restore CS0618 + translation.Id = TranslationId; return ValueTask.CompletedTask;Confirm this matches the repair preconditions (exactly one translation with MissingTranslationId).
backend/FwLite/LcmCrdt/Objects/DbTranslationDeserializationTarget.cs (1)
49-52: Return a mutable empty collection.Returning [] for IList yields a fixed-size array; callers may hit NotSupportedException on mutation. Return a List instead.
Apply this diff:
- public IList<Translation> GetTranslations() - { - return _translations ?? []; - } + public IList<Translation> GetTranslations() + { + return _translations ?? new List<Translation>(); + }backend/FwLite/LcmCrdt/Changes/CustomJsonPatches/JsonPatchExampleSentenceChange.cs (2)
24-31: Use Ordinal comparison for path prefix.Avoid culture-sensitive comparisons for JSON Pointer paths.
Apply this diff:
- if (op.Path?.StartsWith("/Translation/") == true) + if (op.Path?.StartsWith("/Translation/", StringComparison.Ordinal) == true)
34-47: Validate legacy path shape.If op.Path is just /Translation or malformed, ParsedPath.LastSegment won’t be a wsId. Guard to fail fast or no-op.
Proposed addition:
private static void ApplyTranslationOp(Operation<ExampleSentence> op, ExampleSentence entity, IObjectAdapter adapter) { var wsId = new ParsedPath(op.Path).LastSegment; + if (string.IsNullOrEmpty(wsId) || wsId.Equals("Translation", StringComparison.Ordinal)) + { + // unexpected path; ignore to preserve behavior parity with JsonPatchChange or fail fast + return; + }backend/FwLite/LcmCrdt/Changes/ExampleSentences/UpdateTranslationChange.cs (1)
10-15: Block patching Translation.Id.Prevent accidental Id mutations via JSON Patch; limit to /Text paths.
Apply this diff:
public UpdateTranslationChange(Guid entityId, Guid translationId, JsonPatchDocument<Translation> patch) : base(entityId) { TranslationId = translationId; JsonPatchValidator.ValidatePatchDocument(patch); + // Disallow Id changes + if (patch.Operations.Any(o => + o.path != null && + (o.path.Equals("/Id", StringComparison.OrdinalIgnoreCase) || o.path.StartsWith("/Id/", StringComparison.OrdinalIgnoreCase)))) + { + throw new NotSupportedException("Patching Translation.Id is not allowed."); + } Patch = patch; }backend/FwLite/FwLiteProjectSync/CrdtRepairs.cs (2)
59-77: Consider batching or caching API calls.Per-example sequential calls to fwData and crdt APIs may be expensive on large projects. Batch by entry/sense where possible or add minimal concurrency with throttling.
46-54: Edge case: fwdata translation object with null Id.Current behavior is to skip. Optionally log/trace to aid diagnosis during one-time repair runs.
backend/FwLite/MiniLcm/Validators/ExampleSentenceValidator.cs (2)
28-37: Good validator scoping; consider sealing and reducing visibility.The child validator correctly uses the parent ExampleSentence context. To tighten API surface and avoid unintended reuse, make the validator internal sealed.
Apply this diff:
-public class ExampleSentenceTranslationValidator : AbstractValidator<Translation> +internal sealed class ExampleSentenceTranslationValidator : AbstractValidator<Translation>
39-43: Identifier generation can be more robust; also one unused method.
- IndexOf may return -1 in edge-cases; fall back to Id to avoid “Translation #0” messages.
- GetExampleSentenceIdentifier() (Lines 44-47) is unused and can be removed.
Apply this diff:
- private string GetExampleSentenceTranslationIdentifier(Translation arg) - { - return $"Example: {_exampleSentence.Id:D} Translation #{_exampleSentence.Translations.IndexOf(arg) + 1}"; - } + private string GetExampleSentenceTranslationIdentifier(Translation arg) + { + var idx = _exampleSentence.Translations.IndexOf(arg); + var suffix = idx >= 0 ? $"#{idx + 1}" : $"Id:{arg.Id:D}"; + return $"Example: {_exampleSentence.Id:D} Translation {suffix}"; + } - - private string GetExampleSentenceIdentifier() - { - return _exampleSentence.Id.ToString("D"); - }frontend/viewer/src/lib/utils.ts (1)
77-87: Ensure Svelte reactivity and guard against duplicate IDs when saving a draft.
- Pushing mutates the array in-place; prefer immutable assignment to trigger reactivity reliably.
- Optionally prevent inserting a duplicate translation id within the same example.
Apply this diff:
draftTranslation.saveDraft = () => { if (!exampleSentence.translations) { exampleSentence.translations = []; } const {saveDraft, ...translation} = draftTranslation; - exampleSentence.translations.push(translation); + // avoid duplicate ids within the same example + if (!exampleSentence.translations.some(t => t.id === translation.id)) { + exampleSentence.translations = [...exampleSentence.translations, translation]; + } };Please verify that callers update any Svelte stores after invoking saveDraft. If exampleSentence is nested inside a store, you may need to wrap the mutation in a store update for reactivity to propagate.
backend/FwLite/LcmCrdt/QueryHelpers.cs (1)
7-16: Avoid naming extension methods “Finalize”.Extension methods named Finalize can be confusing due to Object.Finalize. Consider a clearer name like Normalize or FinalizeModel to avoid ambiguity at call sites.
backend/FwLite/LcmCrdt/Changes/CreateExampleSentenceChange.cs (1)
40-44: Good legacy bridging; consider defensive copy.Bridging legacy Translation into a one-item Translations list is correct. To avoid later mutation side-effects, consider copying the list when sourced from exampleSentence.
Apply this diff:
- var translations = Translations ?? ((Translation is not null) - ? [MiniLcm.Models.Translation.FromMultiString(Translation)] - : []); + var translations = Translations is not null + ? new List<Translation>(Translations) + : (Translation is not null + ? [MiniLcm.Models.Translation.FromMultiString(Translation)] + : []);backend/FwLite/LcmCrdt.Tests/Data/TranslationDeserializationTests.cs (1)
98-123: Minor: Dispose DbCommand.CreateCommand() result isn’t disposed. Use using to avoid resource leaks in tests.
Apply this diff:
- var dbCommand = dbConnection.CreateCommand(); + using var dbCommand = dbConnection.CreateCommand();backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (1)
747-767: Add guardrails for translation IDs (collisions and legacy placeholder).To reduce FLEx sync risks:
- Avoid adding a translation with MissingTranslationId.
- Optionally, regenerate ID if it already exists in any example (best-effort pre-check).
Apply this diff:
public async Task AddTranslation(Guid entryId, Guid senseId, Guid exampleSentenceId, Translation translation) { - if (translation.Id == Guid.Empty) translation.Id = Guid.NewGuid(); + if (translation.Id == Guid.Empty || translation.Id == Translation.MissingTranslationId) + translation.Id = Guid.NewGuid(); + // Optional: pre-check cross-entity collision (best-effort) + // await using (var repo = await repoFactory.CreateRepoAsync()) + // { + // var exists = await repo.ExampleSentences + // .AnyAsyncEF(es => es.Translations.Any(t => t.Id == translation.Id)); + // if (exists) translation.Id = Guid.NewGuid(); + // } await AddChange(new AddTranslationChange(exampleSentenceId, translation)); }Given CRDT can produce duplicate IDs across examples, consider a runtime repair step (or AddTranslation pre-check) to dedupe translation IDs before FW sync, aligned with your comment in #1948.
backend/FwLite/LcmCrdt.Tests/Data/SnapshotDeserializationTests.cs (1)
85-142: Golden-data round‑trip maintainer looks good.The move-to-legacy and regenerate‑latest flow is pragmatic. Be mindful of Faker determinism to reduce churn.
Consider seeding the faker to stabilize newly generated snapshots across runs.
backend/FwLite/LcmCrdt.Tests/Data/RegressionTestHelper.cs (1)
16-16: Prefer disposing the DbCommand to avoid resource leaks.Wrap the command in a using to dispose it deterministically.
- var dbCommand = dbConnection.CreateCommand(); + using var dbCommand = dbConnection.CreateCommand();backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (4)
1579-1586: Consider guarding against duplicate Translation IDs across the project.A CRDT-created translation might reuse an ID already present under a different example; LCM may reject or corrupt data. Proactively detect and fail with a clear error before Create to avoid partial state.
Would you like a proposed guard that checks for an existing ICmObject with the same GUID (via ServiceLocator) and throws a DuplicateObjectException when found?
1666-1675: AddTranslation: OK, but include duplicate-ID guard as above.Defers to CreateExampleSentenceTranslation for default-ID assignment; add the global duplicate-ID guard here as well for API robustness.
1677-1690: Gracefully handle missing translation on removal.First(...) throws InvalidOperationException; prefer NotFoundException for a clearer API contract.
- () => - { - var translation = lexExampleSentence.TranslationsOC.First(t => t.Guid == translationId); - translation.Delete(); - }); + () => + { + var translation = lexExampleSentence.TranslationsOC.FirstOrDefault(t => t.Guid == translationId); + if (translation is null) + throw new NotFoundException($"Translation {translationId} not found on example sentence {exampleSentenceId}", nameof(Translation)); + translation.Delete(); + });
1692-1710: Gracefully handle missing translation on update; optionally block Id edits.First(...) will throw; prefer NotFoundException. Also ensure patches cannot mutate Id (no-op in proxy today), or validate and reject such operations.
- () => - { - var translation = lexExampleSentence.TranslationsOC.First(t => t.Guid == translationId); - var translationProxy = new UpdateTranslationProxy(translation, this); - update.Apply(translationProxy); - }); + () => + { + var translation = lexExampleSentence.TranslationsOC.FirstOrDefault(t => t.Guid == translationId); + if (translation is null) + throw new NotFoundException($"Translation {translationId} not found on example sentence {exampleSentenceId}", nameof(Translation)); + var translationProxy = new UpdateTranslationProxy(translation, this); + // Optionally: sanitize update.Patch to reject Id mutations + update.Apply(translationProxy); + });backend/FwLite/LcmCrdt.Tests/Changes/ChangeSerializationTests.cs (3)
13-36: Generalize change generation to avoid per-type special-casingThe SetComplexFormComponentChange special case is fine, but other change types without public ctors or with static factories could slip through. Add a reflection-based factory fallback before failing generation to reduce future maintenance.
Apply this diff to introduce a generic fallback pathway:
- object change; - try - { - change = Faker.Generate(type); - } - catch (Exception e) - { - throw new Exception($"Failed to generate change of type {type.Name}", e); - } - - change.Should().NotBeNull($"change type {type.Name} should have been generated").And.BeAssignableTo<IChange>(); - yield return (IChange)change; + object? change = null; + try + { + change = Faker.Generate(type); + } + catch + { + // fall through to reflection factory fallback + } + + if (change is null) + { + // Try public static factory methods returning the type (e.g., New*, From*) + var factories = type.GetMethods(System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Static) + .Where(m => m.ReturnType == type); + foreach (var factory in factories) + { + var args = factory.GetParameters().Select(p => + p.ParameterType == typeof(Guid) ? Guid.NewGuid() : + p.ParameterType == typeof(Guid?) ? (Guid?)Guid.NewGuid() : + p.ParameterType.IsEnum ? Enum.GetValues(p.ParameterType).GetValue(0)! : + p.HasDefaultValue ? p.DefaultValue! : + p.ParameterType == typeof(string) ? Faker.Random.String2(6) : + p.ParameterType == typeof(int) ? Faker.Random.Int() : + p.ParameterType == typeof(bool) ? Faker.Random.Bool() : + Activator.CreateInstance(p.ParameterType) ?? throw new InvalidOperationException($"No default for {p.ParameterType.Name}") + ).ToArray(); + try + { + change = factory.Invoke(null, args); + if (change is not null) break; + } + catch + { + // try next factory + } + } + } + + if (change is null) + throw new Exception($"Failed to generate change of type {type.Name}"); + + change.Should().BeAssignableTo<IChange>($"change type {type.Name} should have been generated"); + yield return (IChange)change!;Note: If needed, add
using System.Reflection;at the top of the file.
118-179: Stabilize the verification step to avoid snapshot race/flakinessRunning two Verify operations concurrently can be flaky depending on the Verify version/plugins. Run them sequentially. Also optional: consider deduping identical entries when moving to legacy to slow file growth.
Apply this diff for sequential verification:
- await Task.WhenAll( - Verify(SerializeRegressionData(legacyJsonArray)) - .UseStrictJson() - .UseFileName("ChangeDeserializationRegressionData.legacy"), - Verify(SerializeRegressionData(newLatestJsonArray)) - .UseStrictJson() - .UseFileName("ChangeDeserializationRegressionData.latest") - ); + await Verify(SerializeRegressionData(legacyJsonArray)) + .UseStrictJson() + .UseFileName("ChangeDeserializationRegressionData.legacy"); + await Verify(SerializeRegressionData(newLatestJsonArray)) + .UseStrictJson() + .UseFileName("ChangeDeserializationRegressionData.latest");
186-188: Use the same serializer as regression tests for the manual generatorKeep output consistent (notably '+' escaping). Build a JsonArray and reuse SerializeRegressionData.
Apply this diff:
- using var jsonFile = File.Open(GetJsonFilePath("NewJson.json"), FileMode.Create); - JsonSerializer.Serialize(jsonFile, GeneratedChanges(), OptionsIndented); + var jsonArray = new JsonArray(); + foreach (var change in GeneratedChanges()) + { + jsonArray.Add(JsonSerializer.SerializeToNode(change, OptionsIndented)); + } + File.WriteAllText(GetJsonFilePath("NewJson.json"), SerializeRegressionData(jsonArray));
…from an old object pass
…te changes setting Example.Translation to work with Translations
414340b to
8b90bc3
Compare
2 participants
closes #1948
EF:
CRDT:
fw-headless snapshot:
IDs:
Fwdata-crdt sync:
Crdt-changes:
An interesting result of this approach is that translations created in Harmony might be left without an ID in the db.
Their "missing ID" will just keep getting mapped to the "default example-sentence ID" before they leave the API, but that ID will never make it into the DB.
However, the sync pre-step only needs to run once per project. It would be nice to track that, but we probably won't put in that effort just yet.
After it's run, any crdt translations that are still missing an ID could be updated to actually persist the default example-sentence ID. We could add that code now, but it's kind of expensive scanning all translations for missing IDs. So, maybe we'll add the code as a one-off per project later (at a point in time when we are highly confident that fw-headless has all translation objects that were created in Harmony before this PR.