Rebuild graph update system with contract-driven relationship semantics v2 rebuild: EF Core 10 targeting with modular refactoring @coderabbitai#16
Merged
Conversation
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.
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.
…cs; targeting EF Core 10
Docstrings generation was requested by @WahidBitar. * #13 (comment) The following files were modified: * `src/Diwink.Extensions.EntityFrameworkCore.TestModel/Configurations/CourseConfiguration.cs` * `src/Diwink.Extensions.EntityFrameworkCore.TestModel/Configurations/CourseMentorAssignmentConfiguration.cs` * `src/Diwink.Extensions.EntityFrameworkCore.TestModel/Configurations/CoursePolicyConfiguration.cs` * `src/Diwink.Extensions.EntityFrameworkCore.TestModel/Configurations/LearningCatalogConfiguration.cs` * `src/Diwink.Extensions.EntityFrameworkCore.TestModel/Configurations/MentorConfiguration.cs` * `src/Diwink.Extensions.EntityFrameworkCore.TestModel/Configurations/MentorWorkspaceConfiguration.cs` * `src/Diwink.Extensions.EntityFrameworkCore.TestModel/Configurations/TopicTagConfiguration.cs` * `src/Diwink.Extensions.EntityFrameworkCore.TestModel/TestDbContext.cs` * `src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/DatabaseBootstrap.cs` * `src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/IntegrationTestBase.cs` * `src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/SeedData.cs` * `src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/SqlServerContainerFixture.cs` * `src/Diwink.Extensions.EntityFrameworkCore/DbContextExtensions.cs` * `src/Diwink.Extensions.EntityFrameworkCore/Exceptions/AmbiguousOwnershipSemanticsException.cs` * `src/Diwink.Extensions.EntityFrameworkCore/Exceptions/GraphUpdateException.cs` * `src/Diwink.Extensions.EntityFrameworkCore/Exceptions/PartialMutationNotAllowedException.cs` * `src/Diwink.Extensions.EntityFrameworkCore/Exceptions/UnloadedNavigationMutationException.cs` * `src/Diwink.Extensions.EntityFrameworkCore/Exceptions/UnsupportedNavigationMutatedException.cs` * `src/Diwink.Extensions.EntityFrameworkCore/Exceptions/UnsupportedRelationshipPatternException.cs` * `src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/EntityKeyHelper.cs` * `src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/GraphUpdateOrchestrator.cs` * `src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/OperationGuard.cs` * `src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/RelatedEntityMutationService.cs` * `src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/OneToOneOwnershipResolver.cs` * `src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/OptionalOneToOneStrategy.cs` * `src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PayloadManyToManyStrategy.cs` * `src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PureManyToManyStrategy.cs` * `src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/RequiredOneToOneStrategy.cs` * `src/Diwink.Extensions.EntityFrameworkCore/Traversal/NavigationLoadGuard.cs`
… into 001-rebuild-graph-update
…and graph update exceptions Enhance test coverage by adding integration tests for the SeedData helper to validate distinct IDs, entity relationships, and in-memory database seeding. Introduce unit tests for `GraphUpdateException` derived types, ensuring robust constructor validations, message formatting, and inheritance chain testing. Remove deprecated `ManyToManyDiffStrategyTests` in favor of the new testing structure. Update dependencies in project files to include EF Core InMemory provider for test scenarios.
Contributor
📝 WalkthroughWalkthroughThis pull request executes a major v2 rebuild targeting .NET 10 and EF Core 10.x. It replaces the legacy v1 infrastructure with a contract-driven graph update system supporting pure/payload many-to-many and required/optional one-to-one relationships, removes outdated test models, establishes comprehensive test coverage, and updates deployment workflows for the new architecture. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![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: 20
🧹 Nitpick comments (9)
.github/workflows/dotnet-core.yml (1)
45-46: Consider using the secret directly in the push command.The API key is exposed as an environment variable at the job level (line 46). While this works, you could use the secret directly in the push step to minimize exposure scope:
- name: Publish to NuGet.org if: secrets.NUGET_API_KEY != '' run: dotnet nuget push "./artifacts/*.nupkg" --api-key "${{ secrets.NUGET_API_KEY }}" --source "https://api.nuget.org/v3/index.json" --skip-duplicateThis keeps the secret scoped to only the step that needs it rather than the entire job.
Also applies to: 67-69
🤖 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 45 - 46, Move usage of the NuGet API key out of the job-level env and reference the secret directly in the publish step: remove or stop setting NUGET_API_KEY at the job env, and in the "Publish to NuGet.org" step (and the other publish step referenced around lines 67-69) update the run command to pass the key as --api-key "${{ secrets.NUGET_API_KEY }}" and keep the step-level guard if: secrets.NUGET_API_KEY != '' so the secret is only exposed to that step.src/Diwink.Extensions.EntityFrameworkCore/Exceptions/UnloadedNavigationMutationException.cs (1)
15-24: Missing validation fornavigationNameparameter.The
relationshipPathis validated via the base class, butnavigationNameis assigned directly without validation. Other derived exceptions in this hierarchy validate their additional parameters using the sharedValidateAndNormalizehelper. Consider validatingnavigationNamefor consistency and to prevent invalid exception states.♻️ Proposed fix to add validation
public UnloadedNavigationMutationException( string relationshipPath, string navigationName) : base( $"Mutation at '{relationshipPath}' depends on navigation '{navigationName}' " + "which was not explicitly loaded. The entire operation was rejected without partial apply.", relationshipPath) { - NavigationName = navigationName; + NavigationName = ValidateAndNormalize(navigationName, nameof(navigationName), "Navigation name"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Diwink.Extensions.EntityFrameworkCore/Exceptions/UnloadedNavigationMutationException.cs` around lines 15 - 24, UnloadedNavigationMutationException assigns navigationName directly without validation; update its constructor to validate and normalize navigationName using the shared helper (e.g., call ValidateAndNormalize or the base class helper used by other derived exceptions) before assigning to the NavigationName property so the parameter is checked/normalized consistently with relationshipPath and other exceptions in the hierarchy.src/Diwink.Extensions.EntityFrameworkCore/Traversal/NavigationLoadGuard.cs (1)
23-25: Method name/docs imply enforcement, but implementation only traverses loaded navigations.
EnsureNavigationsLoadedcurrently skips unloaded navigations, so the name can be misread as a full validation guarantee. Consider renaming/documenting it as traversal-only, or invokingRequireLoadedwhere enforcement is intended.Also applies to: 42-45
🤖 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 23 - 25, The method EnsureNavigationsLoaded in NavigationLoadGuard only traverses navigations that are already loaded but the name implies enforcement; either rename the method to something like TraverseLoadedNavigations and update its XML docs to state it only visits loaded navigations, or change its implementation to call the existing RequireLoaded helper for each navigation it visits so it actually enforces loading; apply the same change to the second overload/occurrence referenced around lines 42-45 and update any callers or docs accordingly to reflect the new behavior or name.src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PayloadManyToManyStrategy.cs (1)
33-38: Consider indexing by key to avoid repeated linear scans.This reconcile path currently performs repeated key extraction and list scans. Building key-index maps for existing/updated entities would reduce complexity and improve behavior on larger collections.
Also applies to: 47-51, 76-81
🤖 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 33 - 38, The current reconcile loops in PayloadManyToManyStrategy perform repeated GetKeyValues and linear searches over updatedItems/existingItems; replace those scans by precomputing key->entity dictionaries for both collections (use EntityKeyHelper.GetKeyValues(context.Entry(item)) as the key) before the three reconcile phases (the removal loop, the addition loop, and the update/merge loop referenced around the other blocks), then perform O(1) lookups against those dictionaries instead of calling EntityKeyHelper.FindByKey repeatedly; ensure the key representation you build is consistent (e.g., join key parts into a single comparable string or use a tuple) and update the loops to remove from/lookup into the dictionaries rather than scanning lists.src/Diwink.Extensions.EntityFrameworkCore.Tests.Unit/GraphDiff/DbContextExtensionsTests.cs (1)
20-45: Rename test methods to matchUpdateGraphAPI.The assertions are correct, but method names still reference
InsertUpdateOrDeleteGraph, which no longer matches the public method under test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Diwink.Extensions.EntityFrameworkCore.Tests.Unit/GraphDiff/DbContextExtensionsTests.cs` around lines 20 - 45, Rename the test methods so their names reference the current API (UpdateGraph) instead of the old InsertUpdateOrDeleteGraph; e.g., rename InsertUpdateOrDeleteGraph_throws_when_context_is_null to UpdateGraph_throws_when_context_is_null, InsertUpdateOrDeleteGraph_throws_when_updated_entity_is_null to UpdateGraph_throws_when_updated_entity_is_null, etc., ensuring all references and any test attributes (Fact) remain intact and the tests still call DbContextExtensions.UpdateGraph<T> (or context.UpdateGraph<T>) with the same assertions.src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/SeedData.cs (1)
29-30: Guardcontextbefore use inSeedFullScenarioAsync.Adding
ThrowIfNullmakes failures immediate and clearer for callers.♻️ Proposed fix
public static async Task SeedFullScenarioAsync(TestDbContext context) { + ArgumentNullException.ThrowIfNull(context); + var tag1 = new TopicTag { Id = Tag1Id, Label = "Architecture" };🤖 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 29 - 30, In SeedFullScenarioAsync, guard the incoming TestDbContext before any use by adding a null-check; call context.ThrowIfNull(nameof(context)) at the top of the method (or if that helper isn't available, throw new ArgumentNullException(nameof(context))). This ensures SeedFullScenarioAsync validates the TestDbContext parameter immediately and fails fast with a clear error.src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/IntegrationTestBase.cs (1)
21-24: Add a constructor guard forfixture.This gives a clearer failure mode if fixture injection ever changes or is bypassed in custom setup.
♻️ Proposed fix
protected IntegrationTestBase(SqlServerContainerFixture fixture) { + ArgumentNullException.ThrowIfNull(fixture); _fixture = fixture; }🤖 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/IntegrationTestBase.cs` around lines 21 - 24, The IntegrationTestBase constructor currently assigns the incoming SqlServerContainerFixture to _fixture without validation; add a null check in the IntegrationTestBase(SqlServerContainerFixture fixture) constructor and throw an ArgumentNullException(nameof(fixture)) (or similar) if fixture is null to provide a clear failure when fixture injection is missing; update the constructor body in IntegrationTestBase to validate fixture before assigning to _fixture.src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/DatabaseBootstrap.cs (1)
121-124: Narrow the retry filter to transient failures only.
InvalidOperationExceptionis too broad here. Deterministic model/configuration bugs will be retried three times and then wrapped as schema-bootstrap failures, which makes red builds slower and harder to diagnose.Suggested change
- return exception is SqlException or TimeoutException or InvalidOperationException || + return exception is SqlException or TimeoutException || (cancellationSource.IsCancellationRequested && exception is OperationCanceledException);🤖 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/DatabaseBootstrap.cs` around lines 121 - 124, The retry predicate IsRetryableSchemaException is too broad because it treats InvalidOperationException as transient; remove InvalidOperationException from the allowed set and only treat true transient failures as retryable (keep SqlException and TimeoutException), and preserve the cancellation check (cancellationSource.IsCancellationRequested && exception is OperationCanceledException); update IsRetryableSchemaException accordingly so deterministic model/configuration errors are not retried.src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/GraphUpdateOrchestrator.cs (1)
151-157: CastingIEnumerable<object>may fail for value-type collections.The pattern
updatedValue is IEnumerable<object> collectionrelies on covariance, which works for reference types but will fail for collections of value types (e.g.,List<int>). If navigation collections could contain value types, this check would incorrectly fall through to line 156.For EF Core entity navigations, this is typically fine since entities are reference types, but worth noting if the codebase ever supports value-type collections.
🤖 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 151 - 157, The check in GraphUpdateOrchestrator that uses "updatedValue is IEnumerable<object> collection" can fail for value-type collections; change it to test for the non-generic IEnumerable (e.g., "if (updatedValue is System.Collections.IEnumerable enumerable)") and determine whether it has items by iterating (or using foreach to return true on the first item) so value-type collections (like List<int>) are handled without casting errors; update the logic around the updatedValue/collection handling in GraphUpdateOrchestrator to use this non-generic enumerable check and early-return when an item is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Diwink.Extensions.EntityFrameworkCore.TestModel/Entities/Course.cs`:
- Around line 12-13: The comment and code disagree: the Course class's Policy
property is marked as nullable (CoursePolicy? Policy) while the comment says
"Required one-to-one"; decide which is correct and make them consistent. If the
relationship is required, change the property to non-nullable (CoursePolicy
Policy) and initialize with null! or ensure constructor assignment so the
compiler is satisfied; if it is optional, update the comment to indicate it's
optional/nullable and ensure EF Core configuration (in the Course/CoursePolicy
mapping) matches. Reference: Course.Policy and the CoursePolicy entity.
In
`@src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Contracts/ManyToMany/ManyToManySafetyContractTests.cs`:
- Around line 82-85: The test ManyToManySafetyContractTests should also assert
that assignment links were removed: after creating verifyCtx and verifying
mentorCount, load the affected Course(s) including the MentorAssignments
navigation (e.g., via verifyCtx.Courses.Include(c => c.MentorAssignments) or
fetching the specific course by id) and assert that course.MentorAssignments is
empty (Count==0 or .Should().BeEmpty()), ensuring the unlink operation actually
cleared Course.MentorAssignments in addition to preserving Mentor entities.
In
`@src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Contracts/Rejection/PartialMutationNotAllowedTests.cs`:
- Around line 14-17: Add the missing xUnit collection attribute to the test
class: add [Collection(IntegrationTestCollection.Name)] directly above the
PartialMutationNotAllowedTests class declaration so the class
(PartialMutationNotAllowedTests) is explicitly placed in the
IntegrationTestCollection (IntegrationTestCollection.Name) rather than relying
on inheritance from IntegrationTestBase.
In
`@src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/DatabaseBootstrap.cs`:
- Around line 18-24: Fix the malformed XML doc comments in DatabaseBootstrap:
remove the nested/duplicate <summary> tags and ensure each summary has one
opening <summary> and one closing </summary> around the intended text (for the
method that "Creates a fresh DbContext..." / the method that "Creates a
TestDbContext configured to use SQL Server..." and the other doc blocks
mentioned). Update the related XML comments (the other blocks at the same spots)
to use a single well-formed <summary> per comment and correct any mismatched
closing tags so the generated XML docs are valid.
In
`@src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/IntegrationTestBase.cs`:
- Around line 41-45: Remove the duplicate XML documentation opening tag in the
summary block for the method in IntegrationTestBase: delete the second
"<summary>" line (the one starting "Creates a new TestDbContext...") so there is
only one opening <summary> and a single closing </summary> for that method's
documentation in IntegrationTestBase.
In
`@src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/SeedData.cs`:
- Around line 23-27: The XML doc comment for the seed routine contains a
duplicated opening <summary> tag; remove the extra "<summary>" so the comment
has a single opening "<summary>" and one closing "</summary>" wrapping the
sentence that begins "Seeds the provided test database..." (locate the
documentation above the SeedData class/method in SeedData.cs). Apply the same
fix to the malformed doc comments in IntegrationTestBase.cs and
DbContextExtensions.cs so each summary block has exactly one opening and one
closing tag.
In `@src/Diwink.Extensions.EntityFrameworkCore/DbContextExtensions.cs`:
- Around line 12-35: The XML documentation for the UpdateGraph<T> method
contains duplicated/overlapping tags; remove the second <summary> block and
consolidate parameter/returns/exception tags into a single coherent XML doc
using the detailed first summary (including supported relationship patterns and
FR references) and keep one set of <param>, <returns>, and <exception> tags;
update the documentation immediately above the UpdateGraph<T> method declaration
so the method has exactly one <summary> and one set of XML doc elements
describing parameters and exceptions.
In `@src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/EntityKeyHelper.cs`:
- Around line 33-36: The XML doc comment for the EntityKeyHelper summary is
malformed (duplicate and unclosed <summary> tag); update the comment above the
EntityKeyHelper (or the method that "Gets the primary key values for a detached
entity") to a single well-formed XML documentation block—e.g. a single
triple-slash /// <summary>Gets the primary key component values for a detached
CLR entity using the provided DbContext's model metadata.</summary>—removing the
duplicate line and ensuring the opening and closing <summary> tags are present
and properly formatted.
- Around line 99-102: The XML doc comment above the method in the
EntityKeyHelper class is malformed: it contains duplicated/incomplete <summary>
tags. Edit the XML comment for the target method in EntityKeyHelper to have a
single well-formed <summary>...</summary> block (and properly close any tags),
and ensure the content reads correctly (e.g., "Finds the first entity in
<paramref name="collection"/> whose primary key components, as determined from
<paramref name="context"/>'s model, match <paramref name="targetKeys"/>.").
Remove the stray or duplicate <summary> line and verify any <param> or other XML
tags are correctly opened and closed.
- Around line 60-63: The XML doc for the method in EntityKeyHelper is malformed:
the <summary> tag is opened twice. Edit the XML comment on the method that
"Compares two key arrays for equality" in class EntityKeyHelper and replace the
second "<summary>" with a closing "</summary>" and ensure the summary text is a
single coherent block describing that the method determines whether two arrays
of primary-key component values are equal element-by-element in order.
- Around line 12-16: The XML doc block in EntityKeyHelper (the summary lines
describing primary key extraction) is malformed because the first <summary> tag
isn't properly closed before a second <summary> begins; fix it by either
consolidating the two lines into a single well-formed <summary>...</summary> or
by closing the first <summary> tag before the second, so the XML comment for the
EntityKeyHelper/GetPrimaryKeyValues (or the surrounding method) is valid and
produces no warnings.
In
`@src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/GraphUpdateOrchestrator.cs`:
- Around line 20-23: The XML documentation comments in the
GraphUpdateOrchestrator class are malformed (unclosed <summary> tags) — update
each triple-slash comment block to use well-formed XML (for example change "///
<summary> ... <summary>" to "/// <summary> ... </summary>") throughout the file,
mirroring the correct pattern used in EntityKeyHelper; check and fix all
occurrences around the GraphUpdateOrchestrator class and its methods (e.g., the
class declaration and the methods referenced in the diff) so every <summary>,
<param>, <returns> or other XML tag is properly opened and closed.
- Around line 68-127: The try block in GraphUpdateOrchestrator.cs is malformed:
ensure the try has a proper opening brace that encloses the subsequent
navigation checks and that the matching closing brace comes immediately before
the finally block that calls recursionPath.Remove(existingEntry.Entity); update
indentation of the code inside the try accordingly so the foreach loops (over
existingEntry.Navigations.Where(...)), the Unsupported/Loaded validation logic
(including calls to ClassifyNavigation, HasMutations, ValidateLoadedChildren)
and the unloaded-navigation checks are all inside the try block and align with
the finally block.
In `@src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/OperationGuard.cs`:
- Around line 25-30: The XML doc comments for the AddError and ThrowIfErrors
methods contain duplicated opening <summary> tags; edit the docblocks in
OperationGuard (methods AddError and ThrowIfErrors) to remove the extra opening
<summary> so each comment uses a single well-formed <summary>...</summary> pair
(and keep the descriptive text intact), ensuring valid XML documentation and
eliminating the compiler warnings.
In
`@src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/OneToOneOwnershipResolver.cs`:
- Around line 12-23: The XML doc for IsRequiredDependent has duplicated/nested
<summary> tags and a misplaced <returns> which makes the XML invalid; fix it by
keeping a single <summary> describing the method (e.g., "Determines whether the
dependent end of the specified one-to-one relationship is required.") and a
single <returns> describing the boolean outcome (e.g., "`true` if required;
`false` if optional"), removing the extra/duplicated <summary> block so the XML
doc is well-formed for the IsRequiredDependent method in
OneToOneOwnershipResolver.
In
`@src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/OptionalOneToOneStrategy.cs`:
- Around line 14-19: The XML documentation in OptionalOneToOneStrategy has
duplicate opening <summary> tags causing malformed docs; edit each affected doc
block (the summaries describing detaching/clearing the optional dependent
navigation in the OptionalOneToOneStrategy class) to have a single opening
<summary> and a single closing </summary> wrapping the combined description,
removing the extra <summary> tags and ensuring proper XML structure for all
similar blocks (the other two summary blocks in the same class) so the doc
comments are valid.
In
`@src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PayloadManyToManyStrategy.cs`:
- Around line 47-61: updatedItems can contain duplicate association keys which,
if no tracked match exists, will cause multiple AddToCollection calls and PK
conflicts; inside the loop over updatedItems (where
EntityKeyHelper.GetKeyValues, FindInTracked and AddToCollection are used) add a
guard: compute a stable signature for updatedKeys (e.g., join key values or use
a structural comparer) and keep a HashSet of seen signatures; if a signature is
already seen, skip or merge payloads into the already-processed association
instead of calling AddToCollection again; ensure this check runs before calling
FindInTracked/AddToCollection so duplicates are handled deterministically.
In
`@src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PureManyToManyStrategy.cs`:
- Around line 86-93: The condition in ApplyValuesIfNotModified incorrectly
includes EntityState.Detached; calling
trackedEntry.CurrentValues.SetValues(updatedEntity) on a detached entry is a
no-op. Update the method ApplyValuesIfNotModified to only call SetValues when
the tracked entry is actually tracked (e.g., when trackedEntry.State is
EntityState.Unchanged) by removing the "EntityState.Detached" branch from the if
condition so SetValues is only invoked for tracked entities (use the
trackedEntry variable and Entry/CurrentValues as in the existing method).
In
`@src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/RequiredOneToOneStrategy.cs`:
- Around line 13-20: The XML doc for RemoveDependent in RequiredOneToOneStrategy
has a malformed tag: the second "<summary>" should be a closing "</summary>"
which breaks the documentation; update the block for the RemoveDependent method
by replacing that erroneous opening <summary> with </summary> and ensure the
remaining XML tags (parameters and summaries) are properly closed so the doc
comment is well-formed.
In `@src/Diwink.Extensions.EntityFrameworkCore/Traversal/NavigationLoadGuard.cs`:
- Around line 16-20: In the XML docs for the NavigationLoadGuard class, remove
the duplicated opening <summary> tag occurrences so each documentation block has
a single <summary> ... </summary> pair; specifically delete the extra <summary>
in the class/header doc and the extra <summary> in the other documentation block
within NavigationLoadGuard.cs so the XML is well-formed around the
NavigationLoadGuard documentation.
---
Nitpick comments:
In @.github/workflows/dotnet-core.yml:
- Around line 45-46: Move usage of the NuGet API key out of the job-level env
and reference the secret directly in the publish step: remove or stop setting
NUGET_API_KEY at the job env, and in the "Publish to NuGet.org" step (and the
other publish step referenced around lines 67-69) update the run command to pass
the key as --api-key "${{ secrets.NUGET_API_KEY }}" and keep the step-level
guard if: secrets.NUGET_API_KEY != '' so the secret is only exposed to that
step.
In
`@src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/DatabaseBootstrap.cs`:
- Around line 121-124: The retry predicate IsRetryableSchemaException is too
broad because it treats InvalidOperationException as transient; remove
InvalidOperationException from the allowed set and only treat true transient
failures as retryable (keep SqlException and TimeoutException), and preserve the
cancellation check (cancellationSource.IsCancellationRequested && exception is
OperationCanceledException); update IsRetryableSchemaException accordingly so
deterministic model/configuration errors are not retried.
In
`@src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/IntegrationTestBase.cs`:
- Around line 21-24: The IntegrationTestBase constructor currently assigns the
incoming SqlServerContainerFixture to _fixture without validation; add a null
check in the IntegrationTestBase(SqlServerContainerFixture fixture) constructor
and throw an ArgumentNullException(nameof(fixture)) (or similar) if fixture is
null to provide a clear failure when fixture injection is missing; update the
constructor body in IntegrationTestBase to validate fixture before assigning to
_fixture.
In
`@src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/SeedData.cs`:
- Around line 29-30: In SeedFullScenarioAsync, guard the incoming TestDbContext
before any use by adding a null-check; call context.ThrowIfNull(nameof(context))
at the top of the method (or if that helper isn't available, throw new
ArgumentNullException(nameof(context))). This ensures SeedFullScenarioAsync
validates the TestDbContext parameter immediately and fails fast with a clear
error.
In
`@src/Diwink.Extensions.EntityFrameworkCore.Tests.Unit/GraphDiff/DbContextExtensionsTests.cs`:
- Around line 20-45: Rename the test methods so their names reference the
current API (UpdateGraph) instead of the old InsertUpdateOrDeleteGraph; e.g.,
rename InsertUpdateOrDeleteGraph_throws_when_context_is_null to
UpdateGraph_throws_when_context_is_null,
InsertUpdateOrDeleteGraph_throws_when_updated_entity_is_null to
UpdateGraph_throws_when_updated_entity_is_null, etc., ensuring all references
and any test attributes (Fact) remain intact and the tests still call
DbContextExtensions.UpdateGraph<T> (or context.UpdateGraph<T>) with the same
assertions.
In
`@src/Diwink.Extensions.EntityFrameworkCore/Exceptions/UnloadedNavigationMutationException.cs`:
- Around line 15-24: UnloadedNavigationMutationException assigns navigationName
directly without validation; update its constructor to validate and normalize
navigationName using the shared helper (e.g., call ValidateAndNormalize or the
base class helper used by other derived exceptions) before assigning to the
NavigationName property so the parameter is checked/normalized consistently with
relationshipPath and other exceptions in the hierarchy.
In
`@src/Diwink.Extensions.EntityFrameworkCore/GraphUpdate/GraphUpdateOrchestrator.cs`:
- Around line 151-157: The check in GraphUpdateOrchestrator that uses
"updatedValue is IEnumerable<object> collection" can fail for value-type
collections; change it to test for the non-generic IEnumerable (e.g., "if
(updatedValue is System.Collections.IEnumerable enumerable)") and determine
whether it has items by iterating (or using foreach to return true on the first
item) so value-type collections (like List<int>) are handled without casting
errors; update the logic around the updatedValue/collection handling in
GraphUpdateOrchestrator to use this non-generic enumerable check and
early-return when an item is found.
In
`@src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PayloadManyToManyStrategy.cs`:
- Around line 33-38: The current reconcile loops in PayloadManyToManyStrategy
perform repeated GetKeyValues and linear searches over
updatedItems/existingItems; replace those scans by precomputing key->entity
dictionaries for both collections (use
EntityKeyHelper.GetKeyValues(context.Entry(item)) as the key) before the three
reconcile phases (the removal loop, the addition loop, and the update/merge loop
referenced around the other blocks), then perform O(1) lookups against those
dictionaries instead of calling EntityKeyHelper.FindByKey repeatedly; ensure the
key representation you build is consistent (e.g., join key parts into a single
comparable string or use a tuple) and update the loops to remove from/lookup
into the dictionaries rather than scanning lists.
In `@src/Diwink.Extensions.EntityFrameworkCore/Traversal/NavigationLoadGuard.cs`:
- Around line 23-25: The method EnsureNavigationsLoaded in NavigationLoadGuard
only traverses navigations that are already loaded but the name implies
enforcement; either rename the method to something like
TraverseLoadedNavigations and update its XML docs to state it only visits loaded
navigations, or change its implementation to call the existing RequireLoaded
helper for each navigation it visits so it actually enforces loading; apply the
same change to the second overload/occurrence referenced around lines 42-45 and
update any callers or docs accordingly to reflect the new behavior or name.
🪄 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: eac5858a-4077-4da8-9c5a-d05d249784ce
📒 Files selected for processing (77)
.github/workflows/dotnet-core.yml.gitignoreEfCore.UpdateGraph.slnREADME.mdsrc/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/DatabaseBootstrapTests.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/SeedDataStructureTests.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/Exceptions/GraphUpdateExceptionTests.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Unit/GraphDiff/DbContextExtensionsTests.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Unit/GraphDiff/EntityKeyHelperTests.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Unit/GraphDiff/ExceptionValidationTests.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Unit/GraphDiff/NestedNavigationValidationTests.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Unit/GraphDiff/OperationGuardTests.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Unit/GraphDiff/RelationshipClassificationTests.cssrc/Diwink.Extensions.EntityFrameworkCore.Tests.Unit/GraphDiff/UnsupportedRelationshipPatternTests.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/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)
- src/UnitTests/appsettings.json
- src/FakeModel/FakeModel.csproj
- EfCore.UpdateGraph.sln
- src/UnitTests/TestHelpers.cs
- src/UnitTests/UnitTests.csproj
- src/UnitTests/Migrations/FakeSchoolsDbContextModelSnapshot.cs
- src/UnitTests/Migrations/20200914083038_initialMigration.cs
- src/UnitTests/Migrations/20200914083038_initialMigration.Designer.cs
- src/UnitTests/VariousTests.cs
- src/UnitTests/FakeSchoolsDbContext.cs
- src/FakeModel/Model.cs
- src/UnitTests/GraphUpdateTests.cs
- src/UnitTests/FakeSchoolsDbContextFactory.cs
Comment on lines
+12
to
+13
| // Required one-to-one | ||
| public CoursePolicy? Policy { get; set; } |
Contributor
There was a problem hiding this comment.
Comment-code inconsistency: "Required" vs nullable type.
The comment states "Required one-to-one" but Policy is declared as nullable (CoursePolicy?). If the relationship is truly required at the database level, consider either:
- Making the property non-nullable (
CoursePolicy Policy) withnull!initialization, or - Updating the comment to clarify it's optional from a C# perspective but enforced by EF Core configuration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Diwink.Extensions.EntityFrameworkCore.TestModel/Entities/Course.cs`
around lines 12 - 13, The comment and code disagree: the Course class's Policy
property is marked as nullable (CoursePolicy? Policy) while the comment says
"Required one-to-one"; decide which is correct and make them consistent. If the
relationship is required, change the property to non-nullable (CoursePolicy
Policy) and initialize with null! or ensure constructor assignment so the
compiler is satisfied; if it is optional, update the comment to indicate it's
optional/nullable and ensure EF Core configuration (in the Course/CoursePolicy
mapping) matches. Reference: Course.Policy and the CoursePolicy entity.
Comment on lines
+82
to
+85
| // Assert | ||
| await using var verifyCtx = CreateContext(); | ||
| var mentorCount = await verifyCtx.Mentors.CountAsync(); | ||
| mentorCount.Should().Be(2, "removing all payload associations must not delete any mentor entities"); |
Contributor
There was a problem hiding this comment.
Also assert that assignment links were actually removed.
This test currently verifies mentor entities are preserved, but it does not assert that Course.MentorAssignments became empty. It can pass even if unlinking fails.
💡 Proposed test hardening
// Assert
await using var verifyCtx = CreateContext();
+ var updatedCourse = await verifyCtx.Courses
+ .Include(c => c.MentorAssignments)
+ .FirstAsync(c => c.Id == SeedData.Course1Id);
+ updatedCourse.MentorAssignments.Should().BeEmpty();
+
var mentorCount = await verifyCtx.Mentors.CountAsync();
mentorCount.Should().Be(2, "removing all payload associations must not delete any mentor entities");📝 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
| // Assert | |
| await using var verifyCtx = CreateContext(); | |
| var mentorCount = await verifyCtx.Mentors.CountAsync(); | |
| mentorCount.Should().Be(2, "removing all payload associations must not delete any mentor entities"); | |
| // Assert | |
| await using var verifyCtx = CreateContext(); | |
| var updatedCourse = await verifyCtx.Courses | |
| .Include(c => c.MentorAssignments) | |
| .FirstAsync(c => c.Id == SeedData.Course1Id); | |
| updatedCourse.MentorAssignments.Should().BeEmpty(); | |
| var mentorCount = await verifyCtx.Mentors.CountAsync(); | |
| mentorCount.Should().Be(2, "removing all payload associations must not delete any mentor entities"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Contracts/ManyToMany/ManyToManySafetyContractTests.cs`
around lines 82 - 85, The test ManyToManySafetyContractTests should also assert
that assignment links were removed: after creating verifyCtx and verifying
mentorCount, load the affected Course(s) including the MentorAssignments
navigation (e.g., via verifyCtx.Courses.Include(c => c.MentorAssignments) or
fetching the specific course by id) and assert that course.MentorAssignments is
empty (Count==0 or .Should().BeEmpty()), ensuring the unlink operation actually
cleared Course.MentorAssignments in addition to preserving Mentor entities.
Comment on lines
+14
to
+17
| public class PartialMutationNotAllowedTests : IntegrationTestBase | ||
| { | ||
| public PartialMutationNotAllowedTests(SqlServerContainerFixture fixture) | ||
| : base(fixture) { } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if IntegrationTestBase has the Collection attribute or if there's assembly-level configuration
# Check IntegrationTestBase for Collection attribute
echo "=== Checking IntegrationTestBase for Collection attribute ==="
rg -n -A5 "class IntegrationTestBase" --type cs
# Check for assembly-level collection configuration
echo "=== Checking for assembly-level attributes ==="
rg -n "CollectionBehavior|Collection\(" --type cs -g "*Assembly*" -g "*Global*"Repository: WahidBitar/EF-Core-Simple-Graph-Update
Length of output: 1150
🏁 Script executed:
#!/bin/bash
# Check complete IntegrationTestBase class definition (attributes and full class declaration)
echo "=== Full IntegrationTestBase definition ==="
rg -B10 "public abstract class IntegrationTestBase" --type cs
# Search for Collection attribute anywhere in the integration tests
echo "=== Searching for Collection attribute in integration tests ==="
rg -n "\[Collection" src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/ --type cs
# Check if AssemblyInfo or module initializer exists
echo "=== Looking for assembly-level configuration ==="
fd -e "AssemblyInfo\|cs\|GlobalUsings\|cs" src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/ | head -20
# Look at the specific test file being reviewed
echo "=== Check PartialMutationNotAllowedTests.cs ==="
cat -n src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Contracts/Rejection/PartialMutationNotAllowedTests.cs | head -30Repository: WahidBitar/EF-Core-Simple-Graph-Update
Length of output: 4601
Add explicit [Collection(IntegrationTestCollection.Name)] attribute to the test class.
xUnit does not inherit attributes from base classes. While IntegrationTestBase has the [Collection(IntegrationTestCollection.Name)] attribute, derived test classes must also have it explicitly to be placed in the collection. Other test classes in this project (e.g., PureManyToManyContractTests, PayloadAssociationContractTests) declare this attribute directly; PartialMutationNotAllowedTests should match this pattern.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Contracts/Rejection/PartialMutationNotAllowedTests.cs`
around lines 14 - 17, Add the missing xUnit collection attribute to the test
class: add [Collection(IntegrationTestCollection.Name)] directly above the
PartialMutationNotAllowedTests class declaration so the class
(PartialMutationNotAllowedTests) is explicitly placed in the
IntegrationTestCollection (IntegrationTestCollection.Name) rather than relying
on inheritance from IntegrationTestBase.
Comment on lines
+18
to
+24
| /// <summary> | ||
| /// Creates a fresh DbContext pointing at the container and ensures the schema exists. | ||
| /// <summary> | ||
| /// Creates a TestDbContext configured to use SQL Server with a test-specific connection string. | ||
| /// </summary> | ||
| /// <param name="connectionString">Base SQL Server connection string; if its initial catalog is empty or equals "master", the catalog will be replaced with the test database name.</param> | ||
| /// <returns>A new TestDbContext configured with the adjusted connection string.</returns> |
Contributor
There was a problem hiding this comment.
Fix the malformed XML doc blocks.
These method comments have the same nested <summary> issue as the strategy file, so the generated XML docs will be invalid here too.
Also applies to: 53-59, 67-73
🤖 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/DatabaseBootstrap.cs`
around lines 18 - 24, Fix the malformed XML doc comments in DatabaseBootstrap:
remove the nested/duplicate <summary> tags and ensure each summary has one
opening <summary> and one closing </summary> around the intended text (for the
method that "Creates a fresh DbContext..." / the method that "Creates a
TestDbContext configured to use SQL Server..." and the other doc blocks
mentioned). Update the related XML comments (the other blocks at the same spots)
to use a single well-formed <summary> per comment and correct any mismatched
closing tags so the generated XML docs are valid.
Comment on lines
+41
to
+45
| /// <summary> | ||
| /// Creates a fresh DbContext for the current test. | ||
| /// <summary> | ||
| /// Creates a new TestDbContext configured for the integration test database. | ||
| /// </summary> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/Diwink.Extensions.EntityFrameworkCore.Tests.Integration/Infrastructure/IntegrationTestBase.cs | sed -n '35,50p'Repository: WahidBitar/EF-Core-Simple-Graph-Update
Length of output: 865
Remove the duplicate <summary> tag on line 43.
The XML documentation contains two opening <summary> tags (lines 41 and 43) with only one closing </summary> tag, which causes malformed documentation. Remove the second opening tag on line 43 to keep a single valid summary block.
🤖 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/IntegrationTestBase.cs`
around lines 41 - 45, Remove the duplicate XML documentation opening tag in the
summary block for the method in IntegrationTestBase: delete the second
"<summary>" line (the one starting "Creates a new TestDbContext...") so there is
only one opening <summary> and a single closing </summary> for that method's
documentation in IntegrationTestBase.
Comment on lines
+14
to
+19
| /// <summary> | ||
| /// Detaches the optional dependent by nulling its FK properties. | ||
| /// The dependent entity is preserved in the database with a null FK. | ||
| /// <summary> | ||
| /// Clears the given optional dependent navigation on the tracked principal so the dependent's foreign key is set to null while leaving the dependent entity intact. | ||
| /// </summary> |
Contributor
There was a problem hiding this comment.
Fix the malformed XML doc blocks.
Each method opens a second <summary> before closing the first one. That leaves the XML docs invalid and can trigger compiler documentation warnings when XML output is enabled.
Also applies to: 36-41, 59-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/OptionalOneToOneStrategy.cs`
around lines 14 - 19, The XML documentation in OptionalOneToOneStrategy has
duplicate opening <summary> tags causing malformed docs; edit each affected doc
block (the summaries describing detaching/clearing the optional dependent
navigation in the OptionalOneToOneStrategy class) to have a single opening
<summary> and a single closing </summary> wrapping the combined description,
removing the extra <summary> tags and ensuring proper XML structure for all
similar blocks (the other two summary blocks in the same class) so the doc
comments are valid.
Comment on lines
+47
to
+61
| foreach (var updatedItem in updatedItems) | ||
| { | ||
| var updatedKeys = EntityKeyHelper.GetKeyValues(context, updatedItem); | ||
| var existingMatch = FindInTracked(context, existingItems, updatedKeys); | ||
|
|
||
| if (existingMatch is not null) | ||
| { | ||
| // Update payload fields on existing association entity | ||
| context.Entry(existingMatch).CurrentValues.SetValues(updatedItem); | ||
| } | ||
| else | ||
| { | ||
| // New association entity — add to collection | ||
| AddToCollection(existingNavigation, updatedItem); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against duplicate keys in updatedCollection to avoid double-add/PK conflicts.
If updatedCollection contains two association entities with the same key and no tracked match exists, both will be added. This can cause duplicate relationship rows or SaveChanges key violations.
💡 Proposed fix
+ var processedKeys = new List<object[]>();
+
// Add new or update existing association entities
foreach (var updatedItem in updatedItems)
{
var updatedKeys = EntityKeyHelper.GetKeyValues(context, updatedItem);
+ if (processedKeys.Any(k => EntityKeyHelper.KeysEqual(k, updatedKeys)))
+ {
+ throw new InvalidOperationException(
+ $"Duplicate association entity key detected for navigation " +
+ $"'{existingNavigation.Metadata.DeclaringEntityType.ClrType.Name}.{existingNavigation.Metadata.Name}'.");
+ }
+ processedKeys.Add(updatedKeys);
+
var existingMatch = FindInTracked(context, existingItems, updatedKeys);
if (existingMatch is not null)🤖 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 47 - 61, updatedItems can contain duplicate association keys which,
if no tracked match exists, will cause multiple AddToCollection calls and PK
conflicts; inside the loop over updatedItems (where
EntityKeyHelper.GetKeyValues, FindInTracked and AddToCollection are used) add a
guard: compute a stable signature for updatedKeys (e.g., join key values or use
a structural comparer) and keep a HashSet of seen signatures; if a signature is
already seen, skip or merge payloads into the already-processed association
instead of calling AddToCollection again; ensure this check runs before calling
FindInTracked/AddToCollection so duplicates are handled deterministically.
Comment on lines
+86
to
+93
| private static void ApplyValuesIfNotModified(DbContext context, object trackedEntity, object updatedEntity) | ||
| { | ||
| var trackedEntry = context.Entry(trackedEntity); | ||
| if (trackedEntry.State is EntityState.Unchanged or EntityState.Detached) | ||
| { | ||
| trackedEntry.CurrentValues.SetValues(updatedEntity); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how ApplyValuesIfNotModified is called and whether detached entities reach this code path
ast-grep --pattern $'ApplyValuesIfNotModified($_, $_, $_)'Repository: WahidBitar/EF-Core-Simple-Graph-Update
Length of output: 433
🏁 Script executed:
cat -n src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PureManyToManyStrategy.cs | sed -n '45,85p'Repository: WahidBitar/EF-Core-Simple-Graph-Update
Length of output: 2049
🏁 Script executed:
cat -n src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PureManyToManyStrategy.cs | sed -n '20,50p'Repository: WahidBitar/EF-Core-Simple-Graph-Update
Length of output: 1950
🏁 Script executed:
cat -n src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PureManyToManyStrategy.cs | sed -n '102,120p'Repository: WahidBitar/EF-Core-Simple-Graph-Update
Length of output: 938
🏁 Script executed:
cat -n src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/PureManyToManyStrategy.cs | sed -n '86,95p'Repository: WahidBitar/EF-Core-Simple-Graph-Update
Length of output: 517
🌐 Web query:
EF Core context.Entry() behavior on detached entities attach
💡 Result:
In EF Core, calling context.Entry on a detached entity returns an EntityEntry instance without starting to track the entity. The entity's state remains Detached. You can then use the returned EntityEntry to change the state (e.g., to Added, Modified, Unchanged), at which point the entity becomes tracked in that state. This differs from EF6, where Entry might behave differently. Unlike Attach or Add, which affect the entire entity graph, Entry primarily affects only the specified root entity and ignores child entities unless explicitly handled. Example from official docs: var newBlog = new Blog; Debug.Assert(context.Entry(newBlog).State == EntityState.Detached); context.Entry(newBlog).State = EntityState.Added; Debug.Assert(context.Entry(newBlog).State == EntityState.Added); This does not automatically attach related entities; it is a lower-level operation focused on the single entity instance provided.
Citations:
- 1: https://learn.microsoft.com/en-us/ef/core/change-tracking/entity-entries
- 2: https://learn.microsoft.com/en-us/ef/core/saving/disconnected-entities
- 3: https://www.entityframeworktutorial.net/efcore/working-with-disconnected-entity-graph-ef-core.aspx
- 4: https://learn.microsoft.com/en-us/ef/core/change-tracking/explicit-tracking
🌐 Web query:
EF Core SetValues CurrentValues detached entity behavior
💡 Result:
In EF Core, calling CurrentValues.SetValues on a detached entity has no effect and does not track the entity or mark properties as modified. The CurrentValues property and its SetValues method are only available and functional on tracked entities (EntityState other than Detached). To update a detached entity: 1. Query the existing tracked entity from the database using the same DbContext: var existing = await context.Entities.FindAsync(id); 2. If found, call context.Entry(existing).CurrentValues.SetValues(detachedEntity); This copies scalar property values from the detached/source entity to the tracked entity's CurrentValues, marks changed properties as Modified, and ensures only changed properties are updated on SaveChanges. 3. Call SaveChanges to persist. Key behaviors and limitations: - Works on primitive/scalar properties of the principal entity. - Does not update navigation properties, collections, or owned types automatically. For owned types, explicitly access the reference: context.Entry(existing).Reference("OwnedNav").TargetEntry.CurrentValues.SetValues(sourceOwned); (sources 1,18). - If the entity doesn't exist, use context.Add(detachedEntity) instead. - For full disconnected scenarios (e.g., web APIs), this pattern is recommended in official docs (source 11). Example: var detached = new Entity { Id = 1, Name = "Updated" }; // from client var existing = await context.Entities.FindAsync(1); if (existing != null) { context.Entry(existing).CurrentValues.SetValues(detached); await context.SaveChangesAsync; } Using context.Update(detached) marks all properties as Modified (less efficient) and tracks the detached instance directly.
Citations:
- 1: https://learn.microsoft.com/en-us/ef/core/saving/disconnected-entities
- 2: https://learn.microsoft.com/en-us/dotnet/api/microsoft.entityframeworkcore.changetracking.entityentry.currentvalues?view=efcore-6.0
- 3: https://learn.microsoft.com/en-us/dotnet/api/microsoft.entityframeworkcore.changetracking.propertyvalues.setvalues?view=efcore-6.0
- 4: [BUG] CurrentValues.SetValues does not update owned types, but any other primitive properties are updated dotnet/efcore#25032
- 5: https://learn.microsoft.com/en-us/ef/core/change-tracking/
- 6: https://learn.microsoft.com/en-us/ef/core/change-tracking/explicit-tracking
Remove EntityState.Detached from the condition—SetValues on a detached entity will have no effect.
When trackedEntry.State is Detached, the entity is not being tracked by the context. Calling CurrentValues.SetValues(updatedEntity) on a detached entry will not update values or mark properties as modified since the operation only works on tracked entities. While detached entities are unlikely to reach this code path in the current flow (both call sites should have tracked entities), including this condition is semantically incorrect and could silently fail if the method is called with a detached entity in the future.
Proposed fix
private static void ApplyValuesIfNotModified(DbContext context, object trackedEntity, object updatedEntity)
{
var trackedEntry = context.Entry(trackedEntity);
- if (trackedEntry.State is EntityState.Unchanged or EntityState.Detached)
+ if (trackedEntry.State is EntityState.Unchanged)
{
trackedEntry.CurrentValues.SetValues(updatedEntity);
}
}📝 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 ApplyValuesIfNotModified(DbContext context, object trackedEntity, object updatedEntity) | |
| { | |
| var trackedEntry = context.Entry(trackedEntity); | |
| if (trackedEntry.State is EntityState.Unchanged or EntityState.Detached) | |
| { | |
| trackedEntry.CurrentValues.SetValues(updatedEntity); | |
| } | |
| } | |
| private static void ApplyValuesIfNotModified(DbContext context, object trackedEntity, object updatedEntity) | |
| { | |
| var trackedEntry = context.Entry(trackedEntity); | |
| if (trackedEntry.State is EntityState.Unchanged) | |
| { | |
| trackedEntry.CurrentValues.SetValues(updatedEntity); | |
| } | |
| } |
🤖 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 86 - 93, The condition in ApplyValuesIfNotModified incorrectly
includes EntityState.Detached; calling
trackedEntry.CurrentValues.SetValues(updatedEntity) on a detached entry is a
no-op. Update the method ApplyValuesIfNotModified to only call SetValues when
the tracked entry is actually tracked (e.g., when trackedEntry.State is
EntityState.Unchanged) by removing the "EntityState.Detached" branch from the if
condition so SetValues is only invoked for tracked entities (use the
trackedEntry variable and Entry/CurrentValues as in the existing method).
Comment on lines
+13
to
+20
| /// <summary> | ||
| /// Removes the required dependent by marking it for deletion. | ||
| /// EF Core will delete the row on SaveChanges. | ||
| /// <summary> | ||
| /// Marks the dependent entity referenced by <paramref name="existingNavigation"/> for deletion if it is currently tracked. | ||
| /// </summary> | ||
| /// <param name="context">The <see cref="DbContext"/> used to mark the dependent entity for removal.</param> | ||
| /// <param name="existingNavigation">The tracked reference entry whose <c>CurrentValue</c> is the dependent entity to remove; no action is taken if its value is <c>null</c>.</param> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify XML-doc tag pairing in this file
rg -n '///\s*</?summary>|///\s*</?param>|///\s*</?remarks>|///\s*</?returns>' src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/RequiredOneToOneStrategy.csRepository: WahidBitar/EF-Core-Simple-Graph-Update
Length of output: 179
Fix malformed XML documentation tags on RemoveDependent.
Line 16 has a second <summary> opening tag instead of closing the first one. This breaks XML documentation validity and may cause build failures.
Proposed fix
/// <summary>
/// Removes the required dependent by marking it for deletion.
/// EF Core will delete the row on SaveChanges.
- /// <summary>
+ /// </summary>
/// Marks the dependent entity referenced by <paramref name="existingNavigation"/> for deletion if it is currently tracked.
- /// </summary>📝 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
| /// <summary> | |
| /// Removes the required dependent by marking it for deletion. | |
| /// EF Core will delete the row on SaveChanges. | |
| /// <summary> | |
| /// Marks the dependent entity referenced by <paramref name="existingNavigation"/> for deletion if it is currently tracked. | |
| /// </summary> | |
| /// <param name="context">The <see cref="DbContext"/> used to mark the dependent entity for removal.</param> | |
| /// <param name="existingNavigation">The tracked reference entry whose <c>CurrentValue</c> is the dependent entity to remove; no action is taken if its value is <c>null</c>.</param> | |
| /// <summary> | |
| /// Removes the required dependent by marking it for deletion. | |
| /// EF Core will delete the row on SaveChanges. | |
| /// </summary> | |
| /// Marks the dependent entity referenced by <paramref name="existingNavigation"/> for deletion if it is currently tracked. | |
| /// </summary> | |
| /// <param name="context">The <see cref="DbContext"/> used to mark the dependent entity for removal.</param> | |
| /// <param name="existingNavigation">The tracked reference entry whose <c>CurrentValue</c> is the dependent entity to remove; no action is taken if its value is <c>null</c>.</param> |
Suggested change
| /// <summary> | |
| /// Removes the required dependent by marking it for deletion. | |
| /// EF Core will delete the row on SaveChanges. | |
| /// <summary> | |
| /// Marks the dependent entity referenced by <paramref name="existingNavigation"/> for deletion if it is currently tracked. | |
| /// </summary> | |
| /// <param name="context">The <see cref="DbContext"/> used to mark the dependent entity for removal.</param> | |
| /// <param name="existingNavigation">The tracked reference entry whose <c>CurrentValue</c> is the dependent entity to remove; no action is taken if its value is <c>null</c>.</param> | |
| /// <summary> | |
| /// Removes the required dependent by marking it for deletion. | |
| /// EF Core will delete the row on SaveChanges. | |
| /// </summary> | |
| /// Marks the dependent entity referenced by <paramref name="existingNavigation"/> for deletion if it is currently tracked. | |
| /// <param name="context">The <see cref="DbContext"/> used to mark the dependent entity for removal.</param> | |
| /// <param name="existingNavigation">The tracked reference entry whose <c>CurrentValue</c> is the dependent entity to remove; no action is taken if its value is <c>null</c>.</param> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/Diwink.Extensions.EntityFrameworkCore/RelationshipStrategies/RequiredOneToOneStrategy.cs`
around lines 13 - 20, The XML doc for RemoveDependent in
RequiredOneToOneStrategy has a malformed tag: the second "<summary>" should be a
closing "</summary>" which breaks the documentation; update the block for the
RemoveDependent method by replacing that erroneous opening <summary> with
</summary> and ensure the remaining XML tags (parameters and summaries) are
properly closed so the doc comment is well-formed.
Comment on lines
+16
to
+20
| /// Ensures all navigations in the existing entity entry that the caller expects | ||
| /// to participate in graph mutation are explicitly loaded. | ||
| /// <summary> | ||
| /// Validates loaded navigations on the tracked entity and its reachable object graph, ensuring required navigations are present before mutation. | ||
| /// </summary> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Detect duplicated <summary> openings before a matching </summary> in this file.
rg -nU '///\s*<summary>\n(?:(?!///\s*</summary>).*\n)*///\s*<summary>' src/Diwink.Extensions.EntityFrameworkCore/Traversal/NavigationLoadGuard.csRepository: WahidBitar/EF-Core-Simple-Graph-Update
Length of output: 366
🏁 Script executed:
cat -n src/Diwink.Extensions.EntityFrameworkCore/Traversal/NavigationLoadGuard.csRepository: WahidBitar/EF-Core-Simple-Graph-Update
Length of output: 4550
Remove the duplicated <summary> opening tags in the XML documentation blocks.
Lines 15-20 and 64-69 each contain two <summary> opening tags but only one closing </summary> tag, causing malformed XML documentation. Delete the second opening <summary> tag (line 18 and line 67 respectively) to fix the structure.
🤖 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 16 - 20, In the XML docs for the NavigationLoadGuard class, remove
the duplicated opening <summary> tag occurrences so each documentation block has
a single <summary> ... </summary> pair; specifically delete the extra <summary>
in the class/header doc and the extra <summary> in the other documentation block
within NavigationLoadGuard.cs so the XML is well-formed around the
NavigationLoadGuard documentation.
WahidBitar
changed the title
WahidBitar
changed the title
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
Release Notes
New Features
UpdateGraph) with explicit support for pure and payload many-to-many, and required/optional one-to-one relationships.Chores