Ft/3832 add switches for disabling rendering of large payloads

[kristiansamuelsson]

Description

Adds two configuration options to control if/how payloads gets rendered, depending on the size of the payload.

1. Disable rendering of payloads with size above X (new config option renderSizeThreshold)
2. Disable pretty-printing / syntax highlighting of payloads with size above Y (new config option syntaxHighlight.sizeThreshold)

Motivation and Context

If a response is too large, the UI hangs for a very long time - sometimes crashes. This is discussed in both #3832 and #4018. ~I am not entirely sure if these options counts as solving those issues, but at least it provides a way of letting users download large responses without waiting for the UI to render, and avoids crashes of even larger responses.~ Edit (see comments below): Fixes #3832 Fixes #4018

How Has This Been Tested?

Added unit tests to cover the new functionality. Also tested on a simple service running locally, which allowed me to control the size of the response. I configured renderSizeThreshold to 6000000 (~6MB) and syntaxHighlight.sizeThreshold to 1000000 (~1MB). I could see that responses with size just under 6MB were very slow, and responses just over 6MB were very quick (but response is not displayed). Responses just under 1MB were very slow, and responses just over 1MB significantly quicker.

Screenshots (if appropriate):

Checklist

My PR contains...

• [ ] No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
• [ ] Dependency changes (any modification to dependencies in package.json)
• [ ] Bug fixes (non-breaking change which fixes an issue)
• [x] Improvements (misc. changes to existing features)
• [x] Features (non-breaking change which adds functionality)

My changes...

• [ ] are breaking changes to a public API (config options, System API, major UI change, etc).
• [ ] are breaking changes to a private API (Redux, component props, utility functions, etc.).
• [ ] are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
• [x] are not breaking changes.

Documentation

• [ ] My changes do not require a change to the project documentation.
• [x] My changes require a change to the project documentation.
• [x] If yes to above: I have updated the documentation accordingly.

Automated tests

• [ ] My changes can not or do not need to be tested.
• [x] My changes can and should be tested by unit and/or integration tests.
• [x] If yes to above: I have added tests to cover my changes.
• [x] If yes to above: I have taken care to cover edge cases in my tests.
• [x] All new and existing tests passed.

[guillaume-alvarez]

Thanks! Would it be possible to activate these by default?

[heldersepu]

Thanks! Would it be possible to activate these by default?

~~You are rocking the boat too much!~~
~~My vote ... just go as is, the issue this fixes has been open for years (since 2017) evidently not a big issue for most~~

EDIT: I take it back, yes we should have it active by default, but the value should be "high" enough not to cause any disruptions to existing users, a few seconds of rendering should be fine for most ... I would say whatever size is the equivalent of 5 seconds wait on a modern browser.

I found one of my old comments in the topic: https://github.com/swagger-api/swagger-ui/issues/3832#issuecomment-340222673

a quick fix could be not to display such a large responses, instead show something like github does on diff: Large diffs are not rendered by default.

if github has a max so should swagger-ui

maybe this is a good start: https://github.blog/2016-12-06-how-we-made-diff-pages-3x-faster/#historical-approach-and-problems

[heldersepu]

This is discussed in both #3832 and #4018. I am not entirely sure if these options counts as solving those issues, but at least ...

Yes I would count these options as solving those issues and close them as soon as this PR is merged.

[kristiansamuelsson]

Thanks! Would it be possible to activate these by default?

[...] if github has a max so should swagger-ui [...]

I considered setting some default value, but decided against it because I thought it would (technically) count as a breaking change. Should we consider it a bug fix/feature, even though someone might be used to getting pretty print after a long wait time?

[guillaume-alvarez]

I considered setting some default value, but decided against it because I thought it would (technically) count as a breaking change. Should we consider it a bug fix/feature, even though someone might be used to getting pretty print after a long wait time?

After the long wait time the pretty print is not usable in most browsers, often preventing proper page display, so I'd say it would count as a bugfix.

Not displaying the document at all may be a breaking change though.

[heldersepu]

Should we consider it a bug fix/feature

Yes I think so, this can be considered a bug fix, most reported cases the browser stops responding

[heldersepu]

Seems no one from the team is looking at these PR to give feedback... In order to speed up releasing this feature I would go as is and if someone in the future request default it can be added in a separate PR

[heldersepu]

@char0n can we get a review on this PR?

[char0n]

Hi @heldersepu,

Yes, @glowcloud will take over this one.

I have a small issue with naming:

renderSizeThreshold

This doesn't tell us what this is related to. I would recommend going for payload.render.sizeThreshold which will make it consistent with syntaxHighlight.sizeThreshold.

What do you think?

[heldersepu]

Hello @char0n ... Yes I agree with the naming convention

@kristiansamuelsson this is your PR you are almost there ... If I don't hear from you I will try make the changes this weekend

[kristiansamuelsson]

@heldersepu I'll try to take a look in the next couple of days. If not, then I'll be gone for vacation and back around the 18th and can sort it out then.

[kristiansamuelsson]

I have updated the config name based on the suggestion from @char0n

[heldersepu]

@kristiansamuelsson ... I see you fixed a bug in the documentation: https://github.com/swagger-api/swagger-ui/pull/9625/files#diff-051010b9d725a9cacb58a6cad529133bacf2f392d3f8643a2c8c40bc99750a97R229 syntaxHighlight.activated it was missing the d nice catch !

I was just testing that and I don't think that parameter ever worked correctly... Here are some that I tested:

• https://petstore.swagger.io/?filter=pet&defaultModelsExpandDepth=-1&docExpansion=None&syntaxHighlight.activated=false#/pet/findPetsByStatus
• https://petstore.swagger.io/?filter=pet&defaultModelsExpandDepth=-1&docExpansion=None&syntaxHighlight.activated=0#/pet/findPetsByStatus
• https://petstore.swagger.io/?filter=pet&defaultModelsExpandDepth=-1&docExpansion=None&syntaxHighlight=false#/pet/findPetsByStatus

We can see other parameter are working fine, but not that syntaxHighlight ...

According to the documentation: https://github.com/swagger-api/swagger-ui/blob/master/docs/usage/configuration.md#display

syntaxHighlight - Set to false to deactivate syntax highlighting of payloads and cURL command, can be otherwise an object with the activate and theme properties.

that did not work for me... maybe a new bug?!? @glowcloud have you ever tried that parameter from the url like that?

[glowcloud]

I can confirm that changing that parameter in settings is fine but it doesn't seem to work in the URL. Indeed, it looks like a new issue.

Created a separate issue for it: https://github.com/swagger-api/swagger-ui/issues/9674

[glowcloud]

Adding some thoughts/questions for this PR:

The change to HighlightCode also makes it so that large example values won't be rendered, here's an example with the default Petstore and very low threshold, just for testing it:

should we perhaps make these values also available for download to the users, just in case if we're not rendering them?

And the other question: if we're limiting rendering of the examples, should we also limit cURL and try it out editor, or is it unnecessary in their cases?

[char0n]

should we perhaps make these values also available for download to the users, just in case if we're not rendering them?

I would say yes, but for that we can create another issue which will be an enhancement to this one. The primary goal here not the crash the UI

And the other question: if we're limiting rendering of the examples, should we also limit cURL and try it out editor, or is it unnecessary in their cases?

Depends - does it crash the UI in the same way as rendering the example?

[heldersepu]

I love the lively discussion! Since we have this going, maybe we should revisit the question from @guillaume-alvarez https://github.com/swagger-api/swagger-ui/pull/9625#issuecomment-1957773605

activate these by default?

[heldersepu]

@kristiansamuelsson this PR shows: This branch is out-of-date with the base branch update when you get a chance

[kristiansamuelsson]

@char0n I am not sure where this PR stands after your comment in #3832. Let me know if you want me to continue on this/you want to take over or if I should just abandon it.

[char0n]

I am not sure where this PR stands after your comment in https://github.com/swagger-api/swagger-ui/issues/3832. Let me know if you want me to continue on this/you want to take over or if I should just abandon it.

Hi @kristiansamuelsson, we'll use parts of this PR and we want to go with general direction as described in my comment and implemented in your PR. The final implementation will be a bit different, utilizing SwaggerUI plugin system and wrapComponent feature. We will properly attribute your contribution though.