Cyclic sequelize sync doesn't handle several cases in the non-cyclic version

[benasher44]

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

When running Sequelize.sync with a model graph with circular references, sequelize doesn't handle the following cases, which the non-cyclic version does:

1. Multi-column primary key with a foreign key constraint
2. Indexes with names that are too long

Reproducible Example

I don't have a reproducible example, but I did debug this already. I hope the detailed analysis below will make up for that and might be enough to keep this issue from being closed 🙏

1. In the cyclical version of Sequelize.sync, model.sync is called first with withoutForeignKeyContraints: true and then again with force: false, alter: true. In this second call when creating the constraints for a multi-column primary key column with a foreign key constraint, the foreign key constraint is never created because the following code skips the call to changeColumn because the column also happens to be a primary key:

sequelize/src/model.js

Lines 1385 to 1391 in 252e6d2

	if (currentAttribute.primaryKey) continue;
	// Check foreign keys. If it's a foreign key, it should remove constraint first.
	const references = currentAttribute.references;
	if (currentAttribute.references) {
	const database = this.sequelize.config.database;
	const schema = this.sequelize.config.schema;
	// Find existed foreign keys

While debugging this, I also noticed this check here is checking a table name against a model, which is unlikely working:

sequelize/src/model.js

Line 1397 in 252e6d2

&& foreignKeyReference.referencedTableName === references.model

2. For indexes with longs names, these are also impacted by that second call to model.sync with alter: true. For the sake of brevity, lets assume postgres truncates all index names to four characters (even though it's 63). If a model has already added an index named ABCDEF, and then you add a circular model reference to your graph, you'll bump into error about sequelize failing to create an index. The following code will look up the existing indexes, see that you've already created one called ABCD, determine that you need to still create one called ABCDEF, generate SQL to create it, but then fail since postgres will automatically truncate it to ABCD, which already exists:

sequelize/src/model.js

Lines 1413 to 1428 in 252e6d2

	const existingIndexes = await this.queryInterface.showIndex(tableName, options);
	const missingIndexes = this._indexes.filter(item1 =>
	!existingIndexes.some(item2 => item1.name === item2.name)
	).sort((index1, index2) => {
	if (this.sequelize.options.dialect === 'postgres') {
	// move concurrent indexes to the bottom to avoid weird deadlocks
	if (index1.concurrently === true) return 1;
	if (index2.concurrently === true) return -1;
	}
	return 0;
	});
	for (const index of missingIndexes) {
	await this.queryInterface.addIndex(tableName, { ...options, ...index });
	}

If you don't have circular references, sync() (possibly accidentally) handles this without issue automagically.

What do you expect to happen?

Sync with circular references generates correct SQL that matches the model graph

What is actually happening?

Sync generates invalid SQL for indexes and misses foreign key constraints for multi-column primary key

Environment

• Sequelize version: 6.37.2
• Node.js version: 20.11.1
• If TypeScript related: TypeScript version:
• Database & Version: postgres 15
• Connector library & Version:

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.

[benasher44]

I think 2 is probably solved by #15362