Open sdebionne opened 8 years ago
@sdebionne that sounds reasonable.
Wouldn't this do it? https://github.com/dchester/epilogue/pull/122
@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.
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.
@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 }
}
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)?
@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.
@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
?
@sdebionne no autoPopulate and autoInclude are separate concepts here. Here's a breakdown of the current state:
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 inassociations: true
. That just sort of magically hooks it all up for you, but its default behavior is to always populate relations in every query.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.)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...
@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 include
d 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.
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?
autoPopulate: true
would be the default to keep backward compatibility.