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 an option to not auto-populate associations on source endpoints #124

Open
sdebionne opened this issue Dec 11, 2015 · 11 comments
Open

Add an option to not auto-populate associations on source endpoints #124

sdebionne opened this issue Dec 11, 2015 · 11 comments

Comments

@sdebionne
Copy link
Contributor

I'm considering a PR to add an option (or change the behavior) of resources with associations. I don't think it is correct to return a resource and its associations when the source resource endpoints are called without specifying an association. In a typical use case where we have users and projects, /users or /users/:id should not return associated projects.

This has already been discussed before in #34 and #85 but without a consensus on what the expect behavior should be. So what about an option?

resources.sites = epilogue.resource({
  model: models.User,
  endpoints: ['/users', '/users/:uid'],
  associations: {
    autoPopulate: false
  }
});

autoPopulate: true would be the default to keep backward compatibility.

@mbroadst
Copy link
Collaborator

@sdebionne that sounds reasonable.

@AddoSolutions
Copy link

Wouldn't this do it? #122

@mbroadst
Copy link
Collaborator

@AddoSolutions sorry I seem to have missed that PR, that does pretty much look like what he's asking for here. I prefer the term autoPopulate, as well as defaulting to true for backwards compatibility. I would also really appreciate tests to accompany these changes if possible.

@sdebionne
Copy link
Contributor Author

I have just tested #122 and that sounds like a good start, e.g same kind of modifications should be applied to list.js.

I wonder if we should have two distincts options for singular and plural endpoints:

resources.sites = epilogue.resource({
  model: models.User,
  endpoints: ['/users', '/users/:uid'],
  associations: {
    autoPopulate: [false, true]
  }
});

It may be handy to auto-populate the associations for singular endpoint while not fetching everything for the plural one.

@mbroadst
Copy link
Collaborator

@sdebionne sounds reasonable, although I'd prefer if we kept it semantically related to the controllers (rather than the endpoints which can be arbitrary):

associations: {
   autoPopulate: { read: true, list: false }
}

@sdebionne
Copy link
Contributor Author

OK. And how should this new option play with the include option? AFAIU include is used to do manual associations, right? Is this option still valid (as I cannot see it documented anywhere)?

@mbroadst
Copy link
Collaborator

@sdebionne include is used to include other Sequelize models in your search, see here for examples. In this case it seems to be enough that you do what @AddoSolutions did and simply clear out the local options.include for the milestone - however this might be an issue if a user has manually added something to the context.includes in a previous milestone.. I guess we'll have to see what breaks.

@sdebionne
Copy link
Contributor Author

@mbroadst Then autoPopulate could be autoInclude since the auto association code is actually populating the include[] option?

We could have this use case, say to include only project's tasks:

include: [{
    model: Task,
    where: { state: Sequelize.col('project.state') }
}],
associations: {
   autoInclude: { read: false, list: false }
}

Shall we trigger a warning if both include is specified and autoInclude = true?

@mbroadst
Copy link
Collaborator

@sdebionne no autoPopulate and autoInclude are separate concepts here. Here's a breakdown of the current state:

  • if you don't want to automatically include all related models, simple do not use the associations key at all: the default is false, it will do nothing. Instead you should use include explicitly and include whatever models you want, as whatever as key you want them included in
  • if you do want to automatically include associations based on the schema of your models, then you would do: associations: true. That just sort of magically hooks it all up for you, but its default behavior is to always populate relations in every query.
  • Which leads us to autoPopulate, which I thought was a proposal for conditionally disabling the default behavior of automatically populating associated resources, leaving that up to the user if they want to (through milestones e.g.)

@sdebionne
Copy link
Contributor Author

Thanks for the clarification.

That just sort of magically hooks it all up for you, but its default behavior is to always populate relations in every query.

@mbroadst Sorry but I don't get why the word populate is used instead of include in this sentence! How populate and include are different concepts? I used populate because it has been used in #85 and I though that would make the reference easier to follow...

@mbroadst
Copy link
Collaborator

@sdebionne include is a concept related to a Sequelize query: "This model will be included in the query". populate is an epilogue concept, such that epilogue will figure out which models you need included and then ensure the result is properly populated.

Most importantly, your desire to use include in the associations section conflicts with the fact that include is supported at the top level of a Resource definition. Very confusing.

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

No branches or pull requests

3 participants