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

Ignore `load_only` fields if `dump=True` in `fields2jsonschema`? #119

Closed
luisincrespo opened this issue Mar 15, 2017 · 15 comments

Comments

@luisincrespo
Copy link

commented Mar 15, 2017

Hi,

I'm making use of https://github.com/jmcarp/flask-apispec to automatically generate docs in a personal flask project. This library makes use of your apispec swagger extension to generate docs for requests and responses. I figured out that responses in the generated docs where including marshmallow's load_only fields, which (at least in my case) is not convenient. In the line that I'm linking below you're excluding dump_only fields if dump=False when invoking that method. Do you think it would be a good idea to also ignore load_only fields if dump=True?

https://github.com/marshmallow-code/apispec/blob/dev/apispec/ext/marshmallow/swagger.py#L492

I'm opening this for discussion, and I'll be happy to create a PR for that in case you are ok with the proposed functionality.

@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 15, 2017

We might have an issue, here, because schema_definition_helper calls schema2jsonschema which in turn calls fields2jsonschema.

When adding a definition, we don't want to exclude fields. The schemas in definitions can be used for both input and output, so we want all the fields, with the dump_only marked readOnly, and the load_only... well we can't mark them writeOnly because this does not exist, and in practice the only workaround I know is to create two schemas registered separately.

I suppose you could use that workaround too (yes, it sucks...).

Or we could try to modify fields2jsonschema for that, but dump parameter as a boolean is unfit as we have three cases: dump_only, load_only, or dump_and_load (which should be the default).

Maybe the whole dump parameter thing ought to be reviewed to support those three cases. For instance, we could have dump_only (boolean) and load_only (boolean) as separate parameters. And having both set would raise an error (no point in having a schema used for neither dumping or loading).

@luisincrespo or anyone, please correct me if I'm wrong.

@luisincrespo

This comment has been minimized.

Copy link
Author

commented Mar 15, 2017

Or we could try to modify fields2jsonschema for that, but dump parameter as a boolean is unfit as we have three cases: dump_only, load_only, or dump_and_load (which should be the default).

@lafrech as you said, "dump_and_load" is the default, so I think the dump boolean arg suffices, I mean:

  • If you're creating a response definition (dump=True) you only have to worry about ignoring load_only fields, everything else shouldn't be ignored (unless inside exclude).
  • If you're creating a request definition (dump=False) you only have to worry about ignoring dump_only fields, everything else shouldn't be ignored (unless inside exclude).
@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

I'm not sure I understand. Would you care to illustrate this with a code example ?

If it's not too much work, you may provide it in the form of a PR draft.

Thanks.

@luisincrespo

This comment has been minimized.

Copy link
Author

commented Mar 16, 2017

Here's a en example to illustrate what I mean:

import pprint
from apispec import APISpec
from flask import Flask, jsonify
from marshmallow import Schema, fields

spec = APISpec(
    title='Swagger Example',
    version='1.0.0',
    plugins=[
        'apispec.ext.flask',
        'apispec.ext.marshmallow',
    ],
)


class SomeSchema(Schema):
    class Meta:
        dump_only = ('dumpAttr1',)
        load_only = ('loadAttr1',)

    attr1 = fields.Str()
    dumpAttr1 = fields.Str()
    loadAttr1 = fields.Str()


app = Flask(__name__)


@app.route('/get')
def get():
    """A get endpoint.
    ---
    get:
        description: Get data
        responses:
            200:
                description: Data to be returned
                schema: SomeSchema
    """
    some_data = dict(attr1='attr1', dumpAttr1='dumpAttr1')
    return jsonify(SomeSchema().dump(some_data).data)


ctx = app.test_request_context()
ctx.push()

spec.add_path(view=get)
pprint.pprint(dict(spec.to_dict()['paths']))

"""
This prints:

{'/get': {'get': {'description': 'Get data',
                  'responses': {200: {'description': 'Data to be returned',
                                      'schema': {u'properties': {'attr1': {u'type': u'string'}, 'dumpAttr1': {u'type': u'string'}, 'loadAttr1': {u'type': u'string'}},
                                                 u'type': u'object'}}}}}}
"""

Notice how in the schema properties for the definition of the response, loadAttr1 is included, which is load_only in SomeSchema, and that's what I don't want to happen. I want load_only attributes to be excluded from the definition of the response properties.

I hope this clarifies the issue @lafrech.

@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

I totally understand this. I meant would you like to add an example of the change you would like to make in fields2jsonschema?

Alternatively, you may send a PR, even a draft, with the change you want. That would be the best illustration. Unless it is too much work.

@luisincrespo

This comment has been minimized.

Copy link
Author

commented Mar 16, 2017

ohhh I see, yeah no problem, I can open a PR with the potential change 👍

@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

Sorry, I could have been clearer.

@luisincrespo

This comment has been minimized.

Copy link
Author

commented Mar 16, 2017

I opened this PR #121, that's what I had in mind, haven't tested it though, let me know what you think @lafrech

@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 19, 2017

The dump attribute was written to specify that the output is used only for input parsing (parameters), therefore dump_only fields should be removed.

Symmetrically, there could be a load field to specify that the output is only used for output dumping (responses).

But you can't use the same boolean for that, because the output of schema2jsonschema can be used for both load and dump: when it is used to create a model definition from a schema. This is what proposed change breaks (see the test I added here: #123).

IOT, just because dump is False, does not mean you can assume the schema is used for loading only. It can be used for both, in which case you want both dump_only and load_only fields to appear.

We could add a symmetrical load parameter for those using schema2jsonschema to create a response description. We could also rename dump and load into dump_only and load_only if it makes them more explicit (I'm not sure about that, but why not). Now would be the right moment since we already modified dump in a breaking way recently.

Let me share my personal experience. I generally don't parse response schema. In practice, the schemas I use in my application are declared as definitions, as they are used in several places (at least in args load / output dump of the POST method for instance). The only use case I have for a load_only field is a password field. In this case, well, the issue is that writeOnly does not exist as an OpenAPI field attribute. The only way OpenAPI offers to deal with this is to create a second schema without the password field. This is what I do. And doing this, I don't need the change proposed by @luisincrespo.

@luisincrespo

This comment has been minimized.

Copy link
Author

commented Mar 21, 2017

@lafrech thanks for your explanation. That makes sense, it'll be breaking to use the dump=False to assume that we're building a schema for loading only.

Let me share my personal experience. I generally don't parse response schema. In practice, the schemas I use in my application are declared as definitions, as they are used in several places (at least in args load / output dump of the POST method for instance). The only use case I have for a load_only field is a password field. In this case, well, the issue is that writeOnly does not exist as an OpenAPI field attribute. The only way OpenAPI offers to deal with this is to create a second schema without the password field. This is what I do. And doing this, I don't need the change proposed by @luisincrespo.

I am doing something similar at the moment, but instead of having a separate schema I programmatically (using some meta-programming) remove the load_only fields from the schema used for responses.

I could improve my existing PR to include both dump_only and load_only attrs in schema2jsonschema, but if you feel its use-case is not that common/useful, I won't bother and will close the PR.

@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 21, 2017

I suggest you close the PR for now and leave this issue open, and wait for feedback from @sloria before you code the dump_only / load_only change.

@sloria

This comment has been minimized.

Copy link
Member

commented Mar 23, 2017

Thank you for opening this discussion, @luisincrespo and @lafrech .

True, the way to work around this issue is to have separate schemas for input vs. output. True, this isn't ideal, but it's not the worst workaround.

I would be open to adding a load parameter that would be used when translating schema for input, though I haven't given much thought to how much complexity would be involved to pass both load and dump correctly. @luisincrespo Would you like to take a stab at a PR?

@frol

This comment has been minimized.

Copy link
Contributor

commented Apr 28, 2018

Ooops. Any progress here?

@sloria

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

I don't have time to deep-dive into this in the near future, but PRs definitely welcome. Would be great to resolve this before 1.0.0.

@lafrech lafrech added this to the 1.0 milestone Jul 27, 2018

@lafrech

This comment has been minimized.

Copy link
Member

commented Jul 27, 2018

There are three possible cases:

  • Load only: parameter schema
  • Dump only: response schema
  • Load and dump: definition

A boolean dump parameter is not enough. We need either two parameters or a tri-state parameter. I don't mind either way.

Anyone willing to tackle this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.