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

Associations where model name clashes with a property already defined on the model should throw an error #1272

Closed
SamVerschueren opened this Issue Jan 20, 2014 · 26 comments

Comments

5 participants
@SamVerschueren

SamVerschueren commented Jan 20, 2014

Update by janmeier The error turned out to be a naming conflict between an attribute and the name of the associated model. An error should be thrown when the association is created if there is a conflict

Dear developers

I was running my application on a development server and everything went fine. Then I moved everything to the production server but something went wrong there. In my package.json file I had "sequelize": "latest" and it downloaded the 2.0.0 beta version. Not sure why that's the default version now, but ok. I removed that version and installed version 1.7.0-rc2 but it seems that it does not solve my problem. The problem is the relationships of my models. Some does not seem to work.

An example that does not work for me. I have a table Person and a table Car. A person has multiple cars, and a car belongs to a person. So far so good. The relationships are defined as such:

Person.hasMany(Car, {foreignKey: 'person'});
Car.belongsTo(Person, {foreignKey: 'person'});

If I retrieve all the persons and include the cars, it works fine.

Person.findAll({include: [Car]}).success(function(persons) {
    // Do whatever you want
});

But when I ask it the other way around, it does not work.

Car.findAll({include: [Person]}).success(function(cars) {
     // Do whatever you want
});

The query that get's executed is:

SELECT `car`.*, `person`.`ID` as `person.ID`, `person`.`name` as `person.name` FROM `car` LEFT OUTER JOIN `person` as `person` ON `person`.`ID`=`car`.`person`;

So it returns a table with the following attributes: ID, make, color, person, person.ID, person.name but when I stringify my response from sequelize, the only thing I get is {ID, make, color}. It seems that the rest of the person date is omitted or something.

Thanks in advance!

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 20, 2014

query looks right. I'll take a look at this, but if you code provide a self contained unit test in the style of our other unit tests that would speed up the process greatly.

@ghost ghost assigned mickhansen Jan 20, 2014

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 20, 2014

What's the result if you run this query manually?

@SamVerschueren

This comment has been minimized.

SamVerschueren commented Jan 20, 2014

When I run the query manually in MySQL workbench then the result seems correct. Not sure how you guys map the result but it returns the following attributes:
ID, make, color, person, person.ID, person.name

When I log the not stringified result I find some trails of the relationship.

options: 
   { isNewRecord: false,
     isDirty: false,
     include: [ [Object] ],
     includeNames: [ 'person', 'person' ],
     includeMap: { person: [Object] },
     includeValidated: true,
     raw: true },

But when I ask the car for the person object as such

car.getPerson();

it returns {fct: [Function]}

And the stringified result of that is just an empty object: {}

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 20, 2014

What does car.dataValues contain?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 20, 2014

car.getPerson() is an association fetcher, it's got nothing to do with prefetching. If you used prefetching (include) then person should be available via car.person or car.toJSON().person or car.get('person').

@SamVerschueren

This comment has been minimized.

SamVerschueren commented Jan 20, 2014

car.dataValues contains:

{ ID: 1,
     make: 'ferrari',
     color: 'red' }
@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 20, 2014

Alright then there must be a bug. Could you provide a self contained test case with seed data etc?
https://github.com/sequelize/sequelize/blob/master/test/include.test.js
If you're able to provide a PR with a failing test case that proves your issue i'll get it fixed for your tonight or tomorrow night.

@SamVerschueren

This comment has been minimized.

SamVerschueren commented Jan 20, 2014

I wrote the test. Now in the test, I cannot reproduce the problem so I must have done something else wrong in my code. Although, when I define the relationship with a foreignKey attribute, it says "Maximum call stack size exceeded".

When you define the relationship as such:

Person.hasMany(Car);
Car.belongsTo(Person);

It works as expected (if you update the person attribute in the bulkcreate to PersonId).

describe('Include', function() {
    var Car, Person;

    beforeEach(function(done) {
        Car = this.sequelize.define('Cars', {
            ID: { type: DataTypes.INTEGER, primaryKey: true, autoIncrement: true, allowNull: false },
            make: { type: DataTypes.STRING(20), defaultValue: null},
            color: { type: DataTypes.STRING(20), defaultValue: null}
        });

        Person = this.sequelize.define('Persons', {
            ID: { type: DataTypes.INTEGER, primaryKey: true, autoIncrement: true, allowNull: false },
            name: { type: DataTypes.STRING(20), defaultValue: null}
        });

        Person.hasMany(Car, {foreignKey: 'person'});
        Car.belongsTo(Person, {foreignKey: 'person'});

        this.sequelize.sync({force: true}).done(function() {
            async.waterfall([
                function(callback) {
                    Person.build({name: 'Darth Vader'}).save().done(function(err) {
                        Person.findAll().success(function(persons) {
                            callback(null, persons);
                        }).error(function(err) {
                            callback(err);
                        });
                    });
                },
                function(results, callback) {
                    Car.bulkCreate([
                        {make: 'Ferrari', color: 'red', person: results[0].ID}
                    ]).success(function() {
                        callback(null);
                    }).error(function(err) {
                        callback(err);
                    });
                }
            ], function(err, result) {
                expect(err).not.to.be.ok;

                done();
            });
        });
    });

    describe('find', function() {
        it('should support one to many relationship', function(done) {
            Person.findAll({include: [Car]}).success(function(persons) {
                expect(persons[0].cars).to.be.ok;

                done();
            }).error(function(err) {
                expect(err).to.be.ok
            });
        });

        it('should support many to one relationship', function(done) {
            Car.findAll({include: [Person]}).success(function(cars) {
                expect(cars[0].person).to.be.ok;

                done();
            }).error(function(err) {
                expect(err).to.be.ok
            });
        })
    });
});
@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 20, 2014

Alright let me know if you can figure out a isolated test case, then i'll happily take a look at it for you. Although your original code should work.

@SamVerschueren

This comment has been minimized.

SamVerschueren commented Jan 20, 2014

I found a use case where it does not work.

describe('Include', function() {
    var Car, Person, Contact, ContactType;

    beforeEach(function(done) {
        Contact = this.sequelize.define('contact', {
            ID:             { type: DataTypes.INTEGER, primaryKey: true, autoIncrement: true, allowNull: false},
            contactvalue:   { type: DataTypes.STRING(200), defaultValue: null},
            contacttype:    { type: DataTypes.INTEGER, defaultValue: null},
            office:         { type: DataTypes.INTEGER, defaultValue: null},
            officer:        { type: DataTypes.INTEGER, defaultValue: null},
        });

        ContactType = this.sequelize.define('contacttype', {
            ID:     { type: DataTypes.INTEGER, primaryKey: true, autoIncrement: true, allowNull: false},
            name:   { type: DataTypes.STRING(50), defaultValue: null}
        });

        ContactType.hasMany(Contact, {foreignKey: 'contacttype'});
        Contact.belongsTo(ContactType, {foreignKey: 'contacttype'});

        this.sequelize.sync({force: true}).done(function() {
            async.waterfall([
                function(callback) {
                    ContactType.build({name: 'Facebook'}).save().done(function(err) {
                        ContactType.findAll().success(function(types) {
                            callback(null, types);
                        }).error(function(err) {
                            callback(err);
                        });
                    });
                },
                function(results, callback) {
                    Contact.bulkCreate([
                        { contactvalue: 'facebook.com/bla', contacttype: results[0].ID, office: 1, officer: 1},
                        { contactvalue: 'facebook.com/bla2', contacttype: results[0].ID, office: 1, officer: 1}
                    ]).success(function() {
                        callback(null);
                    }).error(function(err) {
                        callback(err);
                    });
                }
            ], function(err, result) {
                expect(err).not.to.be.ok;

                done();
            });
        });
    });

    describe('find', function() {
        it('should support one to many relationship', function(done) {
            ContactType.findAll({include: [Contact]}).success(function(contacttypes) {
                expect(contacttypes[0].contacts).to.exist;

                done();
            }).error(function(err) {
                expect(err).to.be.ok
            });
        });

        it('should support many to one relationship for the contact model', function(done) {
            Contact.findAll({include: [ContactType]}).success(function(contacts) {
                expect(contacts[0].contacttype, 'contacttype does not exist').to.exist;

                done();
            })
        });
    });
});

This fails and it says, 'contacttype does not exist'. And indeed, it does not exist. I actually see nothing different then the case above.

Is my relationship definition wrong?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 20, 2014

contacttype as a foreign key might clash. But i believe this should work, i'll try and take a look at it in the following days.

@SamVerschueren

This comment has been minimized.

SamVerschueren commented Jan 20, 2014

Jep, when I remove the contacttype foreignKey attribute, it works as expected and the test succeeds.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 20, 2014

Does it work if you rename it to contacttype_id ?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 20, 2014

(Both the original attribute and the foreignKey option that is)

@SamVerschueren

This comment has been minimized.

SamVerschueren commented Jan 20, 2014

Yes that works. So it's really something in the name of the attribute...

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 20, 2014

Yea it's because the association result has the same key as your attribute, so there's a clash :)

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 20, 2014

I should've noticed that in your original post, sorry i didn't.

@SamVerschueren

This comment has been minimized.

SamVerschueren commented Jan 20, 2014

Is this going to be solved or should I use a workaround or something? It's an already existing database so renaming the keys to something else is a no-go.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Jan 20, 2014

I don't see how it could be "solved". Naming conflicts are just that. Perhaps you can work around it by using the as option.

Car.belongsTo(Person, {as: 'PersonModel', foreignKey: 'person'});
@SamVerschueren

This comment has been minimized.

SamVerschueren commented Jan 20, 2014

Yes, with the as option it works as supposed to. It's weird that it worked on my development server but not on my production server. But well, we tracked it down so I'm happy :).

@bblack

This comment has been minimized.

bblack commented Apr 18, 2014

How do we feel about adding a check for collisions between attributes and associations? It would be a lot easier to deal with Error: name collision between attribute 'foo' and association 'foo' than a mysterious stack overflow somewhere in async-land as the result of a find() call. (I spent much of this afternoon hunting this down).

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Apr 19, 2014

@bblack would not be opposed to this at all - would probably actually be a great idea. Better to fail fast and clearly than late and subtlely ;)

@janmeier janmeier changed the title from One-To-Many not working anymore to Associations where model name clashes with a property already defined on the model should throw an error Apr 29, 2014

@janmeier janmeier reopened this Apr 29, 2014

@janmeier

This comment has been minimized.

Member

janmeier commented Apr 29, 2014

Re-opened this and changed it to a feature request to throw an error if there is a naming collision

@amonaco

This comment has been minimized.

amonaco commented May 30, 2014

I was able to reproduce this. Somehow in the code when a model attribute and model name 'collide' this situation is created. The error is thrown without any stack trace and the application crashes. +1 bblack's idea

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented May 30, 2014

@amonaco well per Janzehs commit it's supposed to throw an error with a message. Are you testing against master?

@amonaco

This comment has been minimized.

amonaco commented May 30, 2014

Thanks for the quick reply. I think we're using 1.7.5. I'll give it a try using master. Thanks!

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