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 overwrites data with default values on update #3994

Merged
merged 2 commits into from Jun 24, 2015

Conversation

5 participants
@FredKSchott
Contributor

FredKSchott commented Jun 23, 2015

Currently, a defaultValue set on the model will be used in every upsert operation. This is fine for when upsert INSERTs, since that is when default values are used/needed. However, upsert is also using defaultValue on INSERT, which is causing data to be overwritten and lost by the default values.

I've included a test that is currently broken by this bug, I haven't had time to look into a fix yet. EDIT: fix also added

/cc @mickhansen @janmeier (from the last upsert issue we worked on)

@FredKSchott

This comment has been minimized.

Contributor

FredKSchott commented Jun 23, 2015

I've updated the PR with a proposed fix. updateValues are currently created out of the full INSERT data object, and has no way of knowing which fields are defaults and which should be changed. This PR moves this creation up closer to the model where default value and change information is still known.

return this.User.findById(42);
}).then(function(user) {
originalCreatedAt = user.createdAt;
return this.sequelize.Promise.delay(1000).bind(this).then(function() {

This comment has been minimized.

@janmeier

janmeier Jun 24, 2015

Member

You can use sinon fake timers here, to artifically advance time http://sinonjs.org/docs/#clock

This comment has been minimized.

@FredKSchott

FredKSchott Jun 24, 2015

Contributor

In this situation I think we need the actual delay so that sequelize can internally get the proper Date() times on both create & upsert. I've used sinon to fake timers before, but iirc sinon can't modify the actual system time which is what we need here.

This comment has been minimized.

@janmeier

janmeier Jun 24, 2015

Member

We use new Date() internally, and that is exactly what fake timers override :)

This comment has been minimized.

@FredKSchott

FredKSchott Jun 24, 2015

Contributor

I must be misremembering then, I remember a lot of trouble last time I used these for anything other than extending timers. I'll experiment a bit tomorrow and see if I can get it to work.

This comment has been minimized.

@FredKSchott

FredKSchott Jun 24, 2015

Contributor

@janmeier definitely misremembered, this worked perfectly!

@janmeier janmeier self-assigned this Jun 24, 2015

@janmeier

This comment has been minimized.

Member

janmeier commented Jun 24, 2015

LGTM - just a single comment

var updatedDataValues = _.pick(instance.dataValues, Object.keys(instance._changed));
var insertValues = Utils.mapValueFieldNames(instance.dataValues, options.fields, this);
var updateValues = Utils.mapValueFieldNames(updatedDataValues, options.fields, this);
var now = Utils.now(this.sequelize.options.dialect);

This comment has been minimized.

@mickhansen

mickhansen Jun 24, 2015

Contributor

Purely style: mind using leading commas with a single var statement?

This comment has been minimized.

@FredKSchott

FredKSchott Jun 24, 2015

Contributor

fasho

@FredKSchott FredKSchott force-pushed the FredKSchott:upsert-no-defaults branch from 438d8d4 to 7d6d2cc Jun 24, 2015

@FredKSchott

This comment has been minimized.

Contributor

FredKSchott commented Jun 24, 2015

@janmeier @mickhansen comments addressed

janmeier added a commit that referenced this pull request Jun 24, 2015

Merge pull request #3994 from FredKSchott/upsert-no-defaults
upsert overwrites data with default values on update

@janmeier janmeier merged commit 3d0af74 into sequelize:master Jun 24, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@janmeier

This comment has been minimized.

Member

janmeier commented Jun 24, 2015

Thanks @FredKSchott ! :)

@FredKSchott

This comment has been minimized.

Contributor

FredKSchott commented Jun 24, 2015

Woot! thanks for the quick responses :)
Is there any way we could push a patch in the next few days to get this out? It sucks to wait 2-3 weeks for every change to get upsert working.

@janmeier

This comment has been minimized.

Member

janmeier commented Jun 24, 2015

If you bug @mickhansen then probably yes :)

In the meantime you can point your package.json to sequelize/sequelize#3d0af74947b71b2523b7cff873963c506e5a7a75

@FredKSchott

This comment has been minimized.

Contributor

FredKSchott commented Jun 24, 2015

Yea good point, I'll point for now. Thanks @janmeier!

@FredKSchott FredKSchott deleted the FredKSchott:upsert-no-defaults branch Jun 24, 2015

mickhansen added a commit that referenced this pull request Jun 25, 2015

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jun 25, 2015

Released in 3.3.2

@FredKSchott

This comment has been minimized.

Contributor

FredKSchott commented Jun 30, 2015

Thanks @mickhansen!

@telpalbrox

This comment has been minimized.

telpalbrox commented Jul 3, 2015

Hi, since this release, the upsert function is not working properly.
I am trying to upsert an entry with 2 unique fields. Those fields are included in order entries so when I execute the upsert there is a MySQL error "error: uncaughtException: Cannot read property 'uniqueKeys' of undefined" and it is no catched with the .catch method so the app goes down.

@janmeier

This comment has been minimized.

Member

janmeier commented Jul 3, 2015

@telpalbrox please open a new thread with some code that shows the error

@ciberyo16

This comment has been minimized.

ciberyo16 commented Jul 3, 2015

thank you @janmeier I opened it here #4059

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