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

allOf Schema and additionalProperties result in unexpected result #684

Open
2 tasks done
artem-kliuev opened this issue Feb 20, 2024 · 10 comments
Open
2 tasks done

Comments

@artem-kliuev
Copy link

artem-kliuev commented Feb 20, 2024

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the regression has not already been reported

Last working version

5.11.1

Stopped working in version

5.12.0

Node.js version

21.x

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

14.2

💥 Regression Report

allOf schema stringify doesn't work as expected. See details below.

Steps to Reproduce

Here is example:

import FJS from 'fast-json-stringify';
import util from 'util';

const AJV_CONFIG = { removeAdditional: 'all', allErrors: true, coerceTypes: true, strictTypes: false };

const schema = {
    allOf: [
        {
            type: 'object',
            properties: {
                id: {
                    type: 'integer'
                },
                parent_id: {
                    type: ['null', 'integer'],
                    default: null
                },
                name: {
                    type: 'string'
                }
            },
            required: ['id', 'name']
        },
        {
            type: 'object',
            properties: {
                integrations: {
                    type: 'array',
                    items: {
                        type: 'object',
                        properties: {
                            id: {
                                type: 'integer'
                            },
                            domain: {
                                type: 'string'
                            },
                            is_enabled: {
                                type: 'boolean'
                            }
                        },
                        required: ['id', 'domain', 'is_enabled']
                    }
                }
            },
            required: ['integrations']
        },
        {
            type: 'object',
            properties: {
                parent: {
                    oneOf: [
                        {
                            type: 'object',
                            properties: {
                                id: {
                                    type: 'integer'
                                },
                                name: {
                                    type: 'string'
                                }
                            },
                            required: ['id', 'name']
                        },
                        {
                            type: 'null'
                        }
                    ],
                    default: null
                }
            }
        }
    ]
};

const s = FJS({ additionalProperties: false, ...schema }, { ajv: AJV_CONFIG });

const input = {
    id: 1,
    name: 'Name',
    integrations: [
        {
            id: 1,
            domain: 'example.com',
            is_enabled: 1
        }
    ],
    parent_id: 1,
    parent: {
        id: 1,
        name: 'Name'
    }
};

console.log(util.inspect(JSON.parse(s(input)), false, null, true));

Expected Behavior

Version 5.11.1

Expected result:

{
    id: 1,
    parent_id: 1,
    name: 'Name',
    integrations: [{ id: 1, domain: 'example.com', is_enabled: true }],
    parent: { id: 1, name: 'Name' }
}

Next, if i comment down integrations.domain inside input object i should receive error:

Error: "domain" is required!

Version 5.12.0

Next in version 5.12.0 result will be following:

{
  id: 1,
  name: 'Name',
  integrations: [ { id: 1, domain: 'example.com', is_enabled: 1 } ],
  parent_id: 1,
  parent: { id: 1, name: 'Name' }
}

We can see that integrations.is_enabled has wrong type.

Next, if i comment down integrations.domain inside input object i should receive error, but got this:

{
  id: 1,
  name: 'Name',
  integrations: [ { id: 1, is_enabled: 1 } ],
  parent_id: 1,
  parent: { id: 1, name: 'Name' }
}

No error fired and domain property is missed. Remove additional props is not working as well.

Thank you for attention.
Cheers.

@climba03003
Copy link
Member

Your example is using allOf but the title tells anyOf.
Which one is the one not working?

@artem-kliuev artem-kliuev changed the title Broken handling of anyOf schemas in latest version Broken handling of allOf schemas in latest version Feb 20, 2024
@artem-kliuev
Copy link
Author

@climba03003 yep my bad, i've updated title and details

@mcollina
Copy link
Member

cc @ivan-tymoshenko it might be a bug in your latest changes

@climba03003
Copy link
Member

climba03003 commented Feb 20, 2024

I cannot reproduce your problem.
Can you please double check if your example is correct?

import { mergeSchemas } from '@fastify/merge-json-schemas'
import { build } from 'fast-json-stringify'

const schema = {
  allOf: [
      {
          type: 'object',
          properties: {
              id: {
                  type: 'integer'
              },
              parent_id: {
                  type: ['null', 'integer'],
                  default: null
              },
              name: {
                  type: 'string'
              }
          },
          required: ['id', 'name']
      },
      {
          type: 'object',
          properties: {
              integrations: {
                  type: 'array',
                  items: {
                      type: 'object',
                      properties: {
                          id: {
                              type: 'integer'
                          },
                          domain: {
                              type: 'string'
                          },
                          is_enabled: {
                              type: 'boolean'
                          }
                      },
                      required: ['id', 'domain', 'is_enabled']
                  }
              }
          },
          required: ['integrations']
      },
      {
          type: 'object',
          properties: {
              parent: {
                  oneOf: [
                      {
                          type: 'object',
                          properties: {
                              id: {
                                  type: 'integer'
                              },
                              name: {
                                  type: 'string'
                              }
                          },
                          required: ['id', 'name']
                      },
                      {
                          type: 'null'
                      }
                  ],
                  default: null
              }
          }
      }
  ]
}

const merged = mergeSchemas(schema.allOf)

console.log(JSON.stringify(merged, null, 2))

const input = {
  id: 1,
  name: 'Name',
  integrations: [
      {
          id: 1,
          domain: 'example.com',
          is_enabled: 1
      }
  ],
  parent_id: 1,
  parent: {
      id: 1,
      name: 'Name'
  }
}


const stringify = build(merged)

const result = stringify(input)
console.log(result)

const stringify2 = build(schema)
const result2 = stringify2(input)
console.log(result2)

@climba03003 climba03003 added the need info More information is needed to resolve this issue label Feb 20, 2024
@climba03003
Copy link
Member

I see where the problem comes from.
It only appear when you have explicitly provide additionalProperties: false

@climba03003 climba03003 removed the need info More information is needed to resolve this issue label Feb 20, 2024
@artem-kliuev
Copy link
Author

@climba03003 hmm, yep i've double checked it now and found that

build({ additionalProperties: false, ...schema }

additionalProperties: false is the problem.

@artem-kliuev
Copy link
Author

@climba03003 yep, seems i should update my codebase then. Thank you for fast response, you're very helpful! Have a good day, guys

@climba03003
Copy link
Member

It should be a bug that need to fix though.
additionalProperties: false poison the whole schema merging process.

@Uzlopak
Copy link
Contributor

Uzlopak commented Feb 20, 2024

If I understand @climba03003 correctly than this is a bug.
Reopening this issue

@climba03003 Close please If i misunderstood

@Uzlopak Uzlopak reopened this Feb 20, 2024
@Uzlopak Uzlopak changed the title Broken handling of allOf schemas in latest version allOf Schema and additionalProperties result in unexpected result Feb 20, 2024
@climba03003
Copy link
Member

climba03003 commented Feb 20, 2024

In the test, it seems like expected result.

https://github.com/fastify/merge-json-schemas/blob/main/test/properties.test.js#L138-L167

It is easy to have additional additionalProperties: false inside the schema.
The current merging not a desirable one for fast-json-stringify.

I would say we should either

  1. Warn the user if they provide exact { additionalProperties: false }.
  2. Only fallback when additionalProperties is object.

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

No branches or pull requests

4 participants