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

New REST API URLs #336

Closed
shivamMg opened this Issue May 17, 2016 · 6 comments

Comments

@shivamMg
Copy link
Member

shivamMg commented May 17, 2016

Spec for the current API can be seen here (you can test it with your local installation). I wanted to propose some changes to the URLs. It would help us when defining endpoints for post, put and delete.

  1. Use the following endpoints:

    HTTP METHOD POST GET PUT DELETE
    CRUD OP CREATE READ UPDATE DELETE
    /events Create new events List events Bulk update Delete all events
    /events/12 Error Show event with id 12 If exists, update event 12; If not, error Delete event 12
    /events/2/sessions Create new sessions List sessions Bult update Delete all sessions
    /events/2/sessions/5 Error Show session with id 5 if exists, update session 5; If not, error Delete session 5

    Same goes for speakers, tracks, etc.

  2. Fix current GET APIs.

    • Fetch data for a single object. For example, /event/2/sessions fetches an array of all the sessions in event 2. But there is no endpoint that fetches a particular session. This should be at /events/2/sessions/<session_id>. Same goes for speakers, tracks, etc.

      You can get a single object, but at a URL like this: /event/sessions/<session_id>. This does not tell about the event that session belongs to.

      Keeping the session ids relative of the event instead of having them absolute (like they are now), is also debatable. See @aviaryan 's issue.

    • Fix GET api for events. Currently /event/2 fetches an array with a single element (event 2). Instead it should be fetching an event object.

    • Use of plural nouns. So /event/23 should be /events/23. Conform to best practices. Use of just plural will help keep urls simpler.

  3. Status codes. Currently it's either OK (200), not found (404) or internal server error (500). I think we should go with how flask-restplus does it, here's example from their docs: http://flask-restplus.readthedocs.io/en/stable/example.html

After the above changes we are going to have sessions (similarly for speakers, tracks, etc.) at /events/<event_id>/sessions/<session_id>. This will also simplify use of status codes. Like in this issue. If no sessions exist, /events/<event_id>/sessions should return an empty array (which is does right now). 404 should be returned when a particular session does not exist.

Relevant doc on api best practices: https://github.com/WhiteHouse/api-standards#http-verbs

Edit: One more thing, flask-restplus already provides auto-doc feature to document APIs, so we would be using that.

@aviaryan

This comment has been minimized.

Copy link
Member

aviaryan commented May 18, 2016

👍 I agree with the things mentioned. That WhiteHouse API docs are really good and we must follow the guidelines mentioned there.
BTW, one thing that bugs me a lot is that sessions, speakers and all other models (except event) have absolute id's. So the first session in event id 243 may have the session_id 10234 and so on. As sessions and other things are inside event, there id's should start from 1 in the ideal case. I have opened #338 to discuss this issue.

@rafalkowalski

This comment has been minimized.

Copy link
Member

rafalkowalski commented May 18, 2016

@shivamMg Please add also speakers, tracks, etc. I don't know what etc. means. I think that we should have option to start/end session.

@shivamMg

This comment has been minimized.

Copy link
Member

shivamMg commented May 18, 2016

@rafalkowalski By "speakers, tracks, etc." I meant like the URL path /events/2/sessions/4 same would go for speakers (/events/2/speakers/3), tracks (/events/2/tracks/4), microlocations and sponsors, i.e. all services will have similar url pattern: /events/<event_id>/<service>/<service_id>.

I think that we should have option to start/end session.

I do not follow. What do you mean?

@rafalkowalski

This comment has been minimized.

Copy link
Member

rafalkowalski commented May 18, 2016

I mean that we have to have a possibility to update state of session, but it will contain in /events/2/sessions url method PUT. Sorry for mess

@juslee

This comment has been minimized.

Copy link
Contributor

juslee commented May 19, 2016

Keep the session id absolute.

Reason: in the db, you just need to make sure the session table has the event id reference. Session ID is the primary key. If session id is relative, it will require more checking in code to determine if session id is part of the event. And there isn't any point to have a separate primary key and session id.

So far I like where this is going. Good job @shivamMg. Well thought out.

@leto

This comment has been minimized.

Copy link
Contributor

leto commented May 19, 2016

In general, this looks good. Nice work @shivamMg . I also agree with @juslee , let's keep all id's absolute.

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