691717 - wbo.py: AttributeError: 'str' object has no attribute 'items'

Status: VERIFIED FIXED

(Cloud Services Graveyard :: Server: Sync, defect)

Description

[:Atoll]

Seen as far back as 2011-06-30, so this isn't new.


2011-10-03 17:19:21,991 ERROR [syncserver] 7d2c98e190166db400ca673d7e374f7e
2011-10-03 17:19:21,991 ERROR [syncserver] Traceback (most recent call last):
  File "/usr/lib/python2.6/site-packages/services/util.py", line 495, in __call__
    return self.app(environ, start_response)
  File "/usr/lib/python2.6/site-packages/paste/translogger.py", line 68, in __call__
    return self.application(environ, replacement_start_response)
  File "/usr/lib/python2.6/site-packages/webob/dec.py", line 147, in __call__
    resp = self.call_func(req, *args, **self.kwargs)
  File "/usr/lib/python2.6/site-packages/webob/dec.py", line 208, in call_func
    return self.func(req, *args, **kwargs)
  File "/usr/lib/python2.6/site-packages/services/baseapp.py", line 191, in __notified
    response = func(self, request)
  File "/usr/lib/python2.6/site-packages/services/baseapp.py", line 270, in __call__
    result = function(request, **params)
  File "/usr/lib/python2.6/site-packages/syncstorage/controller.py", line 346, in set_collection
    wbo = WBO(wbo)
  File "/usr/lib/python2.6/site-packages/syncstorage/wbo.py", line 53, in __init__
    for name, value in data.items():
AttributeError: 'str' object has no attribute 'items'

Comment 1

[Ryan Kelly [:rfkelly]]

Attachment: patch to fail gracefully when JSON contains unexpected datatypes

This seems to be a result of sending unexpected JSON to the server.  It receives a string where it expects a dict, which then gives this error when passed into the WBO constructor.

This patch adds some basic type-checking to fail gracefully under such conditions.  Usually I hate using isinstance() but it seems like the cleanest solution here, since we know that json.loads() will return a proper instance of dict.

Comment 2

[:Atoll]

Comment on attachment 584905 [details] [diff] [review]
patch to fail gracefully when JSON contains unexpected datatypes

Seems reasonable, but this invalid JSON bypasses the isinstance checks:

{'id': ["1", "2"], 'payload': {'3': '4'}}

Rather than directly using isinstance on the objects, perhaps this could be wrapped in a simple exception handler that returns WEAVE_INVALID_WBO if the wbo parsing triggers an exception of any sort?

Comment 3

[Toby Elliott [:telliott]]

rather than sprinkling these throughout the controller, it seems like we could make the various checks part of the wbo __init__ method. Then if we add more, we don't need to make sure they go everywhere. Then initializing the wbo becomes 

try:
 wbo = WBO(data)
except Exception:
 ....

Comment 4

[Ryan Kelly [:rfkelly]]

> Seems reasonable, but this invalid JSON bypasses the isinstance checks:
> {'id': ["1", "2"], 'payload': {'3': '4'}}

Right, but this should be detected by the subsequent call to wbo.validate().  (Turns out this particular case is *not* detected cleanly, so I've added some more checks to that method)

> Rather than directly using isinstance on the objects, perhaps this could be wrapped in a
> simple exception handler that returns WEAVE_INVALID_WBO if the wbo parsing triggers an 
> exception of any sort?

I always hesitate to add generic "except Exception" clauses since they might mask unrelated errors down the track.

So as a compromise: the revised patch moves the checks into WBO.__init__ and has it raise ValueError if things don't look right.  The controller code can then catch *just* ValueError and report it back to the client, while still giving a stack trace if I screw something up in wbo.py in the future.

Comment 5

[Ryan Kelly [:rfkelly]]

Attachment: patch to fail gracefully when WBO data contains unexpected types

Comment 6

[Toby Elliott [:telliott]]

Comment on attachment 585349 [details] [diff] [review]
patch to fail gracefully when WBO data contains unexpected types

Yeah, that's what I meant - figure out what exception we want to throw, then catch it. Looks good to me.

Comment 7

[Ryan Kelly [:rfkelly]]

Committed with some flake8 cleanups in http://hg.mozilla.org/services/server-storage/rev/689923bdefff