Make fwdata api respect nullable return types

[myieye]

Resolves #2047

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough

Walkthrough

This PR addresses issue #2047 by modifying FwDataMiniLcmApi to return null instead of throwing when entities are not found. GetEntry, GetSense, and GetExampleSentence now use TryGetObject for safe retrieval. Comprehensive test coverage is added across MiniLcm base classes and multiple implementations to verify null-returning behavior for missing entities.

Changes

Cohort / File(s)	Summary
FwDataMiniLcmApi null-handling updates backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs	Modified GetEntry, GetSense, and GetExampleSentence methods to use TryGetObject and return null when entity not found, replacing direct GetObject calls.
MiniLcm test base classes backend/FwLite/MiniLcm.Tests/SenseTestsBase.cs, QueryEntryTestsBase.cs, ExampleSentenceTestsBase.cs	Added new SenseTestsBase abstract class with initialization and test methods for missing/existing sense retrieval. Enhanced QueryEntryTestsBase and ExampleSentenceTestsBase with new test methods verifying Get_Missing* and Get_Existing* behavior.
FwDataMiniLcmBridge SenseTests implementation backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/SenseTests.cs	Created new SenseTests class with ProjectLoaderFixture, decorated with Collection attribute, overriding NewApi to return IMiniLcmApi from fixture.
LcmCrdt SenseTests implementation backend/FwLite/LcmCrdt.Tests/MiniLcmTests/SenseTests.cs	Created new SenseTests class inheriting from SenseTestsBase with MiniLcmApiFixture, implementing async NewApi and DisposeAsync methods.
Sync consistency tests backend/FwLite/FwLiteProjectSync.Tests/CrdtRepairTests.cs	Added two new test methods verifying cross-silo consistency when example sentences are removed: CrdtEntryMissingTranslationId_FwExampleSentenceRemoved_FullSync and CrdtEntryMissingTranslationId_CrdtExampleSentenceRemoved_FullSync.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

The core API changes are straightforward but affect critical read methods with nullable return types. Multiple test classes and implementations are added with consistent patterns across heterogeneous contexts. The spread across multiple files and the importance of null-handling correctness for sync operations warrant careful review.

Possibly related PRs

• #1546 — Modifies FwDataMiniLcmApi.cs for null handling of retrieved objects, directly related to same API changes.
• #1951 — Modifies FwDataMiniLcmApi.cs and mapping implementations; changes to Get* methods and FromLex* mappings are related to core PR changes.

Suggested reviewers

• hahn-kev

Poem

🐰 No more throws when entities flee,
TryGetObject sets us free,
Null returns with graceful care,
Missing data? Null's fair share!
Tests dance merrily left and right,
Safe retrieval, bug-free flight! 🎉

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The pull request title "Make fwdata api respect nullable return types" directly and clearly summarizes the main change in the changeset. The title is specific enough to convey the primary objective—modifying the FwData API to handle nullable return types properly by returning null instead of throwing exceptions—and aligns with the core code change in FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs where three API methods (GetEntry, GetSense, GetExampleSentence) were updated to use TryGetObject instead of GetObject.
Linked Issues Check	✅ Passed	The changes directly address the requirements in issue #2047. The core fix is implemented in FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs, where three methods (GetEntry, GetSense, GetExampleSentence) have been modified to use TryGetObject instead of GetObject, returning null when entities are not found rather than throwing exceptions. The supporting changes include new test infrastructure (SenseTestsBase, modifications to ExampleSentenceTestsBase and QueryEntryTestsBase) that verify the null-returning behavior for missing entities, and new test cases in CrdtRepairTests that validate cross-silo consistency when entities are removed, all of which directly support the stated objective of making MiniLcm methods respect nullable return types.
Out of Scope Changes Check	✅ Passed	All changes in this pull request are directly scoped to resolving issue #2047. The primary code changes modify three API methods in FwDataMiniLcmBridge to respect nullable return types, and the supporting changes introduce test infrastructure and test cases to verify the fix works correctly and to set up verification for cross-silo consistency. No unrelated modifications, refactorings, or features are present in the changeset.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[argos-ci]

The latest updates on your projects. Learn more about Argos notifications ↗︎

Awaiting the start of a new Argos build…

[github-actions]

UI unit Tests

  1 files   45 suites   27s ⏱️
111 tests 111 ✅ 0 💤 0 ❌
160 runs  160 ✅ 0 💤 0 ❌

Results for commit a7b85b1.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

backend/FwLite/FwLiteProjectSync.Tests/CrdtRepairTests.cs (2)

217-234: Clarify the assertion message on line 229.

The assertion message states "the crdt translation was removed", but the test actually removes the entire example sentence (line 227), not just a translation. The message should reflect that an example sentence was removed, which cascades to remove its translations.

Apply this diff to clarify the message:

-        result.CrdtChanges.Should().Be(1, "the crdt translation was removed");
+        result.CrdtChanges.Should().Be(1, "the crdt example sentence was removed");

---

277-293: Capture sync result for consistency.

Unlike the similar test at lines 217-234 and the pattern established at line 210, this test doesn't capture the sync result or verify the CrdtChanges count. For consistency and more complete test coverage, consider capturing the result and asserting the expected change count.

Apply this diff to add the assertion:

         // act
         await CrdtApi.DeleteExampleSentence(entryId, senseId, exampleSentenceId);
-        await SyncService.Sync(CrdtApi, FwDataApi);
+        var result = await SyncService.Sync(CrdtApi, FwDataApi);
+        result.CrdtChanges.Should().Be(0, "no crdt changes expected when deleting from crdt side");

         // assert - the fw translation was also removed

backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (1)

9-9: Consider making appleId static readonly.

For consistency with SenseTestsBase (line 5-6 in backend/FwLite/MiniLcm.Tests/SenseTestsBase.cs) and since the ID is only used for test setup, consider making this field static readonly.

Apply this diff:

-    private readonly Guid appleId = Guid.NewGuid();
+    private static readonly Guid appleId = Guid.NewGuid();

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0d076c6 and a7b85b1.

📒 Files selected for processing (7)

• backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/SenseTests.cs (1 hunks)
• backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (3 hunks)
• backend/FwLite/FwLiteProjectSync.Tests/CrdtRepairTests.cs (2 hunks)
• backend/FwLite/LcmCrdt.Tests/MiniLcmTests/SenseTests.cs (1 hunks)
• backend/FwLite/MiniLcm.Tests/ExampleSentenceTestsBase.cs (2 hunks)
• backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (3 hunks)
• backend/FwLite/MiniLcm.Tests/SenseTestsBase.cs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (6)

backend/FwLite/LcmCrdt.Tests/MiniLcmTests/SenseTests.cs (2)

backend/FwLite/MiniLcm.Tests/SenseTestsBase.cs (2)

• SenseTestsBase (3-37)
• Task (8-21)

backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/SenseTests.cs (1)

• Task (8-11)

backend/FwLite/MiniLcm.Tests/ExampleSentenceTestsBase.cs (3)

backend/FwLite/MiniLcm/Models/ExampleSentence.cs (1)

• Guid (20-23)

backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (2)

• Sense (764-779)
• RichString (860-870)

backend/FwLite/MiniLcm/Models/RichString.cs (2)

• RichString (11-13)
• RichString (87-90)

backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/SenseTests.cs (2)

backend/FwLite/LcmCrdt.Tests/MiniLcmTests/SenseTests.cs (3)

• SenseTests (3-19)
• Task (7-12)
• Task (14-18)

backend/FwLite/MiniLcm.Tests/SenseTestsBase.cs (2)

• SenseTestsBase (3-37)
• Task (8-21)

backend/FwLite/MiniLcm.Tests/SenseTestsBase.cs (2)

backend/FwLite/MiniLcm.Tests/MiniLcmTestBase.cs (1)

• MiniLcmTestBase (7-31)

backend/FwLite/MiniLcm.Tests/ExampleSentenceTestsBase.cs (1)

• Task (9-30)

backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (2)

backend/FwLite/MiniLcm.Tests/ExampleSentenceTestsBase.cs (7)

• Fact (32-37)
• Fact (39-45)
• Fact (47-59)
• Fact (61-70)
• Fact (72-98)
• Fact (100-129)
• Fact (131-156)

backend/FwLite/LcmCrdt/Data/MiniLcmRepository.cs (10)

• Task (26-29)
• Task (95-103)
• Task (105-112)
• Task (114-118)
• Task (120-125)
• Task (157-195)
• Task (215-233)
• Task (235-241)
• Task (243-249)
• Task (251-256)

backend/FwLite/FwLiteProjectSync.Tests/CrdtRepairTests.cs (1)

backend/FwLite/MiniLcm.Tests/ExampleSentenceTestsBase.cs (8)

• Fact (32-37)
• Fact (39-45)
• Fact (47-59)
• Fact (61-70)
• Fact (72-98)
• Fact (100-129)
• Fact (131-156)
• Task (9-30)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: Build FwHeadless / publish-fw-headless
• GitHub Check: Analyze (csharp)
• GitHub Check: Build FW Lite and run tests
• GitHub Check: frontend-component-unit-tests
• GitHub Check: frontend

🔇 Additional comments (10)

backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)

956-957: LGTM! Correctly implements null-returning behavior for missing entities.

The changes to GetEntry, GetSense, and GetExampleSentence properly use TryGetObject instead of GetObject, ensuring these methods return null when entities are not found rather than throwing exceptions. This aligns with the PR objectives and the methods' nullable return types.

Also applies to: 1427-1428, 1554-1555

backend/FwLite/MiniLcm.Tests/ExampleSentenceTestsBase.cs (1)

7-7: LGTM! Comprehensive test coverage for null-returning behavior.

The test additions effectively verify that GetExampleSentence returns null for missing entities and returns the correct data for existing ones. The setup in InitializeAsync creates a known example sentence to support the positive test case.

Also applies to: 18-27, 32-45

backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs (3)

27-31: LGTM!

Good modification to use a predefined ID for the Apple entry, enabling the new test to verify retrieval by ID.

---

100-105: LGTM!

The test correctly verifies that GetEntry returns null for a non-existent entry, directly addressing the PR objective.

---

107-113: LGTM!

The test correctly verifies that GetEntry returns the expected entry and validates its content.

backend/FwLite/LcmCrdt.Tests/MiniLcmTests/SenseTests.cs (1)

1-19: LGTM!

The test class correctly implements the fixture pattern for running SenseTestsBase tests against the LcmCrdt implementation. The async lifecycle management is properly handled.

backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/SenseTests.cs (1)

1-12: LGTM!

The test class correctly uses the xUnit collection fixture pattern to run SenseTestsBase tests against the FwData implementation. This is crucial for verifying that the FwData API now returns null instead of throwing when entities are not found.

backend/FwLite/MiniLcm.Tests/SenseTestsBase.cs (3)

8-21: LGTM!

The initialization correctly sets up test data with predefined IDs, enabling predictable testing of sense retrieval behavior across multiple implementations.

---

23-28: LGTM!

The test correctly verifies that GetSense returns null for a non-existent sense, directly addressing the PR objective to return null instead of throwing.

---

30-36: LGTM!

The test correctly verifies that GetSense returns the expected sense and validates its content.