Skip to content

Handle references to deep schema definitions #554

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.

Sign up for GitHub

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

Merged
n1k0 merged 2 commits into from
Apr 26, 2017
Merged

Handle references to deep schema definitions #554

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9cc5aee
Handle references to deep schema definitions
eggyal Apr 21, 2017
95bee41
Handle JSON Pointer escaping
eggyal Apr 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions src/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,10 +314,22 @@ export function optionsList(schema) {

function findSchemaDefinition($ref, definitions = {}) {
// Extract and use the referenced definition if we have it.
const match = /#\/definitions\/(.*)$/.exec($ref);
if (match && match[1] && definitions.hasOwnProperty(match[1])) {
return definitions[match[1]];
const match = /^#\/definitions\/(.*)$/.exec($ref);
if (match && match[1]) {
const parts = match[1].split("/");

@n1k0 n1k0 Apr 21, 2017 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@eggyal eggyal Apr 22, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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?

@glasserc glasserc Apr 22, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd be in favor of that! But if you don't have time, then just a FIXME comment would be fine.

@n1k0 n1k0 Apr 22, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@eggyal eggyal Apr 26, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@n1k0: Test case added.

@epicfaace epicfaace Jan 21, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@eggyal did you ever add a FIXME comment here?

@eggyal eggyal Jan 22, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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?

@epicfaace epicfaace Jan 22, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@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?

let current = definitions;
for (let part of parts) {
part = part.replace(/~1/g, "/").replace(/~0/g, "~");
if (current.hasOwnProperty(part)) {
current = current[part];
} else {
// No matching definition found, that's an error (bogus schema?)
throw new Error(`Could not find a definition for ${$ref}.`);
}
}
return current;
}

// No matching definition found, that's an error (bogus schema?)
throw new Error(`Could not find a definition for ${$ref}.`);
}
Expand Down
21 changes: 21 additions & 0 deletions test/Form_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,27 @@ describe("Form", () => {
expect(node.querySelectorAll("input[type=text]")).to.have.length.of(1);
});

it("should handle references to deep schema definitions", () => {
const schema = {
definitions: {
testdef: {
type: "object",
properties: {
bar: { type: "string" },
},
},
},
type: "object",
properties: {
foo: { $ref: "#/definitions/testdef/properties/bar" },

@n1k0 n1k0 Apr 21, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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?

@glasserc glasserc Apr 21, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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".

@n1k0 n1k0 Apr 21, 2017

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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).

},
};

const { node } = createFormComponent({ schema });

expect(node.querySelectorAll("input[type=text]")).to.have.length.of(1);
});

it("should handle referenced definitions for array items", () => {
const schema = {
definitions: {
Expand Down
8 changes: 8 additions & 0 deletions test/utils_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,14 @@ describe("utils", () => {
expect(retrieveSchema(schema, definitions)).eql(address);
});

it("should 'resolve' escaped JSON Pointers", () => {
const schema = { $ref: "#/definitions/a~0complex~1name" };
const address = { type: "string" };
const definitions = { "a~complex/name": address };

expect(retrieveSchema(schema, definitions)).eql(address);
});

it("should priorize local definitions over foreign ones", () => {
const schema = {
$ref: "#/definitions/address",
Expand Down