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

Add extra fields to a definition #110

Merged
merged 2 commits into from Mar 3, 2017

Conversation

@lafrech
Copy link
Member

commented Mar 2, 2017

Sometimes, I need to add a field to a definition.

Typical use case: the discriminator field (search for "discriminator" in there, this comment also explains a lot and points some limitations). This can also be a vendor field (x-my_field).

I created a specific definition helper for that, but I think it could be in apispec itself. There is nothing specific in it. So here's the PR.

I first added this to marshmallow plugin schema_definition_helper, but then I realized it was not specific to marshmallow schemas, so I moved it to APISpec.definition. I'm leaving the two commits on purpose to expose the two approaches. I can squash into one or remove the last one depending on which approach is preferred.

This feature is pretty permissive. A more conservative approach would be to allow only fields that conform to the spec: a finite hardcoded list à la _VALID_PROPERTIES + the possibility to pass any vendor field (x-). I'm not sure this is needed. After all, the user adds extra_fields explicitly, so he should know what he is doing. It's not like we're harvesting in some Schema's metadata. BTW, I don't think discriminator should be defined in a field's metadata. It is schema level stuff. It shouldn't be in _VALID_PROPERTIES, right?

I'd be happy to hear your feedback about this.

@sloria

This comment has been minimized.

Copy link
Member

commented Mar 3, 2017

Thanks @lafrech for the contribution . I think this serves a very common use case, so lgtm. 👍

@sloria sloria merged commit f467962 into marshmallow-code:dev Mar 3, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sloria

This comment has been minimized.

Copy link
Member

commented Mar 3, 2017

This feature is pretty permissive. A more conservative approach would be to allow only fields that conform to the spec: a finite hardcoded list à la _VALID_PROPERTIES + the possibility to pass any vendor field (x-). I'm not sure this is needed.

I agree. For maintainability, I think it's best that apispec not try too hard to validate inputs. Perhaps we should add some docco that mentions this...

@sloria

This comment has been minimized.

Copy link
Member

commented Mar 3, 2017

BTW, I don't think discriminator should be defined in a field's metadata. It is schema level stuff. It shouldn't be in _VALID_PROPERTIES, right?

Good catch. Fixed.

@lafrech lafrech deleted the Nobatek:dev_def_extra_fields branch Apr 4, 2018

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