Do not assume that request body is present in case of optional body parameter

[macisamuele]

Fixes: #321

The following PR addresses the not needed assumption of body presence in case a body parameter is defined.

In case of an operation defined like

paths:
  /endpoint:
    post:
      parameters:
      - in: body
        name: body
        required: false
        schema:
          type: object

a call like curl -X POST .../endpoint would fail with a stacktrace like

Traceback (most recent call last):
  File "/Users/maci/my_service/virtualenv_run/lib/python3.6/site-packages/bravado_core/param.py", line 183, in unmarshal_param
    raw_value = request.json()
  File "/Users/maci/my_service/virtualenv_run/lib/python3.6/site-packages/pyramid_swagger/tween.py", line 271, in json
    return getattr(self.request, 'json_body', {})
  File "/Users/maci/my_service/virtualenv_run/lib/python3.6/site-packages/pyramid/request.py", line 237, in json_body
    return json.loads(text_(self.body, self.charset))
  File "/usr/lib/python3.6/json/__init__.py", line 354, in loads
    return _default_decoder.decode(s)
  File "/usr/lib/python3.6/json/decoder.py", line 339, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/lib/python3.6/json/decoder.py", line 357, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/maci/my_service/virtualenv_run/lib/python3.6/site-packages/pyramid_swagger/tween.py", line 476, in _validate
    return f(*args, **kwargs)
  File "/Users/maci/my_service/virtualenv_run/lib/python3.6/site-packages/pyramid_swagger/tween.py", line 584, in swaggerize_request
    request_data = unmarshal_request(request, op)
  File "/Users/maci/my_service/virtualenv_run/lib/python3.6/site-packages/bravado_core/request.py", line 66, in unmarshal_request
    param_value = unmarshal_param(param, request)
  File "/Users/maci/my_service/virtualenv_run/lib/python3.6/site-packages/bravado_core/param.py", line 186, in unmarshal_param
    format(str(json_error)))
bravado_core.exception.SwaggerMappingError: Error reading request body JSON: Expecting value: line 1 column 1 (char 0)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/maci/my_service/virtualenv_run/lib/python3.6/site-packages/pyramid/tweens.py", line 39, in excview_tween
    response = handler(request)
  File "/Users/maci/my_service/virtualenv_run/lib/python3.6/site-packages/pyramid_swagger/tween.py", line 185, in validator_tween
    op_or_validators_map,
  File "/Users/maci/my_service/virtualenv_run/lib/python3.6/site-packages/pyramid_swagger/tween.py", line 484, in _validate
    raise e
pyramid_swagger.exceptions.RequestValidationError: Error reading request body JSON: Expecting value: line 1 column 1 (char 0)

NOTE: the current implementation will shallow eventual json validation errors in case the endpoint is called with a body containing invalid JSON

FYI: @erezeshkol

[coveralls]

Coverage increased (+0.002%) to 98.337% when pulling b17fa35 on macisamuele:maci-fix-321 into 43c8002 on Yelp:master.

[macisamuele]

@sjaensch I've analysed alternatives to avoid shallowing JSON decoding errors in case a body parameter is optional.
The only solution that I have in mind is adding an extra expected method to the IncomingResponse that reports if a body is present and modifying the method like

elif location == 'body':
    if param.required or request.has_body():
        try:
            # TODO: verify content-type header
            raw_value = request.json()
        except ValueError as json_error:
            raise SwaggerMappingError(
                "Error reading request body JSON: {0}".format(str(json_error)),
            )
    else:
        raw_value = default_value

This change could be made in a backward compatible way if we ensure that request.has_body() does return True but it would require changes in bravado and pyramid_swagger to actually fix the issue.

Considering this I would still propose to follow with the currently proposed implementation in order to fix the issue (we open the possibility to a new issue, but previously it was broken and now it would be a bit less broken) and provide PRs to define the has_body method.
As we could do it in a different PR we could also decide to have a major version bump and not defaulting has_body to True to preserve compatibility

[sjaensch]

I'm fine with this change since I expect an optional body parameter that receives invalid JSON to be a rare occurrence. :)
We could do some exception analysis if we really care about it. On Python 3 we can use JSONDecodeError.doc to get to the raw data. But I'd probably leave it as-is for now.

[macisamuele]

@sjaensch thanks a lot for the feedback. I was not 100% sure about the next course of action, but at least we can move a tiny bit toward a solution.
I'm planning to release it as version 5.11.0. Do you have a different advice?