Skip to content

Feature/36 consolidate delete behaviour - #53

Open
achilles-portarinos wants to merge 9 commits into
mainfrom
feature/36-consolidate-delete-behaviour
Open

Feature/36 consolidate delete behaviour#53
achilles-portarinos wants to merge 9 commits into
mainfrom
feature/36-consolidate-delete-behaviour

Conversation

@achilles-portarinos

Copy link
Copy Markdown
  • consolidate delete behaviour: Precise deletions (by id and/or partition key) throw. Imprecise (predicate) return number of deleted items.
  • implement soft-delete
  • add tests for soft-delete
  • update XML comments in IDatabaseRepository
  • move GetAsync(predicate) to QuerySingleAsync(predicate) and ensure single result

@achilles-portarinos achilles-portarinos linked an issue Nov 30, 2022 that may be closed by this pull request
@SebastianKuesters

Copy link
Copy Markdown
Member

Review opinion (automated triage by @sebastian via Copilot)

Good direction — consolidating delete semantics and adding soft-delete is valuable, and the test coverage (soft-delete + query-single) is solid.

Points I'd want addressed:

  1. Breaking API changes should be called out explicitly and shipped as a major version bump: DeleteAsync(predicate) changes TaskTask<int>, DeleteAsync(id) now throws EntityNotFound when nothing matched, and GetAsync(predicate) moves to QuerySingleAsync. All reasonable, but they'll break callers.
  2. Soft-delete predicate path. DeleteAsync(predicate) under soft-delete loads every matching entity and fires ReplaceAsync in parallel via Task.WhenAll with unbounded concurrency and no atomicity. On large result sets this can hammer the backing store and partially fail, leaving an inconsistent state. Consider bounded concurrency, paging, or routing this through a batch/transaction.
  3. Staleness. This PR is ~3 years old and now conflicts heavily with main. A lot of this area has likely been reworked since. Please verify against current main — if it's been superseded, let's close it; otherwise it needs a substantial rebase.

Happy to see it land once the breaking changes are documented and the bulk soft-delete path is made safe.

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.

DeleteAsync behaves inconsistently

2 participants