fast-json-stringify icon indicating copy to clipboard operation
fast-json-stringify copied to clipboard

allOf Schema and additionalProperties result in unexpected result

Open artem-kliuev opened this issue 1 year ago • 10 comments

Prerequisites

  • [X] I have written a descriptive issue title
  • [X] 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.

artem-kliuev avatar Feb 20 '24 13:02 artem-kliuev

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

climba03003 avatar Feb 20 '24 14:02 climba03003

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

artem-kliuev avatar Feb 20 '24 14:02 artem-kliuev

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

mcollina avatar Feb 20 '24 14:02 mcollina

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 avatar Feb 20 '24 14:02 climba03003

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

climba03003 avatar Feb 20 '24 14:02 climba03003

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

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

additionalProperties: false is the problem.

artem-kliuev avatar Feb 20 '24 14:02 artem-kliuev

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

artem-kliuev avatar Feb 20 '24 14:02 artem-kliuev

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

climba03003 avatar Feb 20 '24 14:02 climba03003

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

@climba03003 Close please If i misunderstood

Uzlopak avatar Feb 20 '24 14:02 Uzlopak

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.

climba03003 avatar Feb 20 '24 14:02 climba03003