Consider changing configuration to not pretty-print JSON by default

[Lukasa]

Motivation

When debugging an issue with a solar inverter, I bumped into this default:

https://github.com/apple/swift-openapi-runtime/blob/e5b0de7221dd1a0c9b9a6de9afdbe06f3e46a24f/Sources/OpenAPIRuntime/Conversion/Configuration.swift#L165

As it turns out, the issue with this inverter was that it did not handle pretty-printed JSON. This cost me about 30 minutes of debugging time as I tried to root-cause why the curl equivalent of my command worked, but the swift-openapi-generator equivalent didn't.

In general, I was surprised to see swift-openapi-generator defaulting to pretty-printing JSON. While the default of sortedKeys makes some sense to me (certain tests are easier to write), the pretty-printed default seems to me to have more downsides with fewer obvious upsides. Most notably, it diverges from common practice in JSON-encoding libraries, which tend to default to minified JSON and require user action to prettify it.

Proposed solution

It seems to me that the community should at least consider whether the default behaviour here should be flipped.

Alternatives considered

No response

Additional information

No response

[czechboy0]

Thanks @Lukasa, yeah happy to hear what folks think, especially those who've been using the current default for a few years.

Current Swift OpenAPI Generator users, is the default the right one for y'all, or are you agnostic and changing the default wouldn't make a difference to you?

[simonjbeaumont]

Thanks for filing!

It seems to me that the community should at least consider whether the default behaviour here should be flipped.

That's fair. And while technically flipping the default would be a breaking change, I imagine that flipping it from pretty to not would be far less likely to break someone than flipping it in the other direction.

While the default of sortedKeys makes some sense to me (certain tests are easier to write)

In tests, we could always add some helper function that does another roundtrip through a decoder-encoder pair to get both sorted keys and pretty, if we wanted to keep the tests more readable.

IIRC this also affects how strings are escaped so we should consider the impact that might have too.

[MahdiBM]

I never bumped into a problem with pretty-printed-or-not JSONs, but pretty-printing is not free (takes some resources to do + increases the size, sometimes noticeably) so I'd say it should be turned off by default and only (also preferably) be used for error-debugging/tests.
Most tooling would already turn non-pretty-printed JSON into pretty-printed (e.g. Postman), or you could use jq if you're on the command line.

[czechboy0]

That's a fair argument - do we feel the same way about .sortedKeys?

[simonjbeaumont]

That's a fair argument - do we feel the same way about .sortedKeys?

Ofc I'm curious what adopters think but my 2¢ is that we should keep sorted keys as it provides a deterministic output which I think is worth it, with very little chance of causing issues for anyone.

[czechboy0]

Agreed - I'm more trying to figure out which problem we're trying to solve by potentially changing the default. Is it about correctness (i.e. some systems struggle with whitespace in JSON) or is it about performance (with the implication that defaults should be the most performant ones, rather than the most commonly useful ones, or the ones most friendly to newcomers)? Once we agree on that, it should be easier to make a decision here across the options, rather than using different reasoning for each one.

[MahdiBM]

hmm I mean it's not really about correctness since sending pretty-printed JSON is not incorrect.

I think the more actual problem is the performance decrease. Specially for JSONs with too many levels, those whitespaces start to add up and make for a noticeable size increase. I have noticed cases of this when I prettify json files to read what's in them.

We could also consider what @Lukasa bumped into as the secondary reason and say that we want to more closely do what everybody else is doing so we have less chance to accidentally trigger a bug in someone's JSON parsing code, which is not really openapi-runtime's fault, but eh, spec-divergences (or bugs) like this are not generally uncommon in the software world so we could just accept that such thing exists and move on.

[Lukasa]

Yeah I definitely don't think we should be excessively motivated by the correctness bug in my solar inverter. That makes them look silly, but it isn't even the silliest correctness bug I've encountered in an embedded device in my own house.

The main motivations in my mind are the fact that a) it's a minor performance penalty, and b) the default diverges from typical library defaults, which default to compact unless you ask for pretty. This caused me a surprise when I encountered the choice, and such a surprise is often worth investigating. But I don't feel strongly: just thought it should be discussed.