Ws changes cleanup#1588
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change refactors the creation and updating of writing systems by removing the explicit Changes
Assessment against linked issues
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…meter as there's a property for it in the WritingSystem class
# Conflicts: # backend/harmony
C# Unit Tests116 tests 116 ✅ 9s ⏱️ Results for commit d11b26b. ♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/FwLite/LcmCrdt.Tests/Changes/WritingSystemChangeTests.cs (1)
7-26: Test should be more explicit about duplicate deletion behaviorWhile the test correctly verifies that only one writing system remains after creating a duplicate, it would be clearer if the test name or assertion explicitly indicated that duplicates are marked as deleted rather than just not appearing.
Consider renaming the test to be more explicit about the deletion behavior:
- public async Task CreatingTheSameWritingSystemShouldResultInOnlyOne() + public async Task CreatingTheSameWritingSystemShouldMarkDuplicateAsDeleted()And potentially add an assertion that verifies the deleted status if possible.
🧹 Nitpick comments (5)
backend/FwLite/LcmCrdt/Utils/ChangeContextHelpers.cs (1)
30-34: Added convenient helper method for type-safe object retrievalThis new extension method provides a cleaner, strongly-typed way to retrieve objects from the change context without needing to specify the type name string manually. It improves code readability and reduces the chance of errors.
It would be good to add inline documentation for this method given its utility.
+/// <summary> +/// Gets all objects of type T from the change context. +/// </summary> +/// <typeparam name="T">The type of objects to retrieve</typeparam> +/// <returns>An async enumerable of objects of type T</returns> public static IAsyncEnumerable<T> GetObjectsOfType<T>(this IChangeContext context) where T : class, IObjectWithId { return context.GetObjectsOfType<T>(MiniLcmCrdtAdapter.GetObjectTypeName(typeof(T))); }backend/FwLite/MiniLcm/Validators/WritingSystemUpdateValidator.cs (1)
10-18: Duplicate validation for WsId propertyThere's redundant validation for the
WsIdproperty - it's being checked both in this custom validation block and again on line 19 using theNoOperationextension method. Consider removing this duplicate validation to simplify the code.- RuleFor(u => u.Patch).Custom((document, context) => - { - //todo pull this out to make it easy to define properties that aren't allowed to be changed - if (document.Operations.Any(o => - string.Equals(o.Path, - $"/{nameof(WritingSystem.WsId)}", - StringComparison.InvariantCultureIgnoreCase))) - context.AddFailure(nameof(WritingSystem.WsId), "Not allowed to update WsId"); - });backend/FwLite/MiniLcm/Validators/JsonPatchValidator.cs (1)
8-21: Well-implemented reusable validation rule with room for improvementThe
NoOperationextension method provides a clean, reusable way to prevent modification of specific properties. However, as noted in the TODO comment, the string-based property name approach could be improved.Consider enhancing this to use expression-based property selection for type safety:
public static IRuleBuilderOptionsConditions<UpdateObjectInput<T>, JsonPatchDocument<T>> NoOperation<T>( this IRuleBuilder<UpdateObjectInput<T>, JsonPatchDocument<T>> builder, - string propertyName//todo handle this better + Expression<Func<T, object>> propertyExpression ) where T : class { + string propertyName = GetPropertyName(propertyExpression); return builder.Custom((document, context) => { if (document.Operations.Any(o => string.Equals(o.Path, $"/{propertyName}", StringComparison.InvariantCultureIgnoreCase))) context.AddFailure(propertyName, "Not allowed to update " + propertyName); }); } + + private static string GetPropertyName<T, TProperty>(Expression<Func<T, TProperty>> expression) + { + if (expression.Body is MemberExpression memberExpression) + return memberExpression.Member.Name; + + throw new ArgumentException("Expression is not a member access", nameof(expression)); + }Also, consider handling nested property paths, as JSON Patch allows operations on nested properties (e.g.,
/address/city).backend/FwLite/MiniLcm.Tests/Validators/WritingSystemUpdateValidatorTests.cs (1)
15-44: Good test coverage with minor gapsThe tests provide good coverage for the validator, checking both successful and failed validation scenarios. However, there are a couple of improvements that could be made:
- Missing test for
DeletedAtproperty validation - the validator prevents updates to this property, but there's no test for it- The assertion for the "no changes" test could be more specific about which validation rule is being triggered
Add a test for the
DeletedAtproperty validation:[Fact] public void Fails_WhenTryingToUpdateDeletedAt() { var update = NewUpdate(); update.Set(ws => ws.DeletedAt, DateTimeOffset.Now); _validator.TestValidate(update).ShouldHaveValidationErrorFor(nameof(WritingSystem.DeletedAt)); }And make the assertion more specific in the "no changes" test:
[Fact] public void Fails_WhenThereAreNoChanges() { var update = NewUpdate(); - _validator.TestValidate(update).ShouldHaveAnyValidationError(); + _validator.TestValidate(update).ShouldHaveValidationErrorFor("Patch.Operations"); }backend/FwLite/LcmCrdt.Tests/Changes/WritingSystemChangeTests.cs (1)
38-38: Misleading comment about Type being ignoredThe comment suggests that the Type is ignored by the CreateApi, but according to the implementation in FwDataMiniLcmApi.cs and CreateWritingSystemChange.cs, the Type is actually used from the WritingSystem object.
- Type = WritingSystemType.Analysis//ignored by create api + Type = WritingSystemType.Analysis
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (26)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs(2 hunks)backend/FwLite/FwLiteProjectSync.Tests/Fixtures/SyncFixture.cs(1 hunks)backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs(1 hunks)backend/FwLite/FwLiteProjectSync.Tests/SyncTests.cs(1 hunks)backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs(1 hunks)backend/FwLite/FwLiteProjectSync/MiniLcmImport.cs(1 hunks)backend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.cs(1 hunks)backend/FwLite/FwLiteWeb/Hubs/MiniLcmApiHubBase.cs(1 hunks)backend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.cs(1 hunks)backend/FwLite/LcmCrdt.Tests/Changes/WritingSystemChangeTests.cs(1 hunks)backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs(1 hunks)backend/FwLite/LcmCrdt/Changes/CreateWritingSystemChange.cs(4 hunks)backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs(1 hunks)backend/FwLite/LcmCrdt/CrdtProjectsService.cs(1 hunks)backend/FwLite/LcmCrdt/Objects/MiniLcmCrdtAdapter.cs(1 hunks)backend/FwLite/LcmCrdt/Utils/ChangeContextHelpers.cs(2 hunks)backend/FwLite/MiniLcm.Tests/BasicApiTestsBase.cs(1 hunks)backend/FwLite/MiniLcm.Tests/SortingTestsBase.cs(1 hunks)backend/FwLite/MiniLcm.Tests/Validators/WritingSystemUpdateValidatorTests.cs(1 hunks)backend/FwLite/MiniLcm.Tests/WritingSystemTestsBase.cs(2 hunks)backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs(1 hunks)backend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs(1 hunks)backend/FwLite/MiniLcm/Validators/JsonPatchValidator.cs(1 hunks)backend/FwLite/MiniLcm/Validators/MiniLcmValidators.cs(3 hunks)backend/FwLite/MiniLcm/Validators/WritingSystemUpdateValidator.cs(1 hunks)backend/harmony(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
backend/FwLite/MiniLcm.Tests/WritingSystemTestsBase.cs (1)
backend/FwLite/MiniLcm/Models/WritingSystem.cs (1)
Guid(22-25)
backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs (2)
backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (1)
WritingSystem(101-115)backend/FwLite/MiniLcm/Models/WritingSystem.cs (1)
WritingSystem(31-45)
backend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.cs (1)
backend/FwLite/LcmCrdt/Changes/CreateWritingSystemChange.cs (1)
CreateWritingSystemChange(11-55)
backend/FwLite/LcmCrdt/Changes/CreateWritingSystemChange.cs (9)
backend/FwLite/MiniLcm/Models/WritingSystem.cs (2)
WritingSystem(31-45)Guid(22-25)backend/FwLite/LcmCrdt/Objects/MiniLcmCrdtAdapter.cs (1)
Guid(26-29)backend/FwLite/LcmCrdt/Utils/ChangeContextHelpers.cs (2)
ValueTask(9-12)ValueTask(14-18)backend/FwLite/LcmCrdt/Changes/CreateEntryChange.cs (1)
ValueTask(34-44)backend/FwLite/LcmCrdt/Changes/CreateExampleSentenceChange.cs (1)
ValueTask(34-46)backend/FwLite/LcmCrdt/Changes/CreateSenseChange.cs (1)
ValueTask(36-49)backend/FwLite/LcmCrdt/Changes/AddSemanticDomainChange.cs (1)
ValueTask(12-23)backend/FwLite/LcmCrdt/Changes/CreatePartOfSpeechChange.cs (1)
ValueTask(14-17)backend/FwLite/LcmCrdt/Changes/SetPartOfSpeechChange.cs (1)
ValueTask(11-29)
🔇 Additional comments (32)
backend/harmony (1)
1-1:Details
✅ Verification successful
Submodule update for Harmony
This bump aligns the Harmony subproject with upstream changes introducing the newIChangeContextAPI. Please ensure the referenced commit exists in the remote and that the submodule is properly synchronized and initialized before building.Run this script to verify the commit is present in the remote and submodule is up‐to‐date:
🏁 Script executed:
#!/bin/bash set -e # Get the URL for the harmony submodule url=$(git config -f .gitmodules --get submodule.backend/harmony.url) echo "Verifying commit 5207421436e557c6d0704dbd3e61ca9cec680960 in $url" # Check remote for the commit if git ls-remote "$url" | grep -q 5207421436e557c6d0704dbd3e61ca9cec680960; then echo "✅ Commit found in remote." else echo "❌ Commit not found in remote backend/harmony!" exit 1 fi # Sync and update submodule git submodule sync backend/harmony git submodule update --init backend/harmony echo "Submodule backend/harmony is synchronized and updated."Length of output: 1012
Submodule backend/harmony Verified and Updated
Commit 5207421436e557c6d0704dbd3e61ca9cec680960 was found in https://github.com/sillsdev/harmony.git, and the submodule has been synchronized and initialized successfully. Ready to merge.
backend/FwLite/FwLiteProjectSync.Tests/Fixtures/SyncFixture.cs (1)
105-105: Clean signature refactoring.The API call has been updated to the new
CreateWritingSystemsignature, which no longer requires a separate type parameter since it's now derived from theWritingSystemobject'sTypeproperty. This change eliminates potential inconsistencies between the explicit type parameter and the type property in the writing system object.backend/FwLite/MiniLcm/IMiniLcmWriteApi.cs (1)
10-10: Improved API design.The method signature has been simplified to remove the redundant
WritingSystemTypeparameter. This is a good change as it makes the API more intuitive by deriving the type from theWritingSystemobject itself, eliminating the possibility of inconsistencies between the explicit type parameter and theTypeproperty of the object.backend/FwLite/MiniLcm.Tests/WritingSystemTestsBase.cs (2)
26-34: Test updated to match new API signature.The test now properly creates a WritingSystem with the type specified in the object rather than as a separate parameter, which aligns with the API change. This ensures that tests validate the new API contract correctly.
44-61: Duplicate detection test correctly updated.The test now uses the updated API signature, explicitly setting the
Typeproperty in theWritingSystemobject. The test still correctly verifies that creating a writing system with a duplicate WsId throws aDuplicateObjectException.backend/FwLite/MiniLcm.Tests/BasicApiTestsBase.cs (1)
75-84: Test correctly implements new API signature.The test for verifying that multiple writing systems don't have duplicate orders has been correctly updated to use the new API signature. The
Typeproperty is now set directly in theWritingSystemobject rather than passed as a separate parameter.backend/FwLite/MiniLcm/SyncHelpers/WritingSystemSync.cs (1)
63-63: LGTM! API signature update correctly implemented.This change correctly implements the API signature update where
CreateWritingSystemnow accepts only the WritingSystem object and derives the type from itsTypeproperty, eliminating the need for a separate parameter.backend/FwLite/LcmCrdt.Tests/Changes/UseChangesTests.cs (1)
114-114: LGTM! Updated constructor call aligns with new signature.The
CreateWritingSystemChangeconstructor call has been correctly updated to match the new signature that removes the explicit WritingSystemType parameter, relying instead on theTypeproperty of the WritingSystem object.backend/FwLite/FwLiteProjectSync.Tests/Sena3SyncTests.cs (1)
71-71: LGTM! API call correctly updated.The call to
_crdtApi.CreateWritingSystemhas been properly updated to pass only the WritingSystem object without the explicit WritingSystemType parameter, which aligns with the refactored API.backend/FwLite/MiniLcm.Tests/SortingTestsBase.cs (1)
40-48: LGTM! WritingSystem creation correctly includes Type property.The API call has been properly updated to create a WritingSystem object with all necessary properties, including
Type = WritingSystemType.Vernacular, which replaces the previous pattern of passing the type as a separate parameter.backend/FwLite/LcmCrdt/CrdtProjectsService.cs (1)
209-250: LGTM! Simplified API usage by moving type information to the objectThe changes to the
SampleProjectDatamethod are clean and consistent. By incorporating theTypeproperty directly within theWritingSystemobject initialization, the code now follows a more object-oriented approach, which aligns with the PR objective of exposing a new API to fetch writing systems by WsId.backend/FwLite/FwLiteShared/Services/MiniLcmJsInvokable.cs (1)
120-122: Well-handled API evolution with backward compatibilityThe implementation now forwards only the
writingSystemparameter to the underlying API while maintaining the existing method signature. This is an excellent approach that preserves backward compatibility for JavaScript callers while adapting to the new underlying API design.backend/FwLite/FwLiteProjectSync/DryRunMiniLcmApi.cs (1)
21-24: API signature updated consistently for dry run implementationThe
CreateWritingSystemmethod has been properly updated to align with the new API signature, now accepting only theWritingSystemobject. The implementation correctly references the object'sTypeproperty when creating the dry run record.backend/FwLite/FwLiteProjectSync.Tests/SyncTests.cs (1)
418-419: Test calls updated to match new API patternThe test code has been properly updated to use the new API pattern, embedding the writing system type within the
WritingSystemobject rather than passing it as a separate parameter. This ensures the tests remain functional with the updated API design.backend/FwLite/FwLiteProjectSync/MiniLcmImport.cs (2)
106-106: Simplified API call by removing explicit type parameterThe removal of the WritingSystemType parameter from the CreateWritingSystem call aligns with the broader refactoring to simplify the API. Now the method correctly relies on the
Typeproperty that's already part of the WritingSystem object itself.
112-112: Consistent implementation for both Analysis and Vernacular writing systemsThis change mirrors the same simplification made to the Analysis writing systems above, ensuring consistent implementation across both writing system types.
backend/FwLite/LcmCrdt.Tests/MiniLcmApiFixture.cs (1)
41-59:Details
✅ Verification successful
Updated test fixture to use new CreateWritingSystem API pattern
The updated code correctly sets the
Typeproperty directly on the WritingSystem objects rather than passing it as a separate parameter. This aligns with the API changes throughout the codebase.Note: There's a potential issue with creating two writing systems with the same WsId "en" but different types. Verify this is the intended behavior for test purposes.
🏁 Script executed:
#!/bin/bash # Check if this pattern of duplicate WsIds is used elsewhere in tests echo "Checking for other instances of duplicate WsIds in tests..." rg -A 1 -B 1 'WsId = "(\w+)"' --glob="*.cs" | sort | uniq -c | sort -nr | head -10Length of output: 1047
Duplicate WsId usage is intentional and consistent
The pattern of creating two WritingSystem instances with the sameWsId = "en"but differentTypevalues already exists inbackend/FwLite/LcmCrdt/CrdtProjectsService.cs. This confirms the test fixture is mirroring the service behavior and is intended for coverage. No further changes are needed.
- See
backend/FwLite/LcmCrdt/CrdtProjectsService.cswhere both Vernacular and Analysis systems shareWsId = "en".backend/FwLite/LcmCrdt/Utils/ChangeContextHelpers.cs (1)
1-2: New imports support the GetObjectsOfType extension methodThese imports are necessary for the new extension method. The change correctly brings in the required namespaces.
backend/FwLite/LcmCrdt/Objects/MiniLcmCrdtAdapter.cs (1)
41-51: Good refactoring to improve code reuseThe extraction of the type name logic into a static method allows for broader reuse within the system. This is beneficial for the new
GetObjectsOfType<T>extension method mentioned in the AI summary.The existing TODO comment about potential issues with refactor renaming remains relevant. Consider using
Type.FullNameor a more robust type identifier mechanism to avoid potential name collisions if multiple classes with the same name exist in different namespaces.backend/FwLite/MiniLcm/Validators/WritingSystemUpdateValidator.cs (1)
19-23: Clean and concise validator implementationThe
NoOperationcalls for the immutable properties and the check for non-empty operations are well-implemented. This creates a robust validation for writing system updates that properly protects critical fields from modification.The TODO comment on line 12 has been addressed with the
NoOperationextension method fromJsonPatchValidator, which makes it easier to define properties that aren't allowed to be changed.backend/FwLite/MiniLcm/Validators/MiniLcmValidators.cs (3)
14-15: Good addition of the WritingSystemUpdateValidator dependencyThe addition of
WritingSystemUpdateValidatoras a dependency in the validator record follows the established pattern in the codebase for validator dependencies.
51-55: LGTM! Appropriate validation method implementationThe implementation of
ValidateAndThrowforUpdateObjectInput<WritingSystem>correctly follows the pattern used for other model types in this file.
70-70: Properly registered the validator serviceThe transient registration of the
WritingSystemUpdateValidatorin the dependency injection container is consistent with how other validators are registered.backend/FwLite/LcmCrdt/Changes/CreateWritingSystemChange.cs (3)
3-3: Correctly added Utils namespace importThis import is needed for the new functionality that checks for existing writing systems.
22-31: Good simplification of the constructorRemoving the explicit
WritingSystemTypeparameter and deriving it from theWritingSystemobject simplifies the API and makes it more consistent.
39-53: Well-implemented duplicate detection logicThe change from synchronous to asynchronous method properly supports querying the context for existing writing systems. The implementation sets
DeletedAtfor duplicates instead of throwing an exception, which aligns with the CRDT pattern of handling conflicts.backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs (2)
149-152: Good simplification of API method signatureRemoving the explicit type parameter and deriving it from the WritingSystem object makes the API more intuitive and consistent with other implementations in the codebase.
190-190: Properly added validation for writing system updatesThe addition of validation for update inputs is a good security practice that will prevent invalid updates to immutable properties like
WsIdandType.backend/FwLite/LcmCrdt.Tests/Changes/WritingSystemChangeTests.cs (1)
28-45: Good test for multi-type creation scenarioThis test properly verifies that writing systems with the same WsId but different types can coexist, which is a key requirement of the feature.
backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs (3)
96-96: Improved API design by simplifying method signature.The method signature has been improved by removing the explicit
WritingSystemTypeparameter and instead deriving it from theWritingSystemobject itself, which avoids potential inconsistencies.
100-104: Enhanced duplicate checking and error handling.The implementation now:
- Uses
AnyAsyncto check for duplicates with bothWsIdandTypebefore adding changes- Throws a more descriptive exception when duplicates are found
- Properly passes the writing system type from the object to the change creation
- Returns the created writing system with appropriate error handling
This approach is more direct and efficient than relying on database constraint exceptions.
109-109: Added validation to protect immutable properties.Added validation for writing system updates, which prevents modifications to immutable properties like
WsIdandType, enhancing data integrity.
myieye
left a comment
myieye
left a comment
There was a problem hiding this comment.
Looks good to me. Nice to see that redundant ws-type parameter get cleaned up 🤠 👍
2 participants
fixes #1586
Currently this fails as there's no way inside our change context to fetch a writing system by it's WsId. In order to do that I think we need to expose a new api in Harmony on
IChangeContextto allow for this