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

`exclude` in Meta breaks with marshmallow 3 #131

Closed
yaheath opened this issue Jul 10, 2018 · 12 comments

Comments

@yaheath
Copy link

commented Jul 10, 2018

We have a pattern where we exclude some relations from the marshmallow schema like this:

class CustomerSchema(MyBaseSchema):
    class Meta(MyBaseSchema.Meta):
        model = Customer
        exclude = ('orders',)

Where Customer is a sqlalchemy model and orders is a relation from another table.

This works fine with marshmallow 2.x, but with marshmallow 3, an exception is thrown in the instantiation of CustomerSchema because marshmallow-sqlalchemy did not create the orders field, and so orders doesn't exist to be excluded.

See also marshmallow-code/marshmallow#877

@yaheath

This comment has been minimized.

Copy link
Author

commented Jul 10, 2018

as a somewhat kludgy workaround, I did this:

  class MyBaseSchemaOpts(ModelSchemaOpts):
      def __init__(self, meta, **kwargs):
          self.sa_exclude = getattr(meta, 'sa_exclude', [])
          super().__init__(meta, **kwargs)

  class MyBaseSchemaMeta(ModelSchemaMeta):
      @classmethod
      def get_fields(mcs, converter, opts, base_fields, dict_cls):
          if opts.model is not None:
              excludes = list([*opts.exclude, *opts.sa_exclude])
              return converter.fields_for_model(
                  opts.model,
                  fields=opts.fields,
                  exclude=excludes,
                  include_fk=opts.include_fk,
                  base_fields=base_fields,
                  dict_cls=dict_cls,
              )
          return dict_cls()

  class MyBaseSchema(with_metaclass(MyBaseSchemaMeta, ModelSchema)):
      OPTIONS_CLASS = MyBaseSchemaOpts
      ...etc...

  class CustomerSchema(MyBaseSchema):
      class Meta(MyBaseSchema.Meta):
          model = Customer
          sa_exclude = ('orders',)

But I'd like to know if there's a better approach.

@deckar01

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

Is the orders relationship explicitly declared on the Customer model? In my experience sqlalchemy is tolerant of relationships declared on both sides even though it is redundant. That might help nudge marshmallow-sqlalchemy into declaring the schema field (as a work around).

Can you add a minimal repro case to the description? (a working model + schema)

@yaheath

This comment has been minimized.

Copy link
Author

commented Jul 10, 2018

You misinterpreted the problem. The problem is I want to stop marshallow-sqlalchemy from declaring a field for a specific relation. I used exclude in the Meta to do this, but that breaks when using marshmallow 3 as explained above. In the workaround, I added sa_exclude to custom *Opts/*Meta classes which is used to indicate columns that I don't want added to the Schema.

I'm curious if that is a reasonable approach, or if there's a better way to do it.

@deckar01

This comment has been minimized.

Copy link
Member

commented Jul 10, 2018

I understand the problem. My suggestion is to trick marshmallow-sqlalchemy into declaring the relationship as a field during construction so that marshmallow doesn't panic when it tries to exclude the field.

The answers to these questions will help me troubleshoot workarounds:

Is the orders relationship explicitly declared on the Customer model?

Can you add a minimal repro case to the description? (a working model + schema)

@yaheath

This comment has been minimized.

Copy link
Author

commented Jul 11, 2018

Ok, let me rephrase.

If I do not explicitly exclude a column or relation, then marshmallow-sqlalchemy will create fields for all of the columns and relations. That is fine, and it works. There's no problem there.

Now, please consider the case where I do not want a field for a specific relation. So I add exclude=(relationname,) to Meta in my Schema class. With marshmallow 2.x, no problem. With marshmallow 3, an exception is thrown when I instantiate the schema. This is the bug I'm trying to report here.

If it's not actually a bug, then please explain how we're supposed to exclude a specific column/relation from a schema by default.

@yaheath

This comment has been minimized.

Copy link
Author

commented Jul 11, 2018

I can put together a simple repro tomorrow. In our models, the relations are declared using backref.

class Order(Base):
    customer_id = Column(BigInteger, ForeignKey('customer.id', ondelete='cascade', onupdate='cascade'), nullable=False)
    customer = relation('Customer', backref='orders')
    # ... etc
@deckar01

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

I did a quick sanity check to make sure marshmallow by itself is handling Meta.exclude correctly.

from marshmallow import fields, Schema

class Test(Schema):
    a = fields.Str()
    b = fields.Str()
    class Meta():
        exclude = ('a',)

Test().dump({'a': 'a', 'b': 'b'})
# {'b': 'b'}

In our models, the relations are declared using backref.

Have you tried using back_populates on both sides of the relationship?

class Parent(Base):
    __tablename__ = 'parent'
    id = Column(Integer, primary_key=True)
    children = relationship("Child", back_populates="parent")

class Child(Base):
    __tablename__ = 'child'
    id = Column(Integer, primary_key=True)
    parent_id = Column(Integer, ForeignKey('parent.id'))
    parent = relationship("Parent", back_populates="children")

http://docs.sqlalchemy.org/en/latest/orm/basic_relationships.html#one-to-many

That should help determine if it is an issue with all relationships or just implicit backrefs.

@yaheath

This comment has been minimized.

Copy link
Author

commented Jul 15, 2018

Ok, here's a repro: https://gist.github.com/yaheath/ff2d714da1559c4787bb9524248003b1
Turns out that a relation is not required.

Run using marshmallow 2.x, and you get:

CustomerSchema1 instantiated
CustomerSchema2 instantiated

Run using marshmallow 3.0, and you get:

CustomerSchema1 instantiated
Traceback (most recent call last):
  File "repro.py", line 34, in <module>
    cs2 = CustomerSchema2()
  File "/tmp/mm-sa-test/env/lib/python3.6/site-packages/marshmallow_sqlalchemy/schema.py", line 151, in __init__
    super(ModelSchema, self).__init__(*args, **kwargs)
  File "/tmp/mm-sa-test/env/lib/python3.6/site-packages/marshmallow/schema.py", line 356, in __init__
    self._update_fields(many=many)
  File "/tmp/mm-sa-test/env/lib/python3.6/site-packages/marshmallow/schema.py", line 755, in _update_fields
    raise ValueError(message)
ValueError: Invalid fields for <CustomerSchema2(many=False)>: {'created_time'}.
@deckar01

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

@yaheath Thanks for posting that repro and boiling it down to determine that it is unrelated to relationships. I was able to repro.

The marshmallow-sqlalchemy converter is skipping the excluded fields, so they never end up in declared_fields. That violates the semantics of what marshmallow expects declared_fields to contain at schema declaration time.

The simplest workaround is to wait to add the excludes to the schema until after all the fields have been loaded from the model. Here are a couple example workarounds based on your repro script:

# Exclude the fields at construction time.
cs2 = CustomerSchema1(exclude=('created_time',))

# Or if the schema class needs to be reused, inherit the full schema and exclude the fields.
class CustomerSchema2(CustomerSchema1):
    class Meta(ModelSchema.Meta):
        # This works, because the Meta class options are applied at schema declaration time,
        # and not inherited, so all the model's fields are already declared on the parent schema.
        exclude = ('created_time',)

cs2 = CustomerSchema2()

The proper fix is to prevent the converter from removing excluded fields and let marshmallow's core handle it. To avoiding unnecessary calls to property2field, the excluded fields could be assigned a placeholder value, like None. That should declare the model's field names, so that marshmallow can validate against them, without needing to generate field values that are destined to be discarded.

@deckar01

This comment has been minimized.

Copy link
Member

commented Jul 16, 2018

@sloria There is a unit test that covers this functionality, but the tests haven't been ran lately. I triggered a new build and confirmed it is passing for marshmallow 2, but failing for marshmallow 3.

https://github.com/marshmallow-code/marshmallow-sqlalchemy/blob/f0bddaf/tests/test_marshmallow_sqlalchemy.py#L841

https://travis-ci.org/deckar01/marshmallow-sqlalchemy/builds/404271138

It would be nice to catch issues like this before cutting a marshmallow release. Now that Travis supports custom runs (via the API and the GUI), it might be easier to run these tests more often. We could manually run dependent builds before cutting a release to catch blockers. The API and the GUI support custom variables, so dev commits of marshmallow could be installed directly from git. It would probably be a non-trivial amount of work to trigger all the builds, but if it was scripted, the dependent builds could even be triggered after each commit lands in dev using a hook in marshmallow's travis config.

https://docs.travis-ci.com/user/triggering-builds/

https://hiddentao.com/archives/2016/08/29/triggering-travis-ci-build-from-another-projects-build/

This functionality has been on and off Travis's road map for years, but they seem to be landing the building blocks to make it happen, but according to the closed issue it is currently not on their road map.

travis-ci/travis-ci#249

@sloria

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

Yeah, dependent builds would be a nice feature in Travis. Shame that it's not on the roadmap.

Nice work finding that test @deckar01 .

I won't have time to look into this bug this week. @deckar01 or @yaheath would you be up for sending a PR?

@sloria

This comment has been minimized.

Copy link
Member

commented Jul 20, 2018

The bugfix is released in 0.14.1.

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