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

instance.set('id', 1) returns undefined #4702

Closed
lzrski opened this Issue Oct 21, 2015 · 4 comments

Comments

2 participants
@lzrski

lzrski commented Oct 21, 2015

Seems like setting a value of a field that is a primary key returns undefined. Consider this snippet:

Foo
  .findOne()
  .then( (foo) -> foo.set('id', 100).save() )

It throws TypeError: Cannot read property 'save' of undefined. Instead of this I would expect a meaningful error to be thrown when I try to save the instance.

Note: It actually doesn't matter if the field name is id or what it's type is. The fact that it is a primary key is relevant.

I'm using v. 3.12.0 with sqlite dialect.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Oct 21, 2015

Looks like we have a bunch of return paths that simply return; rather than return this;.
https://github.com/sequelize/sequelize/blob/master/lib/instance.js#L335

@lzrski

This comment has been minimized.

lzrski commented Oct 22, 2015

It would be easy to throw instead of return-ing. Do you think it's a good idea?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Oct 22, 2015

throw would be breaking BC, so a major bump.
A few of the returns are safety valves, so they could throw, one or two are just convenience.
In any case for now they should probably all be changed to return this.

@lzrski

This comment has been minimized.

lzrski commented Oct 22, 2015

That's a good point.

In this case return this would result in just silently ignoring the set operation, right?

I would say it is an improvement, but can lead to very subtle bugs. Ultimately we should have some mechanism to notify the application code about a failure to set given field. Do you think throwing is an option for v. 4.x?

mickhansen added a commit that referenced this issue Dec 22, 2015

Merge pull request #5081 from sushantdhiman/fix-4702
Fix(#4702): instance returns this on all paths
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment