Fix default values and matching schemas when oneOf / anyOf subschemas contain references #2272
Conversation
based on formData
|
@epicfaace thanks for this library first of all. I'm really digging it these days. I don't like to poke you like this, but I just noticed that there are a lot of pull requests pending so I'm wondering if there is anything I can do to ease the acceptance of my PRs? This and #2270 since they'll bring a lot of value for my use case and anyone using FastAPI for their backend since it gives you auto/generated OpenAPI schema with references to each model schema (so references has to be supported). |
|
I think it's really just maintainer bandwidth -- I'm the only person available who is able to regularly review PRs, and I can only work for so much on a volunteer basis (the problem with open source in general!) If you'd like to be a maintainer and help review PRs to clear out the queue, let me know 😄 |
|
I figured and totally understandable. I'm not sure what exactly it requires of me to be a maintainer. I'm quite new in the open-source world still but I might be able to help out (limited time for me as well). On another note; I just noticed that this PR is highly related to #2245 |
NixBiks
changed the title
|
Integrated PR #2245 into this PR (copying some tests) |
NixBiks
force-pushed
the
schema-from-formdata
branch
from
20bd8d7
to
2bf49c0
Compare
NixBiks
force-pushed
the
schema-from-formdata
branch
from
8d1a908
to
7992cae
Compare
* simplify withIdRefPrefix * add comments * improve error handling
|
I should've accounted for your directions now and merged the branch with the latest changes. Should I squash the commits btw? It kinda blew up this progress |
Nope, doesn't matter -- we'll squash and merge anyway. |
* Fixed bug with $ref being a property * added tests for withIdRefPrefix
|
Might be ready now 😎 |
packages/core/src/validate.js
Outdated
| // then we rewrite the schema ref's to point to the rootSchema using the id | ||
| // this accounts for the case where schema have references to models | ||
| // that lives in the rootSchema but not in the schema in question. | ||
| if (rootSchema) { |
There was a problem hiding this comment.
I think rootSchema is always given -- so maybe we can just remove this check.
There was a problem hiding this comment.
You're right - except for some tests, but I've just updated those tests to add the schema as rootSchema as well
There was a problem hiding this comment.
Alternatively I could leave the tests as is and have this instead
return ajv
.addSchema(rootSchema || schema, ROOT_SCHEMA_PREFIX)
.validate(withIdRefPrefix(schema), data);So if no rootSchema is provided then the schema itself will be used. Any preferences with one or the other?
There was a problem hiding this comment.
I think it's better to just go ahead and update the tests rather than adding unnecessary functionality to this function.
* Check for # in ref * update tests
|
Are we ready to merge this one? |
|
Would love to see this as well. Thanks for your work. |
|
Haven't taken a look yet -- will do soon |
|
please take a look soon |
Co-authored-by: Ashwin Ramaswami <[email protected]>
Co-authored-by: Ashwin Ramaswami <[email protected]>
|
Ready to go (fingers crossed) |
epicfaace
changed the title
|
Wuhu thanks. My first actual code PR 🎊 |
Reasons for making this change
Atm. schemas are not able to match with the correct option based on the formData if the option schema have any references.
Fixes #2269
Branched out from PR #2270 since it is required
Checklist