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

OpenAPI specification file with header markdown errors on site load #156

Closed
adampridmore opened this issue Jun 29, 2021 · 1 comment
Closed

Comments

@adampridmore
Copy link

If you use an OpenAPI specification file in a tech doc and use a markdown header in a 'description' field, it errors when the site loads.

Error:

TypeError: no implicit conversion of nil into String
        /usr/local/lib/ruby/gems/2.7.0/gems/govuk_tech_docs-2.3.0/lib/govuk_tech_docs/table_of_contents/heading.rb:16:in `+'
        /usr/local/lib/ruby/gems/2.7.0/gems/govuk_tech_docs-2.3.0/lib/govuk_tech_docs/table_of_contents/heading.rb:16:in `href'
        /usr/local/lib/ruby/gems/2.7.0/gems/govuk_tech_docs-2.3.0/lib/govuk_tech_docs/table_of_contents/heading_tree_renderer.rb:23:in `render_tree'
        /usr/local/lib/ruby/gems/2.7.0/gems/govuk_tech_docs-2.3.0/lib/govuk_tech_docs/table_of_contents/heading_tree_renderer.rb:31:in `block in render_tree'
...

Example OAS file:

openapi: 3.0.0
info:
  title: my title
  version: "1.0"
  description: "# My markdown heading"
paths:
  /my-path:
    get:
      responses:
        '200':
          description: OK
          content:
            text/plain:
              schema:
                type: string

Note: If you remove the # prefix in the description value above the site load correctly.

jamietanna added a commit to jamietanna/tech-docs-gem that referenced this issue Nov 10, 2021
As noted in [0], the ability to render a CommonMark document through
OpenAPI specifications' `info.description` is breaking due to how the
Table of Contents code works.

We'd assumed that the `id` would always be set on headers - by Middleman
- but as it's possible for an OpenAPI description to include headers, we
won't always have one.

To solve this, we can make it possible for a link to not be present for
a heading, instead returning just a `<span> for the table of contents.

This allows the headings to still be present, but won't be actionable.

Until the `openapi3_parser` gem supports making this configurable, i.e.
be allowing us to inject in headers, this is a good middle ground.

[0]: alphagov/tdt-documentation#156
jamietanna added a commit to jamietanna/tech-docs-gem that referenced this issue Nov 10, 2021
As noted in [0], the ability to render a CommonMark document through
OpenAPI specifications' `info.description` is breaking due to how the
Table of Contents code works.

We'd assumed that the `id` would always be set on headers - by Middleman
- but as it's possible for an OpenAPI description to include headers, we
won't always have one.

To solve this, we can make it possible for a link to not be present for
a heading, instead returning just a `<span> for the table of contents.

This allows the headings to still be present, but won't be actionable.

Until the `openapi3_parser` gem supports making this configurable, i.e.
be allowing us to inject in headers, this is a good middle ground.

[0]: alphagov/tdt-documentation#156
jamietanna added a commit to jamietanna/tech-docs-gem that referenced this issue Nov 12, 2021
As noted in [0], the ability to render a CommonMark document through
OpenAPI specifications' `info.description` is breaking due to how the
Table of Contents code works.

We'd assumed that the `id` would always be set on headers - by Middleman
- but as it's possible for an OpenAPI description to include headers, we
won't always have one.

Until the `openapi3_parser` gem supports making this configurable, which
is unlikely, due to the way that CommonMark doesn't have this as a
default extension, we can instead utilise our `TechDocsHTMLRenderer` to
render the headings with IDs, as expected.

To do this, we can create a new helper in `ApiReferenceRenderer` which
will render the Markdown document as our custom Markdown, which requires
we construct the `TechDocsHTMLRenderer` class, which needs the Middleman
`ConfigContext` class injected in.

This means that we won't be using CommonMark, as per the OpenAPI spec,
and this is something we'll look to address in alphagov#282.

[0]: alphagov/tdt-documentation#156
jamietanna added a commit to jamietanna/tech-docs-gem that referenced this issue Nov 12, 2021
As noted in [0], the ability to render a CommonMark document through
OpenAPI specifications' `info.description` is breaking due to how the
Table of Contents code works.

We'd assumed that the `id` would always be set on headers - by the
`TechDocsHTMLRenderer` - but as we've been rendering the markdown
through `openapi3_parser`'s CommonMark implementation, these IDs are not
being populated.

Until the `openapi3_parser` gem supports making this configurable, which
is unlikely, due to the way that CommonMark doesn't have this as a
default extension, we can instead utilise our `TechDocsHTMLRenderer` to
render the headings with IDs, as expected.

To do this, we can create a new helper in `ApiReferenceRenderer` which
will render the Markdown document as our custom Markdown, which requires
we construct the `TechDocsHTMLRenderer` class, which needs the Middleman
`ConfigContext` class injected in.

As well as the original `info.description` that we'd planned to amend,
we can also replace any other CommonMark->HTML rendering at the same
time.

This means that we won't be using CommonMark, as per the OpenAPI spec,
and this is something we'll look to address in alphagov#282.

The setup for our tests are now a little more complex, as we're not able
to (easily) inject `RedCarpet`, so need to mock a bit more to make it
work.

[0]: alphagov/tdt-documentation#156
@m-green
Copy link
Contributor

m-green commented Nov 18, 2021

This is now fixed in alphagov/tech-docs-gem#281. Thanks for raising this with us @adampridmore.

@m-green m-green closed this as completed Nov 18, 2021
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

No branches or pull requests

2 participants