Rebuild graph update system with contract-driven relationship semantics and support EF Core 10#12
Closed
WahidBitar wants to merge 8 commits into
Closed
Rebuild graph update system with contract-driven relationship semantics and support EF Core 10#12WahidBitar wants to merge 8 commits into
WahidBitar wants to merge 8 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughMajor v2 rebuild targeting .NET 10 / EF Core 10 that replaces inline graph-diff logic with a GraphUpdate orchestration subsystem, adds relationship strategies and exceptions, introduces a new test model plus unit and integration tests, modernizes project/CI files, and removes legacy v1 tests and models. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DbContext as DbContext
participant Orchestrator as GraphUpdateOrchestrator
participant Guard as OperationGuard
participant Strategies as RelationshipStrategies
Client->>DbContext: InsertUpdateOrDeleteGraph(updated, existing)
DbContext->>Orchestrator: UpdateGraph(context, updated, existing)
Orchestrator->>Guard: create OperationGuard
Orchestrator->>Orchestrator: ValidateNavigations()
loop per navigation
Orchestrator->>Orchestrator: IsLoaded? / ClassifyNavigation
alt unloaded & mutated
Orchestrator->>Guard: AddError(UnloadedNavigationMutationException)
else unsupported & mutated
Orchestrator->>Guard: AddError(UnsupportedNavigationMutatedException)
end
end
Orchestrator->>Guard: ThrowIfErrors()
alt errors exist
Guard-->>Orchestrator: throws GraphUpdateException
Orchestrator-->>DbContext: abort (exception)
DbContext-->>Client: exception (all-or-nothing)
else no errors
Orchestrator->>Orchestrator: ApplyNavigations(loaded only)
loop per loaded navigation
alt PureManyToMany
Orchestrator->>Strategies: PureManyToManyStrategy.Apply()
else PayloadManyToMany
Orchestrator->>Strategies: PayloadManyToManyStrategy.Apply()
else RequiredOneToOne
Orchestrator->>Strategies: RequiredOneToOneStrategy.RemoveDependent()
else OptionalOneToOne
Orchestrator->>Strategies: OptionalOneToOneStrategy.DetachDependent()
end
end
Orchestrator-->>DbContext: entity state updated
DbContext-->>Client: updated entity
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (10)
src/.idea/.idea.EFCore.UpdateGraph.V2/.idea/.gitignore (1)
1-15: Reconsider the approach to IDE configuration files.A
.gitignorefile inside the.ideadirectory is unusual and suggests a mixed strategy: committing some IDE files while excluding others. Typically, teams either:
- Exclude all IDE files (recommended): Add
.idea/to the root.gitignore- Commit all IDE files: Share all IDE settings for team consistency
The current approach may lead to:
- Incomplete IDE configuration sharing
- Merge conflicts on workspace-specific files
- Confusion about which IDE files are tracked
Additionally, Line 1 contains a BOM character (
) that could cause issues with some tools.📋 Recommended approach
Consider moving these ignore patterns to the repository root
.gitignoreand excluding the entire.idea/directory:# JetBrains Rider/IntelliJ .idea/ *.imlIf team consistency on IDE settings is desired, consider using
.editorconfiginstead, which is IDE-agnostic and more widely supported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/.idea/.idea.EFCore.UpdateGraph.V2/.idea/.gitignore` around lines 1 - 15, This .gitignore inside .idea is problematic and contains a leading BOM; remove or relocate it and consolidate IDE ignore rules into the repository root .gitignore (e.g., add entries to ignore .idea/ and *.iml) or choose to commit full IDE settings instead; specifically remove the file shown in the diff (the .idea/.gitignore with entries like /shelf/, /.idea.EFCore.UpdateGraph.V2.iml, /queries/, etc.), delete the BOM character at the start of the file if you keep it temporarily, and ensure the root .gitignore contains a single clear policy for IDE files (.idea/ and *.iml) or document the team decision.src/Diwink.Extensions.EntityFrameworkCore.TestModel/Diwink.Extensions.EntityFrameworkCore.TestModel.csproj (1)
9-11: Consider pinning the EF Core package version for reproducible builds.Using
10.*as version range is acceptable for a test project but may cause build inconsistencies across different environments or over time. Consider pinning to a specific version or using a central package management approach.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Diwink.Extensions.EntityFrameworkCore.TestModel/Diwink.Extensions.EntityFrameworkCore.TestModel.csproj` around lines 9 - 11, The PackageReference for Microsoft.EntityFrameworkCore.SqlServer uses a floating version "10.*" which can make builds non-reproducible; update the PackageReference in Diwink.Extensions.EntityFrameworkCore.TestModel.csproj to a specific pinned version (e.g., "10.1.0" or the exact tested release) or switch to central package/version management (Directory.Packages.props) and reference the centralized version there so the Project's PackageReference for Microsoft.EntityFrameworkCore.SqlServer is deterministic..github/workflows/dotnet-core.yml (2)
25-28: Consider replacing deprecated NuGet publish action.The
brandedoutcast/publish-nuget@v2.5.2action is no longer maintained. Consider using the officialdotnet nuget pushcommand directly or a maintained alternative.♻️ Proposed replacement using dotnet CLI
- - name: Publish SimpleGraph Update - uses: brandedoutcast/publish-nuget@v2.5.2 - with: - PROJECT_FILE_PATH: src/Diwink.Extensions.EntityFrameworkCore/Diwink.Extensions.EntityFrameworkCore.csproj - NUGET_KEY: ${{secrets.NUGET_API_KEY}} + - name: Pack + run: dotnet pack src/Diwink.Extensions.EntityFrameworkCore/Diwink.Extensions.EntityFrameworkCore.csproj --configuration Release --no-build -o ./nupkg + - name: Publish to NuGet + run: dotnet nuget push ./nupkg/*.nupkg --api-key ${{secrets.NUGET_API_KEY}} --source https://api.nuget.org/v3/index.json --skip-duplicate🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/dotnet-core.yml around lines 25 - 28, Replace the deprecated action uses: brandedoutcast/publish-nuget@v2.5.2 by switching to a dotnet CLI-based publish step: remove the brandedoutcast action invocation and add a step that builds the project (using the PROJECT_FILE_PATH value Diwink.Extensions.EntityFrameworkCore.csproj) and then runs dotnet nuget push with the package path and the secret referenced by NUGET_KEY; ensure the job authenticates to NuGet using the provided secret and sets the correct source and API key parameters for dotnet nuget push to publish the built package.
13-17: Update GitHub Actions to current versions.The workflow uses outdated action versions:
actions/checkout@v2→ current isv4actions/setup-dotnet@v1→ current isv4These older versions may lack security patches and improved functionality.
♻️ Proposed update
- - uses: actions/checkout@v2 + - uses: actions/checkout@v4 - name: Setup .NET Core - uses: actions/setup-dotnet@v1 + uses: actions/setup-dotnet@v4 with: - dotnet-version: 10 + dotnet-version: 10.x🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/dotnet-core.yml around lines 13 - 17, Update the GitHub Actions steps to use the current action versions: change the uses reference for actions/checkout from "actions/checkout@v2" to "actions/checkout@v4" and change actions/setup-dotnet from "actions/setup-dotnet@v1" to "actions/setup-dotnet@v4"; ensure the existing step name "Setup .NET Core" and the dotnet-version input remain intact or adapt inputs to the v4 schema if needed in the setup-dotnet action definition.src/Diwink.Extensions.EntityFrameworkCore.Tests.Unit/Diwink.Extensions.EntityFrameworkCore.Tests.Unit.csproj (1)
12-13: Address floating package versions across the solution for deterministic restores.Lines 12-13 use wildcard versions (
8.*and10.*), which can cause CI drift and hard-to-reproduce failures. This pattern extends across the entire codebase—seven packages in four projects use floating versions. Implement lock file-based determinism by:
- Adding
<RestorePackagesWithLockFile>true</RestorePackagesWithLockFile>to the<PropertyGroup>in this project (and ideally in aDirectory.Build.propsfor all projects)- Committing the generated
packages.lock.jsonfile to source controlAlternatively, pin specific versions for all packages (e.g.,
8.2.4instead of8.*).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Diwink.Extensions.EntityFrameworkCore.Tests.Unit/Diwink.Extensions.EntityFrameworkCore.Tests.Unit.csproj` around lines 12 - 13, The project file uses floating package versions (e.g., PackageReference Version="8.*" and "10.*") which causes non-deterministic restores; fix by adding the MSBuild property RestorePackagesWithLockFile set to true inside the project's <PropertyGroup> (or centrally in Directory.Build.props) so NuGet generates a packages.lock.json, commit that packages.lock.json to source control, and/or replace the wildcard Version attributes on PackageReference entries (e.g., FluentAssertions, Microsoft.EntityFrameworkCore.InMemory) with pinned concrete versions (e.g., 8.2.4) to ensure deterministic restores.src/Diwink.Extensions.EntityFrameworkCore/Diwink.Extensions.EntityFrameworkCore.csproj (1)
18-18: Pin the EF Core version to ensure reproducible builds.Using
Version="10.*"allows builds to pull any available 10.x.y version from NuGet, creating inconsistencies across developer environments and CI/CD pipelines. For a library package, specify a fixed version (e.g.,10.0.5, the current stable release) or constrain to a patch range (e.g.,10.0.*).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Diwink.Extensions.EntityFrameworkCore/Diwink.Extensions.EntityFrameworkCore.csproj` at line 18, The PackageReference for Microsoft.EntityFrameworkCore currently uses a floating version "10.*"; update the Project's PackageReference (the <PackageReference Include="Microsoft.EntityFrameworkCore" ...> entry) to a pinned version (e.g., "10.0.5") or a patch-constrained range (e.g., "10.0.*") to ensure reproducible builds—modify the Version attribute on the Microsoft.EntityFrameworkCore PackageReference accordingly.src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/SeedData.cs (1)
7-9: The XML summary is stale.These lines say each seed method returns created IDs, but
SeedFullScenarioAsyncreturns onlyTask. Either update the comment or return a result object so IntelliSense stays accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/SeedData.cs` around lines 7 - 9, The XML summary on the SeedData class is out of date: it claims "Each seed method returns the IDs of created entities" but SeedFullScenarioAsync returns Task (no result). Either update the XML summary to reflect that some seed methods (like SeedFullScenarioAsync) do not return IDs, or change SeedFullScenarioAsync to return a result object containing created IDs (e.g., introduce a SeedResult/SeedIds DTO and change SeedFullScenarioAsync signature to Task<SeedResult>, update callers to consume the result). Locate SeedFullScenarioAsync in SeedData.cs and apply one of these two options so IntelliSense and the method signatures stay consistent.src/Diwink.Extensions.EntityFrameworkCore/DbContextExtensions.cs (1)
30-36: Consider guarding nulls at the extension boundary.Keeping
ArgumentNullExceptionat the public API surface makes failures more predictable for callers than letting nulls fall through into the orchestrator.🛡️ Small API-boundary hardening
public static T InsertUpdateOrDeleteGraph<T>( this DbContext context, T updatedEntity, T existingEntity) where T : class { + ArgumentNullException.ThrowIfNull(context); + ArgumentNullException.ThrowIfNull(updatedEntity); + ArgumentNullException.ThrowIfNull(existingEntity); + return GraphUpdateOrchestrator.UpdateGraph(context, updatedEntity, existingEntity); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Diwink.Extensions.EntityFrameworkCore/DbContextExtensions.cs` around lines 30 - 36, Add null-argument guards to the public extension method InsertUpdateOrDeleteGraph on DbContext: validate that context, updatedEntity and existingEntity are not null and throw ArgumentNullException with the appropriate parameter name if any are null before calling GraphUpdateOrchestrator.UpdateGraph; this keeps the API boundary in DbContextExtensions predictable and prevents nulls from leaking into GraphUpdateOrchestrator.src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PayloadManyToManyStrategy.cs (1)
22-33: Redundant.ToList()call on line 23.
existingItemsis already aList<object>(line 19), soexistingItems.ToList()creates an unnecessary copy. You can iterateexistingItemsdirectly.Suggested fix
// Remove association entities not present in updated collection - foreach (var existingItem in existingItems.ToList()) + foreach (var existingItem in existingItems)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PayloadManyToManyStrategy.cs` around lines 22 - 33, In PayloadManyToManyStrategy (method containing the loop that removes association entities) remove the unnecessary copy by iterating existingItems directly instead of calling existingItems.ToList(); update the foreach to loop over existingItems so you don't allocate a duplicate list while still calling EntityKeyHelper.GetKeyValues(context.Entry(existingItem)) and context.Remove(existingItem) as before.src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/EntityKeyHelper.cs (1)
15-19: Null-forgiving operator onCurrentValuecould cause NRE.Line 18 uses
entry.Property(p.Name).CurrentValue!which assumes the value is never null. For nullable key components (uncommon but possible), this would throw or produce incorrect behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/EntityKeyHelper.cs` around lines 15 - 19, GetKeyValues currently uses the null-forgiving operator on entry.Property(p.Name).CurrentValue in the GetKeyValues method which can mask null key components and lead to NREs; update GetKeyValues to avoid the "!" by safely reading CurrentValue for each key property (entry.Property(p.Name).CurrentValue) and either preserve nulls in the returned object[] or throw a clear ArgumentException/InvalidOperationException with context (including the property name and entry) when a null primary-key component is unacceptable for downstream logic; locate the logic inside the GetKeyValues method and adjust the null handling and return to ensure no forced non-null cast occurs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 28-29: The README currently references a nonexistent v2 source
path 'src-v2/'; update the documentation text that mentions 'src-v2/' to the
actual directory 'src/' so the v2 rebuild path is accurate, ensuring any
sentence that reads "The v2 rebuild (`src-v2/`)" is changed to "The v2 rebuild
(`src/`)" (search for the literal 'src-v2/' in README.md and replace it with
'src/').
In
`@src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/SqlServerContainerFixture.cs`:
- Around line 11-12: The MsSqlContainer is being built with a floating image
tag; update the MsSqlBuilder invocation that constructs the private readonly
MsSqlContainer _container to use a pinned immutable SQL Server image tag (for
example replace "mcr.microsoft.com/mssql/server:2022-latest" with a specific CU
tag such as "mcr.microsoft.com/mssql/server:2022-CU14-ubuntu-22.04") so CI runs
are reproducible; locate the string in the MsSqlBuilder(...) call and replace it
with the chosen cumulative update tag.
In
`@src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/GraphUpdateOrchestrator.cs`:
- Around line 41-90: ValidateNavigations currently only checks the top-level
existingEntry so nested unloaded or unsupported mutations can bypass
OperationGuard when RelatedEntityMutationService.ProcessNavigations recurses;
fix by invoking the same validation for each child existing Entry before
recursing: for each loaded navigation entry (use existingEntry.Navigations where
n.IsLoaded and n.CurrentValue/EntityEntry is available) call ValidateNavigations
(or a new internal helper) with the child EntityEntry, the corresponding updated
child object, the aggregateType and the same OperationGuard, and ensure
HasMutations and HasUnloadedMutationAttempt checks are applied for child
navigations as well so UnsupportedNavigationMutatedException and
UnloadedNavigationMutationException are raised before any recursion/processing
in RelatedEntityMutationService.ProcessNavigations.
- Around line 273-316: HasMutations currently only checks collection membership
and key equality, so in-place edits to scalar fields of related entities are
ignored; update HasMutations (handling both CollectionEntry and ReferenceEntry)
to treat in-place edits as mutations by, after matching items by primary key
(using EntityKeyHelper.GetKeyValues / EntityKeyHelper.KeysEqual), comparing
scalar properties on the existing tracked entity vs the updated entity and
returning true if any scalar property value differs; use the entity type
metadata (entry.Metadata.GetProperties()) to iterate non-navigation, non-shadow
properties and compare values via
context.Entry(existing).Property(prop.Name).CurrentValue vs the corresponding
value from the updated object (null-safe), and apply the same scalar-comparison
logic for the ReferenceEntry branch when keys match.
In `@src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/OperationGuard.cs`:
- Around line 22-25: The AddError method on OperationGuard currently allows a
null GraphUpdateException to be added; change AddError to validate its parameter
(GraphUpdateException error) and throw an ArgumentNullException(nameof(error))
when null instead of adding it to _errors so ThrowIfErrors won't later fail with
a less actionable exception; update any unit-tests or call-sites if they relied
on nulls.
In
`@src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/RelatedEntityMutationService.cs`:
- Around line 111-115: The code in RelatedEntityMutationService that does "if
(updatedValue is null && existingValue is not null) { return; }" drops nested
reference removals during recursion; instead of returning, invoke the same
one-to-one removal strategy used for root-level references so child removals are
handled consistently. Replace the early return with a call into the one-to-one
strategy path (the same handler used elsewhere in this class for
required/optional one-to-one removals), passing existingValue, the parent
entity/context and navigation info so the removal (cascade/nullify) is applied
even when recursing.
- Around line 33-39: The code currently sends every loaded child collection to
ProcessCollectionNavigation in RelatedEntityMutationService, which leads to code
later calling context.Remove(existingItem) and deleting nested association rows
instead of un-linking or rejecting; change the CollectionEntry and
ReferenceEntry branches so nested/recursive child collections are routed through
the existing relationship strategy layer (instead of being processed directly by
ProcessCollectionNavigation/ProcessReferenceNavigation), and update the logic
that currently calls context.Remove(existingItem) (and similar delete paths) to
call the relationship strategy's unlink or reject methods for association-type
relationships (or defer to the strategy to decide delete vs unlink) so nested
association collections are unlinked rather than deleted. Ensure you reference
and use the relationship strategy APIs used elsewhere in
RelatedEntityMutationService when handling collection navigations.
In
`@src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PayloadManyToManyStrategy.cs`:
- Around line 68-72: The AddToCollection method currently silently ignores
missing Add implementations because it uses addMethod?.Invoke(...); update
AddToCollection to validate navigation.CurrentValue and the reflected addMethod:
if navigation.CurrentValue is null throw an InvalidOperationException noting the
navigation, and if GetMethod("Add") returns null throw an
InvalidOperationException (including the collection type name and navigation
info) so the failure is explicit; otherwise call
addMethod.Invoke(navigation.CurrentValue, new object[] { item }). Ensure
exceptions include enough context (collection type and item type) to aid
debugging.
In
`@src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PureManyToManyStrategy.cs`:
- Around line 95-104: Remove the silent no-op behavior in RemoveFromCollection
and AddToCollection by failing fast and using the collection interface instead
of relying on concrete public methods: check navigation.CurrentValue for null,
try to cast it to System.Collections.ICollection (or System.Collections.IList if
you need Remove/Add semantics), and call the appropriate Add/Remove via that
interface; if the cast fails or the operation cannot be performed, throw a clear
InvalidOperationException (mentioning CollectionEntry/navigation.CurrentValue
and the target item) rather than using the null-conditional reflection invoke
path in RemoveFromCollection and AddToCollection.
- Around line 49-55: This code uses pk.Properties.Select(p =>
p.PropertyInfo!.GetValue(updatedItem)!) which assumes CLR PropertyInfo exists
and can throw for shadow/field-only keys; instead reuse the already-resolved
updatedKeys (from the earlier metadata-safe extraction) or otherwise check
p.PropertyInfo for null before using it. Update the extraction in
PureManyToManyStrategy (the block using context.Model.FindEntityType, pk and
pk.Properties) to prefer the existing updatedKeys variable or guard against null
PropertyInfo rather than dereferencing PropertyInfo! to avoid
NullReferenceException for shadow/field-only PKs.
In `@src/Diwink.Extensions.EntityFrameworkCore/Traversal/NavigationLoadGuard.cs`:
- Around line 18-39: The recursive EnsureNavigationsLoaded(EntityEntry
existingEntry) can infinite-loop on cyclic navigation graphs; modify it to
accept or create a visited set (e.g., HashSet<object> visited or
HashSet<(IEntityType, key)> keyed by entry.Entity or entry.Metadata+primary key)
and check/add the current entry before recursing over existingEntry.Navigations;
update all recursive calls of EnsureNavigationsLoaded (including where it
handles CollectionEntry and ReferenceEntry) to pass the visited set so entities
already seen are skipped, preventing cycles and stack overflows.
---
Nitpick comments:
In @.github/workflows/dotnet-core.yml:
- Around line 25-28: Replace the deprecated action uses:
brandedoutcast/publish-nuget@v2.5.2 by switching to a dotnet CLI-based publish
step: remove the brandedoutcast action invocation and add a step that builds the
project (using the PROJECT_FILE_PATH value
Diwink.Extensions.EntityFrameworkCore.csproj) and then runs dotnet nuget push
with the package path and the secret referenced by NUGET_KEY; ensure the job
authenticates to NuGet using the provided secret and sets the correct source and
API key parameters for dotnet nuget push to publish the built package.
- Around line 13-17: Update the GitHub Actions steps to use the current action
versions: change the uses reference for actions/checkout from
"actions/checkout@v2" to "actions/checkout@v4" and change actions/setup-dotnet
from "actions/setup-dotnet@v1" to "actions/setup-dotnet@v4"; ensure the existing
step name "Setup .NET Core" and the dotnet-version input remain intact or adapt
inputs to the v4 schema if needed in the setup-dotnet action definition.
In `@src/.idea/.idea.EFCore.UpdateGraph.V2/.idea/.gitignore`:
- Around line 1-15: This .gitignore inside .idea is problematic and contains a
leading BOM; remove or relocate it and consolidate IDE ignore rules into the
repository root .gitignore (e.g., add entries to ignore .idea/ and *.iml) or
choose to commit full IDE settings instead; specifically remove the file shown
in the diff (the .idea/.gitignore with entries like /shelf/,
/.idea.EFCore.UpdateGraph.V2.iml, /queries/, etc.), delete the BOM character at
the start of the file if you keep it temporarily, and ensure the root .gitignore
contains a single clear policy for IDE files (.idea/ and *.iml) or document the
team decision.
In
`@src/Diwink.Extensions.EntityFrameworkCore.TestModel/Diwink.Extensions.EntityFrameworkCore.TestModel.csproj`:
- Around line 9-11: The PackageReference for
Microsoft.EntityFrameworkCore.SqlServer uses a floating version "10.*" which can
make builds non-reproducible; update the PackageReference in
Diwink.Extensions.EntityFrameworkCore.TestModel.csproj to a specific pinned
version (e.g., "10.1.0" or the exact tested release) or switch to central
package/version management (Directory.Packages.props) and reference the
centralized version there so the Project's PackageReference for
Microsoft.EntityFrameworkCore.SqlServer is deterministic.
In
`@src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/SeedData.cs`:
- Around line 7-9: The XML summary on the SeedData class is out of date: it
claims "Each seed method returns the IDs of created entities" but
SeedFullScenarioAsync returns Task (no result). Either update the XML summary to
reflect that some seed methods (like SeedFullScenarioAsync) do not return IDs,
or change SeedFullScenarioAsync to return a result object containing created IDs
(e.g., introduce a SeedResult/SeedIds DTO and change SeedFullScenarioAsync
signature to Task<SeedResult>, update callers to consume the result). Locate
SeedFullScenarioAsync in SeedData.cs and apply one of these two options so
IntelliSense and the method signatures stay consistent.
In
`@src/Diwink.Extensions.EntityFrameworkCore.Tests.Unit/Diwink.Extensions.EntityFrameworkCore.Tests.Unit.csproj`:
- Around line 12-13: The project file uses floating package versions (e.g.,
PackageReference Version="8.*" and "10.*") which causes non-deterministic
restores; fix by adding the MSBuild property RestorePackagesWithLockFile set to
true inside the project's <PropertyGroup> (or centrally in
Directory.Build.props) so NuGet generates a packages.lock.json, commit that
packages.lock.json to source control, and/or replace the wildcard Version
attributes on PackageReference entries (e.g., FluentAssertions,
Microsoft.EntityFrameworkCore.InMemory) with pinned concrete versions (e.g.,
8.2.4) to ensure deterministic restores.
In `@src/Diwink.Extensions.EntityFrameworkCore/DbContextExtensions.cs`:
- Around line 30-36: Add null-argument guards to the public extension method
InsertUpdateOrDeleteGraph on DbContext: validate that context, updatedEntity and
existingEntity are not null and throw ArgumentNullException with the appropriate
parameter name if any are null before calling
GraphUpdateOrchestrator.UpdateGraph; this keeps the API boundary in
DbContextExtensions predictable and prevents nulls from leaking into
GraphUpdateOrchestrator.
In
`@src/Diwink.Extensions.EntityFrameworkCore/Diwink.Extensions.EntityFrameworkCore.csproj`:
- Line 18: The PackageReference for Microsoft.EntityFrameworkCore currently uses
a floating version "10.*"; update the Project's PackageReference (the
<PackageReference Include="Microsoft.EntityFrameworkCore" ...> entry) to a
pinned version (e.g., "10.0.5") or a patch-constrained range (e.g., "10.0.*") to
ensure reproducible builds—modify the Version attribute on the
Microsoft.EntityFrameworkCore PackageReference accordingly.
In `@src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/EntityKeyHelper.cs`:
- Around line 15-19: GetKeyValues currently uses the null-forgiving operator on
entry.Property(p.Name).CurrentValue in the GetKeyValues method which can mask
null key components and lead to NREs; update GetKeyValues to avoid the "!" by
safely reading CurrentValue for each key property
(entry.Property(p.Name).CurrentValue) and either preserve nulls in the returned
object[] or throw a clear ArgumentException/InvalidOperationException with
context (including the property name and entry) when a null primary-key
component is unacceptable for downstream logic; locate the logic inside the
GetKeyValues method and adjust the null handling and return to ensure no forced
non-null cast occurs.
In
`@src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PayloadManyToManyStrategy.cs`:
- Around line 22-33: In PayloadManyToManyStrategy (method containing the loop
that removes association entities) remove the unnecessary copy by iterating
existingItems directly instead of calling existingItems.ToList(); update the
foreach to loop over existingItems so you don't allocate a duplicate list while
still calling EntityKeyHelper.GetKeyValues(context.Entry(existingItem)) and
context.Remove(existingItem) as before.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b86d08c0-bf2b-4c0a-b5fc-47671c300bc6
📒 Files selected for processing (73)
.github/workflows/dotnet-core.ymlEfCore.UpdateGraph.slnREADME.mdsrc/.idea/.idea.EFCore.UpdateGraph.V2/.idea/.gitignoresrc/.idea/.idea.EFCore.UpdateGraph.V2/.idea/.namesrc/.idea/.idea.EFCore.UpdateGraph.V2/.idea/encodings.xmlsrc/.idea/.idea.EFCore.UpdateGraph.V2/.idea/indexLayout.xmlsrc/.idea/.idea.EFCore.UpdateGraph.V2/.idea/vcs.xmlsrc/Diwink.Extensions.EntityFrameworkCore.TestModel/Configurations/CourseConfiguration.cssrc/Diwink.Extensions.EntityFrameworkCore.TestModel/Configurations/CourseMentorAssignmentConfiguration.cssrc/Diwink.Extensions.EntityFrameworkCore.TestModel/Configurations/CoursePolicyConfiguration.cssrc/Diwink.Extensions.EntityFrameworkCore.TestModel/Configurations/LearningCatalogConfiguration.cssrc/Diwink.Extensions.EntityFrameworkCore.TestModel/Configurations/MentorConfiguration.cssrc/Diwink.Extensions.EntityFrameworkCore.TestModel/Configurations/MentorWorkspaceConfiguration.cssrc/Diwink.Extensions.EntityFrameworkCore.TestModel/Configurations/TopicTagConfiguration.cssrc/Diwink.Extensions.EntityFrameworkCore.TestModel/Diwink.Extensions.EntityFrameworkCore.TestModel.csprojsrc/Diwink.Extensions.EntityFrameworkCore.TestModel/Entities/Course.cssrc/Diwink.Extensions.EntityFrameworkCore.TestModel/Entities/CourseMentorAssignment.cssrc/Diwink.Extensions.EntityFrameworkCore.TestModel/Entities/CoursePolicy.cssrc/Diwink.Extensions.EntityFrameworkCore.TestModel/Entities/LearningCatalog.cssrc/Diwink.Extensions.EntityFrameworkCore.TestModel/Entities/Mentor.cssrc/Diwink.Extensions.EntityFrameworkCore.TestModel/Entities/MentorWorkspace.cssrc/Diwink.Extensions.EntityFrameworkCore.TestModel/Entities/TopicTag.cssrc/Diwink.Extensions.EntityFrameworkCore.TestModel/TestDbContext.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Contracts/ManyToMany/ManyToManySafetyContractTests.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Contracts/ManyToMany/PayloadAssociationContractTests.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Contracts/ManyToMany/PureManyToManyContractTests.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Contracts/OneToOne/OptionalOneToOneContractTests.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Contracts/OneToOne/RequiredOneToOneContractTests.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Contracts/Rejection/PartialMutationNotAllowedTests.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Contracts/Rejection/UnloadedNavigationMutationTests.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Contracts/Rejection/UnsupportedRelationshipPatternTests.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Diwink.Extensions.EntityFrameworkCore.Tests.Integration.csprojsrc/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/DatabaseBootstrap.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/IntegrationTestBase.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/IntegrationTestCollection.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/SeedData.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/SqlServerContainerFixture.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Unit/Diwink.Extensions.EntityFrameworkCore.Tests.Unit.csprojsrc/Diwink.Extensions.EntityFrameworkCore.Tests.Unit/GraphDiff/OperationGuardTests.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Unit/RelationshipSemantics/ManyToManyDiffStrategyTests.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Unit/RelationshipSemantics/OneToOneOwnershipResolverTests.cssrc/Diwink.Extensions.EntityFrameworkCore/DbContextExtensions.cssrc/Diwink.Extensions.EntityFrameworkCore/Diwink.Extensions.EntityFrameworkCore.csprojsrc/Diwink.Extensions.EntityFrameworkCore/Exceptions/AmbiguousOwnershipSemanticsException.cssrc/Diwink.Extensions.EntityFrameworkCore/Exceptions/GraphUpdateException.cssrc/Diwink.Extensions.EntityFrameworkCore/Exceptions/PartialMutationNotAllowedException.cssrc/Diwink.Extensions.EntityFrameworkCore/Exceptions/UnloadedNavigationMutationException.cssrc/Diwink.Extensions.EntityFrameworkCore/Exceptions/UnsupportedNavigationMutatedException.cssrc/Diwink.Extensions.EntityFrameworkCore/Exceptions/UnsupportedRelationshipPatternException.cssrc/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/EntityKeyHelper.cssrc/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/GraphUpdateOrchestrator.cssrc/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/OperationGuard.cssrc/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/RelatedEntityMutationService.cssrc/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/OneToOneOwnershipResolver.cssrc/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/OptionalOneToOneStrategy.cssrc/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PayloadManyToManyStrategy.cssrc/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PureManyToManyStrategy.cssrc/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/RequiredOneToOneStrategy.cssrc/Diwink.Extensions.EntityFrameworkCore/Traversal/NavigationLoadGuard.cssrc/EFCore.UpdateGraph.slnxsrc/FakeModel/FakeModel.csprojsrc/FakeModel/Model.cssrc/UnitTests/FakeSchoolsDbContext.cssrc/UnitTests/FakeSchoolsDbContextFactory.cssrc/UnitTests/GraphUpdateTests.cssrc/UnitTests/Migrations/20200914083038_initialMigration.Designer.cssrc/UnitTests/Migrations/20200914083038_initialMigration.cssrc/UnitTests/Migrations/FakeSchoolsDbContextModelSnapshot.cssrc/UnitTests/TestHelpers.cssrc/UnitTests/UnitTests.csprojsrc/UnitTests/VariousTests.cssrc/UnitTests/appsettings.json
💤 Files with no reviewable changes (13)
- EfCore.UpdateGraph.sln
- src/UnitTests/TestHelpers.cs
- src/UnitTests/appsettings.json
- src/FakeModel/FakeModel.csproj
- src/UnitTests/VariousTests.cs
- src/UnitTests/FakeSchoolsDbContextFactory.cs
- src/UnitTests/Migrations/20200914083038_initialMigration.Designer.cs
- src/UnitTests/Migrations/20200914083038_initialMigration.cs
- src/UnitTests/Migrations/FakeSchoolsDbContextModelSnapshot.cs
- src/UnitTests/UnitTests.csproj
- src/UnitTests/FakeSchoolsDbContext.cs
- src/FakeModel/Model.cs
- src/UnitTests/GraphUpdateTests.cs
Comment on lines
+28
to
+29
| The v2 rebuild (`src-v2/`) targets .NET 10 and EF Core 10.x with explicit, | ||
| contract-driven relationship semantics. |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the documented v2 folder exists at repo root.
fd -t d '^src-v2$'
fd -t d '^src$'Repository: WahidBitar/EF-Core-Simple-Graph-Update
Length of output: 86
Update README to correct the v2 source path.
Line 28 references src-v2/ as the v2 rebuild location, but this directory does not exist in the repository. Only src/ exists. Update the documentation to reflect the actual path where v2 code resides.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 28 - 29, The README currently references a
nonexistent v2 source path 'src-v2/'; update the documentation text that
mentions 'src-v2/' to the actual directory 'src/' so the v2 rebuild path is
accurate, ensuring any sentence that reads "The v2 rebuild (`src-v2/`)" is
changed to "The v2 rebuild (`src/`)" (search for the literal 'src-v2/' in
README.md and replace it with 'src/').
Comment on lines
+11
to
+12
| private readonly MsSqlContainer _container = new MsSqlBuilder("mcr.microsoft.com/mssql/server:2022-latest") | ||
| .Build(); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
`For Testcontainers.MsSql version 4.0.0:
- Is
new MsSqlBuilder("mcr.microsoft.com/mssql/server:...")a supported constructor usage? - If not, what is the supported API to set a custom image (e.g.,
WithImage(...))? - Which immutable SQL Server image tags are recommended for stable CI runs?`
💡 Result:
- Yes, new MsSqlBuilder("mcr.microsoft.com/mssql/server:...") is a supported constructor usage in Testcontainers.MsSql version 4.0.0. The official documentation shows this exact usage: var msSqlContainer = new MsSqlBuilder("mcr.microsoft.com/mssql/server:2022-CU14-ubuntu-22.04").Build; 2) Not applicable, as the constructor is supported. For more advanced customization beyond the constructor, use chainable methods like WithPassword, WithDatabase on the MsSqlBuilder instance before calling Build. There is no specific WithImage method mentioned; the image is set via the constructor. 3) Recommended immutable SQL Server image tags for stable CI runs are specific Cumulative Update (CU) tags like "2022-CU14-ubuntu-22.04", as shown in official Testcontainers docs and source code. These are preferred over "latest" for reproducibility. Available tags listed at https://mcr.microsoft.com/en-us/artifact/mar/mssql/server/tags, with examples including 2022-CUxx-ubuntu-22.04 series for stability.
Citations:
- 1: https://dotnet.testcontainers.org/modules/mssql/
- 2: https://github.com/testcontainers/testcontainers-dotnet/blob/develop/src/Testcontainers.MsSql/MsSqlBuilder.cs
- 3: https://github.com/testcontainers/testcontainers-dotnet/releases/tag/4.10.0
- 4: https://hub.docker.com/r/microsoft/mssql-server
- 5: [Enhancement]: Update mssql default image tag to 2022 from 2019 testcontainers/testcontainers-dotnet#1189
Replace floating SQL Server image tag with a pinned Cumulative Update version.
Using 2022-latest reduces reproducibility across CI runs. The MsSqlBuilder constructor is correctly used, but should specify an immutable tag instead of the floating latest. Replace with a specific Cumulative Update release like 2022-CU14-ubuntu-22.04.
-private readonly MsSqlContainer _container = new MsSqlBuilder("mcr.microsoft.com/mssql/server:2022-latest")
- .Build();
+private readonly MsSqlContainer _container = new MsSqlBuilder("mcr.microsoft.com/mssql/server:2022-CU14-ubuntu-22.04")
+ .Build();Refer to available tags at https://mcr.microsoft.com/en-us/artifact/mar/mssql/server/tags.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private readonly MsSqlContainer _container = new MsSqlBuilder("mcr.microsoft.com/mssql/server:2022-latest") | |
| .Build(); | |
| private readonly MsSqlContainer _container = new MsSqlBuilder("mcr.microsoft.com/mssql/server:2022-CU14-ubuntu-22.04") | |
| .Build(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/SqlServerContainerFixture.cs`
around lines 11 - 12, The MsSqlContainer is being built with a floating image
tag; update the MsSqlBuilder invocation that constructs the private readonly
MsSqlContainer _container to use a pinned immutable SQL Server image tag (for
example replace "mcr.microsoft.com/mssql/server:2022-latest" with a specific CU
tag such as "mcr.microsoft.com/mssql/server:2022-CU14-ubuntu-22.04") so CI runs
are reproducible; locate the string in the MsSqlBuilder(...) call and replace it
with the chosen cumulative update tag.
Comment on lines
+41
to
+90
| private static void ValidateNavigations( | ||
| DbContext context, | ||
| EntityEntry existingEntry, | ||
| object updatedEntity, | ||
| Type aggregateType, | ||
| OperationGuard guard) | ||
| { | ||
| // Check loaded navigations for unsupported mutations | ||
| foreach (var navigation in existingEntry.Navigations | ||
| .Where(n => n.IsLoaded)) | ||
| { | ||
| var navMetadata = navigation.Metadata; | ||
| var entityPath = $"{existingEntry.Metadata.ClrType.Name}.{navMetadata.Name}"; | ||
|
|
||
| var classification = ClassifyNavigation(navMetadata); | ||
|
|
||
| if (classification == NavigationClassification.Unsupported) | ||
| { | ||
| // FR-018/FR-019: Check if mutations exist in unsupported navigation | ||
| if (HasMutations(context, navigation, updatedEntity, navMetadata)) | ||
| { | ||
| guard.AddError(new UnsupportedNavigationMutatedException( | ||
| entityPath, | ||
| GetRelationshipTypeName(navMetadata))); | ||
| } | ||
| // else: silently skip (FR-019) | ||
| } | ||
| } | ||
|
|
||
| // FR-015/FR-016: Check unloaded navigations for attempted mutations | ||
| foreach (var navigation in existingEntry.Navigations | ||
| .Where(n => !n.IsLoaded)) | ||
| { | ||
| var navMetadata = navigation.Metadata; | ||
|
|
||
| // Skip navigations pointing back to the aggregate root | ||
| if (navMetadata is INavigation nav && | ||
| nav.TargetEntityType.ClrType == aggregateType) | ||
| continue; | ||
|
|
||
| var entityPath = $"{existingEntry.Metadata.ClrType.Name}.{navMetadata.Name}"; | ||
|
|
||
| if (HasUnloadedMutationAttempt(updatedEntity, navMetadata)) | ||
| { | ||
| guard.AddError(new UnloadedNavigationMutationException( | ||
| entityPath, | ||
| navMetadata.Name)); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Validate child entries before recursing into them.
ValidateNavigations only inspects the current existingEntry, but Line 194 later recurses into child navigations through RelatedEntityMutationService.ProcessNavigations. Nested unloaded or unsupported mutations therefore bypass OperationGuard, so deeper graph changes can be partially applied instead of failing atomically.
Also applies to: 191-194
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/GraphUpdateOrchestrator.cs`
around lines 41 - 90, ValidateNavigations currently only checks the top-level
existingEntry so nested unloaded or unsupported mutations can bypass
OperationGuard when RelatedEntityMutationService.ProcessNavigations recurses;
fix by invoking the same validation for each child existing Entry before
recursing: for each loaded navigation entry (use existingEntry.Navigations where
n.IsLoaded and n.CurrentValue/EntityEntry is available) call ValidateNavigations
(or a new internal helper) with the child EntityEntry, the corresponding updated
child object, the aggregateType and the same OperationGuard, and ensure
HasMutations and HasUnloadedMutationAttempt checks are applied for child
navigations as well so UnsupportedNavigationMutatedException and
UnloadedNavigationMutationException are raised before any recursion/processing
in RelatedEntityMutationService.ProcessNavigations.
Comment on lines
+273
to
+316
| private static bool HasMutations( | ||
| DbContext context, | ||
| NavigationEntry navigation, | ||
| object updatedEntity, | ||
| INavigationBase navMetadata) | ||
| { | ||
| var navProperty = updatedEntity.GetType().GetProperty(navMetadata.Name); | ||
| if (navProperty is null) | ||
| return false; | ||
|
|
||
| var updatedValue = navProperty.GetValue(updatedEntity); | ||
|
|
||
| if (navigation is CollectionEntry collectionEntry) | ||
| { | ||
| var existingItems = collectionEntry.CurrentValue?.Cast<object>().ToList() ?? []; | ||
| var updatedItems = (updatedValue as IEnumerable<object>)?.ToList() ?? []; | ||
|
|
||
| if (existingItems.Count != updatedItems.Count) | ||
| return true; | ||
|
|
||
| // Compare by primary keys | ||
| foreach (var existingItem in existingItems) | ||
| { | ||
| var existingKeys = EntityKeyHelper.GetKeyValues(context.Entry(existingItem)); | ||
| var match = EntityKeyHelper.FindByKey(context, updatedItems, existingKeys); | ||
| if (match is null) | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| if (navigation is ReferenceEntry referenceEntry) | ||
| { | ||
| var existingValue = referenceEntry.CurrentValue; | ||
| if (existingValue is null && updatedValue is null) | ||
| return false; | ||
| if (existingValue is null || updatedValue is null) | ||
| return true; | ||
|
|
||
| // Both non-null — check if keys match | ||
| var existingKeys = EntityKeyHelper.GetKeyValues(context.Entry(existingValue)); | ||
| var updatedKeys = EntityKeyHelper.GetKeyValues(context, updatedValue); | ||
| return !EntityKeyHelper.KeysEqual(existingKeys, updatedKeys); |
Contributor
There was a problem hiding this comment.
Treat in-place edits on unsupported navigations as mutations.
HasMutations only checks collection membership and related keys. If the same child keys remain but scalar fields inside that unsupported navigation change, this returns false and the update is silently skipped instead of rejected.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/GraphUpdateOrchestrator.cs`
around lines 273 - 316, HasMutations currently only checks collection membership
and key equality, so in-place edits to scalar fields of related entities are
ignored; update HasMutations (handling both CollectionEntry and ReferenceEntry)
to treat in-place edits as mutations by, after matching items by primary key
(using EntityKeyHelper.GetKeyValues / EntityKeyHelper.KeysEqual), comparing
scalar properties on the existing tracked entity vs the updated entity and
returning true if any scalar property value differs; use the entity type
metadata (entry.Metadata.GetProperties()) to iterate non-navigation, non-shadow
properties and compare values via
context.Entry(existing).Property(prop.Name).CurrentValue vs the corresponding
value from the updated object (null-safe), and apply the same scalar-comparison
logic for the ReferenceEntry branch when keys match.
Comment on lines
+22
to
+25
| public void AddError(GraphUpdateException error) | ||
| { | ||
| _errors.Add(error); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard AddError against null input.
Without validation, a null entry can be stored and fail later in ThrowIfErrors with a less actionable exception.
Proposed fix
public void AddError(GraphUpdateException error)
{
+ ArgumentNullException.ThrowIfNull(error);
_errors.Add(error);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public void AddError(GraphUpdateException error) | |
| { | |
| _errors.Add(error); | |
| } | |
| public void AddError(GraphUpdateException error) | |
| { | |
| ArgumentNullException.ThrowIfNull(error); | |
| _errors.Add(error); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/OperationGuard.cs`
around lines 22 - 25, The AddError method on OperationGuard currently allows a
null GraphUpdateException to be added; change AddError to validate its parameter
(GraphUpdateException error) and throw an ArgumentNullException(nameof(error))
when null instead of adding it to _errors so ThrowIfErrors won't later fail with
a less actionable exception; update any unit-tests or call-sites if they relied
on nulls.
Comment on lines
+111
to
+115
| if (updatedValue is null && existingValue is not null) | ||
| { | ||
| // Reference removed — handled by specific one-to-one strategies later | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Don't drop nested reference removals on the recursive path.
Line 114 returns without invoking any required/optional one-to-one strategy. That works only on the root path; when this helper is recursing through a child entity, updatedChild.SomeReference = null is silently ignored.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/RelatedEntityMutationService.cs`
around lines 111 - 115, The code in RelatedEntityMutationService that does "if
(updatedValue is null && existingValue is not null) { return; }" drops nested
reference removals during recursion; instead of returning, invoke the same
one-to-one removal strategy used for root-level references so child removals are
handled consistently. Replace the early return with a call into the one-to-one
strategy path (the same handler used elsewhere in this class for
required/optional one-to-one removals), passing existingValue, the parent
entity/context and navigation info so the removal (cascade/nullify) is applied
even when recursing.
Comment on lines
+68
to
+72
| private static void AddToCollection(CollectionEntry navigation, object item) | ||
| { | ||
| var addMethod = navigation.CurrentValue!.GetType().GetMethod("Add"); | ||
| addMethod?.Invoke(navigation.CurrentValue, [item]); | ||
| } |
Contributor
There was a problem hiding this comment.
Silent failure if Add method is not found.
Using addMethod?.Invoke(...) silently ignores the case where the collection type doesn't have an Add method. This could lead to data loss where new association entities are silently dropped.
Consider throwing an exception if the method is not found to fail fast.
Suggested fix
private static void AddToCollection(CollectionEntry navigation, object item)
{
var addMethod = navigation.CurrentValue!.GetType().GetMethod("Add");
- addMethod?.Invoke(navigation.CurrentValue, [item]);
+ if (addMethod is null)
+ throw new InvalidOperationException(
+ $"Collection type {navigation.CurrentValue!.GetType().Name} does not have an Add method.");
+ addMethod.Invoke(navigation.CurrentValue, [item]);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PayloadManyToManyStrategy.cs`
around lines 68 - 72, The AddToCollection method currently silently ignores
missing Add implementations because it uses addMethod?.Invoke(...); update
AddToCollection to validate navigation.CurrentValue and the reflected addMethod:
if navigation.CurrentValue is null throw an InvalidOperationException noting the
navigation, and if GetMethod("Add") returns null throw an
InvalidOperationException (including the collection type name and navigation
info) so the failure is explicit; otherwise call
addMethod.Invoke(navigation.CurrentValue, new object[] { item }). Ensure
exceptions include enough context (collection type and item type) to aid
debugging.
Comment on lines
+49
to
+55
| var entityType = context.Model.FindEntityType(updatedItem.GetType()); | ||
| var pk = entityType?.FindPrimaryKey(); | ||
| if (pk is not null) | ||
| { | ||
| var keyValues = pk.Properties | ||
| .Select(p => p.PropertyInfo!.GetValue(updatedItem)!) | ||
| .ToArray(); |
Contributor
There was a problem hiding this comment.
Avoid PropertyInfo! for primary-key extraction.
Lines 53-55 assume every PK member has a CLR property. EF models with shadow or field-only keys will throw here even though Line 38 already resolved the key in a metadata-safe way. Reusing updatedKeys avoids turning this path into a NullReferenceException.
🔑 Minimal fix
if (pk is not null)
{
- var keyValues = pk.Properties
- .Select(p => p.PropertyInfo!.GetValue(updatedItem)!)
- .ToArray();
+ var keyValues = updatedKeys;
// Find checks tracker first, then queries store
var knownEntity = context.Find(entityType!.ClrType, keyValues);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| var entityType = context.Model.FindEntityType(updatedItem.GetType()); | |
| var pk = entityType?.FindPrimaryKey(); | |
| if (pk is not null) | |
| { | |
| var keyValues = pk.Properties | |
| .Select(p => p.PropertyInfo!.GetValue(updatedItem)!) | |
| .ToArray(); | |
| var entityType = context.Model.FindEntityType(updatedItem.GetType()); | |
| var pk = entityType?.FindPrimaryKey(); | |
| if (pk is not null) | |
| { | |
| var keyValues = updatedKeys; | |
| // Find checks tracker first, then queries store | |
| var knownEntity = context.Find(entityType!.ClrType, keyValues); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PureManyToManyStrategy.cs`
around lines 49 - 55, This code uses pk.Properties.Select(p =>
p.PropertyInfo!.GetValue(updatedItem)!) which assumes CLR PropertyInfo exists
and can throw for shadow/field-only keys; instead reuse the already-resolved
updatedKeys (from the earlier metadata-safe extraction) or otherwise check
p.PropertyInfo for null before using it. Update the extraction in
PureManyToManyStrategy (the block using context.Model.FindEntityType, pk and
pk.Properties) to prefer the existing updatedKeys variable or guard against null
PropertyInfo rather than dereferencing PropertyInfo! to avoid
NullReferenceException for shadow/field-only PKs.
Comment on lines
+95
to
+104
| private static void RemoveFromCollection(CollectionEntry navigation, object item) | ||
| { | ||
| var removeMethod = navigation.CurrentValue!.GetType().GetMethod("Remove"); | ||
| removeMethod?.Invoke(navigation.CurrentValue, [item]); | ||
| } | ||
|
|
||
| private static void AddToCollection(CollectionEntry navigation, object item) | ||
| { | ||
| var addMethod = navigation.CurrentValue!.GetType().GetMethod("Add"); | ||
| addMethod?.Invoke(navigation.CurrentValue, [item]); |
Contributor
There was a problem hiding this comment.
Don’t let collection mutations fail as silent no-ops.
If GetMethod("Add") or GetMethod("Remove") misses, the current ?.Invoke path reports success while leaving the relationship unchanged. This should fail fast at minimum; ideally it should invoke through the collection interface rather than hoping the concrete type exposes public methods.
🚨 Minimal fail-fast hardening
private static void RemoveFromCollection(CollectionEntry navigation, object item)
{
- var removeMethod = navigation.CurrentValue!.GetType().GetMethod("Remove");
- removeMethod?.Invoke(navigation.CurrentValue, [item]);
+ var collection = navigation.CurrentValue
+ ?? throw new InvalidOperationException($"Navigation '{navigation.Metadata.Name}' is null.");
+ var removeMethod = collection.GetType().GetMethod("Remove")
+ ?? throw new InvalidOperationException($"Navigation '{navigation.Metadata.Name}' does not expose a public Remove method.");
+ removeMethod.Invoke(collection, [item]);
}
private static void AddToCollection(CollectionEntry navigation, object item)
{
- var addMethod = navigation.CurrentValue!.GetType().GetMethod("Add");
- addMethod?.Invoke(navigation.CurrentValue, [item]);
+ var collection = navigation.CurrentValue
+ ?? throw new InvalidOperationException($"Navigation '{navigation.Metadata.Name}' is null.");
+ var addMethod = collection.GetType().GetMethod("Add")
+ ?? throw new InvalidOperationException($"Navigation '{navigation.Metadata.Name}' does not expose a public Add method.");
+ addMethod.Invoke(collection, [item]);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private static void RemoveFromCollection(CollectionEntry navigation, object item) | |
| { | |
| var removeMethod = navigation.CurrentValue!.GetType().GetMethod("Remove"); | |
| removeMethod?.Invoke(navigation.CurrentValue, [item]); | |
| } | |
| private static void AddToCollection(CollectionEntry navigation, object item) | |
| { | |
| var addMethod = navigation.CurrentValue!.GetType().GetMethod("Add"); | |
| addMethod?.Invoke(navigation.CurrentValue, [item]); | |
| private static void RemoveFromCollection(CollectionEntry navigation, object item) | |
| { | |
| var collection = navigation.CurrentValue | |
| ?? throw new InvalidOperationException($"Navigation '{navigation.Metadata.Name}' is null."); | |
| var removeMethod = collection.GetType().GetMethod("Remove") | |
| ?? throw new InvalidOperationException($"Navigation '{navigation.Metadata.Name}' does not expose a public Remove method."); | |
| removeMethod.Invoke(collection, [item]); | |
| } | |
| private static void AddToCollection(CollectionEntry navigation, object item) | |
| { | |
| var collection = navigation.CurrentValue | |
| ?? throw new InvalidOperationException($"Navigation '{navigation.Metadata.Name}' is null."); | |
| var addMethod = collection.GetType().GetMethod("Add") | |
| ?? throw new InvalidOperationException($"Navigation '{navigation.Metadata.Name}' does not expose a public Add method."); | |
| addMethod.Invoke(collection, [item]); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PureManyToManyStrategy.cs`
around lines 95 - 104, Remove the silent no-op behavior in RemoveFromCollection
and AddToCollection by failing fast and using the collection interface instead
of relying on concrete public methods: check navigation.CurrentValue for null,
try to cast it to System.Collections.ICollection (or System.Collections.IList if
you need Remove/Add semantics), and call the appropriate Add/Remove via that
interface; if the cast fails or the operation cannot be performed, throw a clear
InvalidOperationException (mentioning CollectionEntry/navigation.CurrentValue
and the target item) rather than using the null-conditional reflection invoke
path in RemoveFromCollection and AddToCollection.
Comment on lines
+18
to
+39
| public static void EnsureNavigationsLoaded(EntityEntry existingEntry) | ||
| { | ||
| foreach (var navigation in existingEntry.Navigations) | ||
| { | ||
| if (!navigation.IsLoaded) | ||
| continue; | ||
|
|
||
| // For collection navigations, recurse into loaded children | ||
| if (navigation is CollectionEntry collectionEntry && collectionEntry.CurrentValue is not null) | ||
| { | ||
| foreach (var child in collectionEntry.CurrentValue.Cast<object>()) | ||
| { | ||
| var childEntry = existingEntry.Context.Entry(child); | ||
| EnsureNavigationsLoaded(childEntry); | ||
| } | ||
| } | ||
| else if (navigation is ReferenceEntry referenceEntry && referenceEntry.CurrentValue is not null) | ||
| { | ||
| var childEntry = existingEntry.Context.Entry(referenceEntry.CurrentValue); | ||
| EnsureNavigationsLoaded(childEntry); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Guard the recursive walk against cycles.
Lines 30-37 recurse into child entries with no visited-set. A normal bidirectional graph like Catalog -> Courses -> Catalog will re-enter the same entities forever and eventually stack-overflow before validation finishes.
🔁 Minimal hardening
internal static class NavigationLoadGuard
{
/// <summary>
/// Ensures all navigations in the existing entity entry that the caller expects
/// to participate in graph mutation are explicitly loaded.
/// </summary>
public static void EnsureNavigationsLoaded(EntityEntry existingEntry)
+ => EnsureNavigationsLoaded(existingEntry, new HashSet<object>());
+
+ private static void EnsureNavigationsLoaded(
+ EntityEntry existingEntry,
+ HashSet<object> visited)
{
+ if (!visited.Add(existingEntry.Entity))
+ return;
+
foreach (var navigation in existingEntry.Navigations)
{
if (!navigation.IsLoaded)
continue;
// For collection navigations, recurse into loaded children
if (navigation is CollectionEntry collectionEntry && collectionEntry.CurrentValue is not null)
{
foreach (var child in collectionEntry.CurrentValue.Cast<object>())
{
var childEntry = existingEntry.Context.Entry(child);
- EnsureNavigationsLoaded(childEntry);
+ EnsureNavigationsLoaded(childEntry, visited);
}
}
else if (navigation is ReferenceEntry referenceEntry && referenceEntry.CurrentValue is not null)
{
var childEntry = existingEntry.Context.Entry(referenceEntry.CurrentValue);
- EnsureNavigationsLoaded(childEntry);
+ EnsureNavigationsLoaded(childEntry, visited);
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Diwink.Extensions.EntityFrameworkCore/Traversal/NavigationLoadGuard.cs`
around lines 18 - 39, The recursive EnsureNavigationsLoaded(EntityEntry
existingEntry) can infinite-loop on cyclic navigation graphs; modify it to
accept or create a visited set (e.g., HashSet<object> visited or
HashSet<(IEntityType, key)> keyed by entry.Entity or entry.Metadata+primary key)
and check/add the current entry before recursing over existingEntry.Navigations;
update all recursive calls of EnsureNavigationsLoaded (including where it
handles CollectionEntry and ReferenceEntry) to pass the visited set so entities
already seen are skipped, preventing cycles and stack overflows.
Update actions/checkout from v2 to v4, actions/setup-dotnet from v1 to v4, and change dotnet-version to "10.0.x". Also fix indentation and remove extra spaces in branch names.
WahidBitar
changed the title
Add a new publish job to the CI pipeline that builds, packs, and publishes the NuGet package on master pushes, with conditional checks for API key availability. Update action versions (checkout@v6, setup-dotnet@v5), adjust commands to target the solution file, and refine job names and permissions. BREAKING CHANGE: package version bumped from 2.0.0 to 10.0.0 to align with EF Core 10+ targeting, which may introduce incompatibilities.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/dotnet-core.yml (2)
21-26: Pin GitHub Actions to commit SHAs for supply-chain hardening.Floating major version tags are mutable and present a supply-chain risk. Use immutable commit SHAs instead.
Replace
actions/checkout@v6withactions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83ddandactions/setup-dotnet@v5withactions/setup-dotnet@c2fa09f4bde5fbb9d1777cf28262a3eb3db3ced7in both job sections (lines 21–26 and 47–52).Proposed patch
- uses: actions/checkout@v6 + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd - uses: actions/setup-dotnet@v5 + uses: actions/setup-dotnet@c2fa09f4bde5fbb9d1777cf28262a3eb3db3ced7🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/dotnet-core.yml around lines 21 - 26, Replace mutable action version tags with the provided immutable commit SHAs to harden the supply chain: change the uses entries for actions/checkout@v6 to actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd and actions/setup-dotnet@v5 to actions/setup-dotnet@c2fa09f4bde5fbb9d1777cf28262a3eb3db3ced7 in both occurrences of those steps (the Setup .NET SDK and checkout steps) so the workflow references the exact commits rather than floating major tags.
24-29: NuGet caching cannot be enabled without lock files. The repository does not containpackages.lock.jsonfiles, which are required forsetup-dotnet's cache feature to function. If you want to enable NuGet caching to reduce CI time, generate lock files first (e.g., usingdotnet nuget update-lock-fileor your package manager's lock file generation), then enablecache: truein thesetup-dotnetaction with appropriatecache-dependency-pathif needed.Also applies to: 50-55
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/dotnet-core.yml around lines 24 - 29, The workflow currently uses actions/setup-dotnet@v5 with dotnet-version: 10.0.x and runs "dotnet restore src/EFCore.UpdateGraph.slnx" but NuGet caching cannot be enabled without lock files; generate packages.lock.json for projects (e.g., run "dotnet nuget update-lock-file" locally or add a job step to produce lock files) and then update the setup-dotnet step to enable caching (set cache: true and provide cache-dependency-path matching the generated lock files) so the cache feature will work; refer to actions/setup-dotnet@v5, dotnet-version, the Restore step and cache/cache-dependency-path settings when applying the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/dotnet-core.yml:
- Around line 21-26: Replace mutable action version tags with the provided
immutable commit SHAs to harden the supply chain: change the uses entries for
actions/checkout@v6 to actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd
and actions/setup-dotnet@v5 to
actions/setup-dotnet@c2fa09f4bde5fbb9d1777cf28262a3eb3db3ced7 in both
occurrences of those steps (the Setup .NET SDK and checkout steps) so the
workflow references the exact commits rather than floating major tags.
- Around line 24-29: The workflow currently uses actions/setup-dotnet@v5 with
dotnet-version: 10.0.x and runs "dotnet restore src/EFCore.UpdateGraph.slnx" but
NuGet caching cannot be enabled without lock files; generate packages.lock.json
for projects (e.g., run "dotnet nuget update-lock-file" locally or add a job
step to produce lock files) and then update the setup-dotnet step to enable
caching (set cache: true and provide cache-dependency-path matching the
generated lock files) so the cache feature will work; refer to
actions/setup-dotnet@v5, dotnet-version, the Restore step and
cache/cache-dependency-path settings when applying the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ae5d6c6-8070-4d51-8ab2-d5d6c2b0c49f
📒 Files selected for processing (2)
.github/workflows/dotnet-core.ymlsrc/Diwink.Extensions.EntityFrameworkCore/Diwink.Extensions.EntityFrameworkCore.csproj
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Diwink.Extensions.EntityFrameworkCore/Diwink.Extensions.EntityFrameworkCore.csproj
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit