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

Jump to bottom

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

Closed
tad-lispy opened this issue Oct 21, 2015 · 4 comments
Closed

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

tad-lispy opened this issue Oct 21, 2015 · 4 comments
Labels
good first issue For issues. An issue that is a good choice for first-time contributors. type: bug

Comments

@tad-lispy
Copy link

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
Copy link
Contributor

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

@mickhansen mickhansen added type: bug good first issue For issues. An issue that is a good choice for first-time contributors. labels Oct 21, 2015
@tad-lispy
Copy link
Author

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

@mickhansen
Copy link
Contributor

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.

@tad-lispy
Copy link
Author

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?

@sushantdhiman sushantdhiman mentioned this issue Dec 21, 2015
Merged
mickhansen added a commit that referenced this issue Dec 22, 2015
@mickhansen
Merge pull request #5081 from sushantdhiman/fix-4702
75b63d8 
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
Assignees
No one assigned
Labels
good first issue For issues. An issue that is a good choice for first-time contributors. type: bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mickhansen @tad-lispy