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 for many-to-many associations being impossible between models with custom table names #698

Merged
merged 2 commits into from Jun 18, 2013

Conversation

3 participants
@jjclark1982
Contributor

jjclark1982 commented Jun 14, 2013

It is currently impossible to create many-to-many associations between DAOs that have custom table names. Consider this example:

var Sequelize = require("sequelize");

var sequelize = new Sequelize("db", null, null, {
    dialect: "sqlite",
    storage: "example.sqlite3"
});

var Listing = sequelize.define("listing", {
    "name": {type: Sequelize.STRING}
}, {
    tableName: "listings"
});

var Tag = sequelize.define("tag", {
    "name": {type: Sequelize.STRING}
}, {
    tableName: "tags"
});

Listing.hasMany(Tag, {joinTableName: "listing_has_tags"});
Tag.hasMany(Listing, {joinTableName: "listing_has_tags"});

sequelize.sync();

This will try to create an association table with the name tags instead of listing_has_tags, because the association inherits options from the DAO instead of from the sequelize global options.

I propose fixing this by explicitly setting the association's table name to the combined table name when it is being defined.

@jjclark1982

This comment has been minimized.

Contributor

jjclark1982 commented Jun 14, 2013

It would also be possible to change the way options are inherited in associations/mixin.js, but that could change behavior for existing installations.

  var globalOptions = this.daoFactoryManager.sequelize.options.define
  var association = new HasMany(this, associatedDAO, Utils._.extend((options||{}), globalOptions))
@janmeier

This comment has been minimized.

Member

janmeier commented Jun 14, 2013

I believe that the best option would be to use the global sequelize config as you mentioned, but yes that might cause some problems, so for now this seems like the best solution.

Could you add a simple test to ensure that .options.tablename is correct?

@sdepold

This comment has been minimized.

Member

sdepold commented Jun 17, 2013

a test would rock hard :)

@jjclark1982

This comment has been minimized.

Contributor

jjclark1982 commented Jun 17, 2013

Here is a test that verifies the association accessor name matches the association table name.

This test should work even if option inheritance logic changes in the future.

it('uses the specified joinTableName or a reasonable default', function(done) {
for (var associationName in this.User.associations) {
expect(associationName).not.toEqual(this.User.tableName)

This comment has been minimized.

@janmeier

janmeier Jun 18, 2013

Member

I know the error before was that user would be re-used as table name, so it makes sense to test that here. But could you add a line checking that the assoc name is not task either, just to make sure that no-one messes up your beautiful code :)

With that addition your PR should be good to merge

edit:
Nevermind, I can just do it myself! :)

janmeier added a commit that referenced this pull request Jun 18, 2013

Merge pull request #698 from jjclark1982/master
Fix for many-to-many associations being impossible between models with custom table names

@janmeier janmeier merged commit 8b234bf into sequelize:master Jun 18, 2013

1 check passed

default The Travis CI build passed
Details

janmeier added a commit that referenced this pull request Jun 18, 2013

@durango durango referenced this pull request Jul 8, 2013

Closed

State of the Union #749

33 of 45 tasks complete

@AdamPflug AdamPflug referenced this pull request Aug 5, 2013

Closed

Alpha 3 #806

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