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

Multiple hasMany with as & through; TypeError: Cannot call method 'replace' #2316

Closed
haches opened this Issue Sep 21, 2014 · 22 comments

Comments

4 participants
@haches
Contributor

haches commented Sep 21, 2014

Version: 2.0.0-rc1

I have two models that associate each other

var project = ...

project.hasMany(user, {as: 'users', through: 'projectsUsers'});
project.hasMany(user, {as: 'owners', through: 'projectsOwners'});
var user = ...

user.hasMany(project, {as: 'users', through: 'projectsUsers'});
user.hasMany(project, {as: 'owners', through: 'projectsOwners'});

Trying to associate a user as owner to a project like this works fine:

project1.setOwners([user1]);

Trying to associate a user as user to a project fails:

project1.setUsers([user1]);

with a stack trace

Possibly unhandled TypeError: Cannot call method 'replace' of undefined
    at Object.module.exports.removeTicks (/node_modules/sequelize/lib/utils.js:510:14)
    at Object.module.exports.addTicks (/node_modules/sequelize/lib/utils.js:506:29)
    at Object.module.exports.QueryGenerator.quoteIdentifier (/node_modules/sequelize/lib/dialects/mysql/query-generator.js:346:20)
    at generateJoinQueries (/node_modules/sequelize/lib/dialects/abstract/query-generator.js:960:58)
    at Object.<anonymous> (/node_modules/sequelize/lib/dialects/abstract/query-generator.js:1013:29)
    at Array.forEach (native)
    at Object.module.exports.QueryGenerator.selectQuery (/node_modules/sequelize/lib/dialects/abstract/query-generator.js:1012:25)
    at module.exports.QueryInterface.select (/node_modules/sequelize/lib/query-interface.js:579:35)
    at null.<anonymous> (/node_modules/sequelize/lib/model.js:742:34)

...

Only the association defined last in the models can be used. This can be tested by switching the ordering of the associations in the models. In my example, it's then possible to use setUsers but no longer setOwners.

Please note that the above worked on 1.7.10 for me but now seems broken.

@haches haches changed the title from Multiple hasMany with as & through. to Multiple hasMany with as & through; TypeError: Cannot call method 'replace' Sep 21, 2014

@mickhansen mickhansen added the bug label Sep 22, 2014

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Sep 22, 2014

I might have broken something when implementing scopes.
If you have the time we would greatly appreciate a PR with a unit test covering this failing case.

@mickhansen mickhansen modified the milestones: 2.0.0 release candidate, 2.0.0 final release Sep 22, 2014

@rahilwazir

This comment has been minimized.

rahilwazir commented Oct 9, 2014

I have exactly the same problem.

I just upgraded from 2.0.0-beta.2 to 2.0.0-rc1

And I after got this error

[TypeError: Cannot call method 'replace' of undefined ]

When retrieving:

Show.find({
  where: where,
  include: [Venue, Track]
})

However I haved fixed it by defining plural includes property instead of include

Show.find({
  where: where,
  includes: [Venue, Track]
}).then(function() {
 ....
});

But after that I got this error:

[TypeError: Cannot read property 'length' of undefined]

What to do? It doesn't log enough error

@vpontis

This comment has been minimized.

Contributor

vpontis commented Oct 9, 2014

You could try catching the error and throwing it to generate a longer stack trace.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Oct 9, 2014

You can also try logging err.stack

@rahilwazir

This comment has been minimized.

rahilwazir commented Oct 9, 2014

Actually the includes property didn't work, here is stack trace:

TypeError: Cannot call method 'replace' of undefined
at Object.module.exports.removeTicks (/home/rw/html/llamamall2/node_modules/sequelize/lib/utils.js:510:14)
at Object.module.exports.addTicks (/home/rw/html/llamamall2/node_modules/sequelize/lib/utils.js:506:29)
at Object.module.exports.QueryGenerator.quoteIdentifier (/home/rw/html/llamamall2/node_modules/sequelize/lib/dialects/mysql/query-generator.js:346:20)
at generateJoinQueries (/home/rw/html/llamamall2/node_modules/sequelize/lib/dialects/abstract/query-generator.js:892:74)
at Object. (/home/rw/html/llamamall2/node_modules/sequelize/lib/dialects/abstract/query-generator.js:1013:29)
at Array.forEach (native)
at Object.module.exports.QueryGenerator.selectQuery (/home/rw/html/llamamall2/node_modules/sequelize/lib/dialects/abstract/query-generator.js:1012:25)
at [object Object].module.exports.QueryInterface.select (/home/rw/html/llamamall2/node_modules/sequelize/lib/query-interface.js:579:35)
at [object Object]. (/home/rw/html/llamamall2/node_modules/sequelize/lib/model.js:742:34)
From previous event:
at Function.Promise$Bind as bind
at [object Object].module.exports.Model.findAll (/home/rw/html/llamamall2/node_modules/sequelize/lib/model.js:700:20)
at [object Object].module.exports.Model.findOne (/home/rw/html/llamamall2/node_modules/sequelize/lib/model.js:794:17)
at /home/rw/html/llamamall2/lib/controllers/showController.coffee:153:7, :189:28
at renderCallback (/home/rw/html/llamamall2/lib/controllers/helper.coffee:16:3, :31:10)
at /home/rw/html/llamamall2/lib/controllers/helper.coffee:99:7, :145:16
at renderCallback (/home/rw/html/llamamall2/lib/controllers/helper.coffee:16:3, :31:10)
at /home/rw/html/llamamall2/lib/controllers/helper.coffee:58:7, :88:16
at renderCallback (/home/rw/html/llamamall2/lib/controllers/helper.coffee:16:3, :31:10)
at [object Object]. (/home/rw/html/llamamall2/lib/controllers/helper.coffee:41:7, :66:16)
From previous event:
at Function.Promise$Bind as bind
at [object Object].module.exports.Model.findAll (/home/rw/html/llamamall2/node_modules/sequelize/lib/model.js:700:20)
at [object Object].module.exports.Model.findOne (/home/rw/html/llamamall2/node_modules/sequelize/lib/model.js:794:17)
at userExist (/home/rw/html/llamamall2/lib/controllers/helper.coffee:38:3, :64:24)
at getUserRole (/home/rw/html/llamamall2/lib/controllers/helper.coffee:56:3, :86:12)
at Object.isPrivUser (/home/rw/html/llamamall2/lib/controllers/helper.coffee:97:3, :143:12)
at module.exports.listShow (/home/rw/html/llamamall2/lib/controllers/showController.coffee:143:3, :176:19)
at callbacks (/home/rw/html/llamamall2/node_modules/express/lib/router/index.js:164:37)
at restrict_login (/home/rw/html/llamamall2/server.coffee:137:9, :149:14)
at callbacks (/home/rw/html/llamamall2/node_modules/express/lib/router/index.js:164:37)
at param (/home/rw/html/llamamall2/node_modules/express/lib/router/index.js:138:11)
at param (/home/rw/html/llamamall2/node_modules/express/lib/router/index.js:135:11)
at pass (/home/rw/html/llamamall2/node_modules/express/lib/router/index.js:145:5)
at Router._dispatch (/home/rw/html/llamamall2/node_modules/express/lib/router/index.js:173:5)
at Object.router (/home/rw/html/llamamall2/node_modules/express/lib/router/index.js:33:10)
at next (/home/rw/html/llamamall2/node_modules/express/node_modules/connect/lib/proto.js:193:15)
at next (/home/rw/html/llamamall2/node_modules/express/node_modules/connect/lib/proto.js:195:9)
at next (/home/rw/html/llamamall2/node_modules/express/node_modules/connect/lib/proto.js:168:78)
at resume (/home/rw/html/llamamall2/node_modules/express/node_modules/connect/lib/middleware/static.js:65:7)
at SendStream.error (/home/rw/html/llamamall2/node_modules/express/node_modules/connect/lib/middleware/static.js:80:37)
at SendStream.emit (events.js:95:17)
at SendStream.error (/home/rw/html/llamamall2/node_modules/express/node_modules/send/lib/send.js:147:51)
at SendStream.onStatError (/home/rw/html/llamamall2/node_modules/express/node_modules/send/lib/send.js:248:48)
at /home/rw/html/llamamall2/node_modules/express/node_modules/send/lib/send.js:320:26
at Object.oncomplete (fs.js:107:15)

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Oct 9, 2014

Well it's supposed to be include, not includes :) I was referring to your original error, which you in fact, did not fix.

@rahilwazir

This comment has been minimized.

rahilwazir commented Oct 9, 2014

😞 Man I thought I've done some tremendous effort by solving this error but nah! 👊

Have you got any idea why this issue is occurring? Why it was working with previous minor version?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Oct 9, 2014

I'm not sure, can you try posting the err.stack for the original error?
Even though you are within the same major 2.0 there has been a lot of BC breaks working towards the release.

@rahilwazir

This comment has been minimized.

rahilwazir commented Oct 9, 2014

IT IS the ORIGINAL err.stack caught from .catch method

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Oct 9, 2014

@rahilwazir you wrote that it was the stack trace from when you used includes

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Oct 9, 2014

In any case @rahilwazir it would be helpfull if you could provide a full self-contained piece of code including model definitions, relations and find call (so we can easily copy paste and attempt to possibly reprodruce)

@rahilwazir

This comment has been minimized.

rahilwazir commented Oct 10, 2014

@mickhansen No it is stack trace for include (sorry for shaking your mind)

Here's the code (Its coffeescript)

Schema:

Show = sequelize.define 'Show',
  title               : Sequelize.STRING
  date                : Sequelize.DATE
  display_date        : Sequelize.STRING
  year                : Sequelize.INTEGER
  source              : 'MEDIUMTEXT' # Sequelize.TEXT
  lineage             : Sequelize.TEXT
  taper               : Sequelize.TEXT
  description         : Sequelize.TEXT
  archive_identifier  : type: Sequelize.STRING, unique: true, allowNull: true
  bandcamp_id         : type: Sequelize.STRING, unique: true, allowNull: true
  bandcamp_url        : Sequelize.STRING
  duration            : Sequelize.INTEGER
  track_count         : Sequelize.INTEGER
  is_soundboard       : Sequelize.BOOLEAN
  album_cover         : type: Sequelize.STRING, allowNull: true
  featured            : type: Sequelize.BOOLEAN, defaultValue: false

Venue = sequelize.define 'Venue',
    name                : Sequelize.STRING
    city                : Sequelize.STRING
    setlistfm_venue_id  : type: Sequelize.STRING, unique: true
    city_state          : Sequelize.STRING
    city_state_code     : Sequelize.STRING
    city_lat            : Sequelize.FLOAT(10, 6)
    city_long           : Sequelize.FLOAT(10, 6)
    country_code        : Sequelize.STRING
    country_name        : Sequelize.STRING
    slug                : Sequelize.STRING

Track = sequelize.define 'Track',
    title   : Sequelize.STRING
    md5     : 
        type: Sequelize.STRING
        allowNull: true
    track   : Sequelize.INTEGER
    bitrate : 
        type: Sequelize.INTEGER
        allowNull: true
    size    : 
        type: Sequelize.INTEGER
        allowNull: true
    length  : 
        type: Sequelize.INTEGER
        allowNull: true
    file    : Sequelize.TEXT
    slug    : Sequelize.STRING

Show.belongsTo Venue

Show.belongsTo Artist
Venue.hasMany Show
Artist.hasMany Show

Track.belongsTo Show
Show.hasMany Track

And using the same above code for querying..

Show.find({
   where: whereObject,
   include: [Venue, Track]
})
.....
@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Oct 10, 2014

And what error do you get with that? And what is the stack of that error.

@rahilwazir

This comment has been minimized.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Oct 10, 2014

Have you verifeid that both Venue and Track are defined at the time you call find?

@rahilwazir

This comment has been minimized.

rahilwazir commented Oct 10, 2014

NO! (JK)

Of course yes It is working perfectly fine before nothing changed at all. Just upgraded sequelize thats it 📆

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Oct 10, 2014

https://github.com/sequelize/sequelize/blob/v2.0.0-rc1/lib/dialects/abstract/query-generator.js#L892

It's hitting the N:M code path, apparently it thinks one of your associations is N:M although it really shouldn't be.

@rahilwazir

This comment has been minimized.

rahilwazir commented Oct 10, 2014

Solution? 😊

Did you reproduce the error?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Oct 10, 2014

Not sure, it's not a known bug and we have a ton of tests for relations like yours.
You'll need to see if you can reproduce this error in an isolated environment and then perhaps provide a pull request with a failing unit test - Or provide us with a piece of copy-pastable code that fails (not coffeescript).

Can you try posting all your relations? Or was that all of them?

@rahilwazir

This comment has been minimized.

rahilwazir commented Oct 10, 2014

Do you want to see other table schema? Yeah thats all of these three tables, I've many others but they don't have any relation with any of these three tables.

Unit test? I'm not good in unit test lets see on the weekend superman 💪 would come and help me 😎

🚶

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Oct 10, 2014

Well if they don't have any relations with the other tables it likely doesn't matter.
Try and see if you can't reproduce the error in a isolated environment, the error really shouldn't be happening.

And you are absolutely sure that the error is from that find call?

@rahilwazir

This comment has been minimized.

rahilwazir commented Oct 10, 2014

Ok I'll try in a isolated environment and let you know :bowtie:

And you are absolutely sure that the error is from that find call?

😆 (LOL) yes I'm damn sure

haches added a commit to haches/sequelize that referenced this issue Oct 10, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Oct 20, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Oct 20, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Oct 21, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Oct 21, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Oct 27, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Oct 27, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Oct 29, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Oct 29, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Oct 29, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Oct 29, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Oct 29, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Oct 29, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Nov 3, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Nov 3, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Nov 3, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Nov 3, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Nov 3, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Nov 3, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Nov 3, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Nov 3, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Nov 3, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Nov 3, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Nov 3, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Nov 3, 2014

thecaddy pushed a commit to zillow-oc/sequelize that referenced this issue Nov 4, 2014

rpaterson added a commit to signpost/sequelize that referenced this issue Nov 5, 2015

fix(associations): regression in hasMany pairing introduced with thro…
…ugh scopes, fixes sequelize#2316

(cherry picked from commit 314d21e)

Conflicts:
	lib/associations/has-many.js
	test/associations/has-many.test.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment