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

Upsert does not always find/handle unique index #13240

Closed
2 of 7 tasks
JacobLey opened this issue Apr 29, 2021 · 2 comments
Closed
2 of 7 tasks

Upsert does not always find/handle unique index #13240

JacobLey opened this issue Apr 29, 2021 · 2 comments
Labels
stale type: feature For issues and PRs. For new features. Never breaking changes.

Comments

@JacobLey
Copy link
Contributor

Issue Description

What are you doing?

class User extends Model {
}
User.init({
    name: DataTypes.STRING,
    someVal: DataTypes.INTEGER,
}, {
   indexes: [
       fields: [fn('LOWER', 'name')],
       unique: true,
       where: { someVal: 123 },
       name: '<some-name>',
   ],
})

const [{ id }] = await User.upsert({
    name: 'abc',
    someVal: 123,
}, {
    fields: ['name'],
    returning: ['id'],
});
const [{ id: id2 }] = await User.upsert({
    name: 'abc',
    someVal: 123,
}, {
    fields: ['name'],
    returning: ['id'],
});
console.log(id === id2) // true

What do you expect to happen?

In the above code, I would expect the User model to be upserted based on the partial unique index.

What is actually happening?

Sequelize fails to detect the unique index and upserts based on primary key. So Postgres rejects the insert due to unique constraint without explicit handling.

Additional context

To my understanding, here are the general causes to this failure:

The first is that the index detection only looks for string literals for the field name https://github.com/sequelize/sequelize/blob/main/lib/dialects/abstract/query-interface.js#L789

So when an index uses functions, it cannot find the values (because fn() !== fn())

The second is that even if the unique-index selection logic found util methods, it would still fail to stringify the values (currently it only expects string literals).
https://github.com/sequelize/sequelize/blob/main/lib/dialects/abstract/query-generator.js#L186
(Will open PR for this fix)

And third is that only the fields are used in declaring the conflict. An index with a where clause should also have that included.
See index_predicate here https://www.postgresql.org/docs/13/sql-insert.html

Environment

  • Sequelize version: 6.6.2
  • Node.js version: 14.16.1
  • Operating System: MacOS

Issue Template Checklist

How does this problem relate to dialects?

  • I think this problem happens regardless of the dialect.
  • I think this problem happens only for the following dialect(s): Postgres (which supports partial index with functions). May impact other languages with similar functionality
  • I don't know, I was using PUT-YOUR-DIALECT-HERE, with connector library version XXX and database version XXX

Would you be willing to resolve this issue by submitting a Pull Request?

  • Yes, I have the time and I know how to start.
  • Yes, I have the time but I don't know how to start, I would need guidance.
  • No, I don't have the time, although I believe I could do it if I had the time...
  • No, I don't have the time and I wouldn't even know how to start.

I will open a PR for the second part, but the first and third are a bit trickier.

For the first, I think the best solution is to provide the name of the index in the options, and map that to the found index. Note the generated SQL should still use unique index inference for the reason stated in Postgres docs, but those issues don't apply to Sequelize.

For the third, I think the insert query options needs to take a where option as well. And when present, stringify it and attach to the ON CONFLICT statement.

JacobLey added a commit to JacobLey/sequelize that referenced this issue Apr 29, 2021
wbourne0 added a commit to wbourne0/sequelize that referenced this issue May 11, 2021
Add support for where clauses used in the ON CONFLICT part of the queries generated by model.bulkCreate and model.upsert for usage >

Closes sequelize#13240, sequelize#13066, sequelize#13031, sequelize#12774, sequelize#12742, sequelize#12595, sequelize#11656
@wbourne0 wbourne0 added status: wip For issues and PRs. Applied when the PR is not ready yet / when work to close the issue has started. type: feature For issues and PRs. For new features. Never breaking changes. labels Aug 3, 2021
@wbourne0 wbourne0 removed the status: wip For issues and PRs. Applied when the PR is not ready yet / when work to close the issue has started. label Aug 4, 2021
@wbourne0
Copy link
Member

wbourne0 commented Aug 4, 2021

Can you break this up into multiple issues? I'm creating an issue for the partial index part, but ideally each step would be its own issue.

Edit: Here's the issue for the ON CONFLICT WHERE part: #13412

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has been open for 7 days without activity. It will be closed if no further activity occurs. If this is still an issue, just leave a comment or remove the "stale" label. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale type: feature For issues and PRs. For new features. Never breaking changes.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants