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

many to many through association on through table model return array instead of single object #5288

Closed
atiertant opened this issue Sep 2, 2015 · 11 comments

Comments

@atiertant
Copy link

many to many through association on through table model return array instead of single object :

  collections.user = Waterline.Collection.extend({
    identity: 'user',
    connection: 'foo',
    attributes: {
      id: {
        type: 'integer',
        primaryKey: true
      },
      cars: {
        collection: 'car',
        through: 'drive',
        via: 'car'
      }
    }
  });

  collections.drive = Waterline.Collection.extend({
    identity: 'drive',
    connection: 'foo',
    attributes: {
      id: {
        type: 'integer',
        primaryKey: true
      },
      car: {
        model: 'car'
      },
      user: {
        model: 'user'
      }
    }
  });

  collections.car = Waterline.Collection.extend({
    identity: 'car',
    connection: 'foo',
    attributes: {
      id: {
        type: 'integer',
        primaryKey: true
      },
      drivers: {
        collection: 'user',
        via: 'cars',
        dominant: true
      }
    }
  });

  Drive.findOne(1)
  .populate('car')
  .populate('user')

return

{ 
  id: 1, 
  car:[ { id: 1,createdAt: '2015-09-02T08:51:37.393Z', updatedAt: '2015-09-02T08:51:37.393Z' } ],
  user:[ { id: 1,createdAt: '2015-09-02T08:51:37.393Z', updatedAt: '2015-09-02T08:51:37.393Z' } ]
}

istead of

{ 
  id: 1, 
  car: { id: 1,createdAt: '2015-09-02T08:51:37.393Z', updatedAt: '2015-09-02T08:51:37.393Z' } ,
  user: { id: 1,createdAt: '2015-09-02T08:51:37.393Z', updatedAt: '2015-09-02T08:51:37.393Z' }
}
@devinivy
Copy link

Anyone have time to look into this? Failing tests are here #1134. This will be required for proper through association support.

@particlebanana
Copy link
Contributor

@devinivy that's strange, if I had to guess I would say it must have something to do with Waterline Schema flagging the Drive collection as a join table.

@atiertant
Copy link
Author

@particlebanana one more time you were right ! i corrected if in two PR. could you have a look?

@weyert
Copy link

weyert commented Oct 31, 2015

Safe to use in production?

@atiertant
Copy link
Author

@weyert in fact i think that waterline' actual manytomany through association is really buggy and should not be used in production...my PR correct some bugs...

@weyert
Copy link

weyert commented Nov 2, 2015

Thanks for the head ups. You are managing it manually now then?
I will probably just go for the simple case of one user can have many categories.

@atiertant
Copy link
Author

@weyert for simple case use manytomany association

@particlebanana
Copy link
Contributor

@atiertant I think I tracked this down even further. So first some background, when you have two collections pointing at each other (many-to-many) waterline-sequel generates a join table for you and flags it as a junctionTable. The current through associations implementation allows you to map a collection to an existing join table in your database. It works the same as the generated join table and expects there to be two foreign keys. Everything else works the same, the limitation being if you have additional attributes on the join table they will not be returned when populating either of the sides.

So what was happening was basically because it's tagged as a junctionTable Waterline's loader thinks it doesn't have it as an actual user defined collection - because it assumes waterline-sequel created it - and so the definition gets processed twice. This causes the definition to be corrupted, because it's a schema and not an attribute definition, which causes the populates to get mapped incorrectly.

What the proposed patch here does is attempt to fight the incorrect mapping by adding some additional metadata to the table. Unfortunately from my tests, this seems to fix the populates on the join table (Drive) and break the populates on either side (User, Car).

I'm working on a patch to Waterline-schema that adds a throughTable flag on join tables that are defined as actual collections. This will allow us to patch the collection loader and make it so that these don't get processed twice, fixing the population mapping. It should be done momentarily.

I did have a question about your models above and want to make sure I am on the same page as far as the issue is concerned.

In your User collection you define an attribute named cars that is pointing to the join table using the through mapping. However it seems your via key is incorrect. The drivers attribute on the Car collection also seems to be missing the through value and the via seems to be pointing to the User collection.

So just to clear up how that should work you can think of it as: "This population will return the collection User by looking through the Drive collection via the user key".

This would make the collections look like so:

collections.user = Waterline.Collection.extend({
    identity: 'user',
    connection: 'foo',
    attributes: {
      id: {
        type: 'integer',
        primaryKey: true
      },
      cars: {
        collection: 'car',
        through: 'drive',
        via: 'user'
      }
    }
  });

  collections.drive = Waterline.Collection.extend({
    identity: 'drive',
    connection: 'foo',
    attributes: {
      id: {
        type: 'integer',
        primaryKey: true
      },
      car: {
        model: 'car'
      },
      user: {
        model: 'user'
      }
    }
  });

  collections.car = Waterline.Collection.extend({
    identity: 'car',
    connection: 'foo',
    attributes: {
      id: {
        type: 'integer',
        primaryKey: true
      },
      drivers: {
        collection: 'user',
        through: 'drive',
        via: 'car',
        dominant: true
      }
    }
  });

I'll tag you in the patches I am submitting so you can make sure they pass. I'll cherry pick in fc0c671a98acfa997461533f9e34a812ddcd65dc so we have it tested and fix up the via's as well.

@atiertant
Copy link
Author

@particlebanana you are right my via are wrong ...
i think first thing to do is to define spec because i never know if it should contain a side or other ... https://github.com/balderdashy/waterline-docs/blob/master/models/associations/many-to-many-through.md

particlebanana referenced this issue in balderdashy/waterline Nov 23, 2015
Manytomanythrough fix and failling test for #1133
@sailsbot
Copy link

Thanks for posting, @atiertant. I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message. On the other hand, if you are still waiting on a patch, please:

  • review our contribution guide to make sure this submission meets our criteria (only verified bugs with documented features, please; no questions, commentary, or bug reports about undocumented features or unofficial plugins)
  • create a new issue with the latest information, including updated version details with error messages, failing tests, and a link back to the original issue. This allows GitHub to automatically create a back-reference for future visitors arriving from search engines.

Thanks so much for your help!

@devinivy
Copy link

@atiertant @particlebanana should this be reopened?

@raqem raqem transferred this issue from balderdashy/waterline May 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants