Validate required fields based on readOnly & writeOnly

[whoseoyster]

Fixes #1907.

Changes proposed in this pull request:

• Add required and readOnly validators to the Draft4ResponseValidator and Draft4RequestValidator objects

Let me know if anything else needs adjustments.

[sampoh0523]

This regresses #942. readOnly should not result in a HTTP 400 when the field is present, since OpenAPI only says that the field "SHOULD NOT be sent as part of the request", not "MUST NOT".

Arguably the same should apply for writeOnly, but the user can at least control the responses they sent back, yet they cannot control what API clients will do.

(To fix #942 here, all one needs to do is make validate_readOnly a no-op. It cannot be removed from the validator map, however, without changing how validate_required detects it.)

[chrisinmtown]

I cannot agree with @sampoh0523 here. I believe the intent of readOnly is that a request containing the field must be rejected, otherwise what is the point of marking it as such.

[chrisinmtown]

@RobbeSneyders @Ruwann would you please approve a workflow run on this PR?

[chrisinmtown]

I think the prevailing style in this code is to use double quotes (not single quotes) around strings, please consider matching that.

[sampoh0523]

I cannot agree with @sampoh0523 here. I believe the intent of readOnly is that a request containing the field must be rejected, otherwise what is the point of marking it as such. I'm going to add my approval vote to this PR as-is.

The OpenAPI specification argues otherwise, see e.g. https://swagger.io/specification/:

Fields that are both required and read-only are an example of when it is beneficial to ignore a readOnly: true constraint in a PUT, particularly if the value has not been changed.

Note also the wording on the specification for version 3.0.3: https://spec.openapis.org/oas/v3.0.3.html

[readOnly] means that it MAY be sent as part of a response but SHOULD NOT be sent as part of the request.

This is RFC 2119 wording, which explicitly allows readOnly fields to be sent as part of a request (only telling that it should not be done). If it was forbidden, it would be MUST NOT.

[chrisinmtown]

@sampoh0523 thanks for the quick reply. The problem stated in issue #1907 is that Connexion rejects a request that is MISSING a field if the schema marks that field as both required and readOnly, and this PR changes that behavior to accept. I think I finally understand you are stating something very closely related, namely that this change also absolutely blocks a request if it CONTAINS a field that is both required and readOnly; and you are stating Connexion should tolerate the request with the field. Did I finally get that right?

Parsing the difference between SHOULD NOT and MUST NOT gets kind of subtle here. :/

[sampoh0523]

Yes, that is correct. #942 has further discussion on it (including quoted commentary by one of the JSON Schema designers), but (my point as well is that) readOnly should not cause a request to fail validation, even if the field in question is present in that request.

[chrisinmtown]

@sampoh0523 has convinced me that there is no need for this function, the argument is that Connexion should tolerate a readOnly field in requests without complaint.

[sampoh0523]

The current implementation of validate_required checks that some kind of function is specified for validating readOnly fields, since that is used to determine whether such fields can be skipped from the required validation. Deleting the function would therefore require some additional changes.

[chrisinmtown]

Oh good point. Why is it important for the validate_required function to check for presence of a validate_readOnly function? Asked differently, why is just checking for the readOnly subschema property not sufficient?

[sampoh0523]

It is because the same validate_required is used for both requests and responses, and it recognizes which is which by checking the validators used (if there is a readOnly validator, it is a request; if there is a writeOnly validator, it is a response).

[chrisinmtown]

Please let me amend my first comment here. @sampoh0523 has convinced me that this function should be a no-op. It has to exist and be registered, so the validation function can detect it is validating a request, but this function should never reject an instance.

[chrisinmtown]

I think this PR should add the following new test cases:

• Schema defines field as required and readOnly, submit a request without the field, the request is accepted
• Schema defines field as required and readOnly, submit a request with the field, the request is accepted.

[chrisinmtown]

hi @whoseoyster do you have time to update your PR? When I run tox on this PR I see these test failures:

FAILED tests/test_json_validation.py::test_readonly[swagger.yaml-FlaskApp] - assert 400 == 200
FAILED tests/test_json_validation.py::test_readonly[openapi.yaml-FlaskApp] - assert 400 == 200
FAILED tests/test_json_validation.py::test_readonly[openapi.yaml-AsyncApp] - assert 400 == 200
FAILED tests/test_json_validation.py::test_readonly[swagger.yaml-AsyncApp] - assert 400 == 200
FAILED tests/decorators/test_validation.py::test_invalid_type - AssertionError: assert '20 is not of...ance:

The test_readonly test POSTs a request to /user with a user_id field in the body; the schema has that marked readOnly, and the new code rejects it.

[chrisinmtown]

Just for the record, here's a summary of the difference from V2 to V3. In V2, one JSON schema test checks that a request with a readOnly field is rejected. In V3, that same test checks that a request with a readOnly field is accepted. So it's not possible to copy/reuse the V2 code verbatim in V3.

[chrisinmtown]

@whoseoyster please consider this revision to your PR. It changes the validate_readOnly function and adds tests for a required/readOnly field. On GET, check that Connexion rejects a response without the field. On POST, check that Connexion accepts a request without the field.

diff --git a/connexion/json_schema.py b/connexion/json_schema.py
index 47b6ab5..7bee3df 100644
--- a/connexion/json_schema.py
+++ b/connexion/json_schema.py
@@ -148,7 +148,15 @@ def validate_required(validator, required, instance, schema):
 
 
 def validate_readOnly(validator, wo, instance, schema):
-    yield ValidationError("Property is read-only")
+    """
+    The OpenAPI specification states:
+    > [readOnly] means that it MAY be sent as part of a response
+    > but SHOULD NOT be sent as part of the request.
+    Note the phrase is "SHOULD NOT" (not "MUST NOT").
+    Presence of this no-op function indicates to `validate_required`
+    that the instance being validated is a request.
+    """
+    pass
 
 
 def validate_writeOnly(validator, wo, instance, schema):
diff --git a/tests/fakeapi/hello/__init__.py b/tests/fakeapi/hello/__init__.py
index b6d6c00..5351f19 100644
--- a/tests/fakeapi/hello/__init__.py
+++ b/tests/fakeapi/hello/__init__.py
@@ -634,6 +634,10 @@ def get_user():
     return {"user_id": 7, "name": "max"}
 
 
+def get_user_without_userid():
+    return {"name": "max"}
+
+
 def get_user_with_password():
     return {"user_id": 7, "name": "max", "password": "5678"}
 
@@ -644,6 +648,10 @@ def post_user(body):
     return body
 
 
+def post_user_without_userid(body):
+    return post_user(body)
+
+
 def post_multipart_form(body):
     x = body["x"]
     x["name"] += "-reply"
diff --git a/tests/fixtures/json_validation/openapi.yaml b/tests/fixtures/json_validation/openapi.yaml
index c336eb0..59595e4 100644
--- a/tests/fixtures/json_validation/openapi.yaml
+++ b/tests/fixtures/json_validation/openapi.yaml
@@ -22,6 +22,20 @@ components:
         password:
           type: string
           writeOnly: true
+    UserWithRequiredReadOnly:
+      type: object
+      required:
+      - user_id
+      - name
+      properties:
+        user_id:
+          type: integer
+          readOnly: true
+        name:
+          type: string
+        password:
+          type: string
+          writeOnly: true
     X:
       type: object
       properties:
@@ -70,6 +84,32 @@ paths:
             application/json:
               schema:
                 $ref: '#/components/schemas/User'
+
+  /user_without_userid:
+    get:
+      operationId: fakeapi.hello.get_user_without_userid
+      responses:
+        200:
+          description: User object
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/UserWithRequiredReadOnly'
+    post:
+      operationId: fakeapi.hello.post_user_without_userid
+      requestBody:
+        content:
+          application/json:
+            schema:
+              $ref: '#/components/schemas/UserWithRequiredReadOnly'
+      responses:
+        200:
+          description: User object
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/UserWithRequiredReadOnly'
+
   /user_with_password:
     get:
       operationId: fakeapi.hello.get_user_with_password
diff --git a/tests/fixtures/json_validation/swagger.yaml b/tests/fixtures/json_validation/swagger.yaml
index 0f9bcb5..355272a 100644
--- a/tests/fixtures/json_validation/swagger.yaml
+++ b/tests/fixtures/json_validation/swagger.yaml
@@ -20,6 +20,20 @@ definitions:
       password:
         type: string
         x-writeOnly: true
+  UserWithRequiredReadOnly:
+    type: object
+    required:
+    - user_id
+    - name
+    properties:
+      user_id:
+        type: integer
+        readOnly: true
+      name:
+        type: string
+      password:
+        type: string
+        x-writeOnly: true
 
 paths:
   /minlength:
@@ -59,6 +73,29 @@ paths:
           description: User object
           schema:
             $ref: '#/definitions/User'
+
+  /user_without_userid:
+    get:
+      operationId: fakeapi.hello.get_user_without_userid
+      responses:
+        200:
+          description: User object
+          schema:
+            $ref: '#/definitions/UserWithRequiredReadOnly'
+    post:
+      operationId: fakeapi.hello.post_user_without_userid
+      parameters:
+        - name: body
+          in: body
+          required: true
+          schema:
+            $ref: '#/definitions/UserWithRequiredReadOnly'
+      responses:
+        200:
+          description: User object
+          schema:
+            $ref: '#/definitions/UserWithRequiredReadOnly'
+
   /user_with_password:
     get:
       operationId: fakeapi.hello.get_user_with_password
diff --git a/tests/test_json_validation.py b/tests/test_json_validation.py
index df613e1..024dc50 100644
--- a/tests/test_json_validation.py
+++ b/tests/test_json_validation.py
@@ -77,6 +77,20 @@ def test_readonly(json_validation_spec_dir, spec, app_class):
     )
     assert res.status_code == 200
 
+    # reject a response missing a required/readOnly field
+    res = app_client.get("/v1.0/user_without_userid")
+    assert res.status_code == 500
+    assert res.json()["detail"].startswith(
+        "Response body does not conform to specification"
+    )
+
+    # accept a request missing a required/readOnly field
+    res = app_client.post(
+        "/v1.0/user_without_userid",
+        json={"name": "max", "password": "1234"},
+    )
+    assert res.status_code == 200
+
 
 def test_writeonly(json_validation_spec_dir, spec, app_class):
     app = build_app_from_fixture(

[chrisinmtown]

Extending the scope here slightly, for writeOnly the OpenAPI spec section https://spec.openapis.org/oas/v3.0.3.html#fixed-fields-19 says "MAY be sent as part of a request but SHOULD NOT be sent as part of the response." Well, this test case checks that Connexion rejects/fails a response that contains a writeOnly field. I don't much like it, but to conform to the spec, apparently Connexion should allow a response with such a field. The V2 and V3 branches agree on this point, for whatever that is worth. @sampoh0523 would you please comment?

[sampoh0523]

Sorry, I did not see this before.

I do not have strong opinions about writeOnly, because the code using Connexion has control over its responses, unlike with readOnly where it has no control over the requests it receives.

[chrisinmtown]

@whoseoyster I sure was hoping to hear from you. If you cannot find time to update this PR, I can submit a new PR with the essential changes. Please comment.

[chrisinmtown]

I do not recommend merge of the current version due to the behavior of Connexion that deviates from the JSON schema spec on required+readOnly fields in requests.

[whoseoyster]

@chrisinmtown added your changes here