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

Fix nested nullable #845

Closed
wants to merge 2 commits into from
Closed

Fix nested nullable #845

wants to merge 2 commits into from

Conversation

Bangertm
Copy link
Collaborator

This fixes #833. field2nullable executes after other attribute functions including user added attribute functions and now has logic for handling cases with a reference object and an allOf object.

For OpenAPI 3.1 nullable Nested and Pluck fields have nullability
indicated by adding null to the list of types or adding a type
null to an anyOf list including the reference
Now bultin OpenAPI 3.1 nullability logic will run after user defined
attribute functions so that users don't need to reimplement it.
@Bangertm
Copy link
Collaborator Author

@lafrech After adding the case for executing field2nullable after user attribute functions I'm a little on the fence about whether it is needed or not. The only tests I can come up with are a bit contrived, but the change is minor so I'm inclined to go with it.

I'm not super happy with the tests I wrote here. Not sure if there's an obvious way to improve them. They are functional, but parallel to other tests without any elegant parameterization.

@jmgoncalves
Copy link

This works for me! Closing #841 in favour of this one.

@bram-tv
Copy link

bram-tv commented Jul 1, 2023

This change looks good for OpenAPI v3.1.0 but it doesn't look so good for OpenAPI v3.0.2/OpenAPI v3.0.3

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


class Child(Schema):
    name = fields.Str(metadata={"example": "Child-name"})


class Parent(Schema):
    name = fields.Str(metadata={"example": "Parent-name"})
    firstChild = fields.Nested(Child, allow_none=True)


class GrandParent(Schema):
    name = fields.Str(metadata={"example": "GrandParent-name"})
    firstChild = fields.Nested(Parent, allow_none=True)


spec = APISpec(
    title="Swagger Petstore",
    version="1.0.0",
    # openapi_version="3.0.2",
    openapi_version="3.0.3",
    # openapi_version="3.1.0",
    plugins=[MarshmallowPlugin()],
)


spec.path(
    path='/family/one',
    operations={
        'get': {
           'description': 'A family of one',
           'responses': {
               '200': {
                   'description': 'Success response',
                   'content': {
                       'application/json': {
                           "schema": GrandParent,
                       },
                   },
               },
           },
        },
    },
)

print(json.dumps(spec.to_dict(), indent=2))
Old spec
{
  "paths": {
    "/family/one": {
      "get": {
        "description": "A family of one",
        "responses": {
          "200": {
            "description": "Success response",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/GrandParent"
                }
              }
            }
          }
        }
      }
    }
  },
  "info": {
    "title": "Swagger Petstore",
    "version": "1.0.0"
  },
  "openapi": "3.0.3",
  "components": {
    "schemas": {
      "Child": {
        "type": "object",
        "properties": {
          "name": {
            "type": "string",
            "example": "Child-name"
          }
        }
      },
      "Parent": {
        "type": "object",
        "properties": {
          "firstChild": {
            "nullable": true,
            "allOf": [
              {
                "$ref": "#/components/schemas/Child"
              }
            ]
          },
          "name": {
            "type": "string",
            "example": "Parent-name"
          }
        }
      },
      "GrandParent": {
        "type": "object",
        "properties": {
          "firstChild": {
            "nullable": true,
            "allOf": [
              {
                "$ref": "#/components/schemas/Parent"
              }
            ]
          },
          "name": {
            "type": "string",
            "example": "GrandParent-name"
          }
        }
      }
    }
  }
}
New spec
{
  "paths": {
    "/family/one": {
      "get": {
        "description": "A family of one",
        "responses": {
          "200": {
            "description": "Success response",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/GrandParent"
                }
              }
            }
          }
        }
      }
    }
  },
  "info": {
    "title": "Swagger Petstore",
    "version": "1.0.0"
  },
  "openapi": "3.0.3",
  "components": {
    "schemas": {
      "Child": {
        "type": "object",
        "properties": {
          "name": {
            "type": "string",
            "example": "Child-name"
          }
        }
      },
      "Parent": {
        "type": "object",
        "properties": {
          "firstChild": {
            "$ref": "#/components/schemas/Child",
            "nullable": true
          },
          "name": {
            "type": "string",
            "example": "Parent-name"
          }
        }
      },
      "GrandParent": {
        "type": "object",
        "properties": {
          "firstChild": {
            "$ref": "#/components/schemas/Parent",
            "nullable": true
          },
          "name": {
            "type": "string",
            "example": "GrandParent-name"
          }
        }
      }
    }
  }
}

Diff between old and new spec:

diff --git a/spec_3.0.3.json b/spec_3.0.3_new.json
index 5d6f68f..5d442f3 100644
--- a/spec_3.0.3.json
+++ b/spec_3.0.3_new.json
@@ -38,12 +38,8 @@
         "type": "object",
         "properties": {
           "firstChild": {
-            "nullable": true,
-            "allOf": [
-              {
-                "$ref": "#/components/schemas/Child"
-              }
-            ]
+            "$ref": "#/components/schemas/Child",
+            "nullable": true
           },
           "name": {
             "type": "string",
@@ -55,12 +51,8 @@
         "type": "object",
         "properties": {
           "firstChild": {
-            "nullable": true,
-            "allOf": [
-              {
-                "$ref": "#/components/schemas/Parent"
-              }
-            ]
+            "$ref": "#/components/schemas/Parent",
+            "nullable": true
           },
           "name": {
             "type": "string",

Rendering the spec with https://editor-next.swagger.io/ and/or https://editor.swagger.io/

Render of old spec:
spec_3 0 3-editor-next

(Note: https://editor-next.swagger.io/ complains about the allOf usage but it does appear to do the right thing; https://editor.swagger.io/ does not complain about it. See swagger-api/swagger-editor#4026 )

Render of new spec:
spec_3 0 3_new-editor-next

The nullable: true annotation is missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect handling of marshmallow nested schema with allow_none=True in OpenAPI 3.1
3 participants