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

Updating our handling of JSON Schema to be compliant against allowEmptyValue #110

Merged
merged 15 commits into from
Feb 26, 2020

Conversation

erunion
Copy link
Member

@erunion erunion commented Feb 21, 2020

This resolves some issues with how we're constructing JSON Schema with regard to empty default properties.

Previously when an empty default was added into a data schema, a bug would cascade up into @readme/api-explorer and our usage of @readme/oas-to-har and we'd create an API HAR of a payload consisting of empty strings.

Instead now, if a default evaluates to an empty string, and allowEmptyValue: true is present on that property, we'll let the empty value through, otherwise it'll get tossed.

Screen Shot 2020-02-21 at 12 03 18 PM

  • 🧰 Fixes to our construction of parameter and requestBody JSON Schema work to be compliant on allowEmptyValue. See https://swagger.io/specification/#parameterObject for details on the property.
  • 🚧 Rewrites handling of common common parameters to make the code less obtuse and easier to maintain.
  • 🌱 Added a shitload of JSON Schema unit tests.

@erunion erunion added the bug Something isn't working label Feb 21, 2020
@erunion erunion changed the title fix: updating our handling of jsonschema to account for empty defaults Updating our handling of JSON Schema to be compliant against allowEmptyValue Feb 24, 2020
@erunion erunion marked this pull request as ready for review February 26, 2020 08:09
}

function getOtherParams(pathOperation, oas) {
let pathParameters = pathOperation.parameters || [];
Copy link
Member Author

@erunion erunion Feb 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed this from pathParameters to operationParameters because in the context of what this is doing, it's building up an array of every parameter on the option; not an array of every path parameter on the operation.

i spent a good amount of time trying to hunt down some weird edge cases when writing tests before realizing this is what was going on 🙃

@erunion
Copy link
Member Author

erunion commented Feb 26, 2020

@rafegoldberg @gratcliff This is good for review.. Happy to go over it with you if you've got questions about WTF this is.

@gratcliff
Copy link
Member

@jon It's hard to review this without knowing exactly what the outcome of the data transformation should look like.

There don't appear to be any glaring issues - data checks are present so nothing should fail. There is a lot of iteration and the recursion scares me a tad, but overall I think that's a necessity with the schemas you're trying to process.

Copy link

@rafegoldberg rafegoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like Gabe said, a bit tricky to logic through some of this data transformation stuff. But yeah, code looks solid; I'm good on this!

@erunion erunion merged commit 370c5ef into master Feb 26, 2020
@erunion erunion deleted the fix/setting-empty-defaults-in-jsonschema branch February 26, 2020 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants