Cornices/cornice

marshmallow_validator seems to break with marshmallow version 3.0.1

Closed this issue · 12 comments

In my pyramid project I just replaced my marshmallow version from 2.x to 3.0.1 and things broke when I was using marshmallow_body_validator by returning something like the following:

Traceback (most recent call last):
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid_debugtoolbar/toolbar.py", line 257, in toolbar_tween
    response = _handler(request)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid_debugtoolbar/panels/performance.py", line 58, in resource_timer_handler
    result = handler(request)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid_tm/__init__.py", line 171, in tm_tween
    reraise(*exc_info)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid_tm/compat.py", line 36, in reraise
    raise value
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid_tm/__init__.py", line 136, in tm_tween
    response = handler(request)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid/tweens.py", line 43, in excview_tween
    response = _error_handler(request, exc)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid/tweens.py", line 13, in _error_handler
    response = request.invoke_exception_view(exc_info)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid/view.py", line 767, in invoke_exception_view
    request_iface=request_iface.combined,
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid/view.py", line 667, in _call_view
    response = view_callable(context, request)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid/viewderivers.py", line 401, in viewresult_to_response
    result = view(context, request)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid/tweens.py", line 41, in excview_tween
    response = handler(request)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid/router.py", line 148, in handle_request
    registry, request, context, context_iface, view_name
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid/view.py", line 667, in _call_view
    response = view_callable(context, request)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid/config/views.py", line 169, in __call__
    return view(context, request)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid/config/views.py", line 188, in attr_view
    return view(context, request)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid/config/views.py", line 214, in predicate_wrapper
    return view(context, request)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid/viewderivers.py", line 325, in secured_view
    return view(context, request)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid/viewderivers.py", line 436, in rendered_view
    result = view(context, request)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/pyramid/viewderivers.py", line 144, in _requestonly_view
    response = view(request)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/cornice/service.py", line 493, in wrapper
    validator(request, **args)
  File "/path-to-project/src/myproj/myproj/views/__init__.py", line 80, in my_stats_validator
    return marshmallow_body_validator(request, **kwargs)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/cornice/validators/_marshmallow.py", line 99, in _validator
    validator(request, RequestSchema, deserializer, **kwargs)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/cornice/validators/_marshmallow.py", line 166, in validator
    deserialized = schema.load(cstruct)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/marshmallow/schema.py", line 684, in load
    data, many=many, partial=partial, unknown=unknown, postprocess=True
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/marshmallow/schema.py", line 799, in _do_load
    unknown=unknown,
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/marshmallow/schema.py", line 639, in _deserialize
    index=index,
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/marshmallow/schema.py", line 483, in _call_and_store
    value = getter_func(data)
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/marshmallow/schema.py", line 632, in 
    val, field_name, data, **d_kwargs
  File "/path-to-project/venv-3.6/lib/python3.6/site-packages/marshmallow/fields.py", line 329, in deserialize
    output = self._deserialize(value, attr, data, **kwargs)
TypeError: _deserialize() got an unexpected keyword argument 'partial'

after some debugging I realised that the problem was with cornice/validators/_marshmallow.py. Using the following patch fixed things, but I'm not sure that it won't break things elsewhere or if indeed partial will be supported, hence pinging @ergo :).

Here's the suggested unified diff:

--- _marshmallow.py     2019-09-02 15:48:04.811496406 +0300
+++ _marshmallow2.py    2019-09-02 15:59:54.109394801 +0300
@@ -49,7 +49,7 @@
         schema = _instantiate_schema(schema)
 
         class ValidatedField(marshmallow.fields.Field):
-            def _deserialize(self, value, attr, data):
+            def _deserialize(self, value, attr, data, **kwargs):
                 schema.context.setdefault('request', request)
                 deserialized = schema.load(value)
                 # marshmallow 2.x returns a tuple, 3/x will always throw
ergo commented

This looks good to me.

Any update on this?

Hi @chrisgeru, the code that fixes the incompatibility is ready since a few months now (see PR #515); what I haven't had time to do is to write the appropriate tests for it...will do that when I find some free time (it'll be ready this month).

Hello! I just stumbled upon this, too. How far are you with the PR? Any way to support? Just wouldn't like to do the work twice if you are in the middle of it already.

Hi @elbart, it's been a while since I wrote the code that fixes the problem (see PR #515); what I haven't had the time to work with is to write tests for it (it needs some conditional runs for tox, which I haven't worked with before, and I'm always postponing it...). If you want to help with the tests, then be my guest!!! :) If not, I had planned to start working on them next week, so it shouldn't take long.

Do note, however, that through the process of debugging this issue, I stumbled across some weird behaviour with the marshmallow validator (irrelevant to the issue we're talking about here), which seem to affect its robustness and of which I've wrote down in the PR, where unfortunately nobody has replied on. Again, this doesn't affect the current issue, it has been there since a long time now, but it has make me understand that the validator may need some refactoring at some point.

PS. In terms of my systems, I'm using my patched version of cornice/validators/_marshmallow.py, that's why I'm not in such a hurry :).

@mamalos Thanks, I didn't want to be rude or something. I could try to support you here, but I am pretty new, too :)

However I am a bit puzzled about this PR #515. I just cloned the cornice repo and took a look into the master branch, which already contains a commit from you from Sep 2019, which seems to fix the respective problem already (cf: 3d46848). Was it just not tagged / released yet or is #515 required, because the tests are missing in the above mentioned commit? Best regards and thanks for your awesome work!

Tim

@elbart, please don't apologise, I'm the one that has to apologise for not having written correctly what I meant...:( By no means did I want to sound harsh/rude or anything, I was just trying to write down the facts and add some fun/smile/character in there -where I obviously failed.

In terms of the cf you're mentioning (which is for PR #516), this adds some -critical to my understanding- functionality that was missing from cornice's marshmallow validator's helper, but it doesn't address the incompatibility issue. PR #515 does that (and I decided to split these fixes in two PR's exactly because they're addressing different things), where as we said, the tests are missing, but I'll get to them by Friday (don't think I'll finish them, but I think by next week I'll be done with them).

Again, I'm terribly sorry for the misunderstanding...

Hey everybody, I need some help here please:

I tried to complete PR #515 so that it's finally merged, but I stumbled across various obstacles with testing and coverage - which is not where I shine-, and after lots of studying I'm unable to solve the following problem:

Some code is only executed under python2 and some only under python3; likewise, some code is only executed with marshmallow 2.x while some other only with marshmallow 3.x. How can we combine the different coverages to a single, cumulative coverage so that tests on travis don't fail - or more generically, how do we resolve this issue?

Both PR #515 and #516 have been merged. Is this enough to close this issue?

@sponsfreixes, if you have added all the relevant tests and they are passing, then I'm more than happy to proceed. Please, do note that I have asked a couple of things (here and here) which made me realise that the current code doesn't seem very correct (by current I don't mean after I touched it :-) ), and somebody should check that it works as it is supposed to, but since the issues I'm referring to are not directly related to these two PRs, then we could move on.

Uh, I don't know how but when I reviewed #515 the other day I read it as already merged, so I thought the tests had been included. I see now it's still open, my bad!

Please test current master 72c52848f44bce68cf5c0a110 and reopen PR/issues for specific issues 🙏