400 error for readOnly=True

[Jyhess]

Description

When a property marked readOnly is sent, connexion raises a 400 error.

This is not compliant with OpenAPI spec, which document a readOnly property should not be sent, and not must not:

Declares the property as "read only". This means that it MAY be sent as part of a response but SHOULD NOT be sent as part of the request.

Moreover, this force to create a new version of an API if an existing field is marked readOnly, forcing all clients to change their implementation. It's easier if server can just ignore this field.

It also imply more complex code client side, as we need to clean sent object from readOnly fields for simple GET/modify/PUT workflows.

Expected behaviour

When a property marked readOnly is sent, connexion accept it without issue.
If possible, it should be removed in the body received by endpoint.

Actual behaviour

A 400 error is raised.

Steps to reproduce

Create a schema with a readlony property:

properties:
  name:
    type: string
    readOnly: true

and send it with a POST or PUT.

Additional info:

This seems to have been implemented from issue #350

Output of the commands:

• python --version
  Python 3.6.8
• pip show connexion | grep "^Version\:"
  Version: 2018.0.dev1

[badcure]

I am normally against silently ignoring data with API...but agree here if it is part of the OpenAPI standard. That said, I think:
Default behavior:

• Remove the ReadOnly/WriteOnly fields.
• In the headers, such as X-ReadOnly-Drop, list out the fields that were removed from the app that was provided.
• Make sure logs, possibly info, document that dropping of the fields as well and what can be done to cause an error.

Allowed if a flag is set, such as strict_validation:

• Continue with the existing behavior, return a 400.

Essentially make it extremely verbose if the library is ignoring data.

[dtkav]

I used to be in favor of rejecting the entire message (strict behavior) when this was implemented, but I've since changed my opinion. One of the folks from the JSON Schema org (https://github.com/handrews) had this to say about readOnly [0]

readOnly never causes JSON Schema's validation process to fail. It's not an assertion, just an annotation that applications can use to take action as they please.

Perhaps the decision to accept readOnly fields could be toggled by additionalProperties in OAS3.

IIRC we fixed the additionalProperties implementation (true by default) after adding readOnly/writeOnly validation.

By this logic, a field marked readOnly that is provided in a request is interpreted as an additional field, and handled accordingly. If additionalProperties is true, then it's accepted as is.

Let me know what you think!

[0] https://stackoverflow.com/questions/48153961/how-to-validate-input-and-output-on-a-single-json-schema-edge-cases-with-readon

[badcure]

I would lean towards not allowing a field at all if it is marked as readOnly or writeOnly, but not error as well. Simply because I think field level logic should override parent object logic. But going with the additionalProperties approach above, maybe this would work?

• When additionalProperties: true, accept the fields but drop them on pass through. Meaning the app should never see a readOnly and the client should never see a writeOnly even if they are provided.
• When additionalProperties: false, continue with the existing behavior. Return 400s and 500s if a field is provided in the wrong direction.

[juviwhale]

Any update on this?

[FelixS90]

Update:
It seems that I was wrong. Somehow the Draft7 validator still checks for the required field and thus throws in case a readOnly field is not provided.

However, you can still use the existing Draft4RequestValidator by connexion and simply extend it with jsonschema.validators.extend(validator_class, {'properties': set_defaults, 'readOnly': ignore_readonly}), where ignore_readonly is just a function that passes.

@Jyhess @juviwhale
The original problem as posted can be quite easily fixed by validating against JSON Schema Draft 7, which incorporated the readOnly field. The following code snippet combines it with parsing and setting defaults as described in:

• https://connexion.readthedocs.io/en/latest/request.html#custom-validators
• https://github.com/zalando/connexion/blob/master/examples/swagger2/enforcedefaults/enforcedefaults.py

import jsonschema
from connexion.decorators.validation import RequestBodyValidator

def extend_with_set_default(validator_class):
    """
    Extends given validator class with setting defaults as specified in specification
    :param validator_class:
    :return:
    """
    validate_properties = validator_class.VALIDATORS['properties']

    def set_defaults(validator, properties, instance, schema):
        for property, subschema in six.iteritems(properties):
            if 'default' in subschema:
                instance.setdefault(property, subschema['default'])

        for error in validate_properties(
                validator, properties, instance, schema):
            yield error

    return jsonschema.validators.extend(
        validator_class, {'properties': set_defaults})


DefaultsEnforcingDraftValidator = extend_with_set_default(jsonschema.Draft7Validator)


class DefaultsEnforcingRequestBodyValidator(RequestBodyValidator):
    """
    Extends the connexion RequestBodyValidator with enforcing defaults
    See https://connexion.readthedocs.io/en/latest/request.html#custom-validators
    See https://github.com/zalando/connexion/blob/master/examples/swagger2/enforcedefaults/enforcedefaults.py
    """
    def __init__(self, *args, **kwargs):
        super(DefaultsEnforcingRequestBodyValidator, self).__init__(
            *args, validator=DefaultsEnforcingDraftValidator, **kwargs)

validator_map = {'body': DefaultsEnforcingRequestBodyValidator}
    connexion_app = FlaskApp(__name__, debug=False)
    connexion_app.add_error_handler(Exception, create_error_response)
    connexion_app.add_api('SegmentationService.yaml',
                          validate_responses=validate_output,
                          resolver=SnakeCaseResolver(),
                          pythonic_params=True,
                          validator_map=validator_map)

[klarose]

This behaviour actually adds quite a lot of work for us and any clients of our API. We'd like to be able to tell users of our API that setting a particular set of fields has no effect: i.e. they're read only. However, we'd also like it to be easy for our clients to do a read/modify loop on resources stored in our API.

As it stands, any client software written to our API needs to understand that if they want to read then modify a resource stored in our API that they must delete any readOnly fields from it. That's a lot of extra, boilerplate work for every client. Sure, code generation can take care of that, but this is still extra work for every code generator as well, making them more complicated and error prone, and many of the code generators I've worked with don't actually handle this for you.

I'd much prefer the behaviour described above -- if configured to do so, strip the read only properties into a separate set of input passed to the handler. I'd be fine with them even staying in the body -- the old behaviour -- because we've implemented our backends to discard the read-only data anyway. But, to have no option at all? Not very nice. :(