search

dchester / epilogue

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

Association Routes? #34

Open sharathm opened 9 years ago

sharathm commented 9 years ago

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 commented 9 years ago

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

brettstack commented 9 years ago

Keenly anticipating the release :)

sharathm commented 9 years ago

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 commented 9 years ago

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

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 commented 9 years ago

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.. :smile:

mbroadst commented 9 years ago

@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 commented 9 years ago

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

sharathm commented 9 years ago

@mbroadst - All are in-database Ids..

mbroadst commented 9 years ago

@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 commented 9 years ago

Yes.. that is correct..

brettstack commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

@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

/students/:studentId

/students/:studentId/courses

/students/:studentId/courses/:courseId

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 commented 9 years ago

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 commented 9 years ago

@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 :smile:). So it looks like I need to modify my last two presented cases accordingly:

/students/:studentId/courses

/students/:studentId/courses/:courseId

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 commented 9 years ago

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

ackerdev commented 9 years ago

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 commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

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 commented 9 years ago

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. :+1:

ackerdev commented 9 years ago

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 commented 9 years ago

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 https://github.com/dchester/epilogue/issues/34#issuecomment-71523636.

brettstack commented 9 years ago

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

ackerdev commented 9 years ago

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.

brettstack commented 9 years ago

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

Right, which is why I would prefer to just have DELETE /course/:id.

I think this is because of you having a pre-determined idea of what they should have

You could put it that way, but I got to this idea due to my "no superfluous routes" approach. When you look at it through those eyes, it is very intuitive. Delete the associate between :schoolId and :courseId, and you are in fact doing a delete query (or setting the deletedAt column).

I was just about to post saying I had a very quick look and it seemed like it was a relationship/association model. So sticking with our example, you would have a POST /schoolCourse which has schoolId and courseId in the http body to create an association between the two, and DELETE /schoolCourse/1 where 1 is the id in the association/joining table? I don't like this approach at all and feels very "unRESTful".

ackerdev commented 9 years ago

I believe it's more RESTful as it's targeting an actual resource (at least in the view of the client). I think the example is a bit poor and that might be more unattractive than the solution itself, as it is unlikely you'd disassociate a school and it's courses and admittedly giving pivot models a good name can be tough sometimes. I think it's more obvious than DELETE /school/:schoolId/course/:id as it is obvious that we're not actually interacting with the school or course themselves.

But this could just be a difference of personal opinion. :smiley:

Might be worth taking an in-depth look at how other frameworks handle this functionality and see if there are some popular examples in the wild.

brettstack commented 9 years ago

So it looks like it's being done both ways. The top two answers to this question on StackOverflow http://stackoverflow.com/a/6333146/436540 recommend your approach since most of the time when you have a joining table you will want to capture additional data (or you might in the future), which is a good point. I'm happy to go with the "through model" approach. On 27 Jan 2015 07:08, "Nicholas Acker" notifications@github.com wrote:

I believe it's more RESTful as it's targeting an actual resource (at least in the view of the client). I think the example is a bit poor, as it is unlikely you'd disassociate a school and it's courses (and admittedly giving pivot models a good name can be tough sometimes). I think it's more obvious than DELETE /parent/:parentId/child/:id as it is obvious that we're not actually interacting with the school or course themselves.

But this could just be a difference of personal opinion. [image: :smiley:]

Might be worth taking an in-depth look at how other frameworks handle this functionality and see if there are some popular examples in the wild.

— Reply to this email directly or view it on GitHub https://github.com/dchester/epilogue/issues/34#issuecomment-71662799.

codebased commented 9 years ago

I don't think I will be keen on going too nested with the association. If epilogue can look after 2 level association then it is more than enough for most of the scenarios.

By the way, how can I achieve this with sequalizer by myself for now?

mbroadst commented 9 years ago

Hey guys, sorry I've been distracted with the job, but this feature in the works. Preliminary work will only support one level of association (this has been referred to as ditching the "superfluous routes" above), I'll have some time to get a POC in by next weekend hopefully.

@codebased you can get this behavior with sequelize's accessors, but I'd urge you to read up here for a deeper understanding.

tilomitra commented 9 years ago

Hey @mbroadst - is there any update? Really appreciate your work on this feature. Anything we can do to help?

mbroadst commented 9 years ago

@tilomitra I've been sucked into refactoring sequelize hooks, and adding multiple-database support (in sequelize, not here :smile:). Pull requests are certainly welcome here, but I should have time this week to get back into working on this

tilomitra commented 9 years ago

@mbroadst Sweet. Is the current work for this in any branch, or are you pretty much starting from scratch?

mbroadst commented 9 years ago

@tilomitra I've pushed my initial work here. It's very much a work in progress, and quite ugly at the moment, but it provides simple "read" and "list" support for associations.

mikemimik commented 9 years ago

@mbroadst has there been any more updates on this? The last commit was from a month ago. I'm just curious if you've made some adjustments since then. Cheers.

mbroadst commented 9 years ago

@mikemimik unfortunately I've been incredibly pressed for time and no further work has been put into the feature. The existing code works for a number of paths, and I would gladly accept any work towards the completion of the feature if you (or someone else) have time to work on it.

mikemimik commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

@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 commented 9 years ago

@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

schumacher-m commented 9 years ago

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 commented 9 years ago

In branch auto-associations

mrotaru commented 9 years ago

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 commented 9 years ago

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

Fridus commented 9 years ago

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

Fridus commented 9 years ago

@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 commented 9 years ago

@Fridus hm, okay looks like we're both pretty slammed :smile: 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 commented 8 years ago

Is this done yet? if so can we document?

mbroadst commented 8 years ago

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