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

RFC0026: Adopting OpenAPI #43

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@MatMoore
Copy link

MatMoore commented Nov 9, 2018

Context

As a developer on the GOV.UK registers team, it's currently a lot of work to keep the registers specification, the conformance tests, the reference implementation, and the API explorer in sync, while at the same time providing high quality reference documentation for the GOV.UK registers service. Manually updating the API reference feels like duplicated effort, especially since we already define schemas for the API in the conformance tests.

When making decisions about the tooling surrounding the reference implementation, I would prefer to automate as much as possible, and reuse existing tools rather than rolling our own.

We should also avoid building too much tooling that is coupled to our own service. In my opinion both the API explorer built into ORJ and the API reference are serving similar needs, but they have been implemented in a way that makes neither suitable for other teams who might want to host the software themselves.

Changes proposed in this pull request

This RFC proposes using OpenAPI to provide a machine readable specification of the JSON REST API. This would allow any implementor to choose existing open source tools for documentation, testing, code generation etc.

Guidance to review

If this RFC is approved, I suggest I prototype something before merging, as there are a lot of unknowns here.

Another option we could take would be to adopt OpenAPI for internal use first, without committing to providing anything indefinitely.

@MatMoore MatMoore requested review from arnau and gidsg Nov 9, 2018

@MatMoore

This comment has been minimized.

Copy link
Author

MatMoore commented Nov 9, 2018

@bahmady do you have any thoughts about this?

@gidsg

gidsg approved these changes Nov 9, 2018

Copy link

gidsg left a comment

Happy in principal to give this a try. From past experience it has been non-trivial to autogenerate swagger depending on the framework so I agree we should spike this to see if it is actually feasible!

@bahmady

This comment has been minimized.

Copy link

bahmady commented Nov 9, 2018

I think it'd be a good thing to do, and I'd like to be involved if you'll have me (at least to see what's going on under the hood).

I think Open API v3 is likely to be more feasible/easy to implement than Swagger/Open API v2. Pay has set up auto-generation of a v2 file. @kbottla or @heathd are generally very knowledgeable about it so might be able to assist with some advice if they have time.


### Limitations of OpenAPI

The Registers team has briefly investigated OpenAPI before (see [this comment from Paul Downey](https://github.com/alphagov/open-standards/issues/31#issuecomment-319334209)). A reason against adopting it is that parts of the API (such as HATEOAS, and especially links between registers) would be difficult or impossible to capture. This means that a purely machine generated documentation site would be missing information. This is something we should consider carefully when authoring the document, but I don't consider a strong enough reason to not adopt it.

This comment has been minimized.

Copy link
@arnau

arnau Nov 13, 2018

Collaborator

What is the use case of cross-domain APIs in registers? As far as I can tell the Registers API is based on a single domain (and replicated to as many domains as registers exist).

Interlinking is outside of the actual REST API, done mainly through the CURIE datatype which at this level of definition (HTTP) is treated as an opaque string.

This comment has been minimized.

Copy link
@arnau

arnau Nov 13, 2018

Collaborator

To clarify on the schema part, OpenAPI has a set of datatypes and validation rules that, at the moment are incompatible with the Registers type system. Friction could be reduced by providing custom datatype formats but it would fail to express properly the flexibility of certain types we have in the registers type system.

For context, a similar issue exists with CSVW, and a similar partial solution can be applied.

My question would be: What is the purpose of providing OpenAPI? machine readable API definition? autogenerated documentation? schema validation reusing tooling from OpenAPI? All of them? Others?

This comment has been minimized.

Copy link
@MatMoore

MatMoore Nov 13, 2018

Author

My question would be: What is the purpose of providing OpenAPI? machine readable API definition? autogenerated documentation? schema validation reusing tooling from OpenAPI? All of them? Others?

If we ignore our own needs for registers.service.gov.uk, I think the main benefit is the option of providing autogenerated documentation out of the box. Anyone who wants their own version of openregister-java needs to host documentation along with it. Things like code generation are a bonus, but it's hard to say whether anyone would use it for that.

The schema validation is more of a convenience for us - if the RFC is adopted I would swap out the JSON schema for the OpenAPI schema in the conformance tests, but the tests would do the same thing either way.

This comment has been minimized.

Copy link
@MatMoore

MatMoore Nov 13, 2018

Author

To clarify on the schema part, OpenAPI has a set of datatypes and validation rules that, at the moment are incompatible with the Registers type system. Friction could be reduced by providing custom datatype formats but it would fail to express properly the flexibility of certain types we have in the registers type system.

For context, a similar issue exists with CSVW, and a similar partial solution can be applied.

This same issue already exists with the JSON schema we're using in the conformance tests. We treat blobs as arbitrary objects, which means we would need extra code in our python tests to validate them properly (we should probably do this regardless of whether this RFC is accepted).

I'm ok with a partial solution here, because I don't think it prevents autogenerated documentation or client libraries from being useful.

This comment has been minimized.

Copy link
@MatMoore

MatMoore Nov 13, 2018

Author

What is the use case of cross-domain APIs in registers? As far as I can tell the Registers API is based on a single domain (and replicated to as many domains as registers exist).

Interlinking is outside of the actual REST API, done mainly through the CURIE datatype which at this level of definition (HTTP) is treated as an opaque string.

Yeah, I am happy to treat it opaquely as well. As well as CURIEs in the register, we might consider adding links to datatypes in the metadata schema but I can't think of other cross-domain linking at the moment.

Ideally I'd also like a machine readable spec to cover pagination via link headers. I haven't looked into yet but I expect that's an area where we may run into the limits of what OpenAPI can express.

This comment has been minimized.

Copy link
@arnau

arnau Nov 13, 2018

Collaborator

I would like the RFC to be more precise on what do we expect out of OpenAPI, highlighting the friction points we identified in this thread. With that, I'm happy with the approach.

@MatMoore

This comment has been minimized.

Copy link
Author

MatMoore commented Nov 13, 2018

Thanks @bahmady. Let me know on slack if there's a good time to chat about this. I'll probably start by converting the JSON schema we have to an OpenAPI 3 spec and see what problems I run into.

@arnau arnau added the rfc label Nov 14, 2018

@arnau arnau changed the title Add RFC for adopting OpenAPI RFC0026: Adopting OpenAPI Nov 14, 2018

MatMoore added a commit to alphagov/registers-tech-docs that referenced this pull request Dec 11, 2018

Document new /blobs endpoint
I've also restructured this page to work better with the navigation
sidebar.

The intention is that when we release this new API version,
we will rename this thing from "Upgrading from v1 to next" to
"Upgrading from v1 to v2", and add sections as needed when
creating new versions.

At some point we will also need to create a page for `GET /blobs`
but in order to do this we need a way of structuring content by
version number. How we do this depends on whether we accept
openregister/registers-rfcs#43 and
just autogenerate the reference documentation.

MatMoore added a commit to alphagov/registers-tech-docs that referenced this pull request Dec 12, 2018

Document new /blobs endpoint
I've also restructured this page to work better with the navigation
sidebar.

The intention is that when we release this new API version,
we will rename this thing from "Upgrading from v1 to next" to
"Upgrading from v1 to v2", and add sections as needed when
creating new versions.

At some point we will also need to create a page for `GET /blobs`
but in order to do this we need a way of structuring content by
version number. How we do this depends on whether we accept
openregister/registers-rfcs#43 and
just autogenerate the reference documentation.

@MatMoore MatMoore assigned MatMoore and gidsg and unassigned MatMoore Dec 21, 2018

@MatMoore

This comment has been minimized.

Copy link
Author

MatMoore commented Dec 21, 2018

I'm going to hand this over to @gidsg as I didn't have time to get this in a good state before my last day at GDS (sorry!)

MatMoore added a commit to alphagov/registers-tech-docs that referenced this pull request Dec 21, 2018

Document new /blobs endpoint
I've also restructured this page to work better with the navigation
sidebar.

The intention is that when we release this new API version,
we will rename this thing from "Upgrading from v1 to next" to
"Upgrading from v1 to v2", and add sections as needed when
creating new versions.

At some point we will also need to create a page for `GET /blobs`
but in order to do this we need a way of structuring content by
version number. How we do this depends on whether we accept
openregister/registers-rfcs#43 and
just autogenerate the reference documentation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.