Fix #699 serdes missed on items in a collection. #704

robertjustjones · 2022-02-12T17:26:03Z

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 · 2022-04-05T19:45:29Z

Rebased in renewed hope of getting merged.

Fabiencdp · 2022-05-18T15:24:25Z

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!

Fabiencdp · 2022-05-18T15:26:00Z

src/middlewares/parsers/schema.preprocessor.ts

@@ -199,6 +199,9 @@ export class SchemaPreprocessor {
          const child = new Node(node, s, [...node.path, 'anyOf', i + '']);
          recurse(node, child, opts);
        });
+      } else if (/*schema.type == 'array' && */ schema.items) {


schema.type === 'array' ?

Yes schema.type array is correct here. Let’s get that updated and I’ll merge this in

@robertjustjones Can you add it to your merge request ? or i'll do it later

Yes I'll get that done this morning. Sorry that I didn't see the comment earlier.

robertjustjones · 2022-06-16T15:37:22Z

@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 · 2022-06-16T20:07:47Z

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

Fabiencdp · 2022-06-17T08:27:10Z

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 · 2022-06-24T08:08:50Z

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 · 2022-06-24T10:01:09Z

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

Fabiencdp · 2022-08-23T21:49:21Z

@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 · 2022-08-24T15:09:19Z

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

Fabiencdp · 2022-08-24T16:05:53Z

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

Fabiencdp · 2022-08-25T06:24:48Z

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

Fabiencdp · 2022-08-25T07:38:25Z

@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.

@Fabiencdp

Thanks @Fabiencdp.

robertjustjones · 2022-08-25T13:58:07Z

@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

Thanks for your work!

Fabiencdp · 2022-08-25T14:36:10Z

@cdimascio we also need your review

srgg · 2022-08-31T12:17:07Z

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

srgg · 2022-09-01T02:29:56Z

@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 · 2022-09-01T09:11:59Z

@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 · 2022-09-01T11:16:31Z

@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 · 2022-09-02T12:14:09Z

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

Fabiencdp · 2022-10-03T07:16:45Z

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

sgoodluck · 2022-10-13T19:32:52Z

@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 · 2022-10-14T14:07:55Z

@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 · 2022-10-31T21:15:19Z

Hey @cdimascio would some coffee help get this merged?

Fabiencdp · 2022-11-09T11:37:09Z

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 · 2022-11-09T11:55:49Z

@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 · 2022-11-09T14:56:45Z

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 · 2022-11-21T17:06:44Z

Thanks @cdimascio !

robertjustjones · 2022-11-22T20:26:41Z

cdimascio · 2022-11-28T03:44:41Z

:) thank you

robertjustjones force-pushed the issue-699-serdes-items-in-collection branch from c16ab96 to 8af4384 Compare February 12, 2022 19:06

robertjustjones force-pushed the issue-699-serdes-items-in-collection branch from 8af4384 to 77dfd37 Compare April 5, 2022 19:44

Fabiencdp reviewed May 18, 2022

View reviewed changes

robertjustjones force-pushed the issue-699-serdes-items-in-collection branch from 77dfd37 to 10519ec Compare June 16, 2022 15:28

robertjustjones mentioned this pull request Aug 22, 2022

Serdes Does Not Work for Items in Object in Array #699

Closed

robertjustjones force-pushed the issue-699-serdes-items-in-collection branch from 4425793 to e2480e1 Compare August 25, 2022 13:50

Fix cdimascio#699 serdes missed on items in a collection, with tests.

fdebec6

Thanks @Fabiencdp.

robertjustjones force-pushed the issue-699-serdes-items-in-collection branch from e2480e1 to fdebec6 Compare August 25, 2022 13:55

robertjustjones requested a review from Fabiencdp August 25, 2022 13:58

Fabiencdp approved these changes Aug 25, 2022

View reviewed changes

robertjustjones requested a review from cdimascio September 12, 2022 14:46

cdimascio approved these changes Nov 19, 2022

View reviewed changes

cdimascio merged commit 77bc4ae into cdimascio:master Nov 19, 2022

j-bellavista-flexidao mentioned this pull request Nov 9, 2023

Serdes Does Not Work for Items in Request Parameters in Array #873

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #699 serdes missed on items in a collection. #704

Fix #699 serdes missed on items in a collection. #704

robertjustjones commented Feb 12, 2022 •

edited

robertjustjones commented Apr 5, 2022

Fabiencdp commented May 18, 2022

Fabiencdp May 18, 2022

cdimascio May 30, 2022 •

edited

Fabiencdp Jun 15, 2022 •

edited

robertjustjones Jun 16, 2022

robertjustjones commented Jun 16, 2022

Fabiencdp commented Jun 16, 2022

Fabiencdp commented Jun 17, 2022

Fabiencdp commented Jun 24, 2022 •

edited

Fabiencdp commented Jun 24, 2022

Fabiencdp commented Aug 23, 2022 •

edited

robertjustjones commented Aug 24, 2022

Fabiencdp commented Aug 24, 2022

Fabiencdp commented Aug 25, 2022

Fabiencdp commented Aug 25, 2022

robertjustjones commented Aug 25, 2022

Fabiencdp left a comment

Fabiencdp commented Aug 25, 2022

srgg commented Aug 31, 2022

srgg commented Sep 1, 2022

Fabiencdp commented Sep 1, 2022

srgg commented Sep 1, 2022 •

edited

robertjustjones commented Sep 2, 2022

Fabiencdp commented Oct 3, 2022

sgoodluck commented Oct 13, 2022

robertjustjones commented Oct 14, 2022

robertjustjones commented Oct 31, 2022

Fabiencdp commented Nov 9, 2022

srgg commented Nov 9, 2022

robertjustjones commented Nov 9, 2022

robertjustjones commented Nov 21, 2022

robertjustjones commented Nov 22, 2022

cdimascio commented Nov 28, 2022

Fix #699 serdes missed on items in a collection. #704

Fix #699 serdes missed on items in a collection. #704

Conversation

robertjustjones commented Feb 12, 2022 • edited

robertjustjones commented Apr 5, 2022

Fabiencdp commented May 18, 2022

Fabiencdp May 18, 2022

Choose a reason for hiding this comment

cdimascio May 30, 2022 • edited

Choose a reason for hiding this comment

Fabiencdp Jun 15, 2022 • edited

Choose a reason for hiding this comment

robertjustjones Jun 16, 2022

Choose a reason for hiding this comment

robertjustjones commented Jun 16, 2022

Fabiencdp commented Jun 16, 2022

Fabiencdp commented Jun 17, 2022

Fabiencdp commented Jun 24, 2022 • edited

Fabiencdp commented Jun 24, 2022

Fabiencdp commented Aug 23, 2022 • edited

robertjustjones commented Aug 24, 2022

Fabiencdp commented Aug 24, 2022

Fabiencdp commented Aug 25, 2022

Fabiencdp commented Aug 25, 2022

robertjustjones commented Aug 25, 2022

Fabiencdp left a comment

Choose a reason for hiding this comment

Fabiencdp commented Aug 25, 2022

srgg commented Aug 31, 2022

srgg commented Sep 1, 2022

Check1, body:

Check2, body:

Fabiencdp commented Sep 1, 2022

srgg commented Sep 1, 2022 • edited

robertjustjones commented Sep 2, 2022

Fabiencdp commented Oct 3, 2022

sgoodluck commented Oct 13, 2022

robertjustjones commented Oct 14, 2022

robertjustjones commented Oct 31, 2022

Fabiencdp commented Nov 9, 2022

srgg commented Nov 9, 2022

robertjustjones commented Nov 9, 2022

robertjustjones commented Nov 21, 2022

robertjustjones commented Nov 22, 2022

cdimascio commented Nov 28, 2022

robertjustjones commented Feb 12, 2022 •

edited

cdimascio May 30, 2022 •

edited

Fabiencdp Jun 15, 2022 •

edited

Fabiencdp commented Jun 24, 2022 •

edited

Fabiencdp commented Aug 23, 2022 •

edited

srgg commented Sep 1, 2022 •

edited