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

Models with cyclic foreign keys and ENUM-typed fields can't be synced in Postgres #15522

Open
3 of 6 tasks
francofrizzo opened this issue Jan 3, 2023 · 2 comments · May be fixed by #14687
Open
3 of 6 tasks

Models with cyclic foreign keys and ENUM-typed fields can't be synced in Postgres #15522

francofrizzo opened this issue Jan 3, 2023 · 2 comments · May be fixed by #14687

Comments

@francofrizzo
Copy link

francofrizzo commented Jan 3, 2023

Issue Creation Checklist

  • I understand that my issue will be automatically closed if I don't fill in the requested information
  • I have read the contribution guidelines

Bug Description

In PostgreSQL, when there is a circular dependency between models (following foreign keys) and at least some model in the database has an ENUM-typed property, calls to sequelize.sync() will fail, as the enum type is attempted to be created twice.

This bug seems to have been introduced in v6.20.0, when support for cyclic foreign keys was added. The implementation synchronizes the models twice, first to create the tables, and then to create the foreign keys. Both runs are attempting to create the types for the ENUM-typed fields, which is not an idempotent operation.

Reproducible Example

Here is the link to the SSCCE for this issue: https://github.com/francofrizzo/sequelize-sscce-cyclic-associations

What do you expect to happen?

Models should be correctly synced when sequelize.sync() is called.

What is actually happening?

A SequelizeDatabaseError is thrown, indicating that type "enum_<model>_<field>" already exists.

Environment

  • Sequelize version: v6.28.0
  • Node.js version: v14.21.1
  • Database & Version: PostgreSQL 14.5
  • Connector library & Version: pg@8.8.0

Would you be willing to resolve this issue by submitting a Pull Request?

  • Yes, I have the time and I know how to start.
  • Yes, I have the time but I will need guidance.
  • No, I don't have the time, but my company or I are supporting Sequelize through donations on OpenCollective.
  • No, I don't have the time, and I understand that I will need to wait until someone from the community or maintainers is interested in resolving my issue.

Indicate your interest in the resolution of this issue by adding the 👍 reaction. Comments such as "+1" will be removed.

@francofrizzo francofrizzo added pending-approval Bug reports that have not been verified yet, or feature requests that have not been accepted yet type: bug labels Jan 3, 2023
@WikiRik
Copy link
Member

WikiRik commented Jan 3, 2023

I can confirm this on main as well, by copying the following test and adding the enum to there;

it('supports creating tables with cyclic associations', async () => {
const A = sequelize.define('A', {}, { timestamps: false });
const B = sequelize.define('B', {}, { timestamps: false });
// mssql refuses cyclic references unless ON DELETE and ON UPDATE is set to NO ACTION
const mssqlConstraints = dialect === 'mssql' ? { onDelete: 'NO ACTION', onUpdate: 'NO ACTION' } : null;
// These models both have a foreign key that references the other model.
// Sequelize should be able to create them.
A.belongsTo(B, { foreignKey: { allowNull: false, ...mssqlConstraints } });
B.belongsTo(A, { foreignKey: { allowNull: false, ...mssqlConstraints } });
await sequelize.sync();
const [aFks, bFks] = await Promise.all([
sequelize.queryInterface.getForeignKeyReferencesForTable(A.getTableName()),
sequelize.queryInterface.getForeignKeyReferencesForTable(B.getTableName()),
]);
expect(aFks.length).to.eq(1);
expect(aFks[0].referencedTableName).to.eq('Bs');
expect(aFks[0].referencedColumnName).to.eq('id');
expect(aFks[0].columnName).to.eq('BId');
expect(bFks.length).to.eq(1);
expect(bFks[0].referencedTableName).to.eq('As');
expect(bFks[0].referencedColumnName).to.eq('id');
expect(bFks[0].columnName).to.eq('AId');
});

I think part of the fix should be in changeColumn so I'll tag @ephys in here.

@WikiRik WikiRik removed the pending-approval Bug reports that have not been verified yet, or feature requests that have not been accepted yet label Jan 3, 2023
@ephys
Copy link
Member

ephys commented Jan 4, 2023

This is a variant of #7649. The cause is the same, but we can add this test to the new suite to make sure

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

Successfully merging a pull request may close this issue.

3 participants