GTD-2: API reference improvements #48
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…to a reuseable private method.
…operation parameters
…ocs-gem into json-response-output
|
@MatMoore tagging you to review as well |
|
I haven't done a full review yet, but I've tested this branch and found a couple of bugs related to the examples. I'll list them here but let me know if it's easier to report these offline. A response without a schema cannot be renderedTested with https://github.com/OAI/OpenAPI-Specification/blob/master/examples/v3.0/api-with-examples.yaml As I understand it, this is not the specific kind of example you implemented to make the pay spec work, but I would expect this spec to render, even if it doesn't show the examples. In this spec the responses contain examples, but don't contain schemas. When I tried to render this it threw this exception: Inconsistent markdown renderingThe spec says "Throughout the specification description fields are noted as supporting CommonMark markdown formatting. Where OpenAPI tooling renders rich text it MUST support, at a minimum, markdown syntax as described by CommonMark 0.27. Tooling MAY choose to ignore some CommonMark features to address security concerns." However I noticed that the content api spec (after making the changes suggested in alphagov/content-store#460) does not render some markdown in descriptions.
We do render some markdown though, I noticed that in the pay spec Incorrect examples and/or schema namesThis may be a combination of multiple things. I've noticed in several schemas that examples don't render properly and/or the documentation shows a JSON path as the schema name:
It might be something to do with one of these things:
Schemas not renderingIn the content performance manager spec (see above) I also got entire schemas not rendering. I suspect this is due to using For example: TimeSeriesResponse:
description: "An object where each key is a metric ID and each value is a time series."
type: object
additionalProperties:
$ref: '#/components/schemas/TimeSeries'This currently renders using widdershins (https://content-performance-api.publishing.service.gov.uk/reference.html#timeseriesresponse) so we should be able to get similar results from this implementation. |
MatMoore
approved these changes
Sep 3, 2018
There was a problem hiding this comment.
I've had a look through the commits and just have a couple of minor comments about the code.
My main concern is that the rendering of schema objects and example responses doesn't handle some of the other specs we need to be able to render (see above).
That said, I'd prefer to merge this PR as is and then tackle those bugs than do everything as part of one PR, as this work fixes a bunch of issues on master.
| @@ -106,6 +106,14 @@ def schemas_from_schema(schema) | |||
| if schema.type == 'array' | |||
| properties.push schema.items | |||
| end | |||
| allOf = schema["allOf"] | |||
There was a problem hiding this comment.
Minor thing but allOf is camel case here whereas the convention is to use snake case.
Also, do you think it would make sense to handle anyOf/oneOf in the same way as this? Ideally the design of the template should make the semantics clear, but I'm wondering if just showing all the properties is better than doing nothing for these.
| properties = schema.properties | ||
| properties.each do |key, property| | ||
| properties = [] | ||
| schema.properties.each do |property| |
There was a problem hiding this comment.
is schema.properties a hash? If so you can just do properties = schema.properties.values.
|
Just starting to look over. I'm sure you noticed that the build failed due to some rubocop failures. Running the specs locally I also got one spec failure: haven't dug into it |
|
I've been trying to get the pay oais3 spec to parse and getting these errors: |
|
@mikebell Is this the same validation error that you encountered before? |
|
I'd suggest trying it with Ruby 2.5.1 and seeing if that works. I've not seen that specific error before I'm afraid. |
|
Got the same error with 2.5.1 |
|
Try using this attached version. It fixes some validation issues with $ref that I found. |
|
priorities:
we'd expect to see something like: {
"results": [
{
"amount": 1200,
"state": {
"status": "created",
"finished": true,
"message": "User cancelled the payment",
"code": "P010"
},
"description": "Your Service Description",
"reference": "your-reference",
"email": "your email",
"payment_id": "hu20sqlact5260q2nanm0q8u93",
"payment_provider": "worldpay",
"return_url": "http://your.service.domain/your-reference",
"created_date": "2016-01-21T17:15:00Z",
"refund_summary": {
"status": "available",
"amount_available": 0,
"amount_submitted": 0
},
"settlement_summary": {
"capture_submit_time": "2016-01-21T17:15:00Z",
"captured_date": "2016-01-21"
},
"card_details": {
"last_digits_card_number": "1234",
"cardholder_name": "Mr. Card holder",
"expiry_date": "12/20",
"billing_address": {
"line1": "address line 1",
"line2": "address line 2",
"postcode": "AB1 2CD",
"city": "address city",
"country": "UK"
},
"card_brand": "Visa"
},
"card_brand": "Visa"
}
]
}but actually get
|
… an array of [title, object] arrays
lewisnyman
commented
Sep 7, 2018
| @@ -6,3 +6,6 @@ Naming/HeredocDelimiterNaming: | |||
|
|
|||
| Lint/NestedMethodDefinition: | |||
| Enabled: false | |||
|
|
|||
| Performance/HashEachMethods: | |||
There was a problem hiding this comment.
Most objects constructed by openapi3_parser support .each but not .each_value but Rubocop treats it as a hash.
There was a problem hiding this comment.
Fair enough. Makes me wonder if we should add #each_value to the parser at some point though (cc @kevindew)
|
Tests should be passing now. It would be good to have more test coverage, including the request bodies, but the data structure of the pets.yml schema array are different to other files, and will need more work to get them rendering properly as well (see the code below). Ultimately it would be great to include pets-extended.yml with wider coverage of supported features. pets.yml Pets:
type: array
items:
$ref: "#/components/schemas/Pet" |
|
Thanks @lewisnyman. I think the test coverage is something we can look at at a later date - I think it would definitely be helpful for the example generation given the complexity involved there. @heathd if it's ok with you, I'd like to merge this and release a new version. It looks like some of the issues we raised earlier are now fixed, but I can raise github issues for anything that's left over and document any workarounds in the changelog. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The user-facing changes in this PR:
Linting and updated tests will come later.