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

don't overwrite options.foreignKey in associations #4927

Merged
merged 1 commit into from Nov 27, 2015

Conversation

2 participants
@wilzbach
Contributor

wilzbach commented Nov 26, 2015

I use sequelize quite often and just discovered are rare case where it doesn't exactly do what imho it is supposed to do. Let's assume we have three tables, where A & B have a string ID and C has an integer ID. Now when we try to connect B and C with A, we run into the problem that if a foreignKey is given as input, it will be modified by the associations. Hence this to this weird behavior:

var Sequelize = require('sequelize');
var sequelize = new Sequelize('database', 'username', 'password', {
    dialect: 'sqlite',
    storage: 'foo.sqlite'
});

var A = sequelize.define('A', {
    id: {
        type: Sequelize.CHAR(20),
        primaryKey: true
    }
});
var B = sequelize.define('B', {
    id: {
        type: Sequelize.CHAR(20),
        primaryKey: true
    }
});
var C = sequelize.define('C', {});
var reqValidForeignKey = { foreignKey: { allowNull: false }};

//B.hasMany(A, reqValidForeignKey); // other associations also modify the options.foreignKey
A.belongsTo(B, reqValidForeignKey);
A.belongsTo(C, reqValidForeignKey);
var QueryGenerator = A.modelManager.sequelize.queryInterface.QueryGenerator;
console.log(QueryGenerator.attributesToSQL(A.rawAttributes, {
    context: 'createTable'
}));
//{ id: 'CHAR(20) PRIMARY KEY',
//  createdAt: 'DATETIME NOT NULL',
//  updatedAt: 'DATETIME NOT NULL',
//  BId: 'CHAR(20) NOT NULL REFERENCES `Bs` (`id`) ON DELETE CASCADE ON UPDATE CASCADE',
//  CId: 'CHAR(20) NOT NULL REFERENCES `Cs` (`id`) ON DELETE NO ACTION ON UPDATE CASCADE' }

Note: CId should be an integer.

This is due to the fact that we use the same foreignKey object which is passed into the function and modify. In my use case I discovered this because both tables are required to have foreign keys.

This PR adds a very simple fix for this problem and also a test case to check for this ;-)

it('should not be overwritten for belongsTo', function(){
var self = this;
return this.sequelize.sync({force: true}).then(function() {

This comment has been minimized.

@janmeier

janmeier Nov 26, 2015

Member

There's no need to call .sync here - which means the test can be converted to a unit test

@janmeier

This comment has been minimized.

Member

janmeier commented Nov 26, 2015

LGTM, just a single comment

If you change that, add a changelog entry and squash this should be good to merge

@janmeier janmeier self-assigned this Nov 26, 2015

@wilzbach

This comment has been minimized.

Contributor

wilzbach commented Nov 26, 2015

If you change that, add a changelog entry and squash this should be good to merge

Thanks for your help :)

Should I be worried about travis failing for MySQL?

@janmeier

This comment has been minimized.

Member

janmeier commented Nov 26, 2015

Not as long as it's only a single job failing - we have an intermittent failure which we've not been able to track down yet

@janmeier

This comment has been minimized.

Member

janmeier commented Nov 26, 2015

Woops, sorry if my comment was not clear enough, could you move the file to test/unit

@wilzbach

This comment has been minimized.

Contributor

wilzbach commented Nov 26, 2015

Woops, sorry if my comment was not clear enough, could you move the file to test/unit

No worries - my fault ;-)

@janmeier janmeier merged commit 55c9607 into sequelize:master Nov 27, 2015

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

janmeier added a commit that referenced this pull request Nov 27, 2015

Merge pull request #4927 from greenify/master
don't overwrite options.foreignKey in associations
@janmeier

This comment has been minimized.

Member

janmeier commented Nov 27, 2015

Thanks! :)

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