Skip to content

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