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

Error on serialization of custom class - if attr is callable it is assumed its constructor takes no arguments #430

Closed
mirko opened this Issue Apr 13, 2016 · 7 comments

Comments

Projects
None yet
4 participants
@mirko

mirko commented Apr 13, 2016

The issue got introduced in 2.0.0b5 by the following diff and is still present in 2.7.1:

--- marshmallow-2.0.0b4/utils.py        2016-04-12 18:04:58.209260000 -0600
+++ marshmallow-2.0.0b5/utils.py        2016-04-12 18:06:52.287353000 -0600
@@ -316,7 +316,8 @@
         return obj[key]
     except (KeyError, AttributeError, IndexError, TypeError):
         try:
-            return getattr(obj, key)
+            attr = getattr(obj, key)
+            return attr() if callable(attr) else attr
         except AttributeError:
             return default
     return default

While trying to traverse and hence serialize certain objects, it might encounter an object which can't be indexed, calls the exception handler which tests it being callable. If so, it does.

In my scenario the object is a class which constructor requires an argument, which the attr() call doesn't provide. In that case it fails with the following backtrace (version 2.0.0b5):

  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/schema.py", line 526, in dump
    **kwargs
  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/marshalling.py", line 134, in serialize
    index=(index if index_errors else None)
  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/marshalling.py", line 64, in call_and_store
    value = getter_func(data)
  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/marshalling.py", line 128, in <lambda>
    getter = lambda d: field_obj.serialize(attr_name, d, accessor=accessor)
  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/fields.py", line 216, in serialize
    value = self.get_value(attr, obj, accessor=accessor)
  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/fields.py", line 166, in get_value
    return accessor_func(check_key, obj)
  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/utils.py", line 303, in get_value
    return _get_value_for_keys(key.split('.'), obj, default)
  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/utils.py", line 308, in _get_value_for_keys
    return _get_value_for_key(keys[0], obj, default)
  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/utils.py", line 323, in _get_value_for_key
    return attr() if callable(attr) else attr
TypeError: __init__() takes exactly 2 arguments (1 given)

For reference here a backtrace with version 2.7.1:

  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/schema.py", line 494, in dump
    **kwargs
  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/marshalling.py", line 146, in serialize
    index=(index if index_errors else None)
  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/marshalling.py", line 67, in call_and_store
    value = getter_func(data)
  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/marshalling.py", line 140, in <lambda>
    getter = lambda d: field_obj.serialize(attr_name, d, accessor=accessor)
  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/fields.py", line 241, in serialize
    value = self.get_value(attr, obj, accessor=accessor)
  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/fields.py", line 185, in get_value
    return accessor_func(check_key, obj, default)
  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/schema.py", line 402, in get_attribute
    return utils.get_value(attr, obj, default)
  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/utils.py", line 306, in get_value
    return _get_value_for_keys(key.split('.'), obj, default)
  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/utils.py", line 311, in _get_value_for_keys
    return _get_value_for_key(keys[0], obj, default)
  File "/home/mirko/projects/ims.new.new/local/lib/python2.7/site-packages/marshmallow/utils.py", line 323, in _get_value_for_key
    return attr() if callable(attr) else attr
TypeError: __init__() takes exactly 2 arguments (1 given)

Since I'm not fully aware of the intentions of above stated change, it seems rude to suggest reverting that change. However for my very case, it solves the problem.

**EDIT:
Responsible commit is the following:

commit 7c5dd90f42722fb0bfc0ec4ede911eee9c5adc89
Author: Alex Morken <alex@idealist.org>
Date:   Tue Jul 14 18:59:20 2015 -0700

    Updated utils._get_value_for_key return the called attribute if it is callable
@mirko

This comment has been minimized.

mirko commented Jun 16, 2016

any news about that commit which causes mentioned regression?
can we maybe just simply revert it?
@alexmorken ?

@alexmorken

This comment has been minimized.

alexmorken commented Jun 16, 2016

We needed it for a project I was working on, do you have any other possible solutions? Can I help you figure something else out so that we can both be happy?

EDIT:

Can you provide a test to break it? I completely understand your issue, but it would be great to see an example of it to work from.

@deckar01

This comment has been minimized.

Member

deckar01 commented Jul 12, 2016

I don't think that it is intuitive to call a callable unconditionally just because it is callable. Callables should not require explicit serialization handling to avoid being invoked with no arguments.

@alexmorken Instead of adding a feature that allowed you to resolve callables, I'm afraid you locked everyone else into your use case. Other frameworks I have seen only ever resolve callables when they are explicit arguments, not arbitrary data that is being serialized.

@alexmorken

This comment has been minimized.

alexmorken commented Jul 12, 2016

@deckar01 To be fair, the code was approved not by me and merged not by me - therefore not really me locking anyone into anything.

I am fine with changing the code and agree with your point about calling a callable unconditionally.

@deckar01

This comment has been minimized.

Member

deckar01 commented Jul 12, 2016

I feel ya. It was introduced at an appropriate time for breaking changes (v2.0.0b5), so reverting this would probably have to wait for v3.

@sloria sloria added this to the 3.0 milestone Jan 8, 2017

@sloria sloria modified the milestones: 3.0a, 3.0 Feb 18, 2017

@sloria

This comment has been minimized.

Member

sloria commented Feb 18, 2017

I am thinking we'll revert this and include in an early 3.0a release.

@sloria

This comment has been minimized.

Member

sloria commented Feb 19, 2017

This is done in d9dbae1

@sloria sloria closed this Feb 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment