Update openapi_parser #125

kevindew · 2019-10-14T14:07:03Z

Following up from #121 this updates openapi_parser to allow the latest version of gem. Since the gem is below version 1 it seemed safe to maintain the pin.

I've also done some cleaning up of some of the Ruby code in GovukTechDocs::ApiReference::Renderer to make it more shorter and more consistent with modern Ruby. There's a few dubious things in there that I left as I didn't want to risk creating fresh bugs from "fixing" stuff.

I've also rebased #113 onto this as that fix would conflict with the changes here.

Edit:

Since opening this @tijmenb closed #113 so the link to that is no longer accurate. I didn't want to re-instate a bug by undoing that so wanted to keep that change here and had already amended his change to use a new method in openapi_parser to check required status.

To document the issue, an OpenAPI document schema object has a required property which must be an array of strings which is used to indicate what properties of that object are required.

Previously the schema template just checked whether that array was set to determine if a parameter was required:

tech-docs-gem/lib/govuk_tech_docs/api_reference/templates/schema.html.erb

Line 13 in a2ba614

This is incorrect as this only tells you whether that parameter requires properties of its own.

Instead we use the #requires? method on the schema to determine if this property is required.

ChrisBAshton

This looks like a fabulous improvement 🎉

ChrisBAshton · 2019-10-15T07:46:46Z

lib/govuk_tech_docs/api_reference/api_reference_renderer.rb

-          if !schema_name.nil?
-            schemas.push schema_name
+          if property.name
+            schemas.push property.name


ChrisBAshton · 2019-10-15T07:54:03Z

lib/govuk_tech_docs/api_reference/api_reference_renderer.rb

-          # instead of an array of objects
-          text = path_data[0]
-          paths += path(text)
+        paths = @document.paths.keys.inject('') do |memo, text|


Coming from a non-Ruby background, inject threw me (I didn't realise it's just an alias for reduce).

Have tried searching online for what the Ruby convention is but it seems that isn't prescribed anywhere. But found a few articles in favour of using reduce, from notable people like Martin Fowler, who suggests it simply reads better.

If I suggested you change it to reduce, would that violate a ruby convention?

#inject pre-dates #reduce in Ruby so that seems the more idiomatic way to do it for me. I wouldn't suggest it needs changing here as we've never applied any preference in GDS code for one way or the other.

ChrisBAshton · 2019-10-15T08:03:49Z

lib/govuk_tech_docs/api_reference/api_reference_renderer.rb

+                           schema_properties(schema.items)
+                         else
+                           {}
+                         end


This block feels like it could be refactored - something like:

memo[name] = schema_properties(schema.items || schema) memo[name] = (schema.example || schema.type) unless memo[name]

But I might be missing a nuance there - and appreciate that you're concerned about introducing new bugs.

Thanks - I've not been very happy with my changes there. I don't dare quite go to the level you suggest here as there are a few more nuances. I've done a bit of refactoring to try make it all a bit easier to read though.

ChrisBAshton · 2019-10-15T08:04:10Z

lib/govuk_tech_docs/api_reference/api_reference_renderer.rb

+          'post' => path.post,
+          'delete' => path.delete,
+          'patch' => path.patch,
+        }.compact


ChrisBAshton · 2019-10-15T08:06:38Z

CHANGELOG.md

-## 2.0.5
+## Unreleased
+
+Fix bug in API docs that shows required attributes as optional (#113)


Code looks good in this commit - but can you fix the URL in the commit message so that it's not split onto multiple lines?

Sure I've fixed that.

Think we ought to remove this line from this commit now that it's been closed without merging. (Should mean the last commit isn't needed either).

tijmenb · 2019-10-17T10:58:43Z

CHANGELOG.md

-## 2.0.5
+## Unreleased
+
+Fix bug in API docs that shows required attributes as optional (#113)


Sorry @kevindew, closed #113 because I had misunderstood the relationship between JSON Schema and OpenAPI.

@tijmenb oh interesting - what was your concern there? I thought your logic was sound and the behaviour in current tech docs release is wrong.

Great to hear you're planning to push up a bunch of improvements for the API docs 👍 let me know if I can help at all

kevindew · 2019-11-14T09:50:29Z

@ChrisBAshton Thanks for the review and apologies in taking so long to get back to this. I've fixed up various bits noted.

ChrisBAshton

The angry seal reminded me about this PR - have given it a re-review. Looks good to go apart from outdated references to Tijmen's (closed) PR 👍

ChrisBAshton · 2019-12-09T08:49:42Z

lib/govuk_tech_docs/api_reference/templates/schema.html.erb

@@ -11,7 +11,7 @@
 <td><%= property[0] %></td>
 <td><%= property[1].type %></td>
 <td><%= property[1].required.present? %></td>
-<td><%= markdown(property[1].description) %></td>
+<td><%= property[1].description_html %></td>


While you're here, could you apply the same changes to this file as you did in the "Rename some variables for clarity" commit?

I don't think it's necessary to do a change here as it's done in Tijmen's commit which is next.

ChrisBAshton · 2019-12-09T08:52:31Z

CHANGELOG.md

-## 2.0.5
+## Unreleased
+
+Fix bug in API docs that shows required attributes as optional (#113)


Think we ought to remove this line from this commit now that it's been closed without merging. (Should mean the last commit isn't needed either).

By using the name property on a Schema these look ups via node_context can be removed.

The code in this file didn't appear to be very consistent with modern Ruby patterns so I've given it something of a spruce up. I was super nervous about the risk of introducing bugs or changing relied upon quirks so I set myself a couple of rules: try to keep the API identical and try keep the behaviour identical. There are a bunch of dubious things going on in this code that does make me a bit concerned for it's long term usage. I hope in doing this it makes this code a little more inviting for the next person to come a long and try make it better.

There are helper methods available to do this in the gem so this renderer doesn't need it's own markdown conversion system.

kevindew · 2021-03-15T10:26:22Z

@ChrisBAshton about 1 million years later, but this is good for a re-review if you don't mind? 🙏

ChrisBAshton

LGTM 👍 some non-blocking comments below.

ChrisBAshton · 2021-03-15T17:01:26Z

lib/govuk_tech_docs/api_reference/templates/schema.html.erb

@@ -6,18 +6,18 @@
 <tr><th>Name</th><th>Type</th><th>Required</th><th>Description</th><th>Schema</th></tr>
 </thead>
 <tbody>
-<% properties.each do |property| %>
+<% properties.each do |property_name, property_attributes| %>


Minor: the commit message confusingly refers to a attribute_name, I assume it should refer to property_name instead.

Sure - I've changed it (felt a bit weird editing someone else's commit)

ChrisBAshton · 2021-03-15T17:04:21Z

spec/features/api_reference_spec.rb

@@ -55,5 +55,7 @@ def then_there_is_correct_api_schema_content
    expect(page).to have_css("h3#schema-pet", text: "Pet")
    # Schema parameters
    expect(page).to have_css("table", text: /\b(tag )\b/)
+    # Check that the "required" column is true for the `id` attribute
+    expect(page).to have_css("table.schema-pet td:nth(3)", text: "true")


Would be great if this test were a bit more self-documenting - I wonder if we can dynamically infer the correct column number? I appreciate that introduces some complexity to the test, so may not be an improvement.

I don't really want to edit someone else's code there, nor particularly going into the world of testing for the API stuff. It seems a big can of worms as there is a lack of testing for it all.

ChrisBAshton · 2021-03-15T17:04:53Z

lib/govuk_tech_docs/api_reference/templates/api_reference_full.html.erb

@@ -1,7 +1,8 @@
 <h1 id="<%=  info.title.parameterize %>"><%= info.title %> v<%= info.version %></h1>
 <%= info.description_html %>

-<% unless servers.empty? %>
+<%# OpenAPI files default to having a single server of URL "/" %>


Suggested change

<%# OpenAPI files default to having a single server of URL "/" %>

<% # OpenAPI files default to having a single server of URL "/" %>

<%# is actually erb syntax for a comment and means something different to <% #: https://docs.ruby-lang.org/en/2.3.0/ERB.html#class-ERB-label-Recognized+Tags

Ooh, TIL! 👀

I'm trying to fix a bug, but I keep getting confused by the variables in this each loop. This commit refactors it so that we can refer to `property_name` and `property_attributes` (which is an object that we get from OpenApi3Parser).

This commit fixes a bug OpenAPI reference renderer that affects which attributes of a schema (https://swagger.io/docs/specification/data-models/) are considered to be required. For the uninitiated, schemas allow you to define what objects/resources/models your API will return. It uses JSON Schema (https://json-schema.org/) for this. For example: ```yml components: schemas: Pet: required: - id - name properties: id: type: integer format: int64 name: type: string tag: type: string ``` This means that any `Pet` objects will have at least an id and a name, and an optional `tag`. Currently the logic to display which attributes are required is wrong, because the code expects something like: ```yml id: type: integer format: int64 required: true ``` This isn't valid JSON Schema as far as I can see: https://json-schema.org/understanding-json-schema/reference/object.html#required-properties This commit fixes the bug by making the check look at the `required` array in the schema, rather than the property.

The OpenAPI specification dictates that that an empty or missing server object should default to a server object of one item with a URL of "/" [1]. This is reflected in 0.8 of openapi3_parser so the code here is adapted to reflect that. [1]: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#openapi-object

0.8 is no longer the most recent release.

ChrisBAshton reviewed Oct 15, 2019

View reviewed changes

tijmenb reviewed Oct 17, 2019

View reviewed changes

kevindew force-pushed the use-openapi-parser-0.7 branch 3 times, most recently from 81f3dc3 to bdc50e0 Compare November 14, 2019 09:49

ChrisBAshton reviewed Dec 9, 2019

View reviewed changes

kevindew force-pushed the use-openapi-parser-0.7 branch from bdc50e0 to 21e765c Compare February 3, 2020 23:35

cdccollins mentioned this pull request Sep 22, 2020

Upgrade to ruby 2.7.1 alphagov/govuk-developer-docs#2786

Merged

kevindew added 4 commits March 13, 2021 17:27

Upgrade to openapi3_parser 0.8.0

4e8485a

Resolve immediate problems with Openapi3Parser

d03c21d

By using the name property on a Schema these look ups via node_context can be removed.

Use openapi3_parser to render markdown

22fb45b

There are helper methods available to do this in the gem so this renderer doesn't need it's own markdown conversion system.

kevindew force-pushed the use-openapi-parser-0.7 branch from 21e765c to 7b22d30 Compare March 13, 2021 17:39

kevindew mentioned this pull request Mar 15, 2021

Remove unnecessary explicit dependencies: sprockets, activesupport, sass and pry #218

Merged

ChrisBAshton approved these changes Mar 15, 2021

View reviewed changes

tijmenb and others added 5 commits March 15, 2021 17:27

Rename some variables for clarity

b587ae5

I'm trying to fix a bug, but I keep getting confused by the variables in this each loop. This commit refactors it so that we can refer to `property_name` and `property_attributes` (which is an object that we get from OpenApi3Parser).

Bump openapi3_parser to 0.9.0

4b08657

0.8 is no longer the most recent release.

Update changelog

569b8c5

kevindew force-pushed the use-openapi-parser-0.7 branch from 7b22d30 to 569b8c5 Compare March 15, 2021 17:27

kevindew merged commit 557846d into master Mar 15, 2021

kevindew deleted the use-openapi-parser-0.7 branch March 15, 2021 18:00

This was referenced Mar 16, 2021

BAU: Bump govuk_tech_docs from 2.1.0 to 2.2.1 alphagov-mirror/dcs-pilot-docs#15

Closed

Bump govuk_tech_docs from 2.0.11 to 2.2.1 alphagov/govuk-developer-docs#3013

Merged

Bump govuk_tech_docs from 2.0.11 to 2.2.1 alphagov-mirror/govuk-developer-docs#21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update openapi_parser #125

Update openapi_parser #125

kevindew commented Oct 14, 2019 •

edited

Loading

ChrisBAshton left a comment

ChrisBAshton Oct 15, 2019

ChrisBAshton Oct 15, 2019

kevindew Nov 14, 2019

ChrisBAshton Oct 15, 2019

kevindew Nov 14, 2019

ChrisBAshton Oct 15, 2019

ChrisBAshton Oct 15, 2019

kevindew Nov 14, 2019

ChrisBAshton Dec 9, 2019

tijmenb Oct 17, 2019

kevindew Oct 21, 2019

kevindew commented Nov 14, 2019

ChrisBAshton left a comment

ChrisBAshton Dec 9, 2019

kevindew Mar 15, 2021

ChrisBAshton Dec 9, 2019

kevindew commented Mar 15, 2021

ChrisBAshton left a comment

ChrisBAshton Mar 15, 2021

kevindew Mar 15, 2021

ChrisBAshton Mar 15, 2021

kevindew Mar 15, 2021

ChrisBAshton Mar 15, 2021

kevindew Mar 15, 2021

ChrisBAshton Mar 16, 2021

	<%# OpenAPI files default to having a single server of URL "/" %>
	<% # OpenAPI files default to having a single server of URL "/" %>

Update openapi_parser #125

Update openapi_parser #125

Conversation

kevindew commented Oct 14, 2019 • edited Loading

ChrisBAshton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindew commented Nov 14, 2019

ChrisBAshton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindew commented Mar 15, 2021

ChrisBAshton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevindew commented Oct 14, 2019 •

edited

Loading