danielstjules commented 11 years ago

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

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

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

Hi,

For some of reviews still up-to-date on ReviewBoard, I created Github issues with gist associated:

If it is not enough, I can start again reviewboard. Let me know :)

danielstjules commented 11 years ago

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

Hi,

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

jerboaa commented 11 years ago

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

danielstjules commented 11 years ago

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

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

I agree with removing plain text response.

danielstjules commented 11 years ago

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

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

@danielstjules thank you very much

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

Will read it carefully this week-end.

danielstjules commented 11 years ago

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

danielstjules commented 11 years ago

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.

MarkUsProject / Markus

Markus API Design #1002

/api/submission_downloads

/api/users

/api/test_results

Testing

Ideas