Putting values in request.validated before calling colander_body_validator

[czekan]

Let say we have a service:

service = Service(name='service', path='/service')

class ColSchema(MappingSchema):
    field = SchemaNode(String())

def custom_validator(request, **kwargs):
    # do some validation and put something in request.validated
    request.validated['something'] = 'something'

@service.post(schema=ColSchema(), validators=(custom_validator, colander_body_validator))
def service_handler(request):
    request.validated['something']   # <---- raises KeyError
    return {}

Getting something from request.validated raises KeyError because of the line https://github.com/Cornices/cornice/blob/master/cornice/validators/_colander.py#L71
which overwrites request.validated with request.validated['body'].

Fix for that would be just replacing line 71 in the cornice/validators/_colander.py:

        request.validated = request.validated.get(location, {})

with

        validated_location = request.validated.get(location, {})
        request.validated.update(validated_location)
        request.validated.pop(location, None)

but ... there is something that this change breaks.
What if someone sets schema which inherits from SequenceSchema rather than MappingSchema? Current code just rewrites request.validated (which is instantiated as dict in pyramidhook.py wrap_request()) with a list. I'm not sure if this should work like that or I'm just missing something?

[leplatrem]

Since we have that here:

cornice/cornice/pyramidhook.py

Lines 148 to 149 in c8d3ad6

	if not hasattr(request, 'validated'):
	setattr(request, 'validated', {})

I would assume that request.validated should be a dict.
Even the colander code should assume to find a dict there, no?

We can also explicitly document that a SequenceSchema cannot be used.

I like your idea of not replacing the existing stuff, please go on ;)

[czekan]

That's OK for me :)

But ... there is a test:

cornice/tests/test_validation.py

Lines 419 to 426 in c8d3ad6

	def test_valid_json_array(self):
	app = self.make_ordinary_app()
	response = app.post_json(
	'/group_signup',
	[{'username': 'hey'}, {'username': 'how'}]
	)
	self.assertEqual(response.json['data'],
	[{'username': 'hey'}, {'username': 'how'}])

which relay on that behavior. I'm not sure it should because of this default value (empty dict) which would suggest that it should stay dict.
I can remove that test or change it to MappingSchema.
This change would potentially break compatibility. If that's ok I can prepere PR with a solution.

Thing is that if someone would want to use SequenceSchema he can, but it needs to be used with colander_validator and than it will be deserialized to request.validated['body'] and request.validated will still remain a dict.

[leplatrem]

This change would potentially break compatibility. If that's ok I can prepere PR with a solution.

Yes, we can bump a major version, no problem.

Thing is that if someone would want to use SequenceSchema he can, but it needs to be used with colander_validator and than it will be deserialized to request.validated['body'] and request.validated will still remain a dict.

Makes sense! Yes!