fix: refactoring allOf schemas to be pre-merged when we generate JSON Schema #579
Conversation
erunion
added
bug
| @@ -222,11 +222,9 @@ | |||
| "deprecated": true, | |||
| "allOf": [ | |||
| { | |||
| "title": "option 1", | |||
There was a problem hiding this comment.
Removed these because they were always going to be merged into a single title property.
| if ( | ||
| Array.isArray(schema.properties[prop]) || | ||
| (typeof schema.properties[prop] === 'object' && schema.properties[prop] !== null) | ||
| ) { | ||
| schema.properties[prop] = toJSONSchema(schema.properties[prop] as RMOAS.SchemaObject, { | ||
| currentLocation: `${currentLocation}/${encodePointer(prop)}`, | ||
| globalDefaults, | ||
| prevSchemas, | ||
| refLogger, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Made this change because I discovered that if there was an x- extension, and it was a primitive, in an object we'd run it through the generator again and completely the value.
Though thinking about it, maybe this should instead just exclude anything that starts with x-?
src/lib/openapi-to-json-schema.ts
Outdated
| // If we can't merge the `allOf` for whatever reason (like if one item is a `string` and the other is a | ||
| // `object`) then we should completely remove it from the schema and continue with whatever we've got. Why? | ||
| // If we don't, any tooling that's ingesting this will need to account for the incompatible `allOf` and it may | ||
| // be subject to more breakages than just not having it present would be. | ||
| const { ...schemaWithoutAllOf } = schema; | ||
| schema = schemaWithoutAllOf as RMOAS.SchemaObject; | ||
| delete schema.allOf; |
There was a problem hiding this comment.
I'm still kind of iffy on this but this is how our schema form currently handles this case, but I still need to follow up this PR with changes to our schema form to handle if type isn't present and treat them as a string.
| const mapping = schema.discriminator.mapping; | ||
| Object.keys(mapping).forEach(k => { | ||
| refLogger(mapping[k]); |
There was a problem hiding this comment.
Had to make this change because TS didn't like how we were accessing schema.discriminator here and didn't think that discriminator was present on the schema type (despite the same code working two lines above).
| @@ -227,7 +214,12 @@ describe('polymorphism / inheritance', () => { | |||
| }, | |||
| }, | |||
| { | |||
| type: 'integer', | |||
There was a problem hiding this comment.
Changed this because integer and object schemas can't be merged.
| minimum: min, | ||
| maximum: max, | ||
| ])('`type: %s`', (type, format, min, max) => { | ||
| describe(`\`format: ${format}\``, () => { |
There was a problem hiding this comment.
Changed this because we were duping all of these integer + format tests under the same describe block and they weren't properly exposing themselves in the test output. Thankfully they were still being run, it just wasn't clear what was being run for what.
erunion
requested review from
a team,
Dashron and
julshotal
and removed request for
a team
🧰 Changes
Last week we received an API definition that contained an
allOfthat contained another allOf with two things:dataproperty that evaluated to a{}.dataproperty that evaluated to a complete object.Because we have handling in place to not repair
allOfchildren if they are missing atypedeclaration, this case was falling through that and the emptydataobject was getting atype: stringproperty added to it because we thought that it was malformed. Further up the stack when we render this schema out we later attempt to merge this newly "fixed"allOfand becausetype: stringandtype: objectare incompatible we either wouldn't render out anything, or in some cases would fully crash.Less than ideal!
Since we don't have any great way of determining if a sub-object property is contained within an
allOfand isn't malformed and will be okay when it's merged with another schema we decided to completely forgo this repair work that we had to repair schemas that were missing atype(and weren't either implicitly an object or an array), and instead merge allallOfschemas when we generate JSON Schema.Going forward, with this work any tooling that ingests JSON Schema generated with
getParametersAsJsonSchema()will no longer need to worry aboutallOfcases.Fix in support of RM-3146.
🧬 QA & Testing
There's a lot going on here, but all tests should no longer have any
allOfexpectations.As for the case that uncovered this issue lives, it in
__tests__/__datasets__/polymorphism-quirks.jsonand is tested within__tests__/operation/get-parameters-as-json-schema.test.js.