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.

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

registry.add_supplemental_schemas #48

Closed
wants to merge 2 commits into from

Conversation

joeb1415
Copy link
Contributor

@joeb1415 joeb1415 commented Dec 1, 2018

Add registry.add_supplemental_schemas function to export supplemental schemas to swagger, in addition to those used by the API handlers.

In my app, I have a handler:

@registry.handles(
    marshal_schema={201: FooSchema}
)
def create_foo():
    pass

With:

class FooSchema(Schema):
    template = fields.String()
    vars = fields.Dict()

Elsewhere, I validate the vars against the template according to various template schemas:

class Template_1_Schema(Schema):
    var_1 = fields.String()
    var_2 = fields.String()

class Template_2_Schema(Schema):
    var_3 = fields.Boolean()
    var_4 = fields.Integer()

I would like to export these template schemas in the swagger definition, so others can know what to expect to send to my handler in the vars dictionary param.

This PR introduces a registry function add_supplemental_schemas to support this. The function can accept a single schema, or for convenience, a directory containing all the supplemental schemas.

cc: @barakalon

…al schemas to swagger, in addition to those used by the API handlers.
# Conflicts:
#	flask_rebar/swagger_generation/swagger_generator.py
@brockhaywood
Copy link
Contributor

@joeb1415 Just curious, why does vars need to be a Dict field and not a nested schema referencing Template_1_Schema? still see the value in being able to reference other models just curious about this particular use case.

@joeb1415
Copy link
Contributor Author

joeb1415 commented Dec 3, 2018

@brockhaywood There are many template / schema pairs, with different schemas. I added a second example to my PR comment above to make this clearer.

@brockhaywood
Copy link
Contributor

Ah got it, conditional templates?

@barakalon
Copy link
Contributor

It seems like what you actually want is the discriminator field: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/2.0.md#schema-object

But this field sucks. And OpenAPI 3.0 replaces this with the proper jsonSchema way of doing this: oneOf.

If you had one of these features available, would you need this change? I'd rather avoid things that both require manual tweaking of the swagger, especially those that aren't really sanctioned in the swagger spec. But I prefer practicality over purity, so I'm open to this change if we really need it.

@colinhostetter
Copy link
Contributor

Did you look into the possibility of just adding these schemas into the client package we generate / did that not work?

I'm not sure this belongs in rebar, and I'm not sure that adding it to rebar actually gets us anything over doing that, since the generated client won't know which template schema to validate against either way. (Well, I guess it adds a little consistency in that the schemas in the package are the regular swagger-codegen schemas instead of Marshmallow schemas, hmm...)

@joeb1415
Copy link
Contributor Author

joeb1415 commented Dec 4, 2018

@barakalon yeah if we could do something like this that would be awesome!

class FooSchema(Schema):
    template = fields.String()
    vars = fields.Nested(Schema, validate=OneOf(Template_1_Schema, Template_2_Schema))

@joeb1415
Copy link
Contributor Author

joeb1415 commented Dec 4, 2018

This looks interesting:
https://github.com/marshmallow-code/marshmallow-oneofschema

@barakalon
Copy link
Contributor

That seems to be the blessed one: marshmallow-code/marshmallow#649

Doesn't seem too bad to add a converter that uses the discriminator field for OneOfSchema here: https://github.com/plangrid/flask-rebar/blob/master/flask_rebar/swagger_generation/marshmallow_to_swagger.py

However, since OneOfSchema comes from a separate package, dependency management gets a little hairier. We'd probably want to add something like:

try:
    from marshmallow_oneofschema import OneOfSchema
    # define converter, register it
except ImportError:
    pass

Or make it a separate plugin.
Or just add a recipe for the code in documentation, and expect people that need it to add it to copy/paste it into their own application.

@RookieRick
Copy link
Contributor

Hey folks, sorry I'm late to the party here (having just joined the team :D) - Doing some PR housekeeping.. If I've followed the conversations accurately, is it fair to say this will get rolled in with OpenAPI 3.0 support WIP?
cc @joeb1415

@RookieRick
Copy link
Contributor

Closing in favor of OpenAPI 3.0 support which should make this irrelevant

@RookieRick RookieRick closed this Apr 13, 2019
@joeb1415 joeb1415 deleted the export-supplemental-schemas branch May 8, 2019 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants