Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: upsert with composite unique index #9454

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

fix: upsert with composite unique index #9454

wants to merge 4 commits into from

Conversation

michalsn
Copy link
Member

Description
This PR fixes a bug in Postgre and SQLite3 where composite indexes are not fully taken into account when using upsert().

Both handlers:

  • requires specifying all columns in the composite unique constraint
  • and cannot mix columns from multiple unique constraints

Fixes #9450

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@michalsn michalsn added bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer labels Feb 19, 2025
@michalsn michalsn requested review from sclubricants, paulbalandan, kenjis and samsonasik and removed request for sclubricants February 19, 2025 08:50
paulbalandan
paulbalandan reviewed Feb 19, 2025
View reviewed changes
system/Database/Postgre/Builder.php
$constraints[] = current($index);
// only one index can be used?
foreach ($allIndexes as $index) {
$constraints = $index->fields;
Copy link
Member

Choose a reason for hiding this comment

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

Would $constraints be just overwritten on iterations > 1?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, we break the loop after the first item.

@sclubricants
Copy link
Member

Posgre has very different functioning then MySQL when it comes to upserts. Postgre offers much more features while mysql automatically applies any and all constraints. It was attempted apply the primary key if constraints were not set. Its really just a guess but it was done to try and provide the same functionality across all databases. For all the DBMS except mysql it is really recommended to use the onConstraint() method and explicitly define them. Perhaps the documentation should highlight this.

@michalsn
Copy link
Member Author

While I agree that onConstraint() should be used when dealing with multiple keys, in cases where we have one composite key, it should be automatically applied as a whole and not partially.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulbalandan paulbalandan paulbalandan left review comments

@sclubricants sclubricants Awaiting requested review from sclubricants

@kenjis kenjis Awaiting requested review from kenjis

@samsonasik samsonasik Awaiting requested review from samsonasik

Assignees
No one assigned
Labels
bug Verified issues on the current code behavior or pull requests that will fix them database Issues or pull requests that affect the database layer
Projects
None yet
Milestone
No milestone
3 participants
@michalsn @sclubricants @paulbalandan