WIP: Fix validation of endpoint with multiple "produces" mimetypes.

[positron96]

This will fix #593 .

Changes proposed in this pull request:

• response validator does not validate responses of non-json types against schema if they are listed along with json in produces stanza of spec.

[positron96]

This is a WIP, so here be discussion.

Firstly, I've created a failing test that recreates this behavior: an endpoint that produces either json or text/plain (depending on Accept header) and a test that requests both variants. The text variant fails on validating data against json schema.

Since I'm not a swagger specialist, the first question is if this is really a valid example, does such behavior correspond to spec? Swagger 2.0 does not provide a way to supply several schemas for different response content-types, so must connexion validate every content-type against single schema? There are types (e.g text/plain) that do not even have the notion of "validating".

[dtkav]

This is something I got stuck on for OpenAPI 3 support. I'm happy to take on this feature when I get that feature set landed.
Right now I force only the first type, basically copying the "produces", "consumes" behavior from swagger 2.
The feature you suggest raises several interesting questions like:

1. Should there be a handler function per response content type? If not, do we know how to serialize the result of function into every conceivable content type? What's the implementation of serializing the response to text/plan, application/xml?
2. Which content types should be supported for both serialization and validation? Could we just extend the validator_map to include content type, and require the user to provide a validator? Could we copy this pattern for serialization?
3. Which layer should handle the Accept header? Does it make it to the handler function? Does the handler function need to return the correct data based on the Accept header?
4. What about Accept header behavior like q-factor weighting?

Right now, IIRC the behavior is:

• if it's json, serialize the object to json and validate it
• if it's something else, attach the correct header but assume it is serialized and bypass validation

It would be kind of cool from a user perspective to be able to just provide a decorator interface that the user could subclass.
Something like:

@my_xml_serializer
@my_xml_validator
def add5_handler(param1):
    return param1 + 5

[positron96]

As I understand, right now connexion only tries to validate JSON and nothing else (neither XML, which is possible but rather complex to add, nor anything else). The problem is, in some cases (see below) it also tries to validate other Content-types as JSON as well e.g. validate a text/plain against a JSON schema (hence the failing test I've added, it shows this).

My proposal for validator is this:

• if response type is json (check response header), then validate against schema from spec.
• otherwise just pass as is.

It differs from current implementation, which is:

• Response content-type is not used at all,
• text/plain (from produces) is treated as JSON and is validated against schema (https://github.com/zalando/connexion/blob/master/connexion/decorators/response.py#L75)
• when more than one produces items are present and not all of them are JSON, treat response as JSON (https://github.com/zalando/connexion/blob/master/connexion/operation.py#L326) and validate it, even if returned type is not JSON.

As for checking if response Content-type corresponds to Accept header, I propose to not bother with it, as it's governed by HTTP standard, not Swagger standard and connexion is about Swagger. Same goes to weight factors.

Of course, this only relates to swagger 2. In Openapi 3 each content-type has its own schema and that requires its own thinking as well (leaving a question on how to validate a text/plain or text/csv against schema. Probably stick to "only validate JSON" solution for now).

BTW, there is a Produces decorator in decorators/produces.py that gets attached to a handler function and seemingly does nothing. What was it supposed to do?

[dtkav]

You proposal makes sense to me.

Regarding the produces decorator:
I think it basically does nothing as is, but one could override Operation.__content_type_decorator to call a custom version of decorator.produces.Produces. One could then register various serializers for your apps content types like this:

class CustomOperation(Operation):

    @property
    def __content_type_decorator(self):
        mimetype = self.get_mimetype()
        if mimetype == "application/xml":
            decorator = XMLProduces(mimetype)
            return decorator
        else:
            return BaseSerializer()

class XMLProduces(Produces):
    def __call__(self, function):
        @functools.wraps(function)
        def wrapper(request):
            response = function(request)
            return make_this_into_xml(response)

        return wrapper

At least that's my best guess :)

[kornicameister]

Hey, sorry for jumping in. I had a case recently when we discussed having endpoint that may produce sth like application/vnd.team.doc_type;version=1, application/vnd.team.doc_type;version=2. We've been actually wondering how connexion would cope with it. Maybe instead of just passing it through, framework should enable simple mechanism to register content-type validators?

Obviously speaking, some common mime types could've been validated by installing some extras, i.e. your endpoint returns application/xml and you can do pip install connexion[xml-response-validator] or text/yaml and pip install connexion[yaml-response-validator].

What do you think? Is the example good enough to be taken as potential extension here?

[dtkav]

Hey @kornicameister @positron96 have a look at my demo for request validation: #760
Maybe something similar could be used for registering response validation?

[kornicameister]

@dtkav yes, it seems to be going in a nice direction. As long as there will be some simple way to register custom handlers, it will be very nice ;-)

[RobbeSneyders]

Closing this as a PR and suggest to continue discussion on this in #593.