search

dchester / epilogue

Create flexible REST endpoints and controllers from Sequelize models in your Express app
846 stars 116 forks source link

Add autoPopulate option and documentation for the associations options. #125

Open sdebionne opened 8 years ago

mbroadst commented 8 years ago

@sdebionne looks great, would you be willing to include a few tests? (this should mostly be a cut and paste job of existing tests just changing the autoPopulate options)

sdebionne commented 8 years ago

@mbroadst I have update the tests for belongs-to-many and... they do not pass. Let me try to explain the problem with the list controller: deleting the define[] array is incorrect because we lose where clauses that are necessary in the join.

For the first test case that fails: /people/2/hobbies should returns only the hobbies from people 2 but since the include[] is deleted we end up returning all hobbies.

Any idea on this?

mbroadst commented 8 years ago

@sdebionne when we auto associate, we create internal "sub-resources". I think what you really are trying to do is conditionally disable the include on the sub resources, not all resources. Does that make sense?

sdebionne commented 8 years ago

@mbroadst I'm trying to achieve this result: /people does not get populated with hobbies and /people/2/hobbies does not get populated with people. On the main resource, I want to disable eager loading. On the sub resource, I want to apply the association (do the join) but don't want to fetch both models, only the one associated with the sub resource. Hopefully that makes sense...

sdebionne commented 8 years ago

@mbroadst I am not fully happy with this solution as I would prefer that Sequelize generates the query I need rather than cleaning the results in post processing step (send.before milestone). At least that version works as documented.

mbroadst commented 8 years ago

@sdebionne sorry I've got limited time this week to work on this, I'm confident we can get something working but it will be a few days

sdebionne commented 8 years ago

@mbroadst No worry, I have a working version to play with, just not as optimized as I would like to...

cam861 commented 7 years ago

Hi @mbroadst and @sdebionne,

I'm just starting to use this repo on a couple of projects I'm working on and saw this PR sitting here with a feature set that I'm certainly keen to see implemented. Have there been other PRs that achieve a similar thing to this and so this one has been shelved? If it's just the conflicting files that need a revisit, I'm more than happy to have a look. I can't see anything in the docs that point to other PRs related to associations.

Also - on the docs front, there doesn't seem to be anything about associations (even the simple associations: true option to trigger autoAssociate()) in the current readme.md. I'm happy to write something up to explain what I've worked out from the source code.

Thanks for your help and kudos on the work you've done there.

Cheers

Cam

mbroadst commented 7 years ago

@cam861 I believe it was never merged because the tests never passed, and therefore the feature is incomplete. If you want to take a stab at rebasing the PR and getting everything passing, by all means go ahead - your contribution is very welcome.

Also, if you want to commit documentation about auto population these would be very welcomed changes as well!