Add first draft of OEP-13 (API conventions) by efagin · Pull Request #36 · openedx/openedx-proposals

efagin · 2016-12-29T19:46:40Z

Supercedes https://openedx.atlassian.net/wiki/display/AC/edX+REST+API+Conventions (the content is virtually identical). A Best Practices OEP is a more reasonable home for this content and I'd like changes to our conventions to be captured more formally (i.e. through PRs instead of wiki comment threads).

@nasthagiri, would you mind acting as arbiter for this OEP? I can't imagine there are any issues but you're familiar enough with the original source wiki. Once this gets merged, I'm going to update the original wiki page to just link to the OEP.

andy-armstrong

@efagin converting this to an OEP was a great idea, thanks. I'm out of the loop on our current thinking around API development so I look forward to hearing how others react. I have a few minor comments that I hope are helpful.

andy-armstrong · 2016-12-29T22:16:21Z

oeps/oep-0013.rst

+Motivation
+==========
+
+The Open edX platform contains many extension points for third-party developers that wish to integrate with the platform. We wish to approach a standard methodology for API design in order to give developers more consistency in their API client design, and to make our endpoint behaviors more predictable.


Nit: we settled on wrapping lines at 80 characters in OEPs.

From @cpennington:

I tend to prefer wrapped lines in the raw text, simply because it's easier to read in an editor (and in the email summaries that github sends). Github wraps them nicely in the diff, but in this case I lean towards optimizing towards the lowest common denominator.

IMO it would be helpful to have a description of the API gateway and how (if?) it affects the development of APIs.

Good point, I just updated the doc with 80-char line wrapping.

andy-armstrong · 2016-12-29T22:19:22Z

oeps/oep-0013.rst

+
+**Discoverable**
+
+Support `HATEOAS <https://en.wikipedia.org/wiki/HATEOAS>`_ where possible.  This allows us to change our URLs without needing to worry about updating the mobile apps if the app discovers its URLs from a base URL.


Are we using HATEOAS in our newer APIs? I remember that @nasthagiri mentioned there were reasons why mobile doesn't use it. Is this still the recommendation? Does it change with the API gateway?

andy-armstrong · 2016-12-29T22:23:14Z

oeps/oep-0013.rst

+
+*Must*: edX resource identifiers
+
+- Users should be referenced by username


Is this where we landed at the last arch lunch?

andy-armstrong · 2016-12-29T22:25:49Z

oeps/oep-0013.rst

+
+- ``expand`` parameter - Allow clients to request including data from other resources using the expand parameter as explained further in the Expansion section.
+
+.. todo: link to expansion section 


Don't forget to provide this link

Thanks, done.

andy-armstrong · 2016-12-29T22:27:06Z

oeps/oep-0013.rst

+
+Use the appropriate value amongst the following (or document in the code why an exception is needed)
+
+    200 - OK


Nit: this section shows up as quoted on the rendered page:

https://github.com/edx/open-edx-proposals/blob/6abc23aed18fe98f0270934363d744fc3ef2cdec/oeps/oep-0013.rst

I'd probably also bold the status codes themselves.

andy-armstrong · 2016-12-29T22:28:02Z

oeps/oep-0013.rst

+        {
+            "error_code": "course_not_started"  # a short string that the client can rely upon for handling different errors
+            "developer_message" : "Verbose, plain language description of the problem for the app developer with hints about how to fix it.",
+            "user_message":"Pass this message on to the app user if needed.",


Nit: missing a space between the key and value.

andy-armstrong · 2016-12-29T22:32:27Z

oeps/oep-0013.rst

+Documentation
+-------------
+
+Work with the Docs team to update the `edX platform API documentation <http://edx-platform-api.readthedocs.org/en/latest/>`_.


I'm not sure if this document is still being updated, and it if it is then it is not the docs team that does it.

andy-armstrong · 2016-12-29T22:34:57Z

oeps/oep-0013.rst

+- *Must*: Additive changes to the contract of the API, or the results, should not bump the major version, unless those changes are non-optional, and break existing contracts with the API.
+
+
+**Future considerations**


Maybe pull this out to its own top level section? I thought there was a standard Future Directions in OEP-1 but I don't see one.

I removed it entirely, we should keep speculative things out of OEPs and just add content when it's well-formed.

andy-armstrong · 2016-12-29T22:36:14Z

oeps/oep-0013.rst

+Authentication
+--------------
+
+By convention, our REST APIs support the following two authentication schemes:


It might be useful to mention JWT tokens here.

andy-armstrong · 2016-12-29T22:38:18Z

oeps/oep-0013.rst

+Reference Implementation
+========================
+
+There are a number of existing APIs that you can crib from, for example:


It would be good to update this to include some of the newer APIs, such as those for course discovery.

robrap · 2016-12-30T03:19:13Z

oeps/oep-0013.rst

+- ``fields`` parameter - Allow clients to specify/filter the fields in the response by supporting a fields parameter as a comma-delimited list. ::
+
+    /dogs?fields=name,color,location
+    /dogs?fields=title,media:group(media:thumbnail)


I don't think we have implemented the xpathesque type of field filtering that Google has.

If we think we want to use this, for mobile or otherwise, we should choose a solution and duplicate and/or link to docs that provide details. For example, Google's docs (drive api) for partial responses.

robrap · 2016-12-30T03:34:33Z

oeps/oep-0013.rst

+
+**Format**
+
+Use `Swagger <http://swagger.io/specification/>`_ to generate API documentation where possible.


Some of our code seems to be using Django REST Swagger for Swagger documentation of DRF. Maybe this should be documented as our best practice, given that REST Framework Docs (which I think is documented below, was deprecated in favor of Django REST Swagger, both by the same creator. This is described on the DRF site for documenting APIs.

efagin · 2017-01-03T16:32:38Z

@andy-armstrong @robrap thanks for the early feedback! Many of your comments question the content itself. Since all I did was just port the existing wiki page over here, I'm surprised that there's so much in this document that you feel requires major editing. (This, on its own, is a good reason to move to OEP.)

Do you think we should broaden the scope of this OEP to actually go and revise these best practices? I had started this just figuring I'd do a quick port and merge, and then we could modify later, but if you think we should just do it all now, then I'll allocate more of my time to updating these best practices now.

andy-armstrong · 2017-01-03T18:23:48Z

@efagin IMO it would be great if you could allocate the extra time to revise the recommendations. It seems like a good opportunity to make this up-to-date, and then we can all be on the same page going forward.

efagin · 2017-01-03T19:19:37Z

@andy-armstrong OK, I'll do that. This OEP will definitely require more effort before it's ready to merge, and please continue to provide feedback.

andy-armstrong · 2017-01-03T22:27:35Z

oeps/oep-0013.rst

+multiple PATCH styles in any given API, should this become desirable, without
+breaking existing clients.
+
+URL parameters


It would be good to describe how boolean query parameters should be handled. My preference is that they should be passed as true/false, but I'm open to other options.

I mention this because I dislike using 0/1 for boolean values, which I commented as such on this PR: https://github.com/edx/edx-platform/pull/14204

andy-armstrong · 2017-01-06T22:37:12Z

oeps/oep-0013.rst

+Documentation
+-------------
+
+Work with the Docs team to update the `edX platform API documentation


I think we need to revisit our documentation approach for APIs given the changes in the doc team. I'm not sure if this OEP is the best place to describe processes, but it would be good to lay out what is expected of documentation for a new API:

the API developer should use Swagger to auto-generate documentation (as mentioned below)

the developer is in charge of getting this documentation published (but how?)

how does an API get to be part of the API Gateway and have its documentation made available there too?

Note that the link on this line is for edx-platform only, and is for a document that does not appear to be actively maintained. Also, this OEP needs to be for the community as a whole, so the language about "Work with the Docs team" doesn't generalize even if there were a docs team to work with.

clintonb · 2017-01-12T20:41:18Z

oeps/oep-0013.rst

+
+The query parameters are as follows:
+
+- ``page`` - the page of results (zero indexed)


Why zero-indexed? I can't think of any of our own APIs that implement this. If we want to see this, and the other conventions (e.g. num_pages), implemented we should make it easy to do so by creating a pagination class in https://github.com/edx/edx-drf-extensions.

clintonb · 2017-01-12T20:45:14Z

oeps/oep-0013.rst

+
+- ``page`` - the page of results (zero indexed)
+- ``page_size`` - the number of results to return per page
+- ``sort_order`` - a string indicating the desired order (no direction


Sorting/ordering should be separate from pagination.

I would prefer we advocate ordering on explicit fields as it is clear to the client what is being done. Passing sort_order=asc is quite ambiguous.

clintonb · 2017-01-12T20:47:35Z

oeps/oep-0013.rst

+    e.g. https://courses.edx.org/api/user/v1/accounts/AndyA
+
+    See the `DRF documentation
+    <http://tomchristie.github.io/rest-framework-2-docs/topics/documenting-your-api>`_


We use DRF 3 everywhere. This is a more relevant URL: http://www.django-rest-framework.org/topics/documenting-your-api/

clintonb · 2017-01-12T20:49:16Z

oeps/oep-0013.rst

+
+        GET /api/enrollment/v1/enrollment/{user_id},{course_id}
+
+    **Response Values**


This is not necessary when using Swagger. Also, docstrings like these tend to grow stale as APIs evolve since I don't necessarily have to touch a view to modify a serializer.

ormsbee · 2017-03-20T14:32:27Z

oeps/oep-0013.rst

+
+**Simple**
+
+Keep the top-leveI resources clear and simple - focusing on what the client is


Typo on "top-level"

ormsbee · 2017-03-20T14:47:09Z

oeps/oep-0013.rst

+**Prevent information leakage**
+
+    **Must**: Use 404 instead of 403 when the actual existence would be leaking
+    information that we don't want


An example or two might be good here.

ormsbee · 2017-03-20T14:53:10Z

oeps/oep-0013.rst

+The query parameters are as follows:
+
+- ``page`` - the page of results (zero indexed)
+- ``page_size`` - the number of results to return per page


Developers should explicitly set limits on this parameter.

ormsbee · 2017-03-20T14:56:56Z

oeps/oep-0013.rst

+
+We use the conventions established by `DRF
+<http://www.django-rest-framework.org/api-guide/pagination/#pagenumberpagination>`_,
+with some extra fields:


Can we add a note here about how certain very large collections (e.g. users, enrollments) cannot be effectively paged through with DRF's default pagination class? The choices then are to either use CursorPagination, a custom pagination class, or to simply cap the result set in some way:

"Returning the first 500 results that match your query."

"You have to be more specific before I give you any data."

"No, you may not crawl our entire enrollment database by making half a million sequential requests, please don't try that."

efagin · 2017-07-25T19:48:15Z

I know this has been idle for some time now. Part of the issue is the recurring theme that "best practice" OEPs are difficult to maintain and approve. For now, we've made the conscious decision at edX to keep Best Practice content in the wiki and allow freeform editing as practices evolve. Instead of leaving this open indefinitely, I'm going to withdraw this OEP, remove content, and merge the PR as-is. I will NOT squash commits unless this is desired - might be nice to have the commit history for archival purposes.

@andy-armstrong would you mind acting as Arbiter here?

andy-armstrong

👍 makes sense to me. As Arbiter, I'll go ahead and merge this now.

Add first draft of OEP-13 (API conventions)

6abc23a

efagin force-pushed the efagin/oep-13-api-conventions branch from 41871b6 to 6abc23a Compare December 29, 2016 19:54

andy-armstrong reviewed Dec 29, 2016

View reviewed changes

robrap reviewed Dec 30, 2016

View reviewed changes

fix a few points of feedback, and wrap lines where possible

85daad7

andy-armstrong reviewed Jan 3, 2017

View reviewed changes

andy-armstrong mentioned this pull request Jan 3, 2017

backend view to return tree for course outline and any given block. openedx/openedx-platform#14204

Merged

4 tasks

andy-armstrong reviewed Jan 6, 2017

View reviewed changes

clintonb reviewed Jan 12, 2017

View reviewed changes

ormsbee reviewed Mar 20, 2017

View reviewed changes

withdrawing OEP-13

f8c961f

andy-armstrong approved these changes Jul 25, 2017

View reviewed changes

andy-armstrong merged commit 4929541 into master Jul 25, 2017

andy-armstrong deleted the efagin/oep-13-api-conventions branch July 25, 2017 20:06


		Discoverable

		Support `HATEOAS <https://en.wikipedia.org/wiki/HATEOAS>`_ where possible. This allows us to change our URLs without needing to worry about updating the mobile apps if the app discovers its URLs from a base URL.


		Must: edX resource identifiers

		- Users should be referenced by username


		- ``expand`` parameter - Allow clients to request including data from other resources using the expand parameter as explained further in the Expansion section.

		.. todo: link to expansion section


		Use the appropriate value amongst the following (or document in the code why an exception is needed)

		200 - OK

		- Must: Additive changes to the contract of the API, or the results, should not bump the major version, unless those changes are non-optional, and break existing contracts with the API.


		Future considerations


		Format

		Use `Swagger <http://swagger.io/specification/>`_ to generate API documentation where possible.


		The query parameters are as follows:

		- ``page`` - the page of results (zero indexed)


		GET /api/enrollment/v1/enrollment/{user_id},{course_id}

		Response Values


		Simple

		Keep the top-leveI resources clear and simple - focusing on what the client is

Conversation

efagin commented Dec 29, 2016

Uh oh!

andy-armstrong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efagin commented Jan 3, 2017

Uh oh!

andy-armstrong commented Jan 3, 2017

Uh oh!

efagin commented Jan 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efagin commented Jul 25, 2017

Uh oh!

andy-armstrong left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone