Fix #1142: upsert to use first matching unique constraint

[dadansatria]

Q	A
Is bugfix?	✔️
New feature?	❌
Breaks BC?	❌
Fixed issues	#1142

When a table has multiple separate unique constraints (e.g., primary key on id and unique index on slug), the upsert method incorrectly merged all unique column names, generating invalid SQL like:

ON CONFLICT ("id", "slug") DO UPDATE ...

This failed because there's no composite unique constraint on (id, slug).

The fix: Changed getTableUniqueColumnNames() to return columns from only the first matching constraint, instead of merging columns from all constraints.

[codecov]

Codecov Report

❌ Patch coverage is 18.75000% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.55%. Comparing base (178124f) to head (677079a).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/QueryBuilder/AbstractDMLQueryBuilder.php	0.00%	10 Missing ⚠️
src/Command/AbstractCommand.php	25.00%	3 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (178124f) and HEAD (677079a). Click for more details.

HEAD has 24 uploads less than BASE

Flag	BASE (178124f)	HEAD (677079a)
	25	1

Additional details and impacted files

@@              Coverage Diff              @@
##             master    #1144       +/-   ##
=============================================
- Coverage     98.79%   66.55%   -32.24%     
- Complexity     1654     1655        +1     
=============================================
  Files           120      120               
  Lines          4301     4273       -28     
=============================================
- Hits           4249     2844     -1405     
- Misses           52     1429     +1377     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Tigrov]

Looks this will not work correct for some cases.

Better to generate expression for each unique index

Instead of ... ON CONFLICT ("id", "slug") DO UPDATE SET ...
should be ... ON CONFLICT ("id") DO UPDATE SET ... ON CONFLICT ("slug") DO UPDATE SET ...

[dadansatria]

Looks this will not work correct for some cases.

Better to generate expression for each unique index

Instead of ... ON CONFLICT ("id", "slug") DO UPDATE SET ... should be ... ON CONFLICT ("id") DO UPDATE SET ... ON CONFLICT ("slug") DO UPDATE SET ...

I checked the PostgreeSQL docs and unfortunately, multiple ON CONFLICT clauses are not allowed in a single INSERT statement

The syntax only allows ONE ON CONFLICT clause per INSERT

[Tigrov]

I checked the PostgreeSQL docs and unfortunately, multiple ON CONFLICT clauses are not allowed in a single INSERT statement

The syntax only allows ONE ON CONFLICT clause per INSERT

Seems this works for SQLite but not for PostgreSQL.

Then we should add an extra parameter to the upsert() method to specify the exact columns for the ON CONFLICT (...) clause.

[Copilot]

Pull request overview

This PR fixes a bug where tables with multiple separate unique constraints would generate invalid SQL by merging all unique column names into a single ON CONFLICT clause. The fix changes getTableUniqueColumnNames() to return columns from only the first matching constraint instead of merging all constraints.

Key Changes:

• Modified getTableUniqueColumnNames() to return the first matching unique constraint instead of merging all matching constraints
• Added optional $constraintColumns parameter to upsert(), upsertReturning(), and upsertReturningPks() methods to allow explicit constraint specification
• Removed unused imports (array_filter, array_merge, array_unique) that were only needed for the old merging logic

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/QueryBuilder/DMLQueryBuilderInterface.php	Added $constraintColumns parameter to interface method signatures with documentation
src/QueryBuilder/AbstractQueryBuilder.php	Updated wrapper methods to pass through the new $constraintColumns parameter
src/QueryBuilder/AbstractDMLQueryBuilder.php	Core logic changes: simplified getTableUniqueColumnNames() to return first match only, added support for explicit constraint columns, removed unused imports
src/Debug/CommandInterfaceProxy.php	Added $constraintColumns parameter to proxy methods
src/Command/CommandInterface.php	Added $constraintColumns parameter to command interface method signatures with documentation
src/Command/AbstractCommand.php	Updated command methods to accept and pass through the new $constraintColumns parameter
CHANGELOG.md	Documented the change and bug fix

Comments suppressed due to low confidence (1)

src/QueryBuilder/AbstractDMLQueryBuilder.php:483

• The PHPDoc is missing @param documentation for parameters $table, $insertColumns, and $updateColumns. Only $constraints and $constraintColumns are documented. For consistency and completeness, all parameters should be documented.

    /**
     * Prepare column names and constraints for "upsert" operation.
     *
     * @param Index[] $constraints
     * @param string[]|null $constraintColumns The column names to use for the conflict clause. If `null`,
     * the primary key or the first matching unique constraint will be used.
     *
     * @psalm-param array<string, mixed>|QueryInterface $insertColumns
     *
     * @return array Array of unique, insert and update column names.
     * @psalm-return array{0: string[], 1: string[], 2: string[]|null}
     */

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Tigrov]

Dependent packages also need to be fixed (see test results)

[Tigrov]

Suggested change

	private function getTableUniqueColumnNames(string $name, array $columns, array &$indexes = []): array
	private function getTableUniqueColumnNames(string $name, array $columns, ?Index &$index = null): array

array can be changed to ?Index with corresponding fixes of code

[vjik]

These changes break BC. So I suggest split this PR on two:

1. Fix AbstractDMLQueryBuilder::getTableUniqueColumnNames(). This is bugfix, and it doesn't break BC.

2. Introduce $constraintColumns for "upsert"-methods.

And one more idea... Configure globally default upsert constraint columns in connection. This is doesn't break BC and this will be enough in most cases.

[Tigrov]

1. Fix AbstractDMLQueryBuilder::getTableUniqueColumnNames(). This is bugfix, and it doesn't break BC.

This probably doesn't solve the problem; see the issue #1142. This solution will select id field as a unique index, but the correct solution would be to specify the slug field for this.

And one more idea... Configure globally default upsert constraint columns in connection. This is doesn't break BC and this will be enough in most cases.

This may also break BC if configure them via an additional method or parameter.

[vjik]

And one more idea... Configure globally default upsert constraint columns in connection. This is doesn't break BC and this will be enough in most cases.

This may also break BC if configure them via an additional method or parameter.

If we implement this via new optional parameter, it doesn't break BC. Also it may solve issue #1142.

[dadansatria]

Hi @vjik @Tigrov

you're right, adding params to interface methods does break BC for custom implementations. How if I split this into two PRs:

1. Bugfix only: Fix getTableUniqueColumnNames() to use PK first, then first matching unique constraint
2. New feature (next minor): Add the $constraintColumns param
   Let me know if this approach works. I'll start on the split..

[vjik]

Hi @vjik @Tigrov

you're right, adding params to interface methods does break BC for custom implementations. How if I split this into two PRs:

1. Bugfix only: Fix getTableUniqueColumnNames() to use PK first, then first matching unique constraint
2. New feature (next minor): Add the $constraintColumns param
   Let me know if this approach works. I'll start on the split..

Bugfix needs to be made.

New feature requires a discussion. Parameter adding is OK, but maybe it's worth to add global per table defaults for constraint in upsert.

[Tigrov]

Separate PR with bugfix only - OK

New feature requires a discussion. Parameter adding is OK, but maybe it's worth to add global per table defaults for constraint in upsert.

I think it's better to add an additional parameter to the method that breaks BC (as it done in the PR) and wait for the next major update.
If necessary, user can use the branch with this patch in composer.json.