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

use 'returning: true' for postgres to keep patch and its response atomic #93

Merged
merged 3 commits into from
Mar 17, 2017

Conversation

MichaelErmer
Copy link
Contributor

Summary

This PR utilizes the Sequelize returning option for update to do PATCH, in a single action.

This is a non-breaking change.

Other Information

The PATCH procedure for the other dialects should probably also be reworked to use transactions, this may only cause problems on large scale deployments, but it is a potential issue.

@daffl
Copy link
Member

daffl commented Mar 16, 2017

Does this only work for Postgres?

@MichaelErmer
Copy link
Contributor Author

MichaelErmer commented Mar 16, 2017

Yes.

[options.returning=false] Boolean Return the affected rows (only for postgres)

src/index.js Outdated
if (this.dialect === 'postgres') {
options.returning = true;
return this.Model.update(omit(data, this.id), options)
.then((results) => {
Copy link
Member

@daffl daffl Mar 17, 2017

Choose a reason for hiding this comment

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

.then(results => results[1])

src/index.js Outdated
if (id !== null) {
where[this.id] = id;
}

const options = Object.assign({}, params.sequelize, { where });

// This is the best way to implement patch in sql, the other dialects 'should' use a transaction.
if (this.dialect === 'postgres') {
Copy link
Member

Choose a reason for hiding this comment

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

I'd check options.Model.sequelize.options.dialect here and remove setting it in the constructor

Michael Ermer added 2 commits March 17, 2017 22:43
@daffl daffl merged commit a4225fe into feathersjs-ecosystem:master Mar 17, 2017
@daffl
Copy link
Member

daffl commented Mar 17, 2017

Released as v1.4.3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants