Skip to content
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

Jump to bottom

Json schema include nested models in register_model #640

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Json schema include nested models in register_model #640

wants to merge 4 commits into from

Conversation

flayman
Copy link

@flayman flayman commented May 7, 2019

When a SchemaModel type is created passing a Marshmallow schema into OpenAPIConverter.schema2jsonschema from the latest apispec.ext.marshmallow.openapi (and probably through other means), where a nested model is found it is referenced through a '$ref' key in the JSON with the value of "#/definitions/[submodel name]", where [submodel name] is the name given to the nested schema model. However, swagger does not add the nested schema model to the definitions, resulting in errors. This patch solves that problem by parsing out the referenced model name, finding that named model in api.models and registering that.

Tox and qa tasks are passing.

mattflahertypgs and others added 2 commits May 7, 2019 15:58
@mattflahertypgs
In register_model() when working with SchemaModel type, also recursiv…
8b3c151 
…ely register nested models.
@flayman
Update CHANGELOG.rst
bdbe7ba 
Added "In register_model() when working with SchemaModel type, also recursively register nested models"
@coveralls
Copy link

coveralls commented May 7, 2019 •
edited

Coverage Status

Coverage decreased (-0.1%) to 96.681% when pulling e02060e on flayman:json-schema-include-nested into 6978d57 on noirbizarre:master.

@j5awry
Copy link
Collaborator

j5awry commented May 16, 2019

@flayman thank you for your contribution. Would you be able to do the following to help expedite?

  1. please add unit tests for the fix
  2. Could you make sure this matches an issue? There are a bunch of related issues, and we want to make sure we can match PRs to Issues: JSON Schema $ref not working (not even the doc example) #275 Using nested JSON schemata for marshalling #414 are possible matches

I'm going to do some more reading on Marshmallow as well to make sure i'm 100% up on how its working.

@j5awry j5awry self-requested a review May 16, 2019 13:33
@flayman
Copy link
Author

flayman commented May 16, 2019 via email

@flayman
Copy link
Author

flayman commented May 16, 2019 via email

@j5awry
Copy link
Collaborator

j5awry commented May 16, 2019

PR #555 doesn't work as advertised, especially after a rebase. That's another reason why I'm keen on unittests.

There's a core issue of jsonschema, and anything using $ref, being of unknown depth and complexity, and needing to resolve refs which may be circular (which is allowed in jsonschema). I've suggested using the jsonref library which appears to work in most cases. However, i'm still evaluating more things.

I'll give this one a try too and see if it solves complex cases.

@flayman
Copy link
Author

flayman commented May 16, 2019

I'm a little bit busy just now with something else and my own requirements are mostly satisfied, but as soon as I have a bit of time I'll get on with some test cases. Hopefully in the next few days.

@flayman
Copy link
Author

flayman commented May 17, 2019

Hi. I tried the example you gave in PR #555 and it does not crash on the KeyError when run with my branch installed. (Actually, in order to get the example to run I needed to add an import for Namespace and a line with "ns = api.namespace('test')". The reason for the hard error in the other patch is that it assumes the key will exist on the dictionary self.api.models. It needs to be more defensive.

However, as in that codebase mine also fails to include the nested model on the Api instance. The Swagger UI throws an error when you try to expand TEST_SCHEMA or view the endpoint, so the code is not doing enough. I'd seen this error in my own use case and the changes were sufficient to make that go away. I think that was because of the change that I'd asked apispec to merge, which would have handled it inside the OpenAPIConverter class. But this has nothing at all to do with Marshmallow, so I'll need to see what else is needed here.

@flayman
Copy link
Author

flayman commented May 17, 2019

I haven't written the test case yet but it shouldn't be too difficult. I have pushed a change that works with the example. This is a simpler and more reliable approach.

@j5awry
Copy link
Collaborator

j5awry commented May 17, 2019

Thanks! I'm heading out on a short vacation, so won't get a chance to check this, and even more complicated tests, till after I get back on Wednesday.

@j5awry
Copy link
Collaborator

j5awry commented May 17, 2019

For anyone jumping in, i have a quick functional test file we can use. A couple simple cases are at the top:

https://gist.github.com/j5awry/aa8f78791078ddd63a3957ca584e5e60

@flayman
Copy link
Author

flayman commented May 17, 2019 •
edited

Okay, I have a question about how to write the test(s). This needs to test the result of calling register_model() in swagger.py, but nothing else tests that directly. The test_model.py test cases are not useful because the api is not working on them. test_namespace.py seems like the right place to do this, but all the models presented there are trivial. I would like to use your functional test schemas in there but it becomes entangled with the functionality of swagger.py so it may not be cleanly separated enough for unit testing. Do you have any suggestions?

Edit: No, I've answered my own question. My first glance at the test_swagger cases suggested that wasn't the right place, but it is obviously. There are calls to api.model() in there and testing how that affects the definitions. There are no calls to api.schema_model(), which is what's needed.

Another Edit: Okay, so in writing unit tests I've discovered that I haven't covered all the bases. I need to use a combination of looking for definitions and looking for '$ref' keys. I'll have to continue next week.

@flayman
Copy link
Author

flayman commented May 20, 2019

I've been reading the json schema specification and I think supporting references in flask-restplus is probably more complicated than attempts I've seen so far. json schemas can refer to internal definitions like...

{ "$ref": "#/definitions/address" }

... and also definitions in other json schemas as in...

{ "$ref": "definitions.json#/address" }
https://json-schema.org/understanding-json-schema/structuring.html

This is probably how it should be done in Swagger rather than bringing everything into Swagger's definitions and risking conflicts, but it could prove tricky. So for example, if a schema named Person brings in the definition "address", which it is referring to internally, other schemas could refer to it as "Person#/definitions/address". When nested models are registered, they would have to refer to the nested models in that form. This probably represents a major change.

@j5awry
Copy link
Collaborator

j5awry commented May 30, 2019

@flayman you're correct about all the jsonschema stuff. references are a big headache. I've worked out the following test cases for jsonschemas so far:

jsonschema test cases

  1. simple -- one layer, one key
  2. flat -- one layer, many key
  3. nested -- multiple layers, one key per
    3a) two layers, one key per
    3b) 10 layers, one key per
  4. refed simple -- two layers, one ref. abstract 3a into refs
  5. refed nested -- abstract 3b into refs

Right now with your changes we've got 1, 2, 4 down happily. I'm writing the tests for 3 now. I've used an example of a highly complex schema from work for 5, and it's failing. However, I want to simplify it down a bit more so I can track down where everything is happening.

In parallel to this PR, I'm going to further investigate using jsonref to de-reference things:

https://github.com/gazpachoking/jsonref

Since someone has solved the reference part, perhaps that's the best option for us to take, since it's not an easy problem.

@flayman
Copy link
Author

flayman commented May 30, 2019 •
edited

Hi John,

I can't comment on jsonref because I haven't looked at it. It may solve all or most of the problems. But I think that the reference problem is more complicated than you've indicated in your test cases, because there's no reason you can't have one schema reference a definition in another schema, and that creates all sorts of problems. The Swagger document has its own definitions, with each of the referenced models in there. The problem is that if you parse a json schema into a SchemaModel and that jsonschema has its own definitions, it's not clear where those should be placed. Do you put them into the Swagger definitions as global definitions or do you leave them where they are?

If you put the definitions onto the Swagger document's definitions property then there's a risk of collisions, and I think it also goes against the jsonschema spec. If you leave them where they are, then the model created from that json schema (perhaps called MyModel) won't be able to refer to its own definitions through Swagger, because in the Swagger document they will be found under #/definitions/MyModel/definitions. So when you register the model you'd have to actually change all the refs in the MyModel schema to point there. That touches models.py. I actually did some work along these lines which I haven't pushed to GitHub. It's a bit messy because a model with internal refs will show properties looking something like this:

TEST_SCHEMA {
    secret* [
        TEST_SCHEMA/definitions/key {
            key* | string
        }
    ]
}

It's not working correctly yet because the payload schema for expect is not finding the nested definition. I stopped work on it because it's a fairly radical change that the maintainers may not want to bless.

@j5awry j5awry mentioned this pull request Jun 7, 2019
Open
@alvarolopez
Copy link

Hi.

Any idea when this PR would get merged? This is an issue we are facing right now, since we are using JSON schemas with nested models and they are not properly rendered.

@flayman
Copy link
Author

flayman commented Oct 15, 2019

It's doubtful that that this PR will be merged. There were some fundamental questions about how to resolve external JSON refs. I think it has been overtaken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j5awry j5awry Awaiting requested review from j5awry

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@flayman @coveralls @j5awry @alvarolopez @mattflahertypgs