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

Issue with metaclass and Schemas in Nested fields #66

Open
sbrieuc opened this issue Mar 28, 2020 · 2 comments
Open

Issue with metaclass and Schemas in Nested fields #66

sbrieuc opened this issue Mar 28, 2020 · 2 comments

Comments

@sbrieuc
Copy link
Contributor

sbrieuc commented Mar 28, 2020

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
Copy link
Contributor Author

sbrieuc commented Mar 28, 2020

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
Copy link
Owner

apryor6 commented Mar 28, 2020

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.

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

No branches or pull requests

2 participants