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.

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

feat(postgres, sqlite): add conflictWhere option to upsert #13411

Merged
merged 16 commits into from
Feb 11, 2023

Conversation

wbourne0
Copy link
Member

@wbourne0 wbourne0 commented Aug 3, 2021

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

  • Added the option conflictWhere to QueryGenerator.prototype.insertQuery. This is used to generate the ON CONFLICT WHERE part of the query (note: not ON CONFLICT DO UPDATE WHERE).
  • Added an option to Model.upsert, conflictWhere, This is passed to the query generator.
  • Updated types
  • Added tests

Example use case:

const Memberships = sequelize.define(
  'memberships',
  {
    user_id: { type: DataTypes.INTEGER, allowNull: false },
    foreign_key: { type: DataTypes.INTEGER, allowNull: false },
    permissions: { type: DataTypes.ENUM('admin', 'member', 'guest'), allowNull: false }
  },
  {
    deletedAt: 'time_deleted',
    indexes: [
      {
        fields: ['user_id', 'foreign_key'],
        unique: true,
        where: { time_deleted: null },
      },
    ],
  }
);

// Adds a membership for a user as an admin (updates previous membership instead if there is one)
await Memberships.upsert(
  {
    user_id: userId,
    foreign_id: foreignId,
    permissions: 'admin',
  },
  {
    conflictWhere: { time_deleted: null },
  }
);

Closes #13412.

TODO:

Add some tests which validate conflictWhere & conflictFields work together properly.

@wbourne0 wbourne0 added type: feature For issues and PRs. For new features. Never breaking changes. dialect: sqlite For issues and PRs. Things that involve SQLite (and do not involve all dialects). dialect: postgres For issues and PRs. Things that involve PostgreSQL (and do not involve all dialects). status: wip For issues and PRs. Applied when the PR is not ready yet / when work to close the issue has started. labels Aug 3, 2021
@wbourne0 wbourne0 self-assigned this Aug 3, 2021
@wbourne0 wbourne0 changed the title feat(upsert): Add upsert conflictWhere feat(upsert): Add conflictWhere option to Model.upsert Aug 3, 2021
@wbourne0 wbourne0 added status: awaiting maintainer and removed status: wip For issues and PRs. Applied when the PR is not ready yet / when work to close the issue has started. labels Aug 4, 2021
@wbourne0 wbourne0 mentioned this pull request Aug 4, 2021
2 tasks
@wbourne0 wbourne0 linked an issue Aug 4, 2021 that may be closed by this pull request
2 tasks
@wbourne0 wbourne0 added P4: nice to have For issues that are not bugs. and removed P4: nice to have For issues that are not bugs. labels Aug 5, 2021
Keimeno
Keimeno previously requested changes Aug 8, 2021
test/integration/model/upsert.test.js Outdated Show resolved Hide resolved
lib/model.js Outdated Show resolved Hide resolved
Keimeno
Keimeno previously approved these changes Sep 14, 2021
@wbourne0 wbourne0 changed the title feat(upsert): Add conflictWhere option to Model.upsert feat(upsert): add conflictWhere option Oct 10, 2021
@wbourne0
Copy link
Member Author

Update on this: I ended up implementing this in a fork, but IIRC there was an issue I had w/ it (I believe it was around the logic upsert uses to determine which conflict columns to use, which isn't in the scope of this PR).

I want to verify that it wasn't an issue with this change itself before I merge it.

@sdepold
Copy link
Member

sdepold commented Dec 12, 2021

@allawesome497 What shall we do with this one?

@wbourne0
Copy link
Member Author

@allawesome497 What shall we do with this one?

I'll need to resolve merge conflicts, but it should be fine.

Now that #13723 is merged I think I'll also add a few more tests which test the combined behavi for some more specific use cases.

@wbourne0 wbourne0 changed the title feat(upsert): add conflictWhere option feat(postgres, sqlite): add conflictWhere option to upsert Nov 10, 2022
@wbourne0
Copy link
Member Author

wbourne0 commented Nov 17, 2022

Bumping this -- what's the status?

Waiting for reviewers atm; just bumped it in the sequelize slack.

@wbourne0 wbourne0 dismissed ephys’s stale review November 17, 2022 19:46

requested changes made

ephys
ephys previously approved these changes Dec 8, 2022
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

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

I still think we need to do a thorough clean up of the different options upsert accepts but this is good

@wbourne0 wbourne0 requested a review from ephys December 27, 2022 23:46
@wbourne0
Copy link
Member Author

@ephys re-requesting review - had to patch for compatibility with main which dismissed your approval.

WikiRik
WikiRik previously approved these changes Dec 27, 2022
Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
@wbourne0
Copy link
Member Author

wbourne0 commented Feb 9, 2023

@WikiRik can you approve this again? the rebase dismissed it.

@wbourne0 wbourne0 requested a review from WikiRik February 9, 2023 22:42
@WikiRik WikiRik merged commit 32a7aa8 into sequelize:main Feb 11, 2023
@wbourne0 wbourne0 deleted the add-upsert-conflictWhere branch February 13, 2023 19:45
wbourne0 added a commit that referenced this pull request Mar 14, 2023
Adds an option to set the where clause in the `ON CONFLICT` part of postgres/sqlite upserts.  Backport of #13411.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialect: postgres For issues and PRs. Things that involve PostgreSQL (and do not involve all dialects). dialect: sqlite For issues and PRs. Things that involve SQLite (and do not involve all dialects). type: feature For issues and PRs. For new features. Never breaking changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ON CONFLICT WHERE for upserts.
8 participants