Skip to content

feat : Fix for migration and course final #90

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
KietPham-VN merged 1 commit into from
Jun 3, 2026
Merged

feat : Fix for migration and course final #90

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions backend/Src/PIED_LMS.Domain/Entities/CurriculumSection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
using System.Text.Json.Serialization;

namespace PIED_LMS.Domain.Entities;

public class CurriculumSection
{
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; }

@coderabbitai coderabbitai Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 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:

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.


// Required for EF Core serialization/deserialization or default instantiation
private CurriculumSection()
Expand All @@ -14,6 +16,7 @@ private CurriculumSection()
Content = null!;
}

[JsonConstructor]
public CurriculumSection(string title, string summary, List<string> content)
{
if (string.IsNullOrWhiteSpace(title))
Expand All @@ -27,6 +30,6 @@ public CurriculumSection(string title, string summary, List<string> content)

Title = title;
Summary = summary;
Content = new List<string>(content).AsReadOnly();
Content = new List<string>(content);
}
}
Loading
Loading