Handle references to deep schema definitions #554
Conversation
|
Thanks! The code looks good, but the code format check failed, for reasons that aren't related to your change. Maybe this is because we just updated to |
| }, | ||
| type: "object", | ||
| properties: { | ||
| foo: { $ref: "#/definitions/testdef/properties/bar" }, |
There was a problem hiding this comment.
Actually I couldn't find any reference in the spec about this capability. The only part of it using nested definitions is this one but I'm not sure about accessing properties like your implemention does would actually comply.
Do you by chance have a link describing how implementations should access nested definitions?
There was a problem hiding this comment.
I couldn't find anything in the spec either, but I tried the given example on https://jsonschemalint.com/ and it works the way you would expect it to.
Section 8.2.1 says "Schemas can be identified by any URI that has been given to them, including a JSON Pointer".
There was a problem hiding this comment.
Ah, indeed I could validate the example in #553 as well, I think we're good to go then (well once the prettier formatting issue is addressed).
| return definitions[match[1]]; | ||
| const match = /^#\/definitions\/(.*)$/.exec($ref); | ||
| if (match && match[1]) { | ||
| const parts = match[1].split("/"); |
There was a problem hiding this comment.
Makes me think of properties containing a / in their name, which will fail here. I don't want us to cover this edge case, but it may be a good idea to document this limitation, or at least to add a comment about it here.
There was a problem hiding this comment.
@n1k0: Looking at the JSON Pointer spec, §3. Syntax, names containing / or ~ will be escaped as ~1 and ~0 respectively. I will update to handle this, but I can't help wondering whether this project might instead use something like json-schema-ref-parser to handle these and other issues?
There was a problem hiding this comment.
I'd be in favor of that! But if you don't have time, then just a FIXME comment would be fine.
There was a problem hiding this comment.
I'd be in favor of it too, though their API being async it may be more work than needed in this specific case. A comment will be fine for now.
Also, please add a test case for this new covered edge case.
There was a problem hiding this comment.
@epicfaace: Perhaps I had misunderstood what @glasserc was proposing a FIXME comment for—I assumed it was to handle /s, which I fixed in 95bee41 so comment no longer required?
There was a problem hiding this comment.
@epicfaace: Perhaps I had misunderstood what @glasserc was proposing a FIXME comment for—I assumed it was to handle
/s, which I fixed in 95bee41 so comment no longer required?
Oh, I assumed that the FIXME comment was supposed to be for using "json-schema-ref-parser to handle ... other issues."
Since you did fix the escaping of / and ~, do you know if there's anything we're missing in this implementation which json-schema-ref-parser does well?
|
Thank you 👍 |
|
Released in v0.48.0. |
* Handle references to deep schema definitions * Handle JSON Pointer escaping
New features: * Handle references to deep schema definitions (rjsf-team#554) Bugfixes: * Fix rjsf-team#563: Remove remaining React.PropTypes references. Internal changes: * Upgrade prettier to v1.2.2. * Fix coding style check command.
4 participants
Reasons for making this change
Fixes #553
Checklist
npm run cs-formaton my branch to conform my code to prettier coding style