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

Support custom Marshmallow fields #120

Closed
frol opened this issue Mar 16, 2017 · 15 comments

Comments

@frol
Copy link
Contributor

commented Mar 16, 2017

Currently, all custom fields automatically become type="string", format=None due to the hard-coded FIELD_MAPPING. Thus, I cannot extend any marshmallow type, e.g. Dict or Number with apispec support. Currently, the only way to extend apispec support is to patch the FIELD_MAPPING dict.

@frol

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2017

One solution to that would be to extend the implementation of _get_json_type_for_field by adding a loop going through the base field types using isinstance() to detect the best matching field type. The loop will only be used when no exact FIELD_MAPPING is found, so it should not hit the performance in general.

NOTE: The list of the base fields should be maintained separately in this case as the order matters (e.g. Integer should be tried before Number).

Another solution would be to introduce a field attribute, e.g. swagger_type = ('integer', 'int32'), so if the mapping fails, _get_json_type_for_field would look for that attribute before returing the default ('string', None).

The solutions can be combined to enable maximum customization.

@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

Our framework modifies hardcoded FIELD_MAPPING: https://github.com/Nobatek/flask-rest-api/blob/master/flask_rest_api/spec/__init__.py#L76

I find this a bit inelegant, but @sloria says it's fine (see #88).

@frol

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2017

@lafrech Thanks for sharing. Let's wait for @sloria comments on the proposed solutions.

@sloria

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

@frol I like both of your suggestions above, and I think they would work well in concert. I would certainly review and merge a PR implementing them.

@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

I'd really like a way to register a new "field <-> type" association. Like what I currently do in the example I posted by appending to FIELD_MAPPING.

In practice, I have a few fields that are specific to my application, but I use them a lot in the code, so having to specify swagger_type each time I instantiate them would be a pain.

@frol

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2017

@lafrech I don't see a point in passing swagger_type to field instantiation. I meant to have it as a field class attribute:

class MySuperInteger(base_fields.Integer):
    swagger_type = ('integer', 'int32')
    # ...

class MySchema(Schema):
    this_is_my_field = MySuperInteger()
@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

Sorry @frol, I misunderstood.

There is still the case where you get those custom fields from a third-party library. Well, I suppose you can monkeypatch them in your code.

from somelib import SomeField
SomeField.swagger_type = ('integer', 'int32')
@frol

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2017

@lafrech Good point. Only the first of my suggested solutions can partially address this use-case. To avoid such monkey-patching of either the field itself or the FIELD_MAPPING in apispec, some sort of field mapping registering should be implemented on the apispec side as a part of user API. I think, the second solution is counterproductive as it requires cooperation from the field class while it is absolutely doable on the apispec side.

@lafrech

This comment has been minimized.

Copy link
Member

commented Mar 16, 2017

@frol Yes, this is what I had in mind when opening #88, but then @sloria said modifying FIELD_MAPPING was fine so I let it go.

Adding an explicit API to it would be nice, though.

We may want to copy FIELD_MAPPING to an attribute of APISpec at init time, and register custom fields in that instance copy. The point is that if you have an app that you import once then instantiate and initialize several times (not the typical production use case, but it can happen in tests, for instance), you don't really start from scratch the second time because FIELD_MAPPING is already modified.

@sloria

This comment has been minimized.

Copy link
Member

commented Mar 17, 2017

I like @lafrech 's idea of copying FIELD_MAPPING.

Maybe we could add an APISpec method for mapping a field to a swagger type and format. Something along the lines of

@spec.map_to_swagger_type('int', 'int32')
class MyNumber(Field):
    # ...

# and/or

spec.map_to_swagger_type(MyNumber, 'int', 'int32')
@dradetsky

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2017

This just came up for me. I was going to suggest extending _get_json_type_for_field to walk up the inheritance tree if no match was found. It would eventually hit Field, so no default would be required. Of course, this might get complicated with multiple inheritance, and in any case you'd still need to be able to actually override the parent def in some cases.

I'm going to hack together a modified version of @sloria's solution, except within the marshmallow plugin, rather than the core module (b/c i have no idea how bottle/flask/tornado work)

@dradetsky

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2017

So I created a decorator with a similar name, a (not very good or clean) test, and some not terribly helpful documentation. It's not ideal, but if you need this feature now (I do), it does the job.

There are 2 prs:

#140: the actual pr, with merge conflicts
#141: like the actual pr, but rebased. I wasn't sure I did it right, so I thought I'd just make a separate pr.

Assuming it's acceptable, my preference is for pushing the new content to #140 & merging that one, then closing #141 (rather than merging #141). But I don't suppose it makes much difference.

@sloria

This comment has been minimized.

Copy link
Member

commented Aug 5, 2017

@dradetsky Thank you for working on this. I think we can go ahead and merge #141 (it looks like you rebased correctly there) once my suggested changes have been addressed.

@wobeng

This comment has been minimized.

Copy link

commented Aug 14, 2017

any update on this?

@sloria

This comment has been minimized.

Copy link
Member

commented Aug 15, 2017

The swagger.map_to_swagger_type decorator is available in 0.24.0, which has just been released to the PyPI.

@sloria sloria closed this Aug 15, 2017

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