flask_accepts icon indicating copy to clipboard operation
flask_accepts copied to clipboard

Issue with metaclass and Schemas in Nested fields

Open sbrieuc opened this issue 5 years ago • 2 comments
trafficstars

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, flask_accepts 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 avatar Mar 28 '20 10:03 sbrieuc

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 ?

sbrieuc avatar Mar 28 '20 10:03 sbrieuc

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.

apryor6 avatar Mar 28 '20 12:03 apryor6