Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API schema is invalid for fields.Nested with allow_none=True #6015

Open
jc-harrison opened this issue Mar 31, 2023 · 2 comments
Open

API schema is invalid for fields.Nested with allow_none=True #6015

jc-harrison opened this issue Mar 31, 2023 · 2 comments
Labels
bug Something isn't working FlowAPI Issues related to the FlowKit API FlowMachine Issues related to FlowMachine

Comments

@jc-harrison
Copy link
Member

In the flowmachine query schemas, there are several optional query parameters ("hours", "sampling" and "bounds", in several query schemas) which are validated by a nested schema (via marshmallow's fields.Nested), but whose values can be null (or default to None if not provided).

This is handled fine by the marshmallow schemas, but the API spec generated in these cases looks like

"SomeQuerySchema": {
    "properties": {
      "hours": {
        "type": [
          "null"
        ],
        "allOf": [
          {
            "$ref": "#/components/schemas/Hours"
          }
        ],
        "default": null,
        "nullable": true
      }
    },
    "type": "object"
  }

which looks fine, but actually says that the hours value still has to match the Hours schema, even if it's null (see discussion in OAI/OpenAPI-Specification#1368) - and null is not a valid Hours object, so this API spec is not valid. It appears there's actually no way to express this marshmallow schema in an openapi 3.0 API spec, without defining a custom "null type" schema. We got away with this until recently, because openapi-schema-validator interpreted this spec as meaning "can be null OR Hours" rather than "can be null but must still be Hours", but as of v0.4.0, openapi-schema-validator has been fixed to interpret such a schema as per the discussion linked above, so upgrading to openapi-schema-validator>=0.4.0 breaks our integration tests.

In OpenAPI version 3.1, "nullable" was removed in favour of a new "type": "null". This means that our schema can be correctly expressed as

"SomeQuerySchema": {
    "properties": {
      "hours": {
        "type": [
          "null"
        ],
        "anyOf": [
          {
            "$ref": "#/components/schemas/Hours"
          },
          {"type": "null"}
        ],
        "default": null,
        "nullable": true
      }
    },
    "type": "object"
  }

However, apispec currently doesn't handle this correctly (marshmallow-code/apispec#833), so as things stand we'd still end up with a broken API spec if we bump the openapi version to 3.1.

It's not strictly necessary to handle optional parameters by allowing null input. We could just leave them optional without a default value, and handle missing parameters the way we currently handle null ones. I believe this would work fine in the API spec. The difficulty with this approach is that we explicitly add all parameters as attributes on the exposed query objects to aid serialisation, and we'd therefore have to add in extra logic for every optional parameter to skip adding it as an attribute if it's not provided. So if we make this change, it may be beneficial to re-work the query schema / exposed query setup at the same time.

@jc-harrison jc-harrison added bug Something isn't working FlowMachine Issues related to FlowMachine FlowAPI Issues related to the FlowKit API labels Mar 31, 2023
greenape added a commit that referenced this issue Aug 10, 2023
pending resolution of #6015
greenape added a commit that referenced this issue Aug 15, 2023
greenape added a commit that referenced this issue Aug 15, 2023
@jc-harrison
Copy link
Member Author

marshmallow-code/apispec#833 is now fixed in apispec v6.6.1, so we should now be able to bump the openapi version to 3.1 and unpin some FlowAPI dependencies.

greenape added a commit that referenced this issue May 1, 2024
@greenape
Copy link
Member

greenape commented May 1, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working FlowAPI Issues related to the FlowKit API FlowMachine Issues related to FlowMachine
Projects
None yet
Development

No branches or pull requests

2 participants