-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add converter for oneOf/anyOf with nullable types #25
base: main
Are you sure you want to change the base?
Conversation
*/ | ||
export function getRefSchema(node: object, ref: RefObject) { | ||
const propertyName = ref.$ref.split('/').reverse()[0]; | ||
if (node.hasOwnProperty('components')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not a safe assumption; i.e. the $ref could be a reference to a schema in another file, such as
$ref: '../components.yaml/#/components/schemas/foo'
in which case fetching schemas[propertyName]
may fail or may fetch the wrong schema.
This tool does not resolve external references; see the README which says
"The tool only supports self-contained documents. It does not follow or resolve external $ref
documents embedded in the source document."
See https://github.com/apiture/api-ref-resolver for tooling to supports resolving external $ref
references before down converting. (However this tool should not assume that all external $ref
have been resolved)
So instead, I suggest either checking that the $ref starts with '#/components/schemasor after splitting on '/' that
components[0] === '#' && components[1] === 'components' && components[2] === 'schemas'`
return; | ||
} | ||
|
||
const typeNullIndex = entries.findIndex((v) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this would be cleaner with
const isNullable = entries.find( (v) => .... );
a: { | ||
oneOf: [ | ||
{ | ||
$ref: '#/components/b', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be '#/components/schemas/b'
$ref: '#/components/b', | ||
}, | ||
{ | ||
$ref: '#/components/c', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be '#/components/schemas/c'
a: { | ||
oneOf: [ | ||
{ | ||
$ref: '#/components/b', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be '#/components/schemas/b'
$ref: '#/components/b', | ||
}, | ||
{ | ||
$ref: '#/components/c', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be '#/components/schemas/c'
}, | ||
}, | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see a test where the schema d
has an allOf
referencing a
, so that d
is also tagged as nullable
,
or a schema e
that has an allOf
with type f
where f
has type: [string, 'null']
so that both f
and e
are marked nullable
merge upstream
Lock package versions
Resolves #23 - I ended up finding some time this weekend to work on this.
This change allows for the conversion of anyOf/oneOf arrays with $ref/obj and a
type: "null"
, which is a common way to do a type union in 3.1.0 to allow for a type to be a $ref, but also nullable. I've tested it on my own rather beefy spec (250+ endpoints and hundreds of types) with zero changes and it passed typecheck. I recently upgraded my spec to 3.1.0 and the conversion tools used by this project were not able to accurately convert the 3.1.0 spec.Hopefully this is a good enough addition to add, it certainly suits my purposes. I can understand if it should be behind a flag though. Let me know if there are any other situations I may have missed.
Thanks!