feat : Fix for migration and course final#90
Conversation
📝 WalkthroughWalkthroughThe ChangesCurriculumSection Entity Mutation and JSON Support
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/Src/PIED_LMS.Domain/Entities/CurriculumSection.cs (1)
22-29:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix: domain validation is executed during JSON deserialization and can break course reads.
backend/Src/PIED_LMS.Domain/Entities/CurriculumSection.csputs[JsonConstructor]on the constructor that enforces invariants viaArgumentExceptionwhenTitle,Summary, orContentare empty/whitespace (lines 19-34).backend/Src/PIED_LMS.Persistence/Configurations/CourseConfiguration.csthen deserializesCourse.Curriculumduring EF materialization with no exception handling (lines 58-65):
- Any persisted curriculum section with empty/all-whitespace
Content(or emptyTitle/Summary) will throw during deserialization, causing the read to fail for that row—and typically aborts the entire course query/request.Move/soften this validation at the persistence boundary (e.g., custom JSON converter / try-catch in the converter that maps invalid items to a safe default), or run a DB migration/sanitization to guarantee no legacy/external JSON can violate these invariants before merging.
// CurriculumSection.cs (current state) if (string.IsNullOrWhiteSpace(title)) throw new ArgumentException("Curriculum section must have a title."); if (string.IsNullOrWhiteSpace(summary)) throw new ArgumentException("Curriculum section must have a summary."); if (content == null || content.Count == 0 || content.All(string.IsNullOrWhiteSpace)) throw new ArgumentException("Curriculum section must have at least one valid content item.");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/Src/PIED_LMS.Domain/Entities/CurriculumSection.cs` around lines 22 - 29, The JsonConstructor on CurriculumSection currently throws ArgumentException for invalid title/summary/content (checks on title, summary, content) which causes EF materialization in CourseConfiguration when deserializing Course.Curriculum to fail; instead, remove or relax strict validation from the JsonConstructor and push strict invariant checks into domain factory/validation methods used at write-time, or implement a custom JsonConverter used by the persistence layer that wraps deserialization of CurriculumSection and on error (or when content/title/summary are empty) maps to a safe default instance or sanitizes values (e.g., empty strings -> null/empty allowed, content -> empty list) so reads never throw during EF materialization; update CourseConfiguration to use that converter or catch deserialization errors and substitute a safe fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/Src/PIED_LMS.Domain/Entities/CurriculumSection.cs`:
- Line 9: CurriculumSection exposes a mutable List<string> via the Content
property which breaks encapsulation and conflicts with System.Text.Json
constructor binding; change the property signature from List<string> to
IReadOnlyList<string> and update the [JsonConstructor] parameter on the
CurriculumSection constructor to also accept IReadOnlyList<string>, then store a
defensive copy into a private List<string> backing field (e.g., _content) and
have the public Content return the IReadOnlyList view (or the backing field as
IReadOnlyList) so deserialization types match and consumers cannot mutate the
internal list.
---
Outside diff comments:
In `@backend/Src/PIED_LMS.Domain/Entities/CurriculumSection.cs`:
- Around line 22-29: The JsonConstructor on CurriculumSection currently throws
ArgumentException for invalid title/summary/content (checks on title, summary,
content) which causes EF materialization in CourseConfiguration when
deserializing Course.Curriculum to fail; instead, remove or relax strict
validation from the JsonConstructor and push strict invariant checks into domain
factory/validation methods used at write-time, or implement a custom
JsonConverter used by the persistence layer that wraps deserialization of
CurriculumSection and on error (or when content/title/summary are empty) maps to
a safe default instance or sanitizes values (e.g., empty strings -> null/empty
allowed, content -> empty list) so reads never throw during EF materialization;
update CourseConfiguration to use that converter or catch deserialization errors
and substitute a safe fallback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7f8e37d-c3c6-4853-b822-3c32f00b9694
⛔ Files ignored due to path filters (2)
backend/Src/PIED_LMS.Persistence/Migrations/20260603020622_AddMaxCapacityAndCurrentEnrollment.Designer.csis excluded by!**/*.Designer.cs,!**/Migrations/**and included bybackend/Src/**backend/Src/PIED_LMS.Persistence/Migrations/20260603020622_AddMaxCapacityAndCurrentEnrollment.csis excluded by!**/Migrations/**and included bybackend/Src/**
📒 Files selected for processing (1)
backend/Src/PIED_LMS.Domain/Entities/CurriculumSection.cs
| public string Title { get; private set; } | ||
| public string Summary { get; private set; } | ||
| public IReadOnlyList<string> Content { get; private set; } | ||
| public List<string> Content { get; private set; } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Can System.Text.Json deserialize a JSON array into a constructor parameter of type List<string> while the corresponding property is declared as IReadOnlyList<string>?
💡 Result:
No, System.Text.Json cannot directly deserialize a JSON array into a constructor parameter of type List if the corresponding property is declared as IReadOnlyList [1][2][3]. The System.Text.Json serializer requires that the type of a parameter in a parameterized constructor match the type of the corresponding property or field exactly (using case-insensitive name matching) [4][5][1][2]. Even though a List is assignable to an IReadOnlyList, the serializer's constructor binding logic is restrictive and does not support this assignment automatically [2][6]. If you attempt this, the deserializer will typically throw an InvalidOperationException indicating that it cannot bind the constructor parameters [2][6][7]. To resolve this, you must align the types. The most common and recommended solutions include: 1. Update the constructor parameter to use the same interface type (i.e., use IReadOnlyList for both the property and the constructor parameter) [3][8]. System.Text.Json is capable of instantiating and populating IReadOnlyList directly [8]. 2. Use a property with a private setter and a parameterless constructor, allowing the serializer to set the property directly rather than through the constructor [1]. 3. Add an additional constructor specifically for the serializer marked with [JsonConstructor] that accepts the exact type of the property [5][1]. This strict requirement is a known behavior of System.Text.Json designed to ensure clarity and performance during the constructor binding process [2][6].
Citations:
- 1: https://stackoverflow.com/questions/76174798/deserialization-using-system-text-json-fails-when-constructor-parameter-doesnt
- 2: JsonSerializer constructor binding should support differences between property and parameter so long the property is assignable to the parameter dotnet/runtime#53303
- 3: https://stackoverflow.com/questions/71181033/system-text-json-wont-deserialize-to-my-type-even-though-my-constructor-impleme
- 4: https://stackoverflow.com/questions/79519067/how-to-make-deserialization-work-with-a-private-field
- 5: https://stackoverflow.com/questions/72268018/jsonconstructor-fails-on-ienumerable-property
- 6: System.Text.Json deserialization failing on classes with field encapsulation dotnet/runtime#55318
- 7: https://stackoverflow.com/questions/76529636/system-text-json-how-to-deserialize-class-with-interface-properties-net-6
- 8: https://stackoverflow.com/questions/75988393/how-do-you-properly-deserialize-a-class-that-has-an-ireadonlycollectiont-using
Fix CurriculumSection.Content encapsulation and align it with System.Text.Json constructor binding
Exposing public List<string> Content allows consumers to mutate the entity after construction (Add, Clear, etc.), bypassing the constructor validation. The defensive copy in the constructor protects the input list, but the internal list reference is still exposed.
The suggested “change only the property to IReadOnlyList<string>” approach is wrong for persistence: System.Text.Json constructor binding requires the [JsonConstructor] parameter type to match the property/field type exactly—so the constructor parameter must change as well (or deserialization will fail).
♻️ Proposed fix
- public List<string> Content { get; private set; }
+ public IReadOnlyList<string> Content { get; private set; }
@@
- public CurriculumSection(string title, string summary, List<string> content)
+ public CurriculumSection(string title, string summary, IReadOnlyList<string> content)
{
@@
- Content = new List<string>(content);
+ Content = new List<string>(content).AsReadOnly();
}Downstream usage remains fine (e.g., GetCourseCurriculumHandler calling c.Content.ToList()).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/Src/PIED_LMS.Domain/Entities/CurriculumSection.cs` at line 9,
CurriculumSection exposes a mutable List<string> via the Content property which
breaks encapsulation and conflicts with System.Text.Json constructor binding;
change the property signature from List<string> to IReadOnlyList<string> and
update the [JsonConstructor] parameter on the CurriculumSection constructor to
also accept IReadOnlyList<string>, then store a defensive copy into a private
List<string> backing field (e.g., _content) and have the public Content return
the IReadOnlyList view (or the backing field as IReadOnlyList) so
deserialization types match and consumers cannot mutate the internal list.
2 participants
Summary by CodeRabbit