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

targetKey not working in BelongsTo association (sync() problem?) #4455

Closed
guigrpa opened this Issue Sep 8, 2015 · 8 comments

Comments

3 participants
@guigrpa

guigrpa commented Sep 8, 2015

Consider this very simple case (under Sequelize v3.7.1):

User = sequelize.define 'user',
  name: Sequelize.STRING
  uid: 
    type: Sequelize.INTEGER
    unique: true
Photo = sequelize.define 'photo',
  location: Sequelize.STRING
  itemId: 
    type: Sequelize.INTEGER
Photo.belongsTo User,
  foreignKey: 'itemId'
  targetKey: 'uid'

sequelize.sync()

The query generated by sync() ignores the targetKey configuration:

CREATE TABLE IF NOT EXISTS "photos" (
    "id"   SERIAL , 
    "location" VARCHAR(255), 
    "itemId" INTEGER REFERENCES "users" ("id") ON DELETE SET NULL ON UPDATE CASCADE,
    "createdAt" TIMESTAMP WITH TIME ZONE NOT NULL, 
    "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL, 
    PRIMARY KEY ("id")
);

You can see it references id instead of uid, as intended.

If, on the contrary, we specify {references: {model: User, key: 'uid'}} when calling sequelize.define(), the relationship is created by sync() correctly. In this case, the relationship also works correctly when e.g. creating a photo and calling getUser() on it, so I guess the problem is in the implementation of sync().

As a side note, when will targetKey be supported for HasMany relationships? It seems to me a very important feature.

@guigrpa

This comment has been minimized.

guigrpa commented Sep 8, 2015

See also #4269 and #4258

@guigrpa

This comment has been minimized.

guigrpa commented Sep 8, 2015

I can also confirm that, in the example above, finding Photos and includeing the corresponding User works as expected (when {references: {model: User, key: 'uid'}} is used).

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 8, 2015

We have a working test here:

it('should support a belongsTo with the targetKey option', function() {

@mickhansen mickhansen added the bug label Sep 8, 2015

@guigrpa

This comment has been minimized.

guigrpa commented Sep 8, 2015

Interesting. Looking at the test, I find the following line (I don't know why the id is removed in this test):

User.removeAttribute('id');

If I do that in my case, the FK constraint is not created at all (this is not verified in the test):

CREATE TABLE IF NOT EXISTS "photos" (
    "id"   SERIAL , 
    "location" VARCHAR(255), 
    "itemId" INTEGER, 
    "createdAt" TIMESTAMP WITH TIME ZONE NOT NULL, 
    "updatedAt" TIMESTAMP WITH TIME ZONE NOT NULL, 
    PRIMARY KEY ("id")
);

...and getting the User from a created Photo works correctly, just as in the test.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 8, 2015

Hmm, curious if it works without removing the primary key, maybe it doesn't.

@jayfresh

This comment has been minimized.

jayfresh commented Sep 15, 2015

I also stumbled into this problem. Removing the id from the target removes the error, but I also fail to see the relation created in the SQL.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 15, 2015

Looks like the test was written like that because SQLITE does not support constraints on anything other than primary key.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 15, 2015

Ah well, neither does postgres, looks like there shouldn't be a constraint at all then (unless it's a unique).

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