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

fix(model): Made sure errors with messages are passed to AggregateError for bulk creates. #9133

Merged
merged 5 commits into from
Mar 10, 2018

Conversation

aceew
Copy link
Contributor

@aceew aceew commented Mar 3, 2018

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Previously objects passed to the AggregateError did not have a message property, causing them to be
rendered as [object Object] in the console log. This change fixes the issue of a missing message
property.

fixes #8989

…or for bulk creates.

Previously objects passed to the AggregateError did not have a message property, causing them to be
rendered as [object Object] in the console log. This change fixes the issue of a missing message
property

fix sequelize#8989
@codecov
Copy link

codecov bot commented Mar 3, 2018

Codecov Report

Merging #9133 into master will decrease coverage by 7.05%.
The diff coverage is 100%.

Removed some leftover debugging and update the documentation
@aceew
Copy link
Contributor Author

aceew commented Mar 3, 2018

Is it possible that the issue noted here is happening for other dialects than 'mssql'? I can't replicate this test failing locally

…or for bulk creates.

Changed the punctuation in a docblock
lib/model.js Outdated
* Simply extending the value of `err` would cause a breaking change as the `errors`
* property is expected on each item in the array of errors.
*/
errors.push(new sequelizeErrors.RecordLinkedError(err, instance));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just push validation error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would cause a breaking change, a property of record and errors is expected in each item.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can simply do this

err.record = instance;
errors.push(err);

In v5 we can remove record as instance property is already available in ValidationErrorItem

Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution, I made some improvement now its good to og

@sushantdhiman sushantdhiman merged commit 78a1fb6 into sequelize:master Mar 10, 2018
@sushantdhiman
Copy link
Contributor

🎉 This PR is included in version 4.35.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@aceew
Copy link
Contributor Author

aceew commented Mar 10, 2018

Hey thanks, sorry I've been afk all week

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.

Aggregate errors do not print inner error(s) correctly
2 participants