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

Produce OpenAPI document describing CCF's endpoints #1612

Merged
merged 64 commits into from
Sep 25, 2020

Conversation

eddyashton
Copy link
Member

Had this on the side for a while, finally got it into a place where (I think) it mostly works. Opening as a draft PR to see what's still missing, includes a few TODOs.

The core change is that the /api endpoint now returns an OpenAPI document, rather than just a list of paths. This involves some changes to our JSON schema-generating macros, to populate an OpenAPI document rather than returning isolated elements. For full customisability (ie - so the apps can, if they wish, store a separate document or elements and merge them in), we manipulate the OpenAPI document as a raw JSON document, with some helper functions to try and put things in the right place. This is more error prone than a strongly typed wrapper, but I think the flexibility is necessary.

There are 2 test for this. There's an openapi_test unit test, which really only tests compilation of the helper functions + custom types, and does minimal validation that the result looks like a plausible document. Then the existing schema.py e2e test now uses a 3rd party OpenAPI validation library to check that the produced documents are valid OpenAPI. This seems unhappy with some things the online validator was happy with ($schema elements in particular), so I've removed a few useful-but-non-essential features to support it. This test also dumps the documents like it does the individual schema responses, and I've been running those documents through the online swagger validator - this is almost happy, but doesn't like nlohmann::json's "maps are lists of tuples" representations, as OpenAPI schema breaks from JSON schema in disallowing tuples.

Copy link
Member

@achamayou achamayou left a comment

Choose a reason for hiding this comment

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

Very nice!

@eddyashton
Copy link
Member Author

@eddyashton I was under the impression that OpenAPI supported a strict superset of recent JSON schema, since OAI/OpenAPI-Specification#1977

I believe that's the case for v3.1, which is still pre-release/RC. The part we want that isn't supported is described in the current v3.0 spec here: "items - Value MUST be an object and not an array". Our items are sometimes arrays of schemas, to describe a tuple. The equivalent part of the v3.1 spec is far more concise, claiming to be a clear superset, so hopefully it should be supported once that is standard (and the relevant tools support it).

@achamayou
Copy link
Member

@eddyashton do we want to target that directly?

scripts/ci-checks.sh Outdated Show resolved Hide resolved
src/ds/openapi.h Outdated Show resolved Hide resolved
@eddyashton
Copy link
Member Author

@eddyashton do we want to target that directly?

I don't think we should target it while its pre-release, and the validators (even SwaggerHub) don't currently support 3.1. I assume an official release is imminent (its been RC for several months), but can't find a date. Once that's done, we may be waiting for an update to the Python validator package before we can move.

@eddyashton eddyashton changed the title DRAFT: Produce OpenAPI document describing CCF's endpoints Produce OpenAPI document describing CCF's endpoints Sep 24, 2020
@eddyashton eddyashton marked this pull request as ready for review September 24, 2020 09:18
@eddyashton eddyashton requested a review from a team as a code owner September 24, 2020 09:18
@eddyashton eddyashton merged commit 37d78ec into microsoft:master Sep 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants