Split request schemas for each method/endpoint type #1047

gabisurita · 2017-01-29T21:44:22Z

Related to #1033

This PR changes the schema-viewset interaction:

Request schemas are now explicitly declared for each method-endpoint type
We introduce deferred fields on schemas that allow binding.
e.g: we now have a generic RequestSchema where we can bind different QueryString Schemas.

Todos

Add tests.
Add a changelog entry.
Wait for colander release

r? @glasserc @leplatrem

leplatrem

It's pretty good!

The class declarations seem a bit verbose but I don't have any alternative proposition. Except maybe have some kind of helper:

default_collection_arguments = {
    'schema': request_schema(querystring=CollectionQuerySchema),
}

But I'm not sure...

I think the comments can be improved as I commented in some place below..

GG!

leplatrem · 2017-01-30T10:04:39Z

kinto/core/resource/schema.py


    @staticmethod
    def schema_type():
        return colander.Mapping(unknown='preserve')


+class PatchHeaderSchema(HeaderSchema):
+    """Header schema used with patch requests."""


nitpick: http verbs are usually written upercase

Do you think the verb on the class name should be uppercase as well?

No, in comments at least

leplatrem · 2017-01-30T10:06:01Z

kinto/core/resource/schema.py

+
+    @colander.instantiate()
+    class querystring(CollectionQuerySchema, GetQuerySchema):
+        pass


I had never seen this :) Interesting... It took me a while to figure out why _fields was not listed in CollectionQuerySchema!

Maybe there is a matter of wording or comments to really distinguish : general (both endpoints), plural and single enpoints

_fields is not accepted in plural DELETE, so that's why it's not there. I've tried to omit "General" and just add the Collection or Record according to each type. Turns out Record was not used. :P

Don't hesitate to add as many comments as you can for this kind of details :)

leplatrem · 2017-01-30T10:14:17Z

kinto/core/resource/viewset.py

-            args['schema'] = schema
-        else:
-            args['schema'] = RequestSchema()
+            request_schema.add(record_schema)


With all the explicit declarations that we do elsewhere, I wish those few lines become extremely simple

An interesting approach IMO would be to handle the record_schema binding directly on the schema (instead of the viewset), what do you think?

I didn't understand your proposition :(

gabisurita · 2017-01-30T13:40:00Z

kinto/core/resource/viewset.py

+
+        # Instantiate request schema
+        request_schema = request_schema()
+
        if method.lower() in map(str.lower, self.validate_schema_for):


With the current implementation it still is, the body field is still added just on the viewset. I've tried to preserve this for backward compatibility, but we can indeed solve this on the schemas.

Edit: Sorry, I've deleted @leplatrem comment by accident instead of mine, it asked if validate_schema_for was still needed

gabisurita · 2017-01-30T13:57:11Z

The class declarations seem a bit verbose but I don't have any alternative proposition. Except maybe have some kind of helper

An interesting approach for that would be to set the header, query and body on when instantiating a request (setting them on the init method). e.g:

 request_schema = RequestSchema(header=HeaderSchema(), querystring=QuerySchema())

But we still would have to do a bunch of imports. But I'm also not sure about that. I tend to think it's probably better to make this explicit.

gabisurita · 2017-01-30T14:17:47Z

Also, do we have to keep SimpleSchema, PartialSchema and StrictSchema on the viewset?

leplatrem · 2017-01-30T15:01:57Z

better to make this explicit.

Works for me!

keep SimpleSchema, PartialSchema and StrictSchema on the viewset?

If not used, and not mentionned in docs, I'm not again removing them :)

gabisurita · 2017-01-30T20:41:24Z

@leplatrem, I tried this different approach using schema binding, I think it makes the viewset a bit simpler, what do you think?

…a binding

…/kinto into split-request-schemas

leplatrem

The whole result is globally better!

I just don't know if bind() was made for such a use case. Maybe we can ask somewhere?

leplatrem · 2017-01-31T14:43:59Z

kinto/core/resource/schema.py

@@ -278,6 +289,62 @@ def deserialize(self, cstruct=colander.null):
        return values


+class CollectionQuerySchema(QuerySchema):
+    """Querystring validated fields used with collections."""


nitpick: validated fields --> schema

leplatrem · 2017-01-31T14:44:11Z

kinto/core/resource/schema.py

+
+
+class RecordGetQuerySchema(QuerySchema):
+    """Querystring validated fields for GET record requests."""


leplatrem · 2017-01-31T14:44:16Z

kinto/core/resource/schema.py

+
+
+class CollectionGetQuerySchema(CollectionQuerySchema):
+    """Querystring validated fields for GET collection requests."""


leplatrem · 2017-01-31T14:45:29Z

kinto/core/resource/schema.py

+        return colander.Mapping(unknown='raise')
+
+    def bind(self, data=None, permissions=None, **kwargs):
+        binded = super(RecordSchema, self).bind(**kwargs)


nitpick: binded -> bound

leplatrem · 2017-01-31T14:46:34Z

kinto/core/resource/viewset.py

-            args['schema'] = RequestSchema()
+        request_schema = args.get('schema', RequestSchema())
+        record_schema = self.get_record_schema(resource_cls, method)
+        request_schema = request_schema.bind(body=record_schema)


leplatrem · 2017-01-31T15:05:28Z

tests/core/resource/test_schema.py

+
+    def setUp(self):
+        self.schema = schema.RequestSchema().bind(
+            querystring=schema.RecordGetQuerySchema())


nitpick: prefer an intermediary variable to avoid indentation

leplatrem · 2017-01-31T15:06:52Z

kinto/core/resource/schema.py

+
+class RecordSchema(colander.MappingSchema):
+
+    data = colander.deferred


Add a comment to explain that we decide later what to put in data based on the resource schema?

leplatrem · 2017-01-31T15:09:42Z

kinto/core/resource/schema.py

+    def bind(self, header=None, querystring=None, **kwargs):
+        binded = super(RequestSchema, self).bind(**kwargs)
+        if header:
+            binded['header'] = header.bind(**kwargs)


Do we really want to support that .bind(**kwargs). I find it cool and flexible, but it looks like we don't use it and it does not ease the understanding of the code.

leplatrem · 2017-01-31T15:12:34Z

kinto/core/resource/schema.py

+    """Base schema for kinto requests."""
+
+    header = HeaderSchema()
+    querystring = QuerySchema()


Why are they not deferred in this case?

Because they have a default.

glasserc

Overall I like this. It makes the schemas feel more flexible, modular, and composable, and it's certainly better IMHO than mutating them.

glasserc · 2017-01-31T16:47:06Z

tests/core/resource/test_schema.py

+
+    def setUp(self):
+        self.schema = schema.RequestSchema().bind(
+            querystring=schema.RecordGetQuerySchema())


I'm not crazy about seeing exactly the same code to produce a schema that's similar to the one we use in real code. We discussed this on Hangouts, we decided that the best approach would be to change these tests to either be tests of the bind method, or tests of the schema that are actually being used in the viewsets.

glasserc · 2017-01-31T16:52:04Z

tests/core/resource/test_viewset.py


        self.assertDictEqual(
            arguments,
            {
-                'schema': mocked(),
+                'schema': mocked().bind(),


Why do we need to put bind() on our mocks?

This is the return value if we call bind() on a mocked schema. If we want to avoid this, we should also mock patch the bind return value. e.g.

>>> m = mock.MagicMock() >>> m.method() <MagicMock name='mock.method()' id='4417348560'>

And a proposed solution:

>>> m = mock.MagicMock() >>> m.method = lambda: m >>> m.method() <MagicMock id='4417096080'>

Sorry, I had misread this test the first time around!

glasserc · 2017-01-31T16:56:57Z

tests/core/resource/test_viewset.py

-        viewset = ViewSet(
-            validate_schema_for=('GET', )
-        )
+        viewset = ViewSet()


It seems like validate_schema_for isn't useful any more, but I can't tell if we're removing it in this PR or not. I see a couple places in the tests like this where it's being removed, and I see the class attribute is being removed from ViewSet, but I don't see any code that uses this attribute. Are we removing it?

Yes, we are. It was used to tell if we added a body to the request schema or not, but it doesn't make sense anymore as we split the requests into RequestSchema and PayloadRequestSchema.

We could consider this a breaking change because it can be changed from a resource, but I can't see any reason why someone would change this attribute, because probably the requests would fail if it was different.

glasserc · 2017-01-31T16:58:14Z

tests/core/resource/test_viewset.py

+        resource = mock.sentinel.resource
+        resource.schema = ResourceSchema
+        resource.permissions = ('read', 'write')
+        args = viewset.collection_arguments(resource, 'GET')


Why do we need something from mock.sentinel instead of just a MagicMock? (Above, in test_schema_is_added_when_uppercase_method_matches, you turn a sentinel into an ordinary MagicMock.)

It failed on the second bind as one call from colander that inserts a node would not accept a mock object, so I've set the ResourceSchema manually. But I'll try to find a better approach.

Anyways, it's not a problem anymore with the new bind implementation.

gabisurita · 2017-01-31T23:45:31Z

I just don't know if bind() was made for such a use case. Maybe we can ask somewhere?

I've asked #pyramid channel on IRC and they said that it was, and that there should be an easier way to do it. I've submitted a patch to colander which should be on the next release. Pylons/colander#280

…nstead of overwriting bind

leplatrem · 2017-02-01T12:52:10Z

This is excellent work. You never stop raising the bar 👍 👍

gabisurita added 4 commits January 28, 2017 20:26

isolate request schemas by method

3c155e8

Split request schemas for collections, records and verbs

e84a2e0

reorganize schema.py in a cute way.

3a26148

add tests

2866ce0

gabisurita changed the title ~~Split request schemas for each method/resource type~~ Split request schemas for each method/endpoint type Jan 29, 2017

add changelog entry

7d0159a

leplatrem requested changes Jan 30, 2017

View reviewed changes

leplatrem reviewed Jan 30, 2017

View reviewed changes

gabisurita commented Jan 30, 2017

View reviewed changes

gabisurita force-pushed the split-request-schemas branch from 2d2dc5c to 20d3f6a Compare January 30, 2017 21:05

gabisurita added 5 commits January 30, 2017 19:07

try a different approach for setting the schemas using colander schem…

20d3f6a

…a binding

Merge branch 'master' into split-request-schemas

5f47f1b

fix viewset tests

27729bd

fix coverage

d269420

Merge branch 'split-request-schemas' of https://github.com/gabisurita…

99e3b13

…/kinto into split-request-schemas

leplatrem approved these changes Jan 31, 2017

View reviewed changes

glasserc reviewed Jan 31, 2017

View reviewed changes

use explicit deferred attributes declarations (patched on colander) i…

044ea62

…nstead of overwriting bind

gabisurita force-pushed the split-request-schemas branch from 2941b5c to 6c44c2c Compare February 1, 2017 03:37

gabisurita added 2 commits February 1, 2017 01:39

comments and tests

6c44c2c

update colander version

736847b

gabisurita added 2 commits February 1, 2017 16:05

@glasserc comment

0424ed7

pep8

80231a6

gabisurita merged commit c04a62a into Kinto:master Feb 1, 2017

gabisurita deleted the split-request-schemas branch February 23, 2017 00:03



		class RecordGetQuerySchema(QuerySchema):
		"""Querystring validated fields for GET record requests."""



		class CollectionGetQuerySchema(CollectionQuerySchema):
		"""Querystring validated fields for GET collection requests."""


		class RecordSchema(colander.MappingSchema):

		data = colander.deferred

Split request schemas for each method/endpoint type #1047

Split request schemas for each method/endpoint type #1047

Conversation

gabisurita commented Jan 29, 2017 • edited Loading

leplatrem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabisurita Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

gabisurita commented Jan 30, 2017 • edited Loading

gabisurita commented Jan 30, 2017

leplatrem commented Jan 30, 2017

gabisurita commented Jan 30, 2017 • edited Loading

leplatrem left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasserc left a comment

Choose a reason for hiding this comment

glasserc Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabisurita Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabisurita Feb 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabisurita commented Jan 31, 2017 • edited Loading

leplatrem commented Feb 1, 2017

gabisurita commented Jan 29, 2017 •

edited

Loading

gabisurita Jan 30, 2017 •

edited

Loading

gabisurita commented Jan 30, 2017 •

edited

Loading

gabisurita commented Jan 30, 2017 •

edited

Loading

glasserc Jan 31, 2017 •

edited

Loading

gabisurita Feb 1, 2017 •

edited

Loading

gabisurita Feb 1, 2017 •

edited

Loading

gabisurita commented Jan 31, 2017 •

edited

Loading