marshmallow-code / apispec

A pluggable API specification generator. Currently supports the OpenAPI Specification (f.k.a. the Swagger specification)..
https://apispec.readthedocs.io/
MIT License
1.18k stars 177 forks source link

Incorrect handling of marshmallow nested schema with allow_none=True in OpenAPI 3.0 #952

Closed ShepleySound closed 3 weeks ago

ShepleySound commented 1 month ago

This is a spinoff of #833, but for OpenAPI 3.0.

The following schemas:

from apispec import APISpec
from apispec.ext.marshmallow import MarshmallowPlugin
from marshmallow import Schema, fields

spec = APISpec(
    title="My API Spec",
    version="1.0.0",
    openapi_version="3.0.3",
    plugins=[MarshmallowPlugin()],
)

class FooSchema(Schema):
    bar = fields.Integer(required=True)

class MySchema(Schema):
    foo = fields.Nested(FooSchema, allow_none=True)

Generates the following OpenAPI spec:

{
  "Foo": {
    "type": "object",
    "properties": {
      "bar": {
        "type": "integer"
      }
    },
    "required": [
      "bar"
    ]
  },
  "MySchema": {
    "type": "object",
    "properties": {
      "foo": {
        "nullable": true,
        "allOf": [
          {
            "$ref": "#/components/schemas/Foo"
          }
        ]
      }
    }
  }
}

This looks correct at first glance, but isn't compliant with the 3.0.3 revision of OpenAPI, which clarified how nullable should work. See Clarify Semantics of nullable in OpenAPI 3.0 for details.

nullable: true is only meaningful in combination with a type assertion specified in the same Schema Object. nullable acts as a type modifier, allowing null in addition to the specified type.

nullable: true operates within a single Schema Object. It does not "override" or otherwise compete with supertype or subtype schemas defined with allOf or other applicators. It cannot be directly "inherited" through those applicators, and it cannot be applied to an inherited type constraint.

The above spec causes issues when used with data validators such as openapi-core.

I think that the correct version of the above spec would look like:

{
  "Foo": {
    "type": "object",
    "properties": {
      "bar": {
        "type": "integer"
      }
    },
    "required": [
      "bar"
    ]
  },
  "MySchema": {
    "type": "object",
    "properties": {
      "foo": {
        "anyOf": [
          {
            "type": "object",
            "nullable": true
          },
          {
            "$ref": "#/components/schemas/Foo"
          }
        ]
      }
    }
  }
}

I've tested this with apispec 6.7.0 and marshmallow 3.23.1. I also have investigated a fix for this and am happy to open a PR if the above result is acceptable.

lafrech commented 4 weeks ago

I'm not sure about this.

The latter seems to indicate that foo may be either a nullable object or Foo ref.

The former can be found in this SO answer which makes sense to me. Not sure why it doesn't validate correctly in openapi-core, though.

ShepleySound commented 4 weeks ago

This quote from the linked OpenAPI proposal explains why the "allOf" solution does not work -

nullable: true operates within a single Schema Object. It does not "override" or otherwise compete with supertype or subtype schemas defined with allOf or other applicators. It cannot be directly "inherited" through those applicators, and it cannot be applied to an inherited type constraint.

The current behavior certainly looks like it should work, I agree there. But unfortunately OpenAPI seems to have made a different call.

I understand that having it be simply a nullable object isn't ideal. After all, the object can be anything. This is, I think, just a real issue with the 3.0 spec, since 3.1 introduced "null" as an actual "type". But IMO nullable: True, type: object within an anyOf is the clearest one can get while being in line with the spec (and thus producing a doc that is more likely to work with other tools in the ecosystem)

For what it's worth, here's a blog post that explains this issue from another perspective.

lafrech commented 3 weeks ago

PR merged and released. Thanks!