apryor6 / flask_accepts

Easy, opinionated Flask input/output handling mixing Marshmallow with flask-restx
BSD 3-Clause "New" or "Revised" License
169 stars 40 forks source link

Issue with metaclass and Schemas in Nested fields #66

Open sbrieuc opened 4 years ago

sbrieuc commented 4 years ago

Hi,

This refers to #39 and #40

I'm in a situation where I use flask_accepts in association with flask_marshmallow and marshmallow_sqlalchemy.

Basically, all schemas that I define are not constructed from marshmallow base Schema class directly but from another class SQLAlchemySchema (which itself inherits from marshmallow base Schema). In that configuration, when trying to use these Schemas in a Nested field, flaskaccepts throws a TypeError as seen in #39: "Unknown type for marshmallow model field was used."_

Long story short after looking into it, I can see that SQLAlchemySchema class uses its own metaclass (inherited from marshmallow SchemaMeta) which prevents #40 from working as it uses isinstance to check the type of the provided Schema.

In example (simplified), we have:

from marshmallow.schema import Schema, SchemaMeta

class SQLAlchemyMeta(SchemaMeta):
    ...

class SQLAlchemySchema(Schema, metaclass=SQLAlchemyMeta):
    ...

which leads to the following:

>>> isinstance(type(SQLAlchemySchema), SchemaMeta)
False

and causes the error because type(SQLAlchemySchema) returns SQLAlchemyMeta and not SchemaMeta.

I think we can solve this by using issubclass instead of isinstance in #40, which would give:

def map_type(val, api, model_name, operation):
    value_type = type(val)
    if value_type in type_map:
        return type_map[value_type](val, api, model_name, operation)

    if issubclass(value_type, SchemaMeta) or issubclass(value_type, Schema):
        return type_map[Schema](val, api, model_name, operation)

    raise TypeError('Unknown type for marshmallow model field was used.')

I've implemented this change in my project and all seems to work fine.

I'm more than willing to make a PR for this but I'd like to have feedback on my analysis and make sure I did not miss anything.

sbrieuc commented 4 years ago

I'm noticing that the issue occurs only when using the Schema class and not with an instance.

Does not work:

class Foo(Schema):
    pass

class Bar(SQLAlchemySchema):
    items = fields.Nested(Foo)

Works:

class Foo(Schema):
    pass

class Bar(SQLAlchemySchema):
    items = fields.Nested(Foo())

So I'm not sure of the direction to take here: always providing an instance of the schema will do the trick and we can leave it as it is, but I feel that it's not necessarily the best practice in term of memory usage. What do you think ?

apryor6 commented 4 years ago

I totally agree that issubclass would be more suitable and would be more than happy to accept a PR. Just add a test that asserts behavior works as intended if you provide either type of Schema and we should be good to go.