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

3.0: DateTime fields cannot be used as inner field for List or Tuple fields #1357

Closed
zblz opened this issue Aug 21, 2019 · 8 comments · Fixed by #1359

Comments

@zblz
Copy link
Contributor

commented Aug 21, 2019

Between releases 3.0.0rc8 and 3.0.0rc9, DateTime fields have started throwing an error when being instantiated as inner fields of container fields like List or Tuple. The snippet below works in <=3.0.0rc8 and throws the error below in >=3.0.0rc9 (and, worryingly, 3.0.0):

from marshmallow import fields, Schema

class MySchema(Schema):
    times = fields.List(fields.DateTime())

s = MySchema()

Traceback:

Traceback (most recent call last):
  File "test-mm.py", line 8, in <module>
    s = MySchema()
  File "/Users/victor/.pyenv/versions/marshmallow/lib/python3.6/site-packages/marshmallow/schema.py", line 383, in __init__
    self.fields = self._init_fields()
  File "/Users/victor/.pyenv/versions/marshmallow/lib/python3.6/site-packages/marshmallow/schema.py", line 913, in _init_fields
    self._bind_field(field_name, field_obj)
  File "/Users/victor/.pyenv/versions/marshmallow/lib/python3.6/site-packages/marshmallow/schema.py", line 969, in _bind_field
    field_obj._bind_to_schema(field_name, self)
  File "/Users/victor/.pyenv/versions/marshmallow/lib/python3.6/site-packages/marshmallow/fields.py", line 636, in _bind_to_schema
    self.inner._bind_to_schema(field_name, self)
  File "/Users/victor/.pyenv/versions/marshmallow/lib/python3.6/site-packages/marshmallow/fields.py", line 1117, in _bind_to_schema
    or getattr(schema.opts, self.SCHEMA_OPTS_VAR_NAME)
AttributeError: 'List' object has no attribute 'opts'

It seems like it's treating the parent field as a Schema without checking that it is indeed a schema, so the schema.opts statement fails as fields don't have an opts attribute.

@sloria

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

Thanks for reporting. I don't think I'll have time to look into this until the weekend. Would you like to send a PR?

@sloria sloria added the bug label Aug 21, 2019

@zblz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

I'm afraid I don't have any time either, and I don't really have enough context on the _bind_to_schema process to make sure I'm not breaking stuff.

@sloria

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

OK, no problem. @lafrech Will you have a chance to look into this?

@zblz

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2019

I've found the patch below to fix the minimal example above, but I'm not really sure what it's missing out on or how to test it properly:

diff --git a/src/marshmallow/fields.py b/src/marshmallow/fields.py
index 0b18e7d..700732e 100644
--- a/src/marshmallow/fields.py
+++ b/src/marshmallow/fields.py
@@ -1114,7 +1114,7 @@ class DateTime(Field):
         super()._bind_to_schema(field_name, schema)
         self.format = (
             self.format
-            or getattr(schema.opts, self.SCHEMA_OPTS_VAR_NAME)
+            or getattr(getattr(schema, "opts", None), self.SCHEMA_OPTS_VAR_NAME, None)
             or self.DEFAULT_FORMAT
         )
@lafrech

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

git difftool 3.0.0rc8 3.0.0rc9 src/marshmallow/fields.py

When reworking container stuff, I changed

        self.inner.parent = self
        self.inner.name = field_name

into

        self.inner._bind_to_schema(field_name, self)

AFAIR, I did this merely to avoid duplication. On second thought, I think it was the right thing to do, not only for duplication but to actually bind inner fields to the Schema.

Reverting this avoids the error but the inner field's _bind_to_schema method is not called so I'm not sure it is desirable.

I think we really mean to call that method, not only in this case but also generally.

Changing

            or getattr(schema.opts, self.SCHEMA_OPTS_VAR_NAME)

into

            or getattr(self.root.opts, self.SCHEMA_OPTS_VAR_NAME)

might be a better fix. Can anyone confirm (@sloria, @deckar01)?

The fix in #1357 (comment) removes the error but also the feature: DateTime fields buried into container fields won't respect the format set in the Schema.

I didn't double-check that but AFAIU, the change I mentioned above (in container stuff rework) was the right thing to do. The feature was already broken (format set in Schema not respected if DateTime field in container field) and that's just one of the issues that may arise due to the inner field not being bound to the Schema. But I may be wrong.

@sloria

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

On quick glance, your analysis and fix look correct @lafrech

@lafrech

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

Let's do that, then.

Not much time either. The first who gets the time can do it.

For the non-reg tests :

1/ a test that checks the format set in the schema is respected if the DateTime field is in a container field

2/ a set of tests asserting the _bind_to_schema method of inner fields List, Dict, Tuple is called from container fields (we can use DateTime with the same test case for that)

Perhaps 1/ is useless if 2/ is done.

@deckar01 deckar01 self-assigned this Aug 21, 2019

@sloria sloria closed this in #1359 Aug 21, 2019

@sloria

This comment has been minimized.

Copy link
Member

commented Aug 21, 2019

Fix released in 3.0.1

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