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

New feature for iterating over fields_dict in marshalling/unmarshalling #718

Closed
podhmo opened this issue Jan 6, 2018 · 7 comments
Closed

Comments

@podhmo
Copy link

podhmo commented Jan 6, 2018

jsonschema(and swagger) have specific options for unexpected additional fields.

And, I want to translate this feature to marshmallow schema.

marshmallow's current implementation

The way of iteration in marshmallow's current implementation, is just calling iteritems() with fields_dict. So, it is difficult that define schema allowing unexpected field.

for examples, in swagger (openAPI2.0) the schema definition, like bellow.

definitions:
  person:
    type: object
    properties:
      name:
        type: string
    additionalPropeties: integer
    required:
      - name

Then, about person schema, passing {"name": "foo"} is ok, and {"name": "foo", "x": 20, "y": 10} is also ok(unexpected field is treated as integer).

it is a bit difficult, making marshmallow's version of this.
(schema meta's additional is candidate of expected field names, ... and so on.)

proposal

If marshmallow has new hook point that changing the way of iteration, it is enable to implement marshmallow's version, more easily.

For example, get_marshalling_iterator()(this is not good name), is existed in Schema class.

# def default_iterator(data, fields_dict):
#     return iteritems(fields_dict)


class S(Schema):
    def get_marshalling_iterator(self, additional_field=fields.Int()):
        def iterate_with_additional_field(data, fields_dict):
            for k in data:
                field = fields_dict.get(k, additional_field)
                yield k, field

        return iterate_with_additional_field

    name = fields.String(required=True)
  • 💭 get_marshalling_iterator is not good name, maybe
  • 💭 the position of hook point(Schema class's instance method) is not good, maybe
@deckar01
Copy link
Member

deckar01 commented Apr 3, 2018

Have you tested this with https://github.com/marshmallow-code/apispec? If that is part of the 2.0 spec and it isn't compliant we might get more traction over there.

@podhmo
Copy link
Author

podhmo commented Apr 6, 2018

Maybe, apispec's motivation is reversed for me.

  • apispec reads marshmallow's definition, and generating swagger doc
  • foo (my motivated tool) reads swagger doc, and generating python code (marshamllow's definition)

(in my project https://github.com/podhmo/swagger-marshmallow-codegen, supporting additionalProperties, but this code is not good for me (maybe this is not comprehensive support))

@deckar01
Copy link
Member

deckar01 commented Apr 6, 2018

I imagine it could be implemented with a schema Meta attribute named additional_properties that takes a field value.

This feature might also be needed to fix an incompatibility in apispec. Maybe they only implemented the subset of OpenAPI features that marshmallow could already express though.

@podhmo
Copy link
Author

podhmo commented Apr 6, 2018

Ah, yes, it is better that supporting this via Meta attribute, instead of class's hook.
OK, if supporting additional_properties, it is OK for me.

@lafrech
Copy link
Member

lafrech commented Apr 6, 2018

Maybe they only implemented the subset of OpenAPI features that marshmallow could already express though.

Yes, absolutely. If this feature is added to marshmallow, apispec can be modified to expose additional_properties in the spec.

@podhmo
Copy link
Author

podhmo commented Apr 13, 2018

It is decided that supporting additional_properties via Meta attribute, closing this issue, and creating new issue, is better?

@sloria
Copy link
Member

sloria commented Jul 5, 2018

It is now possible to allow additional properties to be included in deserialized data (see #524).

I believe the requested feature here would be covered by #853. Let's continue discussion there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants