Skip to content

SEP-2011: Teach the duplicate-check precheck to read unique constraints - #1553

Open
yyyyyyyan with Copilot wants to merge 2 commits into
mainfrom
copilot/sep-2011-teach-duplicate-check-precheck
Open

yyyyyyyan with Copilot wants to merge 2 commits into
mainfrom
copilot/sep-2011-teach-duplicate-check-precheck

Conversation

Copilot AI commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

BaseSQLModelManager.save's duplicate precheck introspected local_table.indexes only, so the four tables that declare uniqueness as a UniqueConstraint in __table_args__ (syncentityabsence, taskhistory_log, taskhistory_log_state, atw_incident_execution) fell through to commit and answered HTTPBadRequestException (400) where every index-declared table answers HTTPConflictException (409).

app/core/db/crud.py

  • The precheck now runs one loop over the union of unique column-sets from both sources — each unique Index.columns and each UniqueConstraint.columns — so the body that builds equal_filters, queries, and raises stays written once and the message format ("<Model> with the same <columns> already exists") is identical either way. PrimaryKeyConstraint is excluded by the isinstance filter.
  • The completeness test moved from truthiness to presence:
if all(value is not None for value in equal_filters.values()):

A naive port would have kept all(equal_filters.values()), which silently skips any key whose column holds 0 — i.e. taskhistory_log's most common row, the first chunk of a stream at start_offset=0. This narrows the skip condition on the pre-existing index path too, so it can only start catching collisions that were previously missed.

tests/app/core/db/test_crud.py

  • New ConstraintUniqueModel / ConstraintUniqueManager mirroring the CompositeUniqueModel pattern but declaring a single-column and a composite unique key via UniqueConstraint, with one column that legitimately holds 0.
  • Cases in TestSaveDuplicatePrecheck for a single-column collision, a composite collision (a leading-column-only match still saves), a collision whose key member is 0, no write emitted before the conflict, and an update to a free value. All four failing cases reproduce the 400 with the crud.py change reverted.

changelog.d/SEP-2011.fixed.md

  • User-facing status-code change, added via make changelog-add.

No schema change: migrating the four declarations to unique Index objects was the alternative, but it needs Alembic migrations across two tracks and does nothing for the next UniqueConstraint-only table added.

Co-authored-by: yyyyyyyan <24644216+yyyyyyyan@users.noreply.github.com>
Copilot AI changed the title [WIP] SEP-2011: Teach duplicate-check precheck to read unique constraints SEP-2011: Teach the duplicate-check precheck to read unique constraints Sep 16, 2026
Copilot AI requested a review from yyyyyyyan September 16, 2026 20:06
@yyyyyyyan
yyyyyyyan marked this pull request as ready for review September 16, 2026 20:59
Copilot AI balanced review requested due to automatic review settings September 16, 2026 20:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The shared behavior change leaves stale truthiness contracts and needs a complete caller audit.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Expands duplicate prechecks to cover SQLAlchemy unique constraints and valid falsy key values.

Changes:

  • Combines unique indexes and constraints in duplicate detection.
  • Adds regression coverage for constraint collisions.
  • Documents the resulting 409 response behavior.
File summaries
File Description
app/core/db/crud.py Expands duplicate-key discovery.
tests/app/core/db/test_crud.py Adds constraint-path tests.
changelog.d/SEP-2011.fixed.md Records the user-facing fix.
Review details
  • Files reviewed: 3/3 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread app/core/db/crud.py
Comment on lines +1203 to +1205
# Presence, not truthiness: a legitimate 0, "" or False is a
# filled key member, and skipping it would miss the collision.
if all(value is not None for value in equal_filters.values()):
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants