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: filter also on schema name when retrieving foreign keys #16770

Open
wants to merge 6 commits into
base: v6
Choose a base branch
from

Conversation

ghusse
Copy link
Contributor

@ghusse ghusse commented Nov 16, 2023

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • If a documentation update is necessary, have you opened a PR to the documentation repository?
  • 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?
  • Does the name of your PR follow our conventions?

Description Of Change

When retrieving the list of foreign keys of a table on Postgresql, relationships are not filtered by schema. It means that if a different schema contains the same table, getForeignKeyReferencesForTable will also return relationships of this second table.

This PR fixes this issue by filtering also by database name and schema name to be sure that only FKs from the requested table are retrieved.

@ghusse ghusse marked this pull request as draft November 16, 2023 15:52
@ghusse ghusse force-pushed the fix/relationships-multiple-schemas branch from d8c1be1 to ba945ef Compare November 16, 2023 16:14
@ghusse ghusse changed the title fix(postgres): filter also on schema name when retrieving foreign keys fix(postgres,mssql): filter also on schema name when retrieving foreign keys Nov 16, 2023
@ghusse ghusse changed the title fix(postgres,mssql): filter also on schema name when retrieving foreign keys fix: filter also on schema name when retrieving foreign keys Nov 16, 2023
@ghusse ghusse marked this pull request as ready for review November 16, 2023 16:27
@ghusse ghusse marked this pull request as draft November 16, 2023 16:27
@ghusse ghusse marked this pull request as ready for review November 17, 2023 08:31
@ghusse
Copy link
Contributor Author

ghusse commented Nov 20, 2023

PR #16785 on v7 shows that this issue is already fixed on v7, but I'm interested in a fix in v6 as well.

@ghusse
Copy link
Contributor Author

ghusse commented Dec 6, 2023

@WikiRik, the PR on v7 has been merged. Is it possible to merge this one as well? It fixes an issue with foreign keys retrieval.

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, I have one comment regarding mysql/mariadb though. There are unit tests that we could add with different schemas, but we have those in v7 and we hope to release that soon so I think it's fine if we don't add them here

@@ -40,5 +40,51 @@ describe(Support.getTestDialectTeaser('QueryInterface'), () => {
expect(refs[0]).deep.include.all(expectedObject);

});

describe('with schemas', () => {
// MariaDB does not really support schemas (they are synonyms of databases)
Copy link
Member

Choose a reason for hiding this comment

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

Is this behaviour different between mariadb and mysql?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For MySQL, Support.sequelize.dialect.supports.schemas is false, but for MariaDB it's true (even though it does not really support schemas).

You can see it there:

  1. For MySQL
  2. For MariaDB

@ephys
Copy link
Member

ephys commented Dec 6, 2023

This would be a breaking change though, as the behavior would change. It would be better to do this in v7

@ghusse
Copy link
Contributor Author

ghusse commented Dec 6, 2023

This would be a breaking change though, as the behavior would change. It would be better to do this in v7

I added tests in v7 to check that it's already working as expected (and it is).

I agree that the behavior will change, but the behavior is buggy. For instance in PG, there is a condition to retrieve info only about the table in the right schema, but the condition has not been added to the JOIN conditions. Meaning that it'll retrieve foreign keys of tables with the same name from other schemas.

@WikiRik WikiRik added the v6 label Dec 22, 2023
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.

None yet

3 participants