Skip to content
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

Wrong createInstance in hasMany associations. MongoDB. #479

Closed
wants to merge 2 commits into from

Conversation

Kreees
Copy link
Contributor

@Kreees Kreees commented Mar 25, 2014

Hello.

In case of hasMany association in return of instance getAccessor (ex. getPets) you get instances of wrong models with MongoDB driver. In Many.js module extendInstance function (within createInstance call) utilizes Driver.hasMany on mongodb driver.

/lib/Association/Many.js
206: if (Driver.hasMany) {
207:    return Driver.hasMany(Model, association).get(Instance, conditions, options, createInstance, cb);
208: }

But passed createInstance argument is supposed for current model instance creation, not for associated model. Hence all associated objects become wrapped in wrong model class.

That case is swallowed by test due to checks are accomplished by presence and name property presented in both models (Person and Pet)

/test/integration/association-hasmany.js
18: Person = db.define('person', {
19:     name    : String,
20:     surname : String,
21:     age     : Number
22: });
23: Pet = db.define('pet', {
24:     name    : String
25: });
...
77: people[0].getPets("-name", function (err, pets) {
...
82:     pets[0].name.should.equal("Mutt");

At the same time everything works fine with SQL drivers (checked with sqlite).

I would have made a pool request, but in that case createInstance definition in /ib/Model.js is hidden by closure and not accessible directly from model.

@dxg dxg added the bug label Mar 24, 2014
@dxg
Copy link
Collaborator

dxg commented Mar 24, 2014

I recently hit some weird hasmany mongo test failures, might have been caused by the same issue..

@Kreees
Copy link
Contributor Author

Kreees commented Mar 25, 2014

Actually proper test should do some kind of instanceOf check. Ex. pets[0].model().should.be.equal(Pet). (Added in pull-request)

There are two options to fix this issue.

  1. Make createInstance public reference in Model class, ex. some method that allows to create instances of already persisted docs.
  2. Use association.model.find method inside of MongoDB driver getAccessor property (similarly to the case case of SQL bases).
    While the first one might be not so kosher as wanted (in overall ORM logic), the second one has problems with hasMany extra params.
    I've made simple workaround in mongodb driver. But i have no clue about how to make extras to be assigned to the instances (with setters/getters as it's made in Instance module) with proper management.

@YannickDa
Copy link
Contributor

Hello,

What about this issue ? I have the same...

I have just apply your correction in the last version of this module and it's seems ok.

Why this solution is not integrated in the current version ?

@dxg
Copy link
Collaborator

dxg commented Jul 24, 2014

Hi,

I've rebased this and merged to master.
Please test. It will be released along with the next version.

@dxg dxg closed this Jul 24, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants