Skip to content
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

Implementation of APIv3 #4863

Merged
merged 25 commits into from
Mar 12, 2019
Merged

Implementation of APIv3 #4863

merged 25 commits into from
Mar 12, 2019

Conversation

humitos
Copy link
Member

@humitos humitos commented Nov 5, 2018

Initial ideas to discuss the implementation of APIv3.

This is a work in progress PR that only has some pieces of the documentation with the idea to start a discussion around the general proposal.

Closes #4286

@humitos humitos added the PR: work in progress Pull request is not ready for full review label Nov 5, 2018
Copy link
Member Author

@humitos humitos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some thoughts on the overall idea but also where write endpoints make sense.

docs/api/v3.rst Outdated
~~~~~~

The API can be accessed by using the HTTP header ``Authorization``
with a specific ``Token`` that provide access to different resources.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may probably allow a Cookie session also here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@agjohnson has a point about not supporting this that we should consider. He wrote something at #5009 (comment)

docs/api/v3.rst Outdated
and do require authentication even for requesting public information.

Scopes
~~~~~~
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to keep thinking about the implementation of this, since given a Token the request won't be authenticated under a particular user but as a "token with limited permissions".

So, queryset for API endpoints will need to manage these two cases:

  1. projects where the user is admin (in case we support cookie sessions)
  2. projects where that token has specific permissions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we want tokens tied to a user? A user might have tokens that have less than full permissions but I think we definitely want tokens tied to a user. Maybe I'm misunderstanding this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I didn't express myself properly. The 1) case is the same for cookie sessions than for a token user (without custom permission scopes).

Summarizing, we will have these authentication methods:

  1. regular session cookie (authenticated as a user)
  2. token tied to a user (authenticated as a user)
  3. token with specific permissions (not tied to a user)
    • it could be project specific: project:read only in docs project (multiple projects also?)
    • it could be all project from a user specific: project:read for all the projects belonging to user

docs/api/v3.rst Outdated
* repo type
* version active
* version built
* all database?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may want to perform a query like

/api/v3/project/?repo_url=https://github.com/pypa/pip

and get all the projects that are using that URL as source.

Or, all the Spanish projects under Read the Docs,

/api/v3/project/?language=es

but I don't want to allow listing all the projects without any kind of filtering.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to be very careful on this. For projects (where we have ~100k) this might be ok. For builds or versions, we should probably only allow filtering on indexed fields (eg. slug, project__slug, etc.). Otherwise, this can result in API calls that are very heavy from a performance and load perspective.

docs/api/v3.rst Outdated
Project list
++++++++++++

.. http:get:: /api/v3/project/
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably be called /api/v3/project/me/ or similar so we can allow some global filtering now or in the future but leaving the feature open.

docs/api/v3.rst Outdated
{
"href": "/api/v3/project/pip/users/",
"rel": "users",
"type": "GET"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this really verbose but very useful to discover the API and navigate over it.

I suppose that if we really do care about the amount of data returned here, we should re-consider GraphQL ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, you know the slug of your project, you hit this endpoint and you know everything you can do with the API and your project.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't type be method to be more precisely named? However, I question whether it is even needed. If you are just listing, it is always a GET.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

href, rel and type are the "standards" (or what most of the docs I read use).

I think we could remove it completely as you say, but I think that POST would also be allowed in this particular endpoint to add a Maintainer/Owner for this particular project for example.

docs/api/v3.rst Outdated

.. TODO:

Add query string filters to narrow the query:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?featured=true could be useful for the home page also.

docs/api/v3.rst Outdated
"type": "GET"
},
{
"href": "/api/v3/project/pip/builds/",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking that you can do a POST to this URL with something like:

{
  "version": "latest"
}

and trigger a build for that project and version.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and probably similar POST request for the rest of the endpoints listed here as links and GET.

/domains could add a new domain, /notifications a new webhook or email notification, etc

docs/api/v3.rst Outdated
Version detail
++++++++++++++

.. http:get:: /api/v2/project/(string:project-slug)/version/(string:version-slug)/
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By POSTing here, we could activate or disable the version, change the privacy level, etc

docs/api/v3.rst Outdated
"default_version": "stable",
"default_branch": "master",
"documentation_type": "sphinx_htmldir",
"canonical_url": "http://pip.pypa.io/en/stable/",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tags are missing

@humitos humitos added the Needed: design decision A core team decision is required label Nov 5, 2018
docs/api/v3.rst Outdated
Project details
+++++++++++++++

.. http:get:: /api/v3/project/(string:slug)/
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want/need this at all?

We are currently accessing by id to the different endpoint from projects/tasks.py from builders, but I think that this was built like this because there wasn't another way.

I'm not sure that accessing by ID is useful in our case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to expose the ID.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to not exposing ID. slug is the public facing unique identifier in my mind. Though it is possible that it could change over time (by an admin), but I think that's an edge case.

docs/api/v3.rst Outdated
"htmlzip": "//readthedocs.org/projects/pip/downloads/htmlzip/stable/",
"epub": "//readthedocs.org/projects/pip/downloads/epub/stable/"
},
"links": [
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally like the idea of links to show related endpoints to the current resource (and to get more detailed info about it) but the list structure does not convince me since if you want the builds url, you need to loop over the whole list. Maybe a dict is better.

On the other hand, using a dict will also require another nested dict for the method type, like:

"links": {
   "builds": {
      "GET": {
          "href": URL,
...
}

I'm not 100% convinced on the structure, but I think it's a good adoption to have these HATEOAS links here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well... as there is not only one solution for this, I kept googling it and I found that what I'm suggesting here is something already adopted: https://slides.com/jcassee/pycon-sette-hypermedia#/2/6

This is the structure:

links:
  rel:
    href: URI

docs/api/v3.rst Outdated
{
"id": 7367364,
"date": "2018-06-19T15:15:59.135894",
"length": 59,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this field should be called duration

docs/api/v3.rst Outdated
"type": "GET"
},
{
"href": "/api/v3/project/pip/notifications/",
Copy link
Member Author

@humitos humitos Nov 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these URLs can be expressed in the other way:

/api/v3/notifications/pip/

but for some endpoints doesn't look great. Example,

/api/v3/versions/pip/stable/

I'm not 100% on either, though. In this way it's clear that you will be returned a Version resource and /api/v3/project/pip/versions/stable/ maybe it's not that clear.

This document says something about this: https://geemus.gitbooks.io/http-api-design/content/en/requests/minimize-path-nesting.html

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another comment here is that we are using a similar approach of nested URLs for the URLs site: https://readthedocs.org/dashboard/(project-slug)/version/(version-slug)/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would try hard to not have multiple endpoints that return essentially the same thing. I think that point is not that all API nesting is bad but more that it can go too far.

I prefer /api/v3/project/pip/notifications/ as an endpoint to /api/v3/notifications/pip/ (I would prefer /api/v3/notifications/?project__slug=pip to the 2nd one although not the 1st). Since I think /api/v3/notifications/ -- notifications for all projects -- is not particularly useful I think it's fine to only expose the endpoint that is specific to a project.

docs/api/v3.rst Outdated
Build detail
++++++++++++

.. http:get:: /api/v3/build/(int:id)/
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, it makes sense to access by id since it's only unique identifier that we have for a build.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably shoulnd't be allowed (since doesn't make to much sense as a whole) and instead we should make it like /api/v3/project/pip/builds/?commit=a1b2c3 or ?latest=true or ?running=true.

docs/api/v3.rst Outdated
Project list
++++++++++++

.. http:get:: /api/v3/project/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should start using the resource name in plural for v3, /projects/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

I made a mistake and then copy&paste, but it should be plural here.

"slug": "stable",
"verbose_name": "stable",
"identifier": "3a6b3995c141c0888af6591a59240ba5db7d9914",
"built": true,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have a calculated field here: build_date or last_success_build_date or something with that meaning but a better name ;)

Copy link
Contributor

@davidfischer davidfischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like where this is going.

docs/api/v3.rst Outdated
and do require authentication even for requesting public information.

Scopes
~~~~~~
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we want tokens tied to a user? A user might have tokens that have less than full permissions but I think we definitely want tokens tied to a user. Maybe I'm misunderstanding this.

docs/api/v3.rst Outdated
* repo type
* version active
* version built
* all database?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to be very careful on this. For projects (where we have ~100k) this might be ok. For builds or versions, we should probably only allow filtering on indexed fields (eg. slug, project__slug, etc.). Otherwise, this can result in API calls that are very heavy from a performance and load perspective.

docs/api/v3.rst Outdated
Project details
+++++++++++++++

.. http:get:: /api/v3/project/(string:slug)/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to expose the ID.

docs/api/v3.rst Outdated
"type": "GET"
},
{
"href": "/api/v3/project/pip/notifications/",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would try hard to not have multiple endpoints that return essentially the same thing. I think that point is not that all API nesting is bad but more that it can go too far.

I prefer /api/v3/project/pip/notifications/ as an endpoint to /api/v3/notifications/pip/ (I would prefer /api/v3/notifications/?project__slug=pip to the 2nd one although not the 1st). Since I think /api/v3/notifications/ -- notifications for all projects -- is not particularly useful I think it's fine to only expose the endpoint that is specific to a project.

docs/api/v3.rst Outdated
{
"href": "/api/v3/project/pip/users/",
"rel": "users",
"type": "GET"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't type be method to be more precisely named? However, I question whether it is even needed. If you are just listing, it is always a GET.

docs/api/v3.rst Outdated

:statuscode 201: Created sucessfully
:statuscode 400: Some field is invalid
:statuscode 401: Not valid permissions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is necessary to list on every endpoint. This should probably just be in some section on authentication.

docs/api/v3.rst Outdated
:statuscode 401: Not valid permissions


Footer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should come up with a better name for this API endpoint than footer. Maybe displaymetadata. Maybe it can even go away because all the necessary data is in other endpoints. Not critical but something to think about.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought on removing it in favor of all the other endpoints but in that case to build the flyout you may need to make 2 or 3 or maybe more requests. Is that affordable regarding request time?

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the direction that this is going. I think it could use a design document explaining some of the design decisions that went into it. I also think it would be useful to have some explicit use cases that we care about, and want to support and aren't supporting. It feels like they are implied here, but having them be explicit would be very useful. For example, this API feels very focused on automating dashboard operations, but not doing anything with documentation page features, which is arguably ~90% of what our users want to be doing w/ RTD.

docs/api/v3.rst Outdated
--------------------------------

Requests to the Read the Docs public API are for public and private information
and do require authentication even for requesting public information.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm -0 on this. I think having a public API that isn't authed fits a lot of use cases nicely, and I haven't heard an argument for why we should exclude it in this design. I'm fine to heavily rate limit anon access so it isn't built into processes, but I think for learning etc. having it open is good.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is it possible to use this API on a documentation page as designed? eg. I'm a user who wants to get the version listing of my project in my own JS, can I do that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, not requiring authentication duplicates a mistake we made with API v1 and v2. Now that we are deprecating API v1 and v2, there's no way to know who is making outdated API calls because the calls aren't authenticated. Other than broadcast messages in our docs and on the blog, we can't warn users specifically that their API calls are going to break after a certain date.

Secondly, I question the usefulness of an API that allows browsing all projects, versions, and builds rather than "my" projects, versions, and builds. For example, our current v2 endpoints would be a lot more useful if they returned the few projects where you are a maintainer (at least by default) rather than /api/v2/projects/ returning all projects and requiring you to page through them or filter by a single slug.

Still, maybe the right approach is to heavily rate limit the API when used anonymously such that almost everyone will get a token.

Also, is it possible to use this API on a documentation page as designed? eg. I'm a user who wants to get the version listing of my project in my own JS, can I do that?

With authentication, probably not. However, how does one know which projects are "my" projects without authentication?

Copy link
Member

@ericholscher ericholscher Nov 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With authentication, probably not. However, how does one know which projects are "my" projects without authentication?

GET /api/v3/projects/<slug>/versions/

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That gets a project but there's no concept of my project without auth.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well sure, but presumably the JS embedded in a project's docs know what the project is :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree in general auth is useful, but I think we need to support unauth'd endpoints for a lot of pretty important use cases, or have some kind of publishable token.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait a second... I think Eric has a point. How are we going to use our own API (for the footer for example) if we do not allow anonymous access? Will we allow anonymous access only on the endpoints that we know that are going to be used for any doc page?

I'm not sure if we should raise this topic again or not(*), but I tend to agree with Eric that it's good to have the API open. If you are designing a theme for Read the Docs, you won't be able to query anything via the API. Let's say that you want to create your custom version menu flyout:

/api/v3/projects/pip/versions/?active=true

won't be available. What we would do in these cases?

(*) is all about communication with users?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some endpoints that need to be unauthed -- footer api, ads api, etc. But I'm 👎 on a public endpoints like /api/v3/projects listing. @davidfischer sums up the problems nicely.

I think requiring auth is pretty accepted API design at this point from a user perspective, and I don't feel that a public API provides enough value that we need to complicate our design for this use case. There is however a lot of value for us to keep things simple. If there is anything in particular that requires a public API, we should enumerate these though.

What we would do in these cases?

Use a API token :) Part of the intersphinx token work was to eventually pass in a generated token to the build process for this particular case, using our API from the build process.

docs/api/v3.rst Outdated
* ``project``
* ``project:admin``: access to change admin settings (similar to Admin tab)
* ``project:read``: access to read all the information about the projects
* ``project:write``: access to anything related to perform write actions (triggering a Build for this project, activate/deactivate versions, etc)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this different than admin?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking on a scope that allows you to trigger a build, but not to change the name of the project. From my point of view, admin will allow you to change anything under the "Admin" tab of your project dashboard.

docs/api/v3.rst Outdated
Project details
+++++++++++++++

.. http:get:: /api/v3/project/(string:slug)/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to not exposing ID. slug is the public facing unique identifier in my mind. Though it is possible that it could change over time (by an admin), but I think that's an edge case.

@humitos
Copy link
Member Author

humitos commented Nov 6, 2018

I think it could use a design document explaining some of the design decisions that went into it. I also think it would be useful to have some explicit use cases that we care about, and want to support and aren't supporting. It feels like they are implied here, but having them be explicit would be very useful

I wrote a document that describes all the things that we have been talking, mentions the pros/cons and raise some questions. It's probably not 100% complete but we can add more things, discuss about the topics in this PR and we can write the decisions we take after that if we consider or leave them in the comments of PR.

For example, this API feels very focused on automating dashboard operations, but not doing anything with documentation page features, which is arguably ~90% of what our users want to be doing w/ RTD.

@eric could you expand on that?

What I understand from there is basically "having access to the documentation pages via the API". If I'm right on that, I'm basing that on "token scopes and serving any output file via the API" (as first approach at least) like /api/v3/projects/pip/docs/?filename=/objects.inv or similar. I know it's not the best, though, and I'm not sure if I can just use nginx serve file from that endpoint or not... but in any case, I think it's a good first start.

Let me know.



We do NOT want to cover
+++++++++++++++++++++++
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably, we don't want to create users.

docs/api/v3.rst Outdated
"repo_type": "git",
"default_version": "stable",
"default_branch": "master",
"documentation_type": "sphinx_htmldir",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field needs to be removed because it's deprecated.

Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some comments, didn't read the previous discussions, sorry if I'm repeating something

docs/api/v3.rst Outdated
We could extend the scopes to be per-project or per-user. I'm not
sure if this is possible yet, but we should consider it.

* per-project: the user could go to the Project Admin tab and create a Token for this specific project
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea, similar to what we have for webhoooks (just access to /projects/)

docs/api/v3.rst Outdated
:>json array links: Array with HEATEOAS_ links to retrieve related information

:statuscode 200: Success
:statuscode 404: There is no ``Project`` with this slug
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could add a top level section with all the status code and what they return (at the beginning it says we always return a json)

docs/api/v3.rst Outdated
Project import
++++++++++++++

.. http:post:: /api/v3/project/import/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not a post to projects/ endpoint (not import)?

docs/api/v3.rst Outdated
Project edit
++++++++++++

.. http:patch:: /api/v3/project/(string:slug)/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe is more common to put <string:slug>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The syntax that I used comes from the .. http: directive and it uses the (string:slug) to show it nicer.

docs/api/v3.rst Outdated

This endpoint may be under Project section

.. http:post:: /api/v3/project/(string:slug)/builds/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or under /projects/versions/builds?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the slug?

If you missed the slug by mistake, I think it's not necessary to add /versions/ to the URL since "the project has builds" and each "build is tied to a version"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I missed the slug

docs/api/v3.rst Outdated
"API_HOST": "readthedocs.org",
"MEDIA_URL": "https://media.readthedocs.org",
"PRODUCTION_DOMAIN": "readthedocs.io",
"READTHEDOCS": true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are the only fields with uppercase keys

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these come from the template context and I'm keeping compatibility/nomenclature from there.

docs/api/v3.rst Outdated
:query string theme: Theme used in the documentation
:query string docroot: Docroot used to generate external links
:query string source_suffix: Source suffix used to generate external links
:query string format: Output format (``jsonp``, ``json``)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess jsonp is a workaround for CORS on custom domains? I think we want a better way to handle that.


* Rate limit
* ``Request-ID`` header
* `JSON minified by default`_ (maybe with ``?pretty=true``)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess users can just use an API client that does the job to prettified the response

* Rate limit
* ``Request-ID`` header
* `JSON minified by default`_ (maybe with ``?pretty=true``)
* `JSON schema and validation`_ with docs_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if there is a sphinx extension to generate docs from the schema



.. _JSON minified by default: https://geemus.gitbooks.io/http-api-design/content/en/responses/keep-json-minified-in-all-responses.html
.. _JSON schema and validation: https://geemus.gitbooks.io/http-api-design/content/en/responses/keep-json-minified-in-all-responses.html
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the wrong link p:

@agjohnson agjohnson added this to the 3.1 milestone Nov 13, 2018
@agjohnson agjohnson added the Feature New feature label Jan 25, 2019

*See Build details*

:statuscode 201: Created sucessfully
Copy link
Contributor

@marcelstoer marcelstoer Feb 5, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very much looking forward to this new API, thanks!

I suggest this returns 202 rather than 201 as "The request has been accepted for processing, but the processing has not been completed." (assuming the API is non-blocking of course)

@humitos humitos mentioned this pull request Feb 18, 2019
@humitos humitos removed the Needed: design decision A core team decision is required label Feb 19, 2019
@readthedocs readthedocs deleted a comment from codecov bot Feb 19, 2019
@codecov
Copy link

codecov bot commented Feb 19, 2019

Codecov Report

Merging #4863 into master will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4863      +/-   ##
==========================================
+ Coverage   76.41%   76.47%   +0.05%     
==========================================
  Files         158      158              
  Lines        9990     9993       +3     
  Branches     1262     1262              
==========================================
+ Hits         7634     7642       +8     
+ Misses       2016     2009       -7     
- Partials      340      342       +2
Impacted Files Coverage Δ
readthedocs/core/utils/__init__.py 74.73% <0%> (-0.27%) ⬇️
readthedocs/projects/views/private.py 80.1% <0%> (ø) ⬆️
readthedocs/doc_builder/python_environments.py 82.97% <0%> (ø) ⬆️
readthedocs/projects/models.py 85.53% <0%> (+0.03%) ⬆️
readthedocs/builds/models.py 75.8% <0%> (+0.19%) ⬆️
readthedocs/builds/views.py 96% <0%> (+10.28%) ⬆️

@codecov
Copy link

codecov bot commented Feb 19, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@aae11fe). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #4863   +/-   ##
=========================================
  Coverage          ?   76.41%           
=========================================
  Files             ?      158           
  Lines             ?     9990           
  Branches          ?     1262           
=========================================
  Hits              ?     7634           
  Misses            ?     2016           
  Partials          ?      340

@humitos humitos requested a review from a team February 20, 2019 09:33
@humitos humitos removed the PR: work in progress Pull request is not ready for full review label Feb 20, 2019
@humitos
Copy link
Member Author

humitos commented Feb 26, 2019

docs-lint is failing because,

./api/v3.rst:299: (INFO/1) No directive entry for "http:patch" in module "docutils.parsers.rst.languages.en".

but even installing sphinxcontrib-httpdomain doesn't fix it.

@ericholscher
Copy link
Member

ericholscher commented Feb 26, 2019

Looks like we can ignore it:

 --ignore-directives directives
                        comma-separated list of directives to ignore

@stsewd
Copy link
Member

stsewd commented Feb 26, 2019

We have a configuration file for this https://github.com/rtfd/readthedocs.org/blob/master/docs/.rstcheck.cfg

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 on shipping this, and iterating.

We do need to move the api/v3.rst file into a design directory though, since it isn't implemented.

Copy link
Member

@stsewd stsewd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't clear for me why we have the docs for the api and the design docs in the same PR, but the schema looks good.

docs/api/v3.rst Show resolved Hide resolved


Build command details
+++++++++++++++++++++
Copy link
Member

@stsewd stsewd Feb 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we really need this one, I can't imagine why will want to list only one command, this info should be retrieved in /api/v3/projects/(str:project_slug)/builds/(int:build_id)/commands/

@humitos
Copy link
Member Author

humitos commented Feb 28, 2019

We do need to move the api/v3.rst file into a design directory though, since it isn't implemented.

I'm not sure what to do here.

We have design/apiv3.rst where some problems and proposals for APIv3 are written and api/v3.rst shows the current implementation, where the decisions from the design document are written and documented for when we publish APIv3.

On api/v3.rst I added a warning note saying that this is not ready to use since it's under development.

@ericholscher should I remove design/apiv3.rst and move api/v3.rst under design/ folder?

@ericholscher
Copy link
Member

Perhaps we just move the actual user-facing docs into another PR where we can iterate on them with the code, like what Santos did for the YAML config.

@stsewd
Copy link
Member

stsewd commented Feb 28, 2019

Don't we have a way of hide the document? Like make it draft

@stsewd
Copy link
Member

stsewd commented Feb 28, 2019

@stsewd
Copy link
Member

stsewd commented Feb 28, 2019

Perhaps we just move the actual user-facing docs into another PR where we can iterate on them with the code, like what Santos did for the YAML config.

BTW, that was kind of hard to maintain at the end :/, I would have preferred to put/update little chunks with the proper code.

.. sourcecode:: json

{
"version": "latest",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm doubting about this now.

Instead of sending an argument here, we could do:

  • /api/v3/projects/(string:project_slug)/builds/ for triggering the default version configured (usually latest)
  • /api/v3/projects/(string:project_slug)/versions/(string:version_slug)/builds/ to trigger the specific version

This approach seems more clear and follow the pattern we are using, I guess.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build is a weird one, the global pk communicates that this is a top-level object, but it effectively is not in any application. I see a use case for the first endpoint in the case of querying a build, but in the case of triggering a build, i'd definitely vote for the second option. In the case of querying a build, we don't necessarily care what the version is, but it might still help to be explicit about that relationship in our API modeling -- at least just for consistency?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this to a new issue though, the conversation here has dragged this PR out.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of querying a build, we don't necessarily care what the version is, but it might still help to be explicit about that relationship in our API modeling -- at least just for consistency?

This was where I was doubting also. I like consistency but maybe that purity complicates the usage from a user perspective. Although, if we provide good links on the responses with the related object this shouldn't be a problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true too, yeah. Maybe also worth considering, long term, we might want to change the Build model pk to a compound key, in which case these two would be different builds:

  • /api/v3/projects/foo/versions/latest/build/1
  • /api/v3/projects/foo/versions/bar/build/1

The fact that we expose build pk as a global pk has always been sort of odd.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea.

Do we need to "modify the Build model pk"? Can't we make this an API identifier only for now?

  • 1 would be the first (sorted by date) Build for this version
  • 3 would be the third one
  • -1 would be the last one

Internally, we just sort the queryset (by date or -date) and pick the N element.

What do you think?

@stsewd stsewd modified the milestones: 3.3, 3.4 Mar 6, 2019
@humitos
Copy link
Member Author

humitos commented Mar 11, 2019

I removed the APIv3 user documentation from the toctree so it's not linked anywhere, but it's still available (with a warning message saying that's it not implemented yet) by hitting it the right URL manually.

I think we can merge it as is (this PR is insane with the amount of comments), and update the missing pieces/updates on #5356 (most of this PR is based on api/v3.rst document actually).

@agjohnson
Copy link
Contributor

This has enough reviews to merge, feel free to merge if you think it's ready and we're tracking unanswered questions in another PR or issue.

@jaraco
Copy link
Contributor

jaraco commented Feb 1, 2021

@humitos Thanks for the work on this. Were you aware there was a bounty for the feature to enable project creation by API? Please feel free to claim the bounty.

@humitos
Copy link
Member Author

humitos commented Feb 1, 2021

@jaraco nice! (I wasn't aware) I was paid by Read the Docs itself to do this work, so it may make more sense to donate this back to them 😄

On the other hand, I would appreciate a lot if you have some feedback to share with us about APIv3 and if it's has been being useful to achieve your needs.

@jaraco
Copy link
Contributor

jaraco commented Mar 5, 2021

it may make more sense to donate this back to them 😄

The way BountySource works, an individual needs to claim the bounty, at which point I'll approve. I don't have the ability to withdraw a bounty and thereby contribute it elsewhere. I would very much like for you or an RTD owner to claim the bounty. Otherwise, it will just linger there indefinitely. Would you be willing to take the lead in finding someone to claim it?

On the other hand, I would appreciate a lot if you have some feedback to share with us about APIv3 and if it's has been being useful to achieve your needs.

It has been useful. It's been useful in updating the default branch, a routine I've used to facilitate the renaming of default branch across 100+ repos without having to click through a web UI. I still have plans to utilize the project creation API to automatically enroll projects mechanically, but I haven't actually implemented that yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Design APIv3
7 participants