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

Markus API Design #1002

Closed
danielstjules opened this Issue Feb 6, 2013 · 14 comments

Comments

Projects
None yet
3 participants
@danielstjules
Member

danielstjules commented Feb 6, 2013

Rough documentation for the API can be found at: https://github.com/MarkUsProject/Markus/wiki/ApiHowTo
Though not documented, the following is the existing API, my experience with it, and criticism/ideas:

/api/submission_downloads

GET     /api/submission_downloads/:id   : Downloads submission file(s)
                                          Necessary params: group_name, assignment

Because the api uses the show action, the path for getting a submission download must have an :id. This value isn't used or checked in the method's code, but is simply necessary as the default routes requires it. As a result, it would be more suitably written with the index action.

So, to download a submission, the url will look like the following:
http://demo.markusproject.org/api/submission_downloads/1?group_name=c5anthei&assignment=A3
(note the random int)
Instead of, for example (using index rather than show):
http://demo.markusproject.org/api/submission_downloads?group_name=c5anthei&assignment=A3

In other APIs, the 1 that sometimes precedes the request signifies the API version, but in our case it is clearly used by rails and mapped as the value for the parameter "id".

Testing that API call, I use the following:

curl -O --header 'Authorization: MarkUsAuth NzVhZTIwOGE0NDhjNDY2YjRmNTlhNzdiYjdjN2E2ZTQ=' "http://demo.markusproject.org/api/submission_downloads/1?group_name=c5anthei&assignment=A3"

(note: quotes are required to handle the ampersands, and -O to download the zip rather than print its contents)

Which successfully downloads the file:

daniel:~ danielstjules$ curl -O --header 'Authorization: MarkUsAuth MzUxMTljNmE0MGFlOGQzYjE1NmI4OTc2YTljN2M3NDY=' "http://demo.markusproject.org/api/submission_downloads/1?group_name=c5anthei&assignment=A3"
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
116   584    0   584    0     0   1348      0 --:--:-- --:--:-- --:--:-- 15783

/api/users

POST    /api/users      : Create user
                        : Requires first_name,last_name,section_name,grace_credits
GET     /api/users      : Lists details on all users: firstname, lastname, username, type
GET     /api/users/:id  : Returns details on a user: firstname, lastname, username, type
                          Required parameter is user_name
PUT     /api/users/:id  : Updates a user.
                          Requires first_name,last_name,new_user_name,section_name,grace_credits
DELETE  /api/users/:id  : Does nothing, don't allow deleting users

Once again, because we're using "show", but the :id is not provided, we must pass an arbitrary id to get details on a single user:

curl --header 'Authorization: MarkUsAuth NzVhZTIwOGE0NDhjNDY2YjRmNTlhNzdiYjdjN2E2ZTQ=' "http://demo.markusproject.org/api/users/1?user_name=c5anthei"

Which returns:

User Name: c5anthei
Type: Student
First Name: George
Last Name: Antheil

If the API used the :id, the request URL would instead be:
http://demo.markusproject.org/api/users/93 which is much more permanent, given that usernames can be changed via Student Management or even the API. The same goes for PUT, and DELETE actions.

/api/test_results

POST    /api/test_results       : Upload a TestResult for a submission
GET     /api/test_results/:id   : Returns the contents of a TestResult
PUT     /api/test_results/:id   : Overwrites a TestResult
DELETE  /api/test_results/:id   : Delete a TestResult

To upload a new test result for a submission, you can use a request such as the following:

curl --header 'Authorization: MarkUsAuth MzUxMTljNmE0MGFlOGQzYjE1NmI4OTc2YTljN2M3NDY=' -F group_name=c5anthei -F assignment=A1 -F filename=test.txt -F "file_content=asdasdasd" "http://demo.markusproject.org/api/test_results"

If the test framework isn't enabled, rather than get a simple error message, we get a page long trace, including this:

NoMethodError (undefined method `current_submission_used' for nil:NilClass):
  app/models/submission.rb:161:in `get_submission_by_group_and_assignment'
  app/controllers/api/test_results_controller.rb:24:in `create'

Otherwise, upon success, it'll output:

200
Success

Getting a test result, on the other hand, can be done with the following:

curl --header 'Authorization: MarkUsAuth MzUxMTljNmE0MGFlOGQzYjE1NmI4OTc2YTljN2M3NDY=' "http://demo.markusproject.org/api/test_results/1?group_name=c5anthei&assignment=A3&filename=test.txt"

Testing

Testing of main_api_controller, test_results_controller, and users_controller are all very exhaustive. However, no test cases exist for submission_downloads_controller.

Ideas

  • Keep the single entry point: /api
  • A GET request on the entry point should return a list of available features and collections. In the absense of API version numbers, users can know what is available on a particular installation
  • Add access to a variety of collections and resources
  • Use common relationships for accessing collections/subcollections, defined using nested routes
  • Use element ids for accessing a unique resource in a collection
  • Getting a resource should include its href attribute for self referencing
  • Collections should be filterable
  • Allow PUT to update part of a resource in the absense of support for PATCH requests/routing in rails
  • Allow output format to be set explicitly (html, xml, json)
  • Add more exhaustive documentation with simple examples
  • Define explicit error messages involving parameter names and values

Available Methods:

Referring to the standard methods outlined at:
https://restful-api-design.readthedocs.org/en/latest/methods.html &
http://en.wikipedia.org/wiki/Representational_state_transfer
We'd continue to use the methods below, while any that are missing could be added if ever required.

GET    - collection - List resources, their headers and URIs in a collection
POST   - collection - Create a new entry in the collection
GET    - resource   - Retrieve a single resource
PUT    - resource   - Replace a resource, or update parts of a resource (simulate PATCH)
DELETE - resource   - Delete a resource

These correspond to the following default Rails "resources" controller actions: index, show, update, new, and destroy. These methods are the same as what is already available, but their use, in terms of collections/subcollections, and identification of individual resources is what would change. Nested routes would allow us to capture the relationsip between the different collections/resources/sub-collections: http://guides.rubyonrails.org/routing.html#nested-resources. For example, using a recommended maximum 1 level deep of nested routes: /api/collection/resource/sub-collection/resource

Proposed Routes

GET     /api

GET     /api/assignments
POST    /api/assignments
GET     /api/assignments/id
PUT     /api/assignments/id
DELETE  /api/assignments/id

GET     /api/assignments/id/criteria
POST    /api/assignments/id/criteria
GET     /api/assignments/id/criteria/id
PUT     /api/assignments/id/criteria/id
DELETE  /api/assignments/id/criteria/id

GET     /api/assignments/id/annotation_categories
POST    /api/assignments/id/annotation_categories
GET     /api/assignments/id/annotation_categories/id
PUT     /api/assignments/id/annotation_categories/id
DELETE  /api/assignments/id/annotation_categories/id

GET     /api/assignments/id/groups
GET     /api/assignments/id/groups/id

GET     /api/assignments/id/groups/id/submission_downloads

GET     /api/assignments/id/groups/id/test_results/
POST    /api/assignments/id/groups/id/test_results/
GET     /api/assignments/id/groups/id/test_results/id
PUT     /api/assignments/id/groups/id/test_results/id
DELETE  /api/assignments/id/groups/id/test_results/id

GET     /api/marks_spreadsheets
POST    /api/marks_spreadsheets
GET     /api/marks_spreadsheets/id
PUT     /api/marks_spreadsheets/id
DELETE  /api/marks_spreadsheets/id

GET     /api/marks_spreadsheets/id/grades
POST    /api/marks_spreadsheets/id/grades
GET     /api/marks_spreadsheets/id/grades/id
PUT     /api/marks_spreadsheets/id/grades/id
DELETE  /api/marks_spreadsheets/id/grades/id

GET     /api/users
POST    /api/users
GET     /api/users/id
PUT     /api/users/id

GET     /api/notes
POST    /api/notes
GET     /api/notes/id
PUT     /api/notes/id
DELETE  /api/notes/id

Possible headers

Authentication
Accept: text/html
Accept: application/json
Accept: application/xml

Common parameters

limit:
    Use: Collections
    Default: none
    Limit the number of results returned from a collection.
offset:
    Use: Collections
    Default: 0
    Specify the offset, that is the number of resources to skip in the response.
filter:
    Use: Collections
    Filter a collection's results by a resource's attributes (name, date, etc)
    Ie: filter=first_name:daniel,user_name:dst
fields:
    Use: Collections, Resources
    Only return the fields listed in the request parameters.
    Ie: fields=user_name,first_name,last_name

Response formats

This would be written as I continue to work on it...

GET /api
List information on supported features and top level collections.

<markus_api>
    <link method="get" rel="users" href="http://localhost/users/" />
    <link method="get" rel="sections" href="http://localhost/sections/" />
    ...
</markus_api>

Links
Some of the resources I've been reading:
http://publish.luisrei.com/articles/rest.html
http://www.stormpath.com/blog/designing-rest-json-apis
http://www.ics.uci.edu/~fielding/pubs/dissertation/rest_arch_style.htm
https://restful-api-design.readthedocs.org/en/latest/
http://blog.apigee.com/detail/restful_api_design
https://developer.atlassian.com/display/REST/Atlassian+REST+API+Design+Guidelines+version+1

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Feb 13, 2013

Member

Note that in the above, I used a 2-level nested route for the test_results, despite it often being recommended to limit it to 1 level as seen with the rest of our routes. However, I feel as though it would be the most appropriate as it shows the logical relationship between assignments/submissions/test_results, and the alternative would be to have test_results as a top level collection.

Input on that, and anything else in this proposal/issue would be great! Thanks :)

Member

danielstjules commented Feb 13, 2013

Note that in the above, I used a 2-level nested route for the test_results, despite it often being recommended to limit it to 1 level as seen with the rest of our routes. However, I feel as though it would be the most appropriate as it shows the logical relationship between assignments/submissions/test_results, and the alternative would be to have test_results as a top level collection.

Input on that, and anything else in this proposal/issue would be great! Thanks :)

@jerboaa

This comment has been minimized.

Show comment
Hide comment
@jerboaa

jerboaa Feb 13, 2013

Member

This sounds good to me. @etraikov worked on a new api design for assignments quite a while back, which never got merged. It's one of these reviews:

http://review.markusproject.org/r/1210/
http://review.markusproject.org/r/1206/
http://review.markusproject.org/r/1225/
http://review.markusproject.org/r/1230/
http://review.markusproject.org/r/1233/

Reviewboard on markusproject is offline though. We'd need @benjaminvialle to resurrect these (if that's possible at all). Anyhow so far my info. Cheers!

Member

jerboaa commented Feb 13, 2013

This sounds good to me. @etraikov worked on a new api design for assignments quite a while back, which never got merged. It's one of these reviews:

http://review.markusproject.org/r/1210/
http://review.markusproject.org/r/1206/
http://review.markusproject.org/r/1225/
http://review.markusproject.org/r/1230/
http://review.markusproject.org/r/1233/

Reviewboard on markusproject is offline though. We'd need @benjaminvialle to resurrect these (if that's possible at all). Anyhow so far my info. Cheers!

@benjaminvialle

This comment has been minimized.

Show comment
Hide comment
@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Feb 14, 2013

Member

Thanks @jerboaa and @benjaminvialle
The two I saw in the list that seemed relevant were https://gist.github.com/benjaminvialle/4055194 and https://gist.github.com/benjaminvialle/4055313

4055313 contains code relating to assignments, and I'll definitely look at parts of the implementation.

As for 4055194, I haven't tested it, but it seems to me that it would give students full access to the admin API. I'm pretty sure this is currently the only thing stopping it:

  # Student's aren't allowed yet
  if @current_user.student?
    # API is available for TAs and Admins only
    render 'shared/http_status', :locals => { :code => "403", :message => HttpStatusHelper::ERROR_CODE["message"]["403"] }, :status => 403
    return
  end

Are we interested in allowing students to use parts of the API as well? I'm not sure what would be helpful, as they shouldn't be performing nearly as many bulk tasks.

Member

danielstjules commented Feb 14, 2013

Thanks @jerboaa and @benjaminvialle
The two I saw in the list that seemed relevant were https://gist.github.com/benjaminvialle/4055194 and https://gist.github.com/benjaminvialle/4055313

4055313 contains code relating to assignments, and I'll definitely look at parts of the implementation.

As for 4055194, I haven't tested it, but it seems to me that it would give students full access to the admin API. I'm pretty sure this is currently the only thing stopping it:

  # Student's aren't allowed yet
  if @current_user.student?
    # API is available for TAs and Admins only
    render 'shared/http_status', :locals => { :code => "403", :message => HttpStatusHelper::ERROR_CODE["message"]["403"] }, :status => 403
    return
  end

Are we interested in allowing students to use parts of the API as well? I'm not sure what would be helpful, as they shouldn't be performing nearly as many bulk tasks.

@benjaminvialle

This comment has been minimized.

Show comment
Hide comment
@benjaminvialle

benjaminvialle Feb 17, 2013

Member

Hi,

I think that API will be mostly used by admins and MarkUs ATE (Automated Test Engine).

Member

benjaminvialle commented Feb 17, 2013

Hi,

I think that API will be mostly used by admins and MarkUs ATE (Automated Test Engine).

@jerboaa

This comment has been minimized.

Show comment
Hide comment
@jerboaa

jerboaa Feb 18, 2013

Member

Yes, https://gist.github.com/benjaminvialle/4055313 is what I meant. It would be good to get that incorporated.

Member

jerboaa commented Feb 18, 2013

Yes, https://gist.github.com/benjaminvialle/4055313 is what I meant. It would be good to get that incorporated.

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Feb 26, 2013

Member

Thanks, I've gone through it, and will work on assignments next. Also, I just posted a tentative pull request for the updates to /api/users.

Member

danielstjules commented Feb 26, 2013

Thanks, I've gone through it, and will work on assignments next. Also, I just posted a tentative pull request for the updates to /api/users.

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Mar 10, 2013

Member

@benjaminvialle @jerboaa What would either of you think of removing the plaintext responses from the API? That is, only offerring XML and JSON responses. The reason I ask is because some of the language keys in the yml files have HTML syntax, ie:

assignment:
    assignment: "Assignment %{short_identifier}"
    required_fields: "Required Fields <span class='required_field'>*</span>"
    name:           "Assignment Name"
    due_date:       "Due Date<span class='required_field'>*</span>"
    short_identifier:   "Short Identifier<span class='required_field'>*</span>"

So if I try to print short_identifier or due_date, the plaintext response will have the above span elements. To avoid that, we'd either have to modify the corresponding views and place the span in there, or duplicate a lot of existing languages keys, which also isn't a good solution. I'd appreciate some input on this.

Thanks.

Member

danielstjules commented Mar 10, 2013

@benjaminvialle @jerboaa What would either of you think of removing the plaintext responses from the API? That is, only offerring XML and JSON responses. The reason I ask is because some of the language keys in the yml files have HTML syntax, ie:

assignment:
    assignment: "Assignment %{short_identifier}"
    required_fields: "Required Fields <span class='required_field'>*</span>"
    name:           "Assignment Name"
    due_date:       "Due Date<span class='required_field'>*</span>"
    short_identifier:   "Short Identifier<span class='required_field'>*</span>"

So if I try to print short_identifier or due_date, the plaintext response will have the above span elements. To avoid that, we'd either have to modify the corresponding views and place the span in there, or duplicate a lot of existing languages keys, which also isn't a good solution. I'd appreciate some input on this.

Thanks.

@benjaminvialle

This comment has been minimized.

Show comment
Hide comment
@benjaminvialle

benjaminvialle Mar 10, 2013

Member

I agree with removing plain text response.

Member

benjaminvialle commented Mar 10, 2013

I agree with removing plain text response.

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Apr 1, 2013

Member

Updated the issue to include:

 GET     /api/assignments/id/groups
 GET     /api/assignments/id/groups/id
 GET     /api/assignments/id/groups/id/submission_downloads
Member

danielstjules commented Apr 1, 2013

Updated the issue to include:

 GET     /api/assignments/id/groups
 GET     /api/assignments/id/groups/id
 GET     /api/assignments/id/groups/id/submission_downloads
@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Apr 2, 2013

Member

@benjaminvialle Progress on documentation can be found here: https://github.com/danielstjules/Wiki/blob/issue-1002/RESTfulApiDocumentation.rst I'd appreciate any constructive criticism :)

Also, I'll be working on Test Results next. Just going to go back and forth between that and the documentation.

Member

danielstjules commented Apr 2, 2013

@benjaminvialle Progress on documentation can be found here: https://github.com/danielstjules/Wiki/blob/issue-1002/RESTfulApiDocumentation.rst I'd appreciate any constructive criticism :)

Also, I'll be working on Test Results next. Just going to go back and forth between that and the documentation.

@benjaminvialle

This comment has been minimized.

Show comment
Hide comment
@benjaminvialle

benjaminvialle Apr 5, 2013

Member

@danielstjules thank you very much

I think you can ask for a merge into MarkUs' wiki

Will read it carefully this week-end.

Member

benjaminvialle commented Apr 5, 2013

@danielstjules thank you very much

I think you can ask for a merge into MarkUs' wiki

Will read it carefully this week-end.

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules Apr 6, 2013

Member

Thanks, I updated the documentation a bit more. I'm gonna start updating Test Results shortly.

Member

danielstjules commented Apr 6, 2013

Thanks, I updated the documentation a bit more. I'm gonna start updating Test Results shortly.

@danielstjules

This comment has been minimized.

Show comment
Hide comment
@danielstjules

danielstjules May 1, 2013

Member

Once these two pull requests are merged, I'd like to close this issue and open up smaller tickets for other improvements to the current API.

Member

danielstjules commented May 1, 2013

Once these two pull requests are merged, I'd like to close this issue and open up smaller tickets for other improvements to the current API.

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