Allow schema $refs to not have to start with `#/definitions` #954

domharrington · 2018-06-14T00:18:24Z

Reasons for making this change

We're using this module to render forms for Swagger/OAS files. $refs
in swagger are stored in components/schemas at the top level of the object.

This change allows $ref strings to not always start with #/definitions/
which is a much easier solution than us having to recursively modify $refs
to prefix with this value.

All existing tests are passing, with a new test for this functionality.

The change was to make the definitions part of the regular expression an
optional non-capturing group.

Edit: any reason why you dont commit the package-lock.json file?

Edit 2: also, is there any main reason (apart from keeping the repo clean) why you don't commit the built source files (in lib/ and dist/?) This makes it tricky to npm install from a fork, as lib/index.js doesn't exist in git.

Checklist

I'm adding or updating code
- I've added and/or updated tests
- I've updated docs if needed
- I've run npm run cs-format on my branch to conform my code to prettier coding style

This allows you to simply run `mocha test-file.js` without having to remember to add all the extra stuff

We're using this module to render forms for Swagger/OAS files. $refs in swagger are stored in `components/schemas` at the top level of the object. This change allows $ref strings to not always start with `#/definitions/` which is a much easier solution than us having to recursively modify $refs to prefix with this value. All existing tests are passing, with a new test for this functionality. The change was to make the definitions part of the regular expression an optional non-capturing group.

…chema-form's dereferencing instead This gets a little bit messy because you can do these 3 different types of refs for request bodies: - inline schema - $ref directly to a schema - $ref to a request body object, which contains the schema within a mime type object The first 2 work in react-jsonschema-form (after this has been merged rjsf-team/react-jsonschema-form#954) The 3rd one doesn't, and probably never will as it's far too specific to be merged into that module. So in the case where it is referencing a request body in components, I've switched to performing a single lookup there to fetch the actual schema $ref (or inline schema)

This is so that it can be separately styled easily via CSS. It is already done this way in https://github.com/mozilla-services/react-jsonschema-form/blob/aadbbe9701c4db9df761e86f0280e1ecafc509f8/src/components/fields/SchemaField.js#L59

domharrington · 2018-08-07T20:53:50Z

Tests are passing locally on my machine. Anyone know why it would be failing the prettier checks for a file i have not modified?

jbadeau · 2018-11-07T10:28:12Z

@domharrington we are also considering generating forms from OpenAPI. DO u have an example somewhere?

domharrington · 2018-11-07T19:07:45Z

Yes! We're doing it quite successfully over here: https://github.com/readmeio/api-explorer

Demo site: https://preview.readme.io/

jbadeau · 2018-11-07T19:22:04Z

Dude, looks verrrry interesting. Gonna have a look then get back to you. Looks very close to what Im thinking about.

domharrington · 2018-11-07T20:01:15Z

If you're interested in a hosted solution for your docs, you should check out ReadMe: https://readme.io/. The API explorer that I linked to above is our open source project, but we have a whole host of other features built into the product including suggested edits for your docs, an API, user login with variables.

I'm an engineer at ReadMe, please feel free to reach out to me dom[at]readme.io if you have any more questions.

glasserc · 2018-11-09T15:49:13Z

Hey, sorry for not responding to this at the time.

You only change the regular expression not to start with definitions, but all definitions are still looked up starting from the schema definitions, so e.g. #/components/schema/address pulls stuff out of #/definitions/components/schema/address.

To be honest, we handle definitions specially but I'm not sure if we need to. If they're true JSON pointers, they should be resolvable against the schema root. Maybe we should be passing that around instead.

Edit: any reason why you dont commit the package-lock.json file?

I'll admit ignorance here, but the reason is that rjsf is a library rather than an application, so constraining packages should happen in the calling code.

[Edit: I guess this is what I get for replying to months-old comments. We have a package-lock.json file now.]

Edit 2: also, is there any main reason (apart from keeping the repo clean) why you don't commit the built source files (in lib/ and dist/?) This makes it tricky to npm install from a fork, as lib/index.js doesn't exist in git.

As you say, it's just to keep the repo clean.

domharrington · 2018-11-09T19:40:24Z

Hey! No worries. I made the definitions part of the regex optional because the refactor to change everywhere else in the code and examples to not start with definitions, plus it probably would constitute a major version bump to do so.

This change just allowed the possibility for it to not have to start with definitions. We're rendering OAS files, and it's much easier to just patch on the components object directly onto each schema than it is to recursively crawl the object for all $refs and modify them all to start with definitions.

Hope that makes it a little clearer for the reasoning for this change. I'm using a fork of this library currently but it would be great to get back onto the main repo. I've got one more PR too: #1001.

Thanks in advance.

glasserc · 2018-11-16T16:06:35Z

I think no longer treating definitions as special but just resolving JSON pointers against the root of the schema is largely backwards-compatible. Schemas that used to work should continue to work, for instance. I think the only user-facing place where we use definitions is in registry, and I think we could produce a backwards-compatible way to manage that (combine the user's schema and definitions using realSchema = {...schema, definitions: registry.definitions}). I know that would be more work, but I think it would be much clearer than having a regexp with an optional string buried deep in the code, plus I think it would be cleaner for your use case. What do you think?

…onschema-form

…til edited You can test this locally here: http://localhost:9966/?selected=swagger-files%2Ftypes.json Search for "default value" and see that it is already in the code sample. This issue luckily got fixed upstream: rjsf-team/react-jsonschema-form#1034 So to bring that in I needed to update my fork of the module. I would love to get off of my fork and get back onto the main release, but there's some outstanding work on my PR which means we can't right now: rjsf-team/react-jsonschema-form#954

epicfaace

@domharrington , thanks for the PR; I think the approach @glasserc mentioned makes the most sense (i.e. doing #/components/abc should reference the object with the path components.abc in the schema, not definitions.components.abc).

Would you be able to modify your PR to have that behavior -- and would that behavior suffice for your needs for your own project?

epicfaace · 2019-01-21T17:56:08Z

package.json

@@ -16,8 +16,8 @@
    "publish-to-npm": "npm run build:readme && npm run dist && npm publish",
    "preversion": "npm run build:playground && npm run dist && npm run build:readme && npm run cs-check && npm run lint",
    "start": "node devServer.js",
-    "tdd": "cross-env NODE_ENV=test mocha --require babel-register --watch --require ./test/setup-jsdom.js test/**/*_test.js",


Thanks for the changes for mocha config, but can you please remove these changes from this PR and make a separate PR for it?

domharrington · 2019-01-23T00:34:11Z

Hey @epicfaace yup i agree with everything that was said before. It's on my todo list to update this PR with the more generic solution, i've just not had the capacity to get round to it yet.

epicfaace · 2019-02-08T04:21:42Z

This PR fixes #988 .

…onschema-form

domharrington · 2019-02-15T01:06:22Z

I took a stab at finishing this off today in a fresh fork from master but kept hitting upon problems around this block here: https://github.com/mozilla-services/react-jsonschema-form/blob/bece2a5a847e216c2386d4f7f0ce9cf25b549562/src/utils.js#L402-L404

Since the $ref and the definitions are coming from the same object now, it appears to get stuck in an infinite loop during that while block. E.g.

const schema = {
  definitions: {
    testdef: { type: "string" },
  },
  $ref: "#/definitions/testdef",
};

The code attempts to resolve the $ref before it gets to the point where it looks in the current object: https://github.com/mozilla-services/react-jsonschema-form/blob/bece2a5a847e216c2386d4f7f0ce9cf25b549562/src/utils.js#L405

Can anyone with more knowledge of this code point me in the right direction or suggest a way forward?

…onschema-form

domharrington · 2019-04-08T18:18:35Z

Hey @epicfaace, any suggestions on a way forward for this? This PR as it is is backwards compatible and though it's not completely ideal, when I tried to refactor completely I came up against some issues.

minhnq013 · 2019-04-22T06:41:36Z

Anyone know what's the status for this one?
I have several schemas that are un-usable due to this.

Thanks

domharrington · 2019-04-22T20:03:08Z

I'm using a fork of this module, which only has this change. You can use it by running this command:

npm i "github:domharrington/react-jsonschema-form#dist-committed"

epicfaace · 2019-04-22T20:54:41Z

I took a stab at finishing this off today in a fresh fork from master but kept hitting upon problems around this block here:

@domharrington Can you share the code for the approach you tried? I'm trying to understand what was your approach there. I'm thinking that a successful approach might need to make findSchemaDefinition accept the entire schema object rather than the definitions object.

I'm not comfortable with the way it's implemented now, because having #/components/schemas/address actually point to #/definitions/components/schemas/address seems to be unintuitive.

domharrington · 2019-04-24T17:15:03Z

It's been over 2 months since I attempted it, so the code I put together at the time is probably lost to the ether or in a git stash somewhere, so I cannot remember what I tried.

The fix I implemented makes having definitions completely optional in the $ref path, it doesn't have it present in one place whilst actually pointing somewhere else. It's a backwards compatible fix

…onschema-form

domharrington · 2019-05-22T00:00:48Z

Any insight you can give into getting this merged would be amazing! I couldn't figure it out at the time.

epicfaace · 2019-06-02T18:25:10Z

Hmm, not sure, will look into it and let you know

erunion · 2019-07-26T23:53:36Z

@epicfaace @glasserc Is there anything else that needs to be done to this PR before it gets merged in?

epicfaace · 2019-12-22T08:23:43Z

Closing this in favor of #1506 .

Dom Harrington added 2 commits June 13, 2018 17:10

Move some mocha options to mocha.opts

2c887bc

This allows you to simply run `mocha test-file.js` without having to remember to add all the extra stuff

Dom Harrington added 2 commits August 7, 2018 13:36

Merge branch 'master' into feature/definitions-improvements

4d09c10

Merge branch 'master' of https://github.com/mozilla-services/react-js…

c551ecd

…onschema-form

domharrington mentioned this pull request Dec 12, 2018

Fix issue where default values wouldn't be saved into the formData until edited readmeio/api-explorer#179

Merged

epicfaace requested changes Jan 21, 2019

View reviewed changes

epicfaace added the awaiting changes label Jan 21, 2019

glasserc mentioned this pull request Feb 4, 2019

'Could not find definition' in nested property definition #988

Closed

Merge branch 'master' of https://github.com/mozilla-services/react-js…

a4512c6

…onschema-form

epicfaace added the help wanted label Apr 5, 2019

Merge branch 'master' of https://github.com/mozilla-services/react-js…

fd03c01

…onschema-form

Merge branch 'master' into feature/definitions-improvements

b48a56a

domharrington added 2 commits May 20, 2019 16:49

Merge branch 'master' of https://github.com/mozilla-services/react-js…

f426b06

…onschema-form

Merge branch 'master' into feature/definitions-improvements

a71e66c

epicfaace mentioned this pull request Sep 13, 2019

react-jsonschema-form to generate a form itself #1465

Closed

3 tasks

erunion mentioned this pull request Nov 10, 2019

Allow references that don't start with #/definitions/ #1506

Merged

epicfaace closed this Dec 22, 2019

erunion mentioned this pull request Jan 11, 2020

Support for allOf and a host of small improvements and bug fixes readmeio/api-explorer#436

Merged

13 tasks

erunion mentioned this pull request Feb 13, 2020

fix: allowing schema refs to not start with #/definitions readmeio/react-jsonschema-form#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow schema $refs to not have to start with `#/definitions` #954

Allow schema $refs to not have to start with `#/definitions` #954

domharrington commented Jun 14, 2018 •

edited

Loading

domharrington commented Aug 7, 2018

jbadeau commented Nov 7, 2018

domharrington commented Nov 7, 2018

jbadeau commented Nov 7, 2018

domharrington commented Nov 7, 2018

glasserc commented Nov 9, 2018 •

edited

Loading

domharrington commented Nov 9, 2018

glasserc commented Nov 16, 2018

epicfaace left a comment

epicfaace Jan 21, 2019

domharrington commented Jan 23, 2019

epicfaace commented Feb 8, 2019

domharrington commented Feb 15, 2019 •

edited

Loading

domharrington commented Apr 8, 2019

minhnq013 commented Apr 22, 2019

domharrington commented Apr 22, 2019

epicfaace commented Apr 22, 2019 •

edited

Loading

domharrington commented Apr 24, 2019

domharrington commented May 22, 2019

epicfaace commented Jun 2, 2019

erunion commented Jul 26, 2019

epicfaace commented Dec 22, 2019

Allow schema $refs to not have to start with #/definitions #954

Allow schema $refs to not have to start with #/definitions #954

Conversation

domharrington commented Jun 14, 2018 • edited Loading

Reasons for making this change

Checklist

domharrington commented Aug 7, 2018

jbadeau commented Nov 7, 2018

domharrington commented Nov 7, 2018

jbadeau commented Nov 7, 2018

domharrington commented Nov 7, 2018

glasserc commented Nov 9, 2018 • edited Loading

domharrington commented Nov 9, 2018

glasserc commented Nov 16, 2018

epicfaace left a comment

Choose a reason for hiding this comment

epicfaace Jan 21, 2019

Choose a reason for hiding this comment

domharrington commented Jan 23, 2019

epicfaace commented Feb 8, 2019

domharrington commented Feb 15, 2019 • edited Loading

domharrington commented Apr 8, 2019

minhnq013 commented Apr 22, 2019

domharrington commented Apr 22, 2019

epicfaace commented Apr 22, 2019 • edited Loading

domharrington commented Apr 24, 2019

domharrington commented May 22, 2019

epicfaace commented Jun 2, 2019

erunion commented Jul 26, 2019

epicfaace commented Dec 22, 2019

Allow schema $refs to not have to start with `#/definitions` #954

Allow schema $refs to not have to start with `#/definitions` #954

domharrington commented Jun 14, 2018 •

edited

Loading

glasserc commented Nov 9, 2018 •

edited

Loading

domharrington commented Feb 15, 2019 •

edited

Loading

epicfaace commented Apr 22, 2019 •

edited

Loading