Skip to content

fix: stop silent data loss and wrong-SQL edge cases in bulk operations - #34

Merged
hongjs merged 2 commits into
mainfrom
fix/bulk-hardening
Aug 23, 2026
Merged

hongjs merged 2 commits into
mainfrom
fix/bulk-hardening

Conversation

@hongjs

@hongjs hongjs commented Aug 23, 2026

Copy link
Copy Markdown
Owner

Library

  • Reject entities with a shadow property (NotSupportedException). Values are read from CLR properties by reflection; a shadow FK / TPH discriminator has none and was sent as NULL without warning.
  • Read option expressions from the expression tree. x => x.CreatedAt used to be compiled, run on a blank instance, and have DateTime's own properties read back, so IgnoreOnInsert/IgnoreOnUpdate/UniqueKeys silently did nothing. Only x => x.Prop and x => new { x.A, x.B } are accepted; anything else throws.
  • Fall back to the primary key when the model has no unique index; only a keyless entity throws MissingPrimaryKeyException. Key resolution is now one method shared by update and delete, and a UniqueKeys name that is not a mapped property is an ArgumentException instead of a silent fallback.
  • Keep key columns in the derived table when IgnoreOnUpdate names them; the JOIN referenced tmp.Id that the SELECT no longer produced.
  • Update with nothing to SET throws instead of emitting SET on its own; merge of a key-only entity assigns a key to itself, MySQL's insert-if-absent idiom.
  • Close the connection only if this call opened it. One the caller opened (to share across calls, or enlisted in a TransactionScope) was closed under them.
  • Join Database.CurrentTransaction when the context already has one open instead of failing with "Transactions may not be nested".
  • Name the actual type in "Unable to resolve EntityType" (was the literal 'T').

Dependencies

  • EF Core 8.0.4 -> 8.0.30 removes Microsoft.Extensions.Caching.Memory 8.0.0 (GHSA-qj66-m88j-hmgj) from the shipped package; Pomelo 8.0.2 -> 8.0.3.
  • Testcontainers.MySql 3.8.0 -> 4.14.0 drops SSH.NET 2023.0.0 (GHSA-q939-rpr3-3284) from the test project. Its own wait strategy replaces the port check.
  • Drop the unused EntityFrameworkCore.InMemory reference.

CI / repo

  • sonarcloud.yml: pass the token through the env var already declared instead of expanding the secret inside the run block (Sonar security hotspot).
  • .gitignore the mysql/ data volume docker-compose mounts.
  • Remove the container ExecAsync in the test factory whose arguments were wrong (-p mysql prompts; ${...} in C# is a literal) and whose result was ignored.

Docs

  • README / package readme: PK fallback, accepted expression shapes, merge rows-affected semantics, connection and transaction ownership, shadow props.
  • csproj Description no longer claims compatibility with most databases.
  • CLAUDE.md: key resolution, expression handling, ownership rule.

Tests: 40 -> 57. EdgeCaseTest (model-only) covers each generator change; ConnectionOwnershipTest covers connection and ambient-transaction ownership against MySQL. The two Log tests that asserted the old PK-only failure now assert the fallback.

Claude-Session: https://claude.ai/code/session_01E63oHuiKenf9H6tcgvDXVG

hongjs and others added 2 commits August 23, 2026 11:30
Library
- Reject entities with a shadow property (NotSupportedException). Values are read
  from CLR properties by reflection; a shadow FK / TPH discriminator has none and
  was sent as NULL without warning.
- Read option expressions from the expression tree. `x => x.CreatedAt` used to be
  compiled, run on a blank instance, and have DateTime's own properties read back,
  so IgnoreOnInsert/IgnoreOnUpdate/UniqueKeys silently did nothing. Only
  `x => x.Prop` and `x => new { x.A, x.B }` are accepted; anything else throws.
- Fall back to the primary key when the model has no unique index; only a
  keyless entity throws MissingPrimaryKeyException. Key resolution is now one
  method shared by update and delete, and a UniqueKeys name that is not a mapped
  property is an ArgumentException instead of a silent fallback.
- Keep key columns in the derived table when IgnoreOnUpdate names them; the JOIN
  referenced tmp.`Id` that the SELECT no longer produced.
- Update with nothing to SET throws instead of emitting `SET ` on its own; merge
  of a key-only entity assigns a key to itself, MySQL's insert-if-absent idiom.
- Close the connection only if this call opened it. One the caller opened (to
  share across calls, or enlisted in a TransactionScope) was closed under them.
- Join Database.CurrentTransaction when the context already has one open instead
  of failing with "Transactions may not be nested".
- Name the actual type in "Unable to resolve EntityType" (was the literal 'T').

Dependencies
- EF Core 8.0.4 -> 8.0.30 removes Microsoft.Extensions.Caching.Memory 8.0.0
  (GHSA-qj66-m88j-hmgj) from the shipped package; Pomelo 8.0.2 -> 8.0.3.
- Testcontainers.MySql 3.8.0 -> 4.14.0 drops SSH.NET 2023.0.0 (GHSA-q939-rpr3-3284)
  from the test project. Its own wait strategy replaces the port check.
- Drop the unused EntityFrameworkCore.InMemory reference.

CI / repo
- sonarcloud.yml: pass the token through the env var already declared instead of
  expanding the secret inside the run block (Sonar security hotspot).
- .gitignore the mysql/ data volume docker-compose mounts.
- Remove the container ExecAsync in the test factory whose arguments were wrong
  (`-p mysql` prompts; `${...}` in C# is a literal) and whose result was ignored.

Docs
- README / package readme: PK fallback, accepted expression shapes, merge
  rows-affected semantics, connection and transaction ownership, shadow props.
- csproj Description no longer claims compatibility with most databases.
- CLAUDE.md: key resolution, expression handling, ownership rule.

Tests: 40 -> 57. EdgeCaseTest (model-only) covers each generator change;
ConnectionOwnershipTest covers connection and ambient-transaction ownership
against MySQL. The two Log tests that asserted the old PK-only failure now
assert the fallback.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E63oHuiKenf9H6tcgvDXVG
…ge row-count claim

EdgeCaseTest asserts on the generated SQL; these four send it to the server.
The key-only merge (ON DUPLICATE KEY UPDATE t.Id = t.Id) is accepted and
leaves matched rows alone; update and delete with no UniqueKeys option match
on the primary key the library fell back to; a key named in IgnoreOnUpdate
is used for the JOIN but not written.

Running them showed the README's claim that a matched-but-unchanged row
counts 0 was wrong: MySqlConnector connects with CLIENT_FOUND_ROWS
(UseAffectedRows=false) by default, so such a row counts 1. Both readmes and
the test now say so, and note that UseAffectedRows=true gives 0.

The key-only entity lives in EdgeCaseDbContext, which has no table in the
shared database, and EnsureCreated does nothing on a database that already
has tables; it is created in a second database on the same container as root.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01E63oHuiKenf9H6tcgvDXVG
@sonarqubecloud

Copy link
Copy Markdown

@hongjs
hongjs merged commit 977fd87 into main Aug 23, 2026
6 checks passed
@hongjs
hongjs deleted the fix/bulk-hardening branch August 23, 2026 04:46
hongjs added a commit that referenced this pull request Aug 23, 2026
Security: the one open vulnerability was publish_nuget.yml expanding
${{ secrets.NUGET_TOKEN }} inside the run block - the same finding fixed in
sonarcloud.yml by #34. The token now reaches the script through env.

Coverage: the library was at 99%; the 89.9% overall came from the API
project, where OrderRepository and Order had no test at all and
ProductRepository.SyncDataThenCommit's catch never ran. OrderRepositoryTest
sends the FK / DateOnly entity the benchmarks use through every repository
method against MySQL; EfCoreTest gains the commit-path failure.

Maintainability (the 10 new issues from #34): the ON / SET / ON DUPLICATE
KEY UPDATE loops in BulkCommand become two helpers built on string.Join,
column selection moves out of the update and merge generators so each drops
below the complexity limit, the TODO and commented-out FieldsToUpdate block
leave BulkOption (the design now lives in docs/roadmap.md, not yet
committed), EdgeCaseTest uses static readonly arrays, and two members
nothing read - ColumnInfo.IsKey, BatchData.SuccessCount - are removed along
with a redundant OpenAsync in ExecuteBatchDataAsync. Generated SQL is
unchanged: every exact-string assertion in BulkCommandTest still passes.

Tests: 61 -> 68.


Claude-Session: https://claude.ai/code/session_01E63oHuiKenf9H6tcgvDXVG

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant