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.

Sign up for GitHub

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

Jump to bottom

nullable is not recognized / respected in schema #51

Closed
cgredig opened this issue Nov 4, 2018 · 9 comments
Closed

nullable is not recognized / respected in schema #51

cgredig opened this issue Nov 4, 2018 · 9 comments
Labels
bug Something isn't working conformance

Comments

@cgredig
Copy link

cgredig commented Nov 4, 2018 •
edited
Loading

Hey there,

I was trying to convert a former Swagger 2 application to OpenAPI 3, switching to Exegesis. I began by following the Exegesis Tutorial which worked. Then I replaced the OpenAPI yaml and adjusted some names. Now my requests are still accepted and reply as expected - except if one of the fields is null. This was the original reason why I wanted to switch to OpenAPI 3 - it has the "nullable" attribute. Sadly, this seems to be ignored by exegesis.

So, here is my OpenAPI doc (it describes something similar to the payload sent by Bitbucket with webhooks):

openapi: 3.0.1
info:
  title: My API
  version: 1.0.0
paths:
  '/push':
    post:
      summary: Greets the user
      operationId: processMessageForPush
      x-exegesis-controller: incoming
      parameters:
        - name: GIT_BRANCH
          in: query
          schema:
            type: string
          style: form
          explode: false
      requestBody:
        required: true
        content:
          application/json:
            schema:
              $ref: "#/components/schemas/Payload"
      responses:
        '200':
          description: YEAH
components:
  schemas:
    Payload:
      description: >-
        Message object. Containing set of control parameters and a payload
        KeyValuePair array.
      type: object
      properties:
        actor:
          $ref: "#/components/schemas/User"
        repository:
          type: object
        push:
          $ref: "#/components/schemas/PushInfo"
    PushInfo:
      type: object
      properties:
        changes:
          type: array
          items:
            $ref: "#/components/schemas/ChangeInfo"
    ChangeInfo:
      type: object
      properties:
        new:
          $ref: "#/components/schemas/StateInfo"
        old:
          $ref: "#/components/schemas/StateInfo"
        links:
          $ref: "#/components/schemas/Links"
        created:
          type: boolean
        forced:
          type: boolean
        closed:
          type: boolean
        commits:
          type: array
          items:
            $ref: "#/components/schemas/CommitInfo"
        truncated:
          type: boolean
    CommitInfo:
      type: object
      properties:
        hash:
          type: string
        type:
          type: string
        message:
          type: string
        author:
          $ref: "#/components/schemas/User"
        links:
          $ref: "#/components/schemas/Links"
    StateInfo:
      type: object
      nullable: true
      properties:
        type:
          type: string
        name:
          type: string
        target:
          $ref: "#/components/schemas/TargetInfo"
        links:
          $ref: "#/components/schemas/Links"
    TargetInfo:
      type: object
      properties:
        type:
          type: string
        hash:
          type: string
        author:
          $ref: "#/components/schemas/User"
        message:
          type: string
        date:
          type: string
        parents:
          type: array
          items:
            $ref: "#/components/schemas/Parent"
        links:
          $ref: "#/components/schemas/Links"
    Parent:
      type: object
      properties:
        type:
          type: string
        hash:
          type: string
        links:
          $ref: "#/components/schemas/Links"
    User:
      type: object
    Links:
      type: object
      properties:
        html:
          $ref: "#/components/schemas/href"
        diff:
          $ref: "#/components/schemas/href"
        commits:
          $ref: "#/components/schemas/href"
        self:
          $ref: "#/components/schemas/href"
    href:
      type: object
      properties:
        href:
          type: string

The request I send is a POST request to the URL http://localhost:3000/push?GIT_BRANCH=develop with the body as follows:

{
  "push": {
    "changes": [
      {
        "forced": false,
        "old": null,
        "links": {
          "commits": {
            "href": "asdf"
          },
          "html": {
            "href": "asdf"
          }
        },
        "truncated": true,
        "commits": [
          {
            "hash": "123412341234",
            "links": {
              "self": {
                "href": "asdf"
              },
              "comments": {
                "href": "asdf"
              },
              "patch": {
                "href": "asdf"
              },
              "html": {
                "href": "asdf"
              },
              "diff": {
                "href": "asdf"
              },
              "approve": {
                "href": "asdf"
              },
              "statuses": {
                "href": "asdf"
              }
            },
            "author": {
              "raw": "One Author",
              "type": "author",
              "user": {
                "username": "test",
                "display_name": "One Author",
                "account_id": "asdf",
                "links": {
                  "self": {
                    "href": "asdf"
                  },
                  "html": {
                    "href": "asdf"
                  },
                  "avatar": {
                    "href": "asdf"
                  }
                },
                "type": "user",
                "nickname": "test",
                "uuid": "asdf"
              }
            },
            "summary": {
              "raw": "sdfhsdhfsdf\n",
              "markup": "markdown",
              "html": "<p>sdfhsdhfsdf</p>",
              "type": "rendered"
            },
            "parents": [
              {
                "type": "commit",
                "hash": "123412341234",
                "links": {
                  "self": {
                    "href": "asdf"
                  },
                  "html": {
                    "href": "asdf"
                  }
                }
              }
            ],
            "date": "2018-10-30T16:19:16+00:00",
            "message": "sdfhsdhfsdf\n",
            "type": "commit"
          },
          {
            "hash": "123412341234",
            "links": {
              "self": {
                "href": "asdf"
              },
              "comments": {
                "href": "asdf"
              },
              "patch": {
                "href": "asdf"
              },
              "html": {
                "href": "asdf"
              },
              "diff": {
                "href": "asdf"
              },
              "approve": {
                "href": "asdf"
              },
              "statuses": {
                "href": "asdf"
              }
            },
            "author": {
              "raw": "One Author",
              "type": "author",
              "user": {
                "username": "test",
                "display_name": "One Author",
                "account_id": "asdf",
                "links": {
                  "self": {
                    "href": "asdf"
                  },
                  "html": {
                    "href": "asdf"
                  },
                  "avatar": {
                    "href": "asdf"
                  }
                },
                "type": "user",
                "nickname": "test",
                "uuid": "asdf"
              }
            },
            "summary": {
              "raw": "because I need real commits to test my triggers, I will fix all the spelling mistakes\n",
              "markup": "markdown",
              "html": "<p>because I need real commits to test my triggers, I will fix all the spelling mistakes</p>",
              "type": "rendered"
            },
            "parents": [
              {
                "type": "commit",
                "hash": "123412341234",
                "links": {
                  "self": {
                    "href": "asdf"
                  },
                  "html": {
                    "href": "asdf"
                  }
                }
              }
            ],
            "date": "2018-10-30T15:46:20+00:00",
            "message": "because I need real commits to test my triggers, I will fix all the spelling mistakes\n",
            "type": "commit"
          },
          {
            "hash": "123412341234",
            "links": {
              "self": {
                "href": "asdf"
              },
              "comments": {
                "href": "asdf"
              },
              "patch": {
                "href": "asdf"
              },
              "html": {
                "href": "asdf"
              },
              "diff": {
                "href": "asdf"
              },
              "approve": {
                "href": "asdf"
              },
              "statuses": {
                "href": "asdf"
              }
            },
            "author": {
              "raw": "asdf",
              "type": "author",
              "user": {
                "username": "asdf",
                "display_name": "asdf",
                "account_id": "asdf",
                "links": {
                  "self": {
                    "href": "asdf"
                  },
                  "html": {
                    "href": "asdf"
                  },
                  "avatar": {
                    "href": "asdf"
                  }
                },
                "type": "user",
                "nickname": "asdf",
                "uuid": "asdf"
              }
            },
            "summary": {
              "raw": "Updated Version to 1.0\n",
              "markup": "markdown",
              "html": "<p>Updated Version to 1.0</p>",
              "type": "rendered"
            },
            "parents": [
              {
                "type": "commit",
                "hash": "123412341234",
                "links": {
                  "self": {
                    "href": "asdf"
                  },
                  "html": {
                    "href": "asdf"
                  }
                }
              }
            ],
            "date": "2018-09-07T12:38:19+00:00",
            "message": "Updated Version to 1.0\n",
            "type": "commit"
          },
          {
            "hash": "123412341234",
            "links": {
              "self": {
                "href": "asdf"
              },
              "comments": {
                "href": "asdf"
              },
              "patch": {
                "href": "asdf"
              },
              "html": {
                "href": "asdf"
              },
              "diff": {
                "href": "asdf"
              },
              "approve": {
                "href": "asdf"
              },
              "statuses": {
                "href": "asdf"
              }
            },
            "author": {
              "raw": "asdf",
              "type": "author",
              "user": {
                "username": "asdf",
                "display_name": "asdf",
                "account_id": "asdf",
                "links": {
                  "self": {
                    "href": "asdf"
                  },
                  "html": {
                    "href": "asdf"
                  },
                  "avatar": {
                    "href": "asdf"
                  }
                },
                "type": "user",
                "nickname": "asdf",
                "uuid": "asdf"
              }
            },
            "summary": {
              "raw": "Updated Version to 1.0.2-testprod\n",
              "markup": "markdown",
              "html": "<p>Updated Version to 1.0.2-testprod</p>",
              "type": "rendered"
            },
            "parents": [
              {
                "type": "commit",
                "hash": "123412341234",
                "links": {
                  "self": {
                    "href": "asdf"
                  },
                  "html": {
                    "href": "asdf"
                  }
                }
              }
            ],
            "date": "2018-07-19T09:24:19+00:00",
            "message": "Updated Version to 1.0.2-testprod\n",
            "type": "commit"
          },
          {
            "hash": "123412341234",
            "links": {
              "self": {
                "href": "asdf"
              },
              "comments": {
                "href": "asdf"
              },
              "patch": {
                "href": "asdf"
              },
              "html": {
                "href": "asdf"
              },
              "diff": {
                "href": "asdf"
              },
              "approve": {
                "href": "asdf"
              },
              "statuses": {
                "href": "asdf"
              }
            },
            "author": {
              "raw": "One Author",
              "type": "author",
              "user": {
                "username": "test",
                "display_name": "One Author",
                "account_id": "asdf",
                "links": {
                  "self": {
                    "href": "asdf"
                  },
                  "html": {
                    "href": "asdf"
                  },
                  "avatar": {
                    "href": "asdf"
                  }
                },
                "type": "user",
                "nickname": "test",
                "uuid": "asdf"
              }
            },
            "summary": {
              "raw": "asdf",
              "markup": "markdown",
              "html": "asdf",
              "type": "rendered"
            },
            "parents": [
              {
                "type": "commit",
                "hash": "123412341234",
                "links": {
                  "self": {
                    "href": "asdf"
                  },
                  "html": {
                    "href": "asdf"
                  }
                }
              }
            ],
            "date": "2018-07-19T08:29:02+00:00",
            "message": "asdf",
            "type": "commit"
          }
        ],
        "created": true,
        "closed": false,
        "new": {
          "target": {
            "hash": "123412341234",
            "links": {
              "self": {
                "href": "asdf"
              },
              "html": {
                "href": "asdf"
              }
            },
            "author": {
              "raw": "One Author",
              "type": "author",
              "user": {
                "username": "test",
                "display_name": "One Author",
                "account_id": "asdf",
                "links": {
                  "self": {
                    "href": "asdf"
                  },
                  "html": {
                    "href": "asdf"
                  },
                  "avatar": {
                    "href": "asdf"
                  }
                },
                "type": "user",
                "nickname": "test",
                "uuid": "asdf"
              }
            },
            "summary": {
              "raw": "sdfhsdhfsdf\n",
              "markup": "markdown",
              "html": "<p>sdfhsdhfsdf</p>",
              "type": "rendered"
            },
            "parents": [
              {
                "type": "commit",
                "hash": "123412341234",
                "links": {
                  "self": {
                    "href": "asdf"
                  },
                  "html": {
                    "href": "asdf"
                  }
                }
              }
            ],
            "date": "2018-10-30T16:19:16+00:00",
            "message": "sdfhsdhfsdf\n",
            "type": "commit"
          },
          "links": {
            "commits": {
              "href": "asdf"
            },
            "self": {
              "href": "asdf"
            },
            "html": {
              "href": "asdf"
            }
          },
          "default_merge_strategy": "merge_commit",
          "merge_strategies": [
            "merge_commit",
            "squash",
            "fast_forward"
          ],
          "type": "branch",
          "name": "spelling"
        }
      }
    ]
  },
  "repository": {
    "scm": "git",
    "website": "",
    "name": "asdf",
    "links": {
      "self": {
        "href": "asdf"
      },
      "html": {
        "href": "asdf"
      },
      "avatar": {
        "href": "asdf"
      }
    },
    "project": {
      "links": {
        "self": {
          "href": "asdf"
        },
        "html": {
          "href": "asdf"
        },
        "avatar": {
          "href": "asdf"
        }
      },
      "type": "project",
      "uuid": "asdf",
      "key": "asdf",
      "name": "asdf"
    },
    "full_name": "asdf/asdf",
    "owner": {
      "username": "asdf",
      "type": "team",
      "display_name": "asdf",
      "uuid": "asdf",
      "links": {
        "self": {
          "href": "asdf"
        },
        "html": {
          "href": "asdf"
        },
        "avatar": {
          "href": "asdf"
        }
      }
    },
    "type": "repository",
    "is_private": true,
    "uuid": "asdf"
  },
  "actor": {
    "username": "test",
    "display_name": "One Author",
    "account_id": "asdf",
    "links": {
      "self": {
        "href": "asdf"
      },
      "html": {
        "href": "asdf"
      },
      "avatar": {
        "href": "asdf"
      }
    },
    "type": "user",
    "nickname": "test",
    "uuid": "asdf"
  }
}

As you can see, the value for push.changes[0].old is null, which is why I made the according schema nullable. Still, I receive the following answer:

{
    "message": "Validation errors",
    "errors": [
        {
            "message": "should be object",
            "location": {
                "in": "request",
                "name": "body",
                "docPath": "/paths/~1push/post/requestBody/content/application~1json/schema",
                "path": "/push/changes/0/old"
            }
        }
    ]
}

How is that possible? Apart from some names, I did not change the options or the content of index.js as explained in the tutorial.

(Field contents in the payload have been partly anonymized.)
@cgredig
Copy link
Author

cgredig commented Nov 4, 2018

Some more research suggested to change the syntax as following:

        old:
          nullable: true
          allOf:
            - $ref: "#/components/schemas/StateInfo"

That did not change anything.

@niallmccullagh
Copy link
Contributor

I think this is a side effect of the differences in Open API spec and JSON schema. The validator library that exegesis uses is AJV. I came across this bug ajv-validator/ajv#486 which suggests adding a custom keyword.

@jwalton
Copy link
Contributor

jwalton commented Nov 4, 2018

Ah, I'm afraid I misread the spec:

Other than the JSON Schema subset fields, the following fields MAY be used for further schema documentation:

nullable - boolean - Allows sending a null value for the defined schema. Default value is false.

I assumed "for further schema documentation" meant these fields were mostly informational (and, indeed, most of these we don't care about from a schema validation viewpoint, except maybe readOnly and writeOnly).

I assumed you could do something like:

        old:
          nullable: true
          oneOf:
            - $ref: "#/components/schemas/StateInfo"
            - type: null

Except, for some weird reason, null is not supported as a type. So yeah, we're going to need to add a custom keyword.

@jwalton jwalton added bug Something isn't working conformance labels Nov 4, 2018
@cgredig
Copy link
Author

cgredig commented Nov 4, 2018

Glad to see this is indeed a bug, so it can be fixed. :-) Thank you both for the quick responses. I meanwhile confirmed that the code example @jwalton gave in his comment indeed does not work (schema is then invalid and app will not start).

@cgredig
Copy link
Author

cgredig commented Nov 9, 2018

Oh and @jwalton, if I can do anything to help fix this issue let me know!

@niallmccullagh
Copy link
Contributor

@jwalton It hasn't been released yet but there is a fix for this on the master branch ajv-validator/ajv@f2010f4 so worth keeping an eye on.

@cgredig Writing a failing test, that is small and concise, is always a good way to contribute. For example, writing a patch like the following which can be used to confirm the issue and also to validate the fix.

diff --git a/test/oas3/Schema/validatorsTest.ts b/test/oas3/Schema/validatorsTest.ts
index 8f32482..bbaba7c 100644
--- a/test/oas3/Schema/validatorsTest.ts
+++ b/test/oas3/Schema/validatorsTest.ts
@@ -49,6 +49,16 @@ const openApiDoc : oas3.OpenAPIObject = Object.assign(
                         }
                     }
                 },
+                aNullableObject: {
+                    type: 'object',
+                    required: ['a'],
+                    nullable: true,
+                    properties: {
+                        a: {
+                            type: 'string',
+                        }
+                    }
+                },
                 withDefault: {
                     type: 'object',
                     properties: {
@@ -234,6 +244,15 @@ describe('schema validators', function() {
         expect(validator(undefined).errors).to.eql(null);
     });
 
+    it('should not error for a missing object if nullable', function() {
+        const context = makeContext(openApiDoc, '#/components/schemas/aNullableObject');
+
+        const validator = validators.generateRequestValidator(context, QUERY_PARAM_LOCATION, false);
+
+        expect(validator(null).errors).to.eql(null);
+        expect(validator(undefined).errors).to.eql(null);
+    });
+
     it('should fill in default values', function() {
         const context = makeContext(openApiDoc, '#/components/schemas/withDefault');

@jwalton
Copy link
Contributor

jwalton commented Nov 26, 2018

@cgredig If you want to take a stab at writing a custom keyword for AJV, I wouldn't say no. :) I was hoping I could get to it but I'm buried in a rewrite of our front end right now. :(

niallmccullagh added a commit to niallmccullagh/exegesis that referenced this issue Nov 29, 2018
@niallmccullagh
fix: 🐛 Add support for OpenAPI nullable field
bc154f6 
As a result of the divergence of the OpenAPI spec and JSON Schema the
underlying Ajv libary needed upgraded to support for the nullable field.

Issues: Fixes exegesis-js#51
@niallmccullagh niallmccullagh mentioned this issue Nov 29, 2018
Merged
@jwalton
Copy link
Contributor

jwalton commented Nov 29, 2018

🎉 This issue has been resolved in version 1.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@cgredig
Copy link
Author

cgredig commented Nov 30, 2018

Awesome! Thanks a lot for fixing this, I upgraded exegesis in my app and can confirm the fix resolves my issues. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working conformance
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@niallmccullagh @jwalton @cgredig