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

Handle load_only marshmallow schema #147

Closed
wants to merge 1 commit into from

Conversation

theirix
Copy link
Contributor

@theirix theirix commented Aug 20, 2017

A small fix that handles load_only schema parameter as well as dump_only. It allows to correctly handle schemas with Meta.load_only field.

@theirix
Copy link
Contributor Author

theirix commented Aug 20, 2017

Sorry, missed a test. It's an interesting question whether the dump_only field should appear when dump=False specified and load_only field for dump=True. Previously they appeared with a read_only=True swagger attribute and I doubt this is an expected behaviour.

@sloria
Copy link
Member

sloria commented Aug 24, 2017

@theirix I'm not sure about the failing test either. Intuitively, it does seem like load_only fields should be excluded if dump=True. Perhaps if the dump parameter isn't explicitly passed, both load_only and dump_only fields should appear. If dump=True is passed, load_only fields should not appear.

@theirix
Copy link
Contributor Author

theirix commented Sep 2, 2017

Yes, tri-state logic that you have described could help if we change signature for patched functions and a bunch of others to:

def schema2jsonschema(schema, spec=None, use_refs=True, dump=None, name=None):

And it is not a fully backward-compatible change. A patch can broke a library for those who uses load_only fields and expects these fields to be in result anyway. If apispec is used as a request/response serializer when dump parameter is explicitly specified or implied as a default parameter, a patch can be useful as it properly excludes load-only fields.

I don't insist in accepting this PR in a minor release. It's totally up to you to decide is it worth to change a behaviour in a such way.

@sloria
Copy link
Member

sloria commented Jul 15, 2018

Closing as this has gone stale, and I'm not sure this is an issue anymore. Feel free to re-open if it's still a valid issue.

@sloria sloria closed this Jul 15, 2018
@lafrech
Copy link
Member

lafrech commented Jul 16, 2018

This is still an issue. It is discussed in #119.

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.

@sloria
Copy link
Member

sloria commented Jul 16, 2018

OK, so it sounds like we'll need a different solution from this PR anyway.

@lafrech
Copy link
Member

lafrech commented Jul 16, 2018

Exactly.

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.

None yet

3 participants