Fix #699 serdes missed on items in a collection.

[robertjustjones]

In the schema preprocessor recursion, items was left out of the handling for allOf, oneOf, anyOf, properties.

In my addition, I have commented out a check for schema.type == 'array'. Please advice on whether it's prudent to add or should be dropped out.

The test 699.spec.ts fails without this fix and passes with it. Alternatively, serdes.spec.ts could incorporate that change.

One note, I use Date rather than DateTime because json serialization is doing date.toString, so the serdes code isn't really being tested for date-time. Perhaps changing the test to use any other date format that toISOString would help.

#699

[robertjustjones]

Rebased in renewed hope of getting merged.

[Fabiencdp]

Coming here after a bunch of error like this : .response.items[0].createdAt should be string with this kind of declarations:

                  "items": {
                    "type": "array",
                    "items": {
                      "type": "object",
                      "required": ["createdAt"],
                      "properties": {
                        "createdAt": {
                          "type": "string",
                          "format": "date-time"
                        }
                      }
                    }
                  }

I was not getting this errors before, so that strange...

I tried your MR and i confirm your fix work well! Adding schema.type == 'array' looks good to me, but you should use a strict equality operator for code consistence.

Hope it will be merge soon!

[robertjustjones]

@Fabiencdp this was more work that I anticipated because the serdes test has been changed. What's odd is that when I run 699.spec with the "array" lines commented out, I would expect the test to fail but it's passing. I think something solved the array issue more subtly, perhaps bumping AJV. Could that be? There's no sense adding the code (202-204 in schema.preprocessor) if it's not needed. I would feel more comfortable getting tests for "array" in serdes.spec, but without them failing it's hard to see if I have them correct.

I've left my commit unsquashed it you'd like to look, not intending for them to be merged as is. Let me know about the above and I can resubmit.

Also, very happy to see the dicer issue addressed.

[Fabiencdp]

@robertjustjones, i'll check as soon as i can, I had some good test cases on a living application

[Fabiencdp]

I just tried with the last master version (4.14.0-beta.2), builded locally, imported to the live project, still have the serdes errors about dates. Then i added your fix (202-204 in schema.preprocessor), build it again, and it work well!

I didn't worked on the test for the moment, i'll try in the weekend.

[Fabiencdp]

Hi @robertjustjones, can you tell me wich test you expect to fail ?

EDIT: get it, should be the "should control GOOD id format and get a response in expected format" wich check for date format as string, so when commenting the 202-204, it should fail, am i right ? Good catch

[Fabiencdp]

I did some test using a reduced an anonymized part of our project schema, get it to work correctly. here is a gist: https://github.com/robertjustjones/express-openapi-validator/commit/9631dfd99edfeaae1c58cb402c80d3d6c367ce7b#diff-79c6ff5eb8a158b26e0c693cf08386a520d8bd27866f170ca808134d17317d5fR212 (it's an ugly WIP, please don't pay attention to linter stuff)

[Fabiencdp]

@robertjustjones, I digged about the test who should not pass without your fix, I found that it pass only when the array use a $ref. Instead, if you define the schema directly, the test fail as expected. This may be the reason why this bug has not yet been discovered.

To be clear, without the fix, the initial problem is: Date object is not converted to a date-time string, and validating against a schema property defined as date-time fail with this kind of error: "path/0/prop must be string"

For example, let say this part of schema:

       history:
          type: array
          items:
            type: object
            properties:
              updateDate:
                type: string
                format: date-time

with this response object :

let json = {
  history: [
    {
      updateDate: new Date()
    },
  ],
};

Will return {message: "/response/history/0/updateDate must be string", code: 500} wich is not good. Date is a valid date.

But, if you define the schema with a $ref, like this:

        history:
          type: array
          items:
            $ref: "#/components/schemas/HistoryItem"

The validator don't throw and response is valid: { history: [ { updateDate: '2022-08-23T21:46:36.588Z' } ] } It look's like serdes serialization is only used with $ref

So... What should we do ?

Did we must find why it happen ? It could be a problem with the way that reference are parsed ?

Or did we should accept your fix ?

I'll dig a little more while waiting your opinion.

EDIT: I think your fix is right, without it array items don't pass throught schemaVisitor method, so serdes are not applied to items.

[robertjustjones]

@Fabiencdp does the latest commit with the modified 699.spec cover the case you mention with historyWithoutRef?

[Fabiencdp]

Nop, i didn't push my last test at the moment

[Fabiencdp]

Sorry, I misunderstood your last comment, I'll check that today

[Fabiencdp]

@robertjustjones, no, it does not cover the case, the schema must be :

        historyWithoutRef:
          type: array
          items:
            type: object
            properties:
              modificationDate:
                type: string
                format: date

Then, comment your fix (schema.preprocessor.ts l202-204)

The run the test, you should get a 500 with body {message: "/response/historyWithoutRef/0/modificationDate must be string", code: 500}

If you re-active your fix, the test pass with success.

[robertjustjones]

@Fabiencdp you were exactly right! The tests looks to be passing and failing (without the code fix) as expected now.

I squashed the commits. Let's gets this one merged. Thanks, so much!

[Fabiencdp]

@cdimascio we also need your review

[srgg]

@cdimascio when do you think you will have time to take a look at this PR?

[srgg]

@robertjustjones I've faced with another allOf corner case and curious: if your changes also fixing that:

    post:
      requestBody:
        required: true
        content:
          application/json:
            schema:
              additionalProperties: false
              oneOf:
                - type: array
                  items:
                    $ref: '#/components/schemas/Variable'
                - $ref: '#/components/schemas/Variable'

  schemas:
    Variable:
      description: ""
      type: object
      required:
        - name
        - value
      additionalProperties: false
      properties:
        name:
          type: string
        value:
          type: string
          nullable: true

Check1, body:

{
  "name": "new variable",
  "value": "just new"
}

Fails and this is not expected:

{
  "code": 400,
  "message": "request/body must NOT have additional properties, request/body must NOT have additional properties",
  "errors": [
    {
      "path": "/body/name",
      "message": "must NOT have additional properties",
      "errorCode": "additionalProperties.openapi.validation"
    },
    {
      "path": "/body/value",
      "message": "must NOT have additional properties",
      "errorCode": "additionalProperties.openapi.validation"
    }
  ]
}

Check2, body:

[
  {
    "name": "new variable",
    "value": "just new"
  },
  {
    "name": "new variable",
    "value": "just new"
  }
]

Pass request validation as expected

[Fabiencdp]

@srgg

I'm not sure if "additionalProperties" is valid before "oneOf", "additionalProperties" should be inside a schema of type object, so only in components/schemas/Variable

You should try to change this part:

    post:
      requestBody:
        required: true
        content:
          application/json:
            schema:
              oneOf:
                - type: array
                  items:
                    $ref: '#/components/schemas/Variable'
                - $ref: '#/components/schemas/Variable'

[srgg]

@Fabiencdp It seems that you are tight, thank you. So it is another issue, I have turned schema validation and did not get any error about that invalid additionalProperties declaration.

UPD: One more proof that case I mistakenly broad up is not relevant for express-openapi-validator, it seems that allOf with additionalProperties a major issue at JSON schema level: https://stackoverflow.com/questions/22689900/json-schema-allof-with-additionalproperties

[robertjustjones]

@Fabiencdp @cdimascio looks like this PR would also close out #666. Identical code fix with similar test.

[Fabiencdp]

@cdimascio Allow me to revive you about this MR, if we can close this quickly :) Thanks you

[sgoodluck]

@robertjustjones, I digged about the test who should not pass without your fix, I found that it pass only when the array use a $ref. Instead, if you define the schema directly, the test fail as expected. This may be the reason why this bug has not yet been discovered.

To be clear, without the fix, the initial problem is: Date object is not converted to a date-time string, and validating against a schema property defined as date-time fail with this kind of error: "path/0/prop must be string"

For example, let say this part of schema:

       history:
          type: array
          items:
            type: object
            properties:
              updateDate:
                type: string
                format: date-time

with this response object :

let json = {
  history: [
    {
      updateDate: new Date()
    },
  ],
};

Will return {message: "/response/history/0/updateDate must be string", code: 500} wich is not good. Date is a valid date.

But, if you define the schema with a $ref, like this:

        history:
          type: array
          items:
            $ref: "#/components/schemas/HistoryItem"

The validator don't throw and response is valid: { history: [ { updateDate: '2022-08-23T21:46:36.588Z' } ] } It look's like serdes serialization is only used with $ref

So... What should we do ?

Did we must find why it happen ? It could be a problem with the way that reference are parsed ?

Or did we should accept your fix ?

I'll dig a little more while waiting your opinion.

EDIT: I think your fix is right, without it array items don't pass throught schemaVisitor method, so serdes are not applied to items.

Just spent several hours on this and found your comment which solved. Looking forward to having this one merged! Thank you for finding.

[robertjustjones]

@cdimascio many of us have spent a fair amount of time chasing this solved issue after the clear fix was known. Please do stamp this one approved and release it.

[robertjustjones]

Hey @cdimascio would some coffee help get this merged?

[Fabiencdp]

The owner has no github activity since july 2022... hope he is well. A backup reviewer would help in this case... I can try to send him an email, or should we fork this and declare this repository as dead?

[srgg]

@Fabiencdp You can check the owner's activity for the previous year, it seems to me that the owner appears once in a while (in 6-7 months). Not a good schedule as for production usage, but I have no chance to support this repo :(. Therefore, the question is there a chance that somebody can fork it and re-bake community around a new repo

[robertjustjones]

He's still recently active on LinkedIn, presumably just focused in other areas like we are. Adding reviewers and/or co-owners would seem to be the way to go to share the load.

[robertjustjones]

Thanks @cdimascio !

[robertjustjones]

[cdimascio]

:) thank you