812432 - Refactor server-syncstorage to use cornice validators, decorators and error-reporting

Status: RESOLVED FIXED

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

Description

[Ryan Kelly [:rfkelly]]

Attachment: new "views" submodule to replace the controller class

This is a substantial refactoring of the view/controller code in sync2.0.  It removes the "controller" class, which was largely a hangover from the pre-pyramid codebase, and replaces it with a suite of cornice validator and decorator functions.

The primary aim here is to use cornice's validation features for error reporting, per Bug 784592.  But I think it also makes the code a bit simpler overall, by removing a level of indirection (from [Service => Controller => Storage] to just [Service => Storage]).

The first patch replaces the existing controller.py and views.py with a "views" submodule.  I'm attaching the contents as a zipfile since they're all new files, so the diff would be useless.

Each of the functions in "validators.py" and "decorators.py" basically takes over one aspect of the functionality of the controller.  For example, there's a function to extract and validate the precondition headers, one to extract and validate the querystring params, one to obtain collection-level locks while executing the view, and so-on.

The main __init__.py then ties these all together into a service definition.  Where these functions previously just called out the Controller method with the same name, they now talk to the storage backend directly.

Comment 1

[Ryan Kelly [:rfkelly]]

Attachment: extra changes necessary to work with the refactoring

The second part is the diff for the other parts of the codebase.  These are small changes to work with the refactoring, such as:

  * changing tests to expect cornice-format error responses
  * reworking various bits that interact with the controller
  * tweaking BSO validation routines to integrate easier with cornice validator
  * some variable renamings for consistency

Comment 2

[Toby Elliott [:telliott]]

My preliminary reviews are good - a few things I want to go over on Monday - but I think something of this magnitude should have a second set of eyes look over it as well.

I'm adding Tarek and hopefully he'll get a chance to provide some useful feedback.

Comment 3

[Toby Elliott [:telliott]]

Comment on attachment 682353 [details]
new "views" submodule to replace the controller class

This is fantastic work. We've talked about a few of my questions, but on the whole, it's good.

Tarek may have a few Cornice-related technical notes, but I'm signing off on this from an architecture and code design perspective.

Comment 4

[Toby Elliott [:telliott]]

Comment on attachment 682354 [details] [diff] [review]
extra changes necessary to work with the refactoring

I question some of these changes as necessary to work with the refactor - some of them look like cleanup for other (valid) reasons, but this is fine. :)

Comment 5

[Ryan Kelly [:rfkelly]]

Comment on attachment 682354 [details] [diff] [review]
extra changes necessary to work with the refactoring

Flagging for additional review by Tarek

Comment 6

[Tarek Ziadé (:tarek)]

Comment on attachment 682354 [details] [diff] [review]
extra changes necessary to work with the refactoring

>diff --git a/syncstorage/__init__.py b/syncstorage/__init__.py
[...]
>         # Add it to the current timestamp to get an absolute time.
>-        if "ttl" not in data:
>+        if data.get("ttl") is None:

what's the difference ? I am asking because I find the first form more appealing

Comment 7

[Tarek Ziadé (:tarek)]

decorators.py : - typo in "name": "offst",

util.py: you could use Cornice's json_error: https://github.com/mozilla-services/cornice/blob/master/cornice/util.py#L40   instead of your own implementation

Comment 8

[Ryan Kelly [:rfkelly]]

(In reply to Tarek Ziadé (:tarek) from comment #6)
> >diff --git a/syncstorage/__init__.py b/syncstorage/__init__.py
> [...]
> >         # Add it to the current timestamp to get an absolute time.
> >-        if "ttl" not in data:
> >+        if data.get("ttl") is None:
> 
> what's the difference ? I am asking because I find the first form more
> appealing

The user can now explicitly send {"ttl": None} to reset the TTL on an existing item, which is not handled properly by the first form.

> decorators.py : - typo in "name": "offst",

Whoops, thanks.  Clearly a missing test here then.
 
> util.py: you could use Cornice's json_error:
> https://github.com/mozilla-services/cornice/blob/master/cornice/util.py#L40 
> instead of your own implementation

I'd like to call cornice directly to do this, but it's not quite compatible as currently stands.  As described in Bug 792674, I want to use a custom "status" field on some of the errors, which isn't currently supported in cornice.

If you approve of the general direction Bug 792674 is taking, I will put together a PR for cornice to allow easy customization of this field.

Comment 9

[Ryan Kelly [:rfkelly]]

Committed with two trivial changes based on feedback:
1) documented why "offset" is not parsed/validated upfront (and opened Bug 816799 to look into doing it better)
2) added some more assertions in one of the tests, to catch the "offst" typo

https://github.com/mozilla-services/server-syncstorage/commit/8a9b749497ab8966c594e4de0b819cf40de5ef62