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

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sdebionne
Copy link
Contributor

No description provided.

@mbroadst
Copy link
Collaborator

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

@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
Copy link
Contributor Author

@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...

Delete options.include for the primary resource only. For the
subresource, delete the properties of the associated model in then
`list.send.before` milestone.
@sdebionne
Copy link
Contributor Author

@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
Copy link
Collaborator

@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
Copy link
Contributor Author

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

@cam861
Copy link

cam861 commented Jan 8, 2017

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
Copy link
Collaborator

mbroadst commented Jan 8, 2017

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants