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

feat: add options.rawErrors to Sequelize#query method #13881

Merged
merged 4 commits into from Jan 25, 2022

Conversation

joaoe
Copy link
Contributor

@joaoe joaoe commented Jan 2, 2022

Pull Request Checklist

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

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

Description Of Change

feat(query): added options.rawErrors to query() method

This option causes all errors from the underlying connection/database library
to be propagated unformatted and unmodified. Use this for lesser common errors
sequelize can't parse or in case you prefer to see the full raw errors.

Use case: in my project, we have a central database where different apps connect, like some scientific libraries and a REST API implemented using Node.JS. All errors in all apps are logged centrally. So, if there are DB errors, it is mostly productive to get the original error messages from the database so all developers understand them, and not just those that develop with node.js+sequelize. Also, Sequelize unfortunatelly cannot parse all errors, like at some point there was a cryptic error, because sequelize could not insert in a table with a trigger, something that was easily visible once we saw the original db error.

So, the rawErrors option is a nice to have for those that are not so interested in Sequelize's own error parsing. Also, it saves some CPU cyles. It can be set per query or in the global sequelize.options.query.

Also bonus: stack handling for db2.

Todos

  • Wait for all pipelines to run green

Copy link
Member

@WikiRik WikiRik 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 the PR! I only looked at the mssql docs changes and the eslint changes for now and have some comments on that

package.json Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@WikiRik
Copy link
Member

WikiRik commented Jan 2, 2022

And it seems that Microsoft currently only offers 2017 & 2019 Docker images so we need to figure out a different way to test on 2014 if we really want to; https://hub.docker.com/_/microsoft-mssql-server

@joaoe
Copy link
Contributor Author

joaoe commented Jan 2, 2022

And it seems that Microsoft currently only offers 2017 & 2019 Docker images so we need to figure out a different way to test on 2014 if we really want to; https://hub.docker.com/_/microsoft-mssql-server

Bummer.
Perhaps phase out ms sql 2014 and 2016 support for Sequelize v7 due to lack of support from MS ? Their mainstream support date have expired.

package.json Outdated Show resolved Hide resolved
lib/dialects/abstract/query.js Show resolved Hide resolved
build.js Outdated Show resolved Hide resolved
Copy link
Member

@sdepold sdepold left a comment

Choose a reason for hiding this comment

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

Good stuff. Thanks :)

test/support.js Outdated Show resolved Hide resolved
build.js Outdated Show resolved Hide resolved
build.js Outdated Show resolved Hide resolved
test/integration/sequelize/query.test.js Outdated Show resolved Hide resolved
test/integration/sequelize/query.test.js Outdated Show resolved Hide resolved
test/integration/sequelize/query.test.js Outdated Show resolved Hide resolved
ephys
ephys previously approved these changes Jan 10, 2022
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

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

Good on my end. Just need to resolve other reviewer's requests & fix tests :)

@joaoe
Copy link
Contributor Author

joaoe commented Jan 21, 2022

Howdy.

Life's busy, but I've just updated the PR with a rebased branch.
The MS SQL commit I shall do another PR.
The commit to pass the root js files to the linter is now not needed since someone else changed that in another PR.

@joaoe joaoe force-pushed the slightly_opinionated_changes branch from 8815f6c to f406fe8 Compare January 21, 2022 21:20
@joaoe
Copy link
Contributor Author

joaoe commented Jan 21, 2022

@ephys Hi. The pipeline is acting weird. A lot of MS SQL and MySQL fails which are completely unrelated with my work.

@ephys
Copy link
Member

ephys commented Jan 22, 2022

Updating the branch to see if it still happens. It's not happening in the other PRs so if it still happens here it must be a subtle issue that was introduced :/

@ephys
Copy link
Member

ephys commented Jan 22, 2022

Unfortunately they are still failing. Could be related to the changes made in test/support.js?

@joaoe
Copy link
Contributor Author

joaoe commented Jan 23, 2022

Unfortunately they are still failing. Could be related to the changes made in test/support.js?

That would be quite fascinating given the randomness of the fails. Oh well, tomorrow I might try to push the branch one commit at a time.

@joaoe joaoe force-pushed the slightly_opinionated_changes branch from e03ceb0 to 0ce8e76 Compare January 24, 2022 13:03
@joaoe
Copy link
Contributor Author

joaoe commented Jan 24, 2022

Fascinating. The pipeline passed without the _.merge() changes. Oh well.
@ephys Lets keep that commit out then just so I'm done with this PR.

Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

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

Last note on my end I think, the rest looks good to me

build.js Outdated Show resolved Hide resolved
@joaoe
Copy link
Contributor Author

joaoe commented Jan 24, 2022

@ephys I'm very confused. Something happened recently with the project now the partial build takes as long at the whole build... bah ! Since there is no benefit here, I'll just skip this commit as well... :/

@joaoe joaoe force-pushed the slightly_opinionated_changes branch from 0ce8e76 to a3a2562 Compare January 24, 2022 20:56
@WikiRik
Copy link
Member

WikiRik commented Jan 24, 2022

@ephys I'm very confused. Something happened recently with the project now the partial build takes as long at the whole build... bah ! Since there is no benefit here, I'll just skip this commit as well... :/

Strange. Do you happen to know around what time this change happened? For example a release/commit in which it still worked. So we can try to figure out what happened

@joaoe joaoe changed the title Slightly opinionated changes feat(query): added options.rawErrors to query() method Jan 24, 2022
@joaoe
Copy link
Contributor Author

joaoe commented Jan 24, 2022

@ephys I'm very confused. Something happened recently with the project now the partial build takes as long at the whole build... bah ! Since there is no benefit here, I'll just skip this commit as well... :/

Strange. Do you happen to know around what time this change happened? For example a release/commit in which it still worked. So we can try to figure out what happened

The past two weeks or so. There was a big commit about making eslint becoming more stringent and doing autoformatting. Perhaps that changed some settings. I could try to bisect this but then can leave it for another PR.

@joaoe
Copy link
Contributor Author

joaoe commented Jan 24, 2022

@ephys I'm very confused. Something happened recently with the project now the partial build takes as long at the whole build... bah ! Since there is no benefit here, I'll just skip this commit as well... :/

Strange. Do you happen to know around what time this change happened? For example a release/commit in which it still worked. So we can try to figure out what happened

I might be hallucinating. I tried a month old commit from early december. The age check doesn't work, it is still slow.
I think I might be confusing build.js not doing anything when I just edited test files.
Well, then this PR is ready.

WikiRik
WikiRik previously approved these changes Jan 24, 2022
Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

Looks good to me

@WikiRik WikiRik requested a review from ephys January 24, 2022 21:50
This option causes all errors from the underlying connection/database library
to be propagated unformatted and unmodified. Use this for lesser common errors
sequelize can't parse or in case you perfer to see the full raw errors.
@joaoe
Copy link
Contributor Author

joaoe commented Jan 24, 2022

I see a fail in DB2 + Node 16 but that is also happening in the main branch.

@WikiRik
Copy link
Member

WikiRik commented Jan 25, 2022

Yeah we got a flaky test there, don't worry about that

Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

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

I'm sorry to hear that the incremental build did not work :/
The rest looks good to me though

@ephys
Copy link
Member

ephys commented Jan 25, 2022

I think we can safely add this to v6 too

@ephys ephys changed the title feat(query): added options.rawErrors to query() method feat: add options.rawErrors to query() method Jan 25, 2022
@ephys ephys changed the title feat: add options.rawErrors to query() method feat: add options.rawErrors to Sequelize#query method Jan 25, 2022
@joaoe
Copy link
Contributor Author

joaoe commented Jan 25, 2022

ok, who can merge ?

@ephys ephys merged commit 4c8fa9a into sequelize:main Jan 25, 2022
@ephys
Copy link
Member

ephys commented Jan 25, 2022

Thank you for the PR! :)

sdepold pushed a commit that referenced this pull request Jan 29, 2022
Co-authored-by: Zoé <zoe@ephys.dev>
Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
@github-actions
Copy link
Contributor

github-actions bot commented Feb 7, 2022

🎉 This PR is included in version 7.0.0-alpha.6 🎉

The release is available on:

Your semantic-release bot 📦🚀

aliatsis pushed a commit to creditiq/sequelize that referenced this pull request Jun 2, 2022
)

Co-authored-by: Zoé <zoe@ephys.dev>
Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants