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 ERROR detail to output of migration #1142

Merged
merged 1 commit into from
Sep 27, 2022

Conversation

snewcomer
Copy link
Contributor

@snewcomer snewcomer commented Aug 20, 2022

Pull Request check-list

  • Does npm run test 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)?

Description of change

close #1138

This PR adds additional error output. Currently the error output may be insufficient to hone in on the problem.

https://www.postgresql.org/docs/current/plpgsql-errors-and-messages.html

Desired behaviour -

ERROR: SequelizeForeignKeyConstraintError: insert or update on table "my_table" violates foreign key constraint "...."
ERROR DETAIL: Key (my_column)=(1) is not present in table "my_table"

Alternatives

Behind a debug flag. My initial hunch is to consider this a helpful feature that users get out of the box. Happy to put behind a debug flag though!

@WikiRik
Copy link
Member

WikiRik commented Aug 21, 2022

Hi! Thanks for the PR! I haven't given the alternative more thought yet, but can you add one or more tests for this? More tests is especially preferred if on some cases more information is given that might be considered confidential

@snewcomer
Copy link
Contributor Author

@WikiRik Gave a quick look around and not seeing a good extension point for this test. Do you happen to know of a place?

@fzn0x
Copy link
Member

fzn0x commented Sep 27, 2022

The changes just returning void and hard to test, maybe to test the detail property existence? @WikiRik

Copy link
Member

@fzn0x fzn0x left a comment

Choose a reason for hiding this comment

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

It's good to have this log

@WikiRik
Copy link
Member

WikiRik commented Sep 27, 2022

@snewcomer sorry for not responding before, I took a look as well and we do not really have any tests to check our error behaviour. We'll add that in a later stage. For now, this is fine

Thanks for the PR!

@WikiRik WikiRik merged commit 497b805 into sequelize:main Sep 27, 2022
@github-actions
Copy link

🎉 This PR is included in version 6.5.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add DETAIL to migration error output
3 participants