Fix: Don't break TOC when OpenAPI description includes headers

[jamietanna]

Fix: Don't break TOC when OpenAPI description includes headers

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 #282.

---

Example rendering:

[jamietanna]

I've verified this works correctly by:

middleman init another-example -T alphagov/tech-docs-template

Then amending the Gemfile:

gem 'govuk_tech_docs', path: ".."

And pointing it to the OpenAPI document as mentioned in alphagov/tdt-documentation#156 (comment).

[lfdebrux]

Hi @jamietanna, nice work identifying the problem so fast! 🙌

Thanks for the proposed fix, the code changes look fine to me, but I am a bit worried about the user experience if there are situations where some headings have anchor links and some don't.

I was wondering if you'd managed to figure out why these headings don't get autogenerated anchor links like other headings?

[jamietanna]

Hey @lfdebrux, yes it's due to the OpenAPI document parser library that we use which, when rendering the document doesn't configure the Markdown generator to emit IDs.

This is different to the way that Middleman's markdown is generated, which does produce the IDs we need, and right now there's no way to do this.

I see our options as:

• Investigate further about how to do this (for instance using Monkey Patching)
• Leave it as-is, and follow-up when the upstream openapi3_parser library supports it
• Exclude headings in the TOC i.e. for anything not generated by Middleman

[lfdebrux]

@jamietanna so we don't do any post-processing of the markdown before inserting it into our documents? 😞

[kevindew]

Hello 👋 I'm the author of the gem and work at GDS. Happy to help try resolve this problem.

Am I understanding correctly that the core of this problem is that the tech docs gem expects every h1-6 to have an id attribute, however any markdown data from openapi_parser is rendered with h1-6 elements without ids?

E.g. You want, for a description of # test info.description_html to equal <h1 id="test">Test</h1> and not <h1>Test</h1>

I think the easiest way you can resolve that problem without upstream changes would be to switch from using the openapi3_parser markdown renderer (which use the *_html methods) and have this gem render the markdown directly.

So you could change say:

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

Line 2 in bc4bf6d

<%= info.description_html %>

to

<%= render_markdown(info.description) %> 

Where render_markdown is a helper function of:

def render_markdown(text)
  redcarpet = Redcarpet::Markdown.new(Redcarpet::Render::HTML.new(with_toc_data: false), {})
  redcarpet.render(text)
end

This would mean you get the ids as the with_toc_data flag turns that feature on in the redcarpet rendering library.

---

It'd be good to open an issue on the gem, https://github.com/kevindew/openapi3_parser, if this is a feature you expect of the gem. I'm not too sure how contentious header ids are as a Markdown feature (it doesn't seem to be a part of the CommonMarker library) - or if applying it to an OpenAPI doc may mean you risk duplicate ids.

[jamietanna]

Thanks Kev, that's really appreciated.

That is a good point, that gives us the functionality we want.

My only concern is, as far as I know, Redcarpet isn't a CommonMark compliant Markdown geenrator, so we risk consumers not being able to do certain things / having slightly different output than they're expecting.

If we're happy with that, I'm happy to amend it, and raise an issue on the repo for visibility?

Are we happy with going the approach of using Redcarpet?

OpenAPI3 expects the Markdown to be CommonMark, which I don't believe Redcarpet uses, so although we'd get the IDs, there's the risk that

[lfdebrux]

@kevindew thanks, that's really cool of you to see this and help out!

@jamietanna we have our own patched version of the middleman redcarpet generator that we might want to use? https://github.com/alphagov/tech-docs-gem/blob/master/lib/govuk_tech_docs/tech_docs_html_renderer.rb

In terms of CommonMark, I think users will expect to see the same behaviour whether writing doc pages or api specs?

[jamietanna]

Sounds like a plan re using our version 👍

That's fair - I think if it were me, I'd potentially be using this OpenAPI description with different tools (i.e. it may go onto the API Catalogue or other internal/external tooling, such as rendering inside tools like JIRA, or an IDE) and I'd be expecting a consistent formatting across all of those.

It's unlikely that many users will be affected, but the reason CommonMark became a thing is because so many Markdown flavours are inconsistent, so it's possible that people will be affected by quirks that RedCarpet has.

I agree that it'd be odd to write a doc page that doesn't render the same as the API docs, and to that I wonder if maybe we should look at using CommonMark everywhere?

[lfdebrux]

Sounds like a plan re using our version 👍

That's fair - I think if it were me, I'd potentially be using this OpenAPI description with different tools (i.e. it may go onto the API Catalogue or other internal/external tooling, such as rendering inside tools like JIRA, or an IDE) and I'd be expecting a consistent formatting across all of those.

It's unlikely that many users will be affected, but the reason CommonMark became a thing is because so many Markdown flavours are inconsistent, so it's possible that people will be affected by quirks that RedCarpet has.

I agree that it'd be odd to write a doc page that doesn't render the same as the API docs, and to that I wonder if maybe we should look at using CommonMark everywhere?

That's an interesting point, worth raising an issue about perhaps! We'd have to think carefully about backwards compatibility of course :/

[jamietanna]

Moving to draft as there's still a bit of work to be done 👍

[kevindew]

Thanks Kev, that's really appreciated.

That is a good point, that gives us the functionality we want.

My only concern is, as far as I know, Redcarpet isn't a CommonMark compliant Markdown geenrator, so we risk consumers not being able to do certain things / having slightly different output than they're expecting.

If we're happy with that, I'm happy to amend it, and raise an issue on the repo for visibility?

Are we happy with going the approach of using Redcarpet?

OpenAPI3 expects the Markdown to be CommonMark, which I don't believe Redcarpet uses, so although we'd get the IDs, there's the risk that

Yeah I get your concern there. The reason I use the commonmarker gem in openapi3_parser is because the OpenAPI spec specifies commonmark.

But it doesn't look like the header ids are a feature of a Commonmark (aside: I'm not sure how Github generate them, perhaps they have a post processer) so I think if that bonus feature of markdown is required you'd be best placed to use a different markdown generator. I imagine the amount of headaches you'd get from writing your own post-processor to add ids to Commonmark generated markdown would be much higher than any headaches you'd have of redcarpet spec differences (which I imagine is pretty small, without any significant research)

[jamietanna]

Thanks folks, that worked quite nicely, once I managed to inject the render correctly 👍

[jamietanna]

I believe GitHub gets around it by not being CommonMark, but instead using GitHub Flavoured Markdown?

[jamietanna]

Sorry, had missed that there were unit tests for this class - I'll sort them, and then put this back out for review 👍

[lfdebrux]

Thanks @jamietanna! Once tests are passing I will take a look, I appreciate you taking the time for this!

[kevindew]

I believe GitHub gets around it by not being CommonMark, but instead using GitHub Flavoured Markdown?

Interestingly commonmarker uses Github Flavoured Markdown (https://github.com/github/cmark-gfm) but the header ids aren't a part of the spec: https://github.github.com/gfm/

[lfdebrux]

Code changes look good, and it works for me! 👍

Thanks again @jamietanna (and @kevindew for the assist).

[m-green]

LGTM! Thanks for all the hard work on this @jamietanna and everyone.

[lfdebrux]

I'm just going to push a change to the release notes and then will merge.