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

Association Routes? #34

Open
sharathm opened this issue Jan 19, 2015 · 60 comments
Open

Association Routes? #34

sharathm opened this issue Jan 19, 2015 · 60 comments
Assignees
Milestone

Comments

@sharathm
Copy link

Do you support association routes as well?
If I have a scenario for example..

Students have many courses
Each course has many semesters
Each semester has many subjects

and assuming the associations are defined correctly in Sequelize, can we auto generate routes like
POST /student/:studentId/course
GET /student/:studentId/course
PUT /student/:studentId/course/:courseId

POST /student/:studentId/course/:courseId/semester

POST /student/:studentId/course/:courseId/semester/:semesterId/subject

@mbroadst
Copy link
Collaborator

@sharathm this will be in shortly, I have the feature about half done

@brettstack
Copy link

Keenly anticipating the release :)

@mbroadst mbroadst self-assigned this Jan 19, 2015
@sharathm
Copy link
Author

This is the stack overflow thread that led me here.. http://stackoverflow.com/questions/25801426/sequelize-rest-api-generator
I have been looking for a solution for a while now..and there is a fair bit of interest in this feature..so i am confident it will be well received ..

@mbroadst
Copy link
Collaborator

Yeah I'm working on it as much as time allows, unfortunately it's not my full time job 😄.

I currently have auto-association working for simple cases (BelongsTo, HasOne), but need a further discussion on how to handle the many to many cases and deeper levels that your example requires (/student/:studentId/course/:courseId/semester/:semesterId/subject).

Specifically, those routes could be rather easily generated, but extending our idea of hooks to them is going to be more difficult. How would one express that exactly:

userResource.read.when.it.is.many.to.many.before(function(req, res, context) { // do something } );

(^-- this is a joke)

@sharathm
Copy link
Author

Does sequelize give a DB level representation of all associations between tables? I am thinking if its possible to generate a sort of in-memory database diagram and then map that to function calls to execute the required calls against the tables.
Just a thought.. maybe am crazy here.. 😄

@mbroadst
Copy link
Collaborator

@sharathm yeah sequelize does give us this information, it's more of a matter of massaging the epilogue design to accommodate deep levels of association. I have code that generates all the endpoints, a few tests put together and am now in the process of trying to tie it all together. I have a few questions.

In your most complex example above:
POST /student/:studentId/course/:courseId/semester/:semesterId/subject

studentId is obviously the id of the student entry, but are subsequent id's meant to be the actual in-database id of the entry or an zero-indexed value based on the number of e.g. courses associated with that particular student.

The simple cases aren't that difficult, it's once I start recursing into these deep levels of association that we really start stressing epilogue's original design.

@mbroadst
Copy link
Collaborator

@breandr it would be useful if you could add your test/use cases here as well!

@sharathm
Copy link
Author

@mbroadst - All are in-database Ids..

@mbroadst
Copy link
Collaborator

@sharathm oh I see, so that pairs down the number of things that need to be done. For instance in your example:
PUT /student/:studentId/course/:courseId
is just an alias for:
PUT /course/:courseId

@sharathm
Copy link
Author

Yes.. that is correct..

@brettstack
Copy link

I'd advise against using aliases. This is how I see those two routes:

/student/:studentId/course/:courseIds
(note that I would allow :courseIds to be a comma delimited list of ids)
GET: Get course information
POST: Add courses to a student
DELETE: Remove courses from a student
PUT: Replace current courses with supplied courses

/course/:courseId
GET: Get course information
POST: Create a new course
DELETE: Delete a course
PUT: Update course information
PATCH: Update partial course information

As you can see, the only ones that look the same on the face of it is GET, however, more than likely you will want to return different data for each of these routes. For example, GET /student/:studentId/course/:courseId could return information about the student as well as the course. Or perhaps GET /course/:courseId could return a lot of data about a course, whereas GET /student/:studentId/course/:courseId would return a subset.

@sharathm
Copy link
Author

I agree.. now that I think about it.. the whole idea of this is to resolve the objects in the route.. so that in the end, if i create a after hook, I should be able to access the objects such as req.body.record.student.. so that appropriate business logic can be applied..

@mbroadst
Copy link
Collaborator

@breandr, @sharathm

Hmm, I'm not sure I completely agree here but I'm trying to get a better handle on what the requirements are here since I don't actually require this functionality in anything I'm using epilogue for. If you don't mind I'd like to nix the "multiple course ids" bit for the sake of simplifying an already pretty big feature (a next step indeed).

It seems to me that if you have the simple association case: Student.hasMany(Course);, then you'd end up with the following routes:

/students

  • GET return a list of all students as json records
  • POST adds a new student to the list of all students
  • PUT (not applicable)
  • DELETE (not applicable)

/students/:studentId

  • GET return json record of student database item with id === :studentId
  • POST (not applicable)
  • PUT update student id record with posted json where id === :studentId
  • DELETE delete record from database where id === :studentId

/students/:studentId/courses

  • GET return all courses associated with a given student
  • POST add a course to courses, and associate it with a given student
  • PUT (not applicable)
  • DELETE (not applicable)

/students/:studentId/courses/:courseId

  • GET return json record of a given courseId (<-- this is where seem to have some disconnect see below)
  • POST (not applicable)
  • PUT update course with courseId
  • DELETE delete the association between this course id and this student (but not the course itself)

So as you can see I think we agree with everything up until the last example. You said you think it should return some relevant student data as well? I haven't seen any other solutions that offer this, and I'm not even sure what student data you would be (the studentId or something?). As far as I can reason it, the last case there is essentially a full alias to /course/:courseId (except for the delete case I think), it has very little to do with the student, and would be provided almost for convenience.

Am I understanding you guys correctly? I'm just trying to build up the test suite here to make sure when it actually is implemented we've covered all your use cases.

NOTE: there is one more case I'm not mentioning but have added to my autogeneration code which is for hasOne/belongsTo. In this case if model User.hasOne(Role), and extra route would be provided such that /users/:userId/role would return the role associated with a given userId.

I believe I've covered all the basic cases here (at least for one level deep), so let's see what holes you guys can punch in that before moving on to the next levels (Subjects, Semesters, etc).

@brettstack
Copy link

I totally understand putting the multiple id approach on hold, so we will move forward assuming :courseId is a single numeric value.

I have a couple of issues.

| POST /students/:studentId/courses add a course to courses, and associate it with a given student

Are you saying this should create a new record in the course table and associate that course to the userCourse table? I'm not sure if this is a common enough use-case. If we are to stick to our example of courses and students, the interface would probably have you create the course first, and then associate it to many students. If this is your intention, it raises the question: how do I associate an existing course to a user (which, IMO, is the more common use case). I would suggest the answer would be POST /students/:studentId/courses/:courseId which is what I suggest in my example, and is marked "not applicable" in yours.

| PUT /students/:studentId/courses/:courseId update course with courseId
I really don't like the idea of this updating the course when we already have PUT /courses/:courseId to do that. Having multiple routes do the same thing can be confusing. I would use this route when my many-to-many table has more in it than just id, courseId, and studentId (is it still called a many-to-many joining table at this point? if not, what is the proper term here?) Say for example we added a finalGrade column. I would then PUT /students/:studentId/courses/:courseId to update that finalGrade.

| GET /students/:studentId/courses/:courseId return json record of a given courseId (<-- this is where seem to have some disconnect see below)
As before, I really think you should just GET /courses/:courseId to get info on a course and not add complexity by having multiple routes do the same thing. I am yet to even use epilogue so I'm not entirely sure how this all works, but I'm assuming it's possible to just have an empty route for this, that the user would be able to add logic to using only the hooks provided by epilogue? If not an empty route, it might even be better to return a union of student and course data.

On the User.hasOne(Role), I was actually thinking of this scenario earlier. Sounds perfect.

@mbroadst
Copy link
Collaborator

@breandr okay cool, thanks for the very helpful insight here (sorry everyone this seems like a pain but I'd rather get the feature done right the first try 😄). So it looks like I need to modify my last two presented cases accordingly:

/students/:studentId/courses

  • GET return all courses associated with a given student
  • POST (not applicable)
  • PUT/PATCH (not applicable)
  • DELETE (not applicable)

/students/:studentId/courses/:courseId

  • GET (not applicable) or can be an alias to /course/courseId
  • POST associate course courseId with student of studentId
  • PUT/PATCH (not applicable)
  • DELETE delete the association between this course id and this student (but not the course itself)

Does that line up more with what you guys were thinking? I know you were saying you wanted to be able to update a specific column with PUT/PATCH on the second case here - can you be more specific about that?

Also, it doesn't seem to make sense to me that the GET for the association would return a union of the student and course data, since by default if you use includes (auto association it will be termed), prefetch will be true so whatever associated data you have will be filled in in e.g. /student/:studentId

@mbroadst
Copy link
Collaborator

@johngiudici @dchester do you guys have any insight you could lend to this?

@ackerdev
Copy link
Contributor

Nested resources more than one level deep are an anti-pattern, and should be avoided. A route like POST /student/:studentId/course/:courseId/semester/:semesterId/subject is a superfluous mess that is functionally identical to POST /semester/:semesterId/subject.

Furthermore, I believe it is nonstandard for nested resource routes to return data regarding the parent. GET /students/:studentId/course should not return information about the student; it should return a list of courses associated with that student. Same goes for GET /students/:studentId/course/:id. I should only be getting the data for the specific resource I am targeting, not it's parent.

The functionality described for associated routes also doesn't make much sense. Especially when regarding the DELETE route, if the association being targeted is a belongs-to with a required association, the route will fizzle or error. This seems to be fairly uncommon functionality, where other API frameworks tend to treat that DELETE as deleting the targeted resource, not the association of that resource.

This is how Rails, as one example, defines their nested resource routes functionality:

Verb Path Used For
GET /magazines/:magazine_id/ads List ads associated with the given magazine
GET /magazines/:magazine_id/ads/:id Get a specific ad belonging to a specific magazine
POST /magazines/:magazine_id/ads Create a new ad belonging to a specific magazine
PUT /magazines/:magazine_id/ads/:id Update a specific ad belonging to a specific magazine
DELETE /magazines/:magazine_id/ads/:id Delete a specific ad belonging to a specific magazine

You should notice that the POST and DELETE methods affect the actual resource, they do not merely dis-associate the resource from it's parent (Though I believe it will update the associations in doing so).

The Loopback framework also follows this convention, but also offers the functionality being described in this issue for many-to-many relations under a different route: /magazines/:magazine_id/ads/rel/:id. I believe this is fairly nonstandard, as one would not likely consider the association in itself as truly being a resource in the system.

@sharathm
Copy link
Author

@ackerdev - Thank you for the detailed explanation.. I now understand.. a RESTful API is suppose to return the state of the data as its stored.. its not intended to be a way to identify a data access pattern ..correct?

@brettstack
Copy link

@ackerdev those mostly look fine for one-to-many relationships (one magazine has many ads), but certainly don't work for many-to-many (many students have many courses example we were using previously). Furthermore, I would assume epilogue would generate /ads routes as well, in which case some of these nested routes would become an alias, which I think is an anti-pattern (more than one way to do the same thing).

@ackerdev
Copy link
Contributor

@sharathm Eh... I think that's a bit too simplistic a view of what a RESTful API is for me to say that that is correct.

@breandr

those mostly look fine for one-to-many relationships, but certainly don't work for many-to-many.

Won't work in what way? I assume you mean that you can't associate or dis-associate many-to-many relations, in which case, yeah. I'm not personally sure what makes for the most appropriate solution here for that. If you have a through model it's easy, but without a through model I'd have to give it some thought.

I don't necessarily disagree with removing the superfluous routes. I believe only the GET for the collection and POST in the table above are unique in functionality.

@brettstack
Copy link

I assume you mean that you can't associate or dis-associate many-to-many relations

Correct. My approach would be to use the same routes and methods for both one-to-many and many-to-many, but based on what it is, it will be handled differently. One-to-many will do as you suggest (create/delete), and many-to-many will instead associate/disassociate. The only negative I can see with this approach is that you would need to have an understanding of the relationships between resources (i.e. know if they are one-to-many or many-to-many). I don't think this is an unreasonable requirement when creating a private API of which you should have an intimate knowledge of the relationships anyway. The Loopback approach of adding "rel" to the route makes it more obvious as to what the route will do, but I have the same issue with it that you do: "rel" isn't a resource.

I don't necessarily disagree with removing the superfluous routes

I guess it comes down to taste. I'm sure there are a large number of people who would like to access it using either route. @mbroadst is it possible to have a config that will include or not include these routes? Or would you prefer just going with one way (for now at least)?

@brettstack
Copy link

Nested resources more than one level deep are an anti-pattern, and should be avoided. A route like POST /student/:studentId/course/:courseId/semester/:semesterId/subject is a superfluous mess that is functionally identical to POST /semester/:semesterId/subject.

I agree. And if this is the approach, I think it goes even more in favor of not including the other superfluous routes. 👍

@ackerdev
Copy link
Contributor

I think you'll run into a problem with assuming that you can expect dis/associating routes on every resource is that not every resource's association can be interacted with in that way.

A many-to-one model that depends on it's parent for existence (eg a course cannot exist without a school) cannot realistically respond to a disassociation request. It's also a bit unintuitive when looking at the route; DELETE /school/:schoolId/course/:id looks like a delete request targeting a course; but it's actually targeting the association.

Making the framework smartly choose whether to create/delete or dis/associate based on whether or not it is a 1:N or N:M could create a pretty significant gotcha that I think we would really be best off avoiding.

In my opinion, the ideal solution is to have a 'through' model. To the API user, it just looks like another resource, and they can manipulate it just the same as any other. POST /subscriptions and DELETE /subscriptions/1 have very obvious meanings and keeps REST principles in tact.

I don't know if it's realistic to tell anyone who wants association manipulation to use through models, but I think it creates the most appropriate API for manipulating them.

@brettstack
Copy link

not every resource's association can be interacted with in that way.

Right, but I would say that in most cases they can be, and epilogue should
be targeting the most popular use case and allow edge cases to be dealt
with accordingly. You could also add a property to the epilogue API which
will allow you to not generate these routes for specific
models/relationships.

A many-to-one model that depends on it's parent for existence (eg a course cannot exist without a school) cannot realistically respond to a disassociation request

If this is a one-to-many (a school has many courses) and you DELETE /school/:schoolId/course/:id, this would simply delete the course record
(although I'd prefer to just have DELETE /course/:id). Only in the case
that courses can exist in many schools (many-to-many) would this instead
disassociate.

DELETE /school/:schoolId/course/:id looks like a delete request targeting a course; but it's actually targeting the association.

As above, it would delete for one-to-many (if we have the superfluous
routes), or disassociate for many-to-many. I disagree that this route looks
like it is deleting the resource. To me it is plain that it disassociates,
and that is probably due to my position of not having those superfluous
routes.

Could you expand on "through model"? A quick google makes me think this is
terminology that exists only in django.
On 26 Jan 2015 11:41, "Nicholas Acker" notifications@github.com wrote:

I think you'll run into a problem with assuming that you can expect
dis/associating routes on every resource is that not every resource's
association can be interacted with in that way.

A many-to-one model that depends on it's parent for existence (eg a course
cannot exist without a school) cannot realistically respond to a
disassociation request. It's also a bit unintuitive when looking at the
route; DELETE /school/:schoolId/course/:id looks like a delete request
targeting a course; but it's actually targeting the association.

In my opinion, the ideal solution is to have a 'through' model. To the API
user, it just looks like another resource, and they can manipulate it just
the same as any other. POST /subscriptions and DELETE /subscriptions/1
have very obvious meanings and keeps REST principles in tact.


Reply to this email directly or view it on GitHub
#34 (comment).

@brettstack
Copy link

Can we decide on the removal of the superfluous routes for now? I think that will remove some confusion.

@ackerdev
Copy link
Contributor

If this is a one-to-many (a school has many courses) and you DELETE /school/:schoolId/course/:id, this would simply delete the course record
(although I'd prefer to just have DELETE /course/:id).

I don't see it making sense to generating that route at all given it'll just be an alias, then.

I disagree that this route looks like it is deleting the resource. To me it is plain that it disassociates, and that is probably due to my position of not having those superfluous routes.

I think this is because of you having a pre-determined idea of what functionality it should have, but I don't think it's actually intuitive. The verb DELETE indicates we will be destroying something, and the fact that school/:schoolId/course/:id ends with a resource course and an :id indicates we are targeting a course with a specific id (for some reason scoped to a school); it doesn't really make it obvious that we are targeting an association here.

Could you expand on "through model"?

It is a model for your pivot table. With Sequelize when defining a has-many association you can specify a through model in the options object. It lets you manipulate the middle table directly. In the case of epilogue, it allows you to define the association as a resource and then you can have routes that manipulate it directly, eg POST /subscription & DELETE /subscription/1 where subscription is a pivot model that contains a userId and mailingListId.

@mikemimik
Copy link

@mbroadst I most definitely will take a look and see if there is anything I can contribute at all. I'm currently working on a project that will depend strongly on epilogue and associations.

This might seem like a rookie question but I simply used npm to install the publish version of epilogue. Can I simply replace the folder in node_modules with the auto-association branch of this repo, and have everything work swimmingly? or is there some massaging that needs to be done?

@mbroadst
Copy link
Collaborator

mbroadst commented Apr 3, 2015

@mikemimik if you want to try it out in another project you'll want to replace your dependency with a git url as documented here. So you'll want something like:

   "epilogue": "dchester/epilogue#auto-associations"

@mikemimik
Copy link

@mbroadst oh that's great, I didn't realize you could do that in the package.json file. Is there any broken functionality from the master branch to the auto-association branch?

@mbroadst
Copy link
Collaborator

mbroadst commented Apr 3, 2015

@mikemimik shouldn't be, it was all being hidden behind a top level "associations" flag. Look at the added test cases for help, I also think the commits should be incremental enough to follow

@mbroadst mbroadst added this to the 0.6.0 milestone Apr 10, 2015
@schumacher-m
Copy link

Hey. I'm using epilogue to refactor a rest interface and I'm glad I don't have to write all this CRUD crap anymore. What's missing to get this to master and how can I help?

@Fridus
Copy link
Contributor

Fridus commented May 27, 2015

In branch auto-associations

@mrotaru
Copy link

mrotaru commented Aug 1, 2015

auto-associations branch works pretty nicely, but it looks like there's a problem with self-referencing belongsToMany; if you add one to the test, like:

test.models.Person.belongsToMany(test.models.Person, {
  as: 'related',
  through: 'related_people'
});

No route will be created for this association; GET /api/person/1/related will result in 404. I did a little bit of debuggig and found that it fails here: https://github.com/dchester/epilogue/blob/auto-associations/lib/associations/belongs-to-many.js#L20:

if (!associationPaired) {
  // Do not create the resource
  return;
}

Not sure where to from here though; I don't know much about sequelize internals.

@mbroadst
Copy link
Collaborator

mbroadst commented Aug 1, 2015

@Fridus any interest in taking a stab at this? looks like the broken test is already provided

@Fridus
Copy link
Contributor

Fridus commented Aug 3, 2015

@mbroadst I'll look at that. It is when the associated object is the same model

@Fridus
Copy link
Contributor

Fridus commented Aug 4, 2015

@mbroadst Sorry but I don't find how to do that.

if (!associationPaired) {
// Do not create the resource
return;
}

If association.isSelfAssociation then associationPaired must to be the inverse relation (PK and FK).
I haven't managed to do that and I haven't more time at this moment.
Try to fix this or wait until I have more time :)

@mbroadst
Copy link
Collaborator

mbroadst commented Aug 5, 2015

@Fridus hm, okay looks like we're both pretty slammed 😄 FYI, I rebased the auto-associations branch and there are a number of failing tests

Looks like this was due to a change in Sequelize, I posted a fix. @Fridus do you feel confident that the auto-association is ready for an initial release? I'm releasing a 0.5.2 today, but I'd theoretically like to get the auto-associations into a 1.0 release relatively soon

@xizhao
Copy link

xizhao commented Nov 12, 2015

Is this done yet? if so can we document?

@mbroadst
Copy link
Collaborator

@xizhao as you can see here it's not complete, but its as complete as it's going to be until someone is inspired to implement the "write" methods for all the association routes. Documentation also very much welcomed, if you're inclined to write it!

@AddoSolutions
Copy link

I am pretty anxious to get this working as well.

Are there any workarounds for this yet? It would appear from here that something does work?

@mbroadst
Copy link
Collaborator

mbroadst commented Dec 2, 2015

@AddoSolutions this is partially implemented (all read-related operations) as you can see from the above comment. If you need support for further operations we can discuss it here, and I can provide assistance as needed.

@AddoSolutions
Copy link

Ok, that's cool. How did we want to implement this?

My thought is that we will can loop through the relational attributes. Example:

Person:

  • belongsTo: Group
  • hasMany: Posts
  • belongsToMany: Organizations

Then to post to it:

{
  "name": "John Doe",
  "Group": 123,
  "Posts": [1,2,3],
  "Organizations": [3,2,1]
}

From there we can map the relations for the given object.

I have tried this a bit today, and to do the actual mapping, this is how I did it:

for(var type in relations){
  var model = relations[type].target.build({"id": request[type]});
  model.isNew = false;

  targetModel['set' + type](model);
}

Of course accounting for multiple relations, that is a simple example. Is this the appropriate way to do this, or is there a better way?

Thoughts?

@AddoSolutions
Copy link

Not to bump this, but just wanted to get thoughts before I go about this. I am not totally sure on the best way to utilize the sequalize API here.

@atulhm
Copy link

atulhm commented Apr 26, 2016

hell of a long thread. Great to see all the interest. I ran into an issue with dealing with composite keys. Thanks to the hook/controller structure it was trivial for me to address the issue:
basically most of the legacy tables I'm working with have 2-3 foreign keys that make up the composite key with the last key being a string that is not unique.

Using the school example...
/schools//buidlings//classrooms//seats/
/schools//buidlings//classrooms//students/

Ran into issues with updates and accidental changes because the last key is not unique. It would probably good to have a warning/check either here or in sequelize that if the id is one of a composite key, then to require all keys for an update.

My fix was that in the before of all requests, I parse the route and inject the additional criteria. Just wanted to document this here in case someone runs into the same issues. Thanks for making this.

@danielo515
Copy link

Can I try this out on the main repo? I'm just interested on the list functionality in single routes ( devices/:id/changes )
Is this activated automatically based on relations? Does it work in relations defined on the table definition instead of belongsTo or hasMany?

@mbroadst
Copy link
Collaborator

@danielo515 yes what implementation exists is indeed a part of the main repo and has been released. It's unfortunately undocumented (please let me know if you're interested in doing some of this 😄), but you can enable automatic associations by specifying associations: true in your resource definition. There are a number of examples of this in the associations tests: https://github.com/dchester/epilogue/tree/master/tests/associations

@asmodehn
Copy link
Contributor

asmodehn commented Oct 5, 2016

I recently came to know epilogue, and I am just starting to play around with associations, refactoring all my CRUD code to use the cleaner Resource concept...

One thing I find missing from the current implementation (and that wasn't really discussed here it seems) is the creation of a resource with associations in one request.
ie : #168

Implementing this would follow the sequelize behavior when doing :

ModelWithAssociation.create({
  data: 'mydata',
  associated: {
    assoc_data: 'myassoc_data'
  }
}, {
  include: [{
    model: AssociatedModel,
    as: 'associated',
  }]
})

It would also help to be able to configure, for a resource:

  • which association is exposed as subresource
  • which association is supposed to be "embedded" (not another REST resource, but another sequelize model)

This would allow decoupling the Data Models from the REST Resources.
It seems to me that the REST Resource concept is usually broader and more abstract and complex than the "ORM Model" concept, and therefore we usually have N Resources effectively implemented with M models, with M <= N.
Enabling a Resource to be implemented with a set of associated Models, optionally embedding/including or just "linking", could be very useful...

I'll try to implement this and I'll post a PR if I can come up with something usable...

@asmodehn
Copy link
Contributor

#193

@tommybananas
Copy link

Is this broken on sequelize 4?

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