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

oneOf/anyOf doesn't seem to work with objects #290

Open
bluenote10 opened this issue Mar 6, 2021 · 16 comments · Fixed by cjsewell/fast-json-stringify#1
Open

oneOf/anyOf doesn't seem to work with objects #290

bluenote10 opened this issue Mar 6, 2021 · 16 comments · Fixed by cjsewell/fast-json-stringify#1
Labels
bug Confirmed bug good first issue Good for newcomers

Comments

@bluenote10
Copy link

bluenote10 commented Mar 6, 2021

🐛 Bug Report

oneOf/anyOf doesn't seem to work with objects.

To Reproduce

The following schema models a union type, i.e. in TypeScript terms:

type Foo = { foo: string, baz: string };
type Bar = { bar: string, baz: string };
type FooBar = Foo | Bar;
const fastJson = require("fast-json-stringify");

const schemaFoo = {
  properties: {
    foo: { type: "string" },
    baz: { type: "string" },
  },
  required: ["foo", "baz"],
};

const schemaBar = {
  properties: {
    bar: { type: "string" },
    baz: { type: "string" },
  },
  required: ["bar", "baz"],
};

const stringify = fastJson({
  type: "object",
  anyOf: [schemaFoo, schemaBar],
});

console.log(stringify({ foo: "foo", baz: "baz" }));
console.log(stringify({ bar: "bar", baz: "baz" }));

Output:

{}
{}

Expected behavior

Since both objects match the schema, both should be stringified.

Your Environment

  • node version: v14.16.0
  • fastify version: 3.13.0
  • fast-json-stringify: 2.4.2
  • os: Ubuntu 18.04
@mcollina
Copy link
Member

mcollina commented Mar 7, 2021

Would you mind trying to add type: 'object' to both schemas?

@bluenote10
Copy link
Author

Yes I did that as well before, but it didn't make a difference.

@Eomm
Copy link
Member

Eomm commented Mar 7, 2021

this schema works:

const stringify = fastJson({
  anyOf: [schemaFoo, schemaBar]
})

Defining the type beside the anyOf would conflict when your data could be an object or a string like:

const stringify = fastJson({
  anyOf: [schemaFoo, schemaBar, { type: 'string' }]
})

@bluenote10
Copy link
Author

Good catch, it makes certainly more sense without specifying type in the top level schema. I'm not sure where I got that from. So perhaps there is no need to support that.

I just did a quick check and ran

{
  "type": "object",
  "anyOf": [
    {
      "properties": {
        "foo": { "type": "string" }
      }
    },
    {
      "properties": {
        "bar": { "type": "string" }
      }
    }
  ]
}

through the online JSON schema validator, which considers it valid against all schema drafts, but probably cannot verify such semantic violations anyway.

@mcollina
Copy link
Member

mcollina commented Mar 7, 2021

I think this is a bug in this module: we should throw if we cannot deal with this ambiguity. Better of, we could handle it if it wasn't really hard.

The schema is valid JSON schema (we validate it on creation).

@mcollina mcollina added the bug Confirmed bug label Mar 7, 2021
@mcollina
Copy link
Member

mcollina commented Mar 7, 2021

Would you like to send a PR?

@bluenote10
Copy link
Author

Sure, I can have a look, but it may take a bit until I get to it.

@mcollina mcollina added the good first issue Good for newcomers label Mar 7, 2021
@stalniy
Copy link

stalniy commented Jul 16, 2021

same issue on my side, trying to serialize

const schema = {
  type: 'array',
  items: {
    anyOf: [
      {
        type: 'object',
        required: [ 'id', 'createdAt', 'author', 'type', 'body' ],
        properties: {
          id: { type: 'string' },
          createdAt: { type: 'string', format: 'date-time' },
          author: {
            type: 'object',
            required: [ '_id', 'fullName' ],
            properties: { _id: { type: 'string' }, fullName: { type: 'string' } },
            additionalProperties: false
          },
          type: { type: 'string', enum: [ 'Text' ] },
          body: { type: 'string' }
        },
        additionalProperties: false
      },
      {
        type: 'object',
        required: [ 'id', 'createdAt', 'author', 'type', 'status' ],
        properties: {
          id: { type: 'string' },
          createdAt: { type: 'string', format: 'date-time' },
          author: {
            type: 'object',
            required: [ '_id', 'fullName' ],
            properties: { _id: { type: 'string' }, fullName: { type: 'string' } },
            additionalProperties: false
          },
          type: { type: 'string', enum: [ 'StatusTransition' ] },
          status: {
            type: 'string',
          }
        },
        additionalProperties: false
      }
    ]
  }
}

And I get :

{
  "messages": [
    null
  ]
}

Update: if I add type: "object" to anyOf I get

{
  "messages": [
    {}
  ]
}

Update 2: Full example to test: https://gist.github.com/stalniy/05f3940d5a55d80a38557edd8ffbfe14

@stalniy
Copy link

stalniy commented Jul 16, 2021

is there any support for discriminated union types?

@Eomm
Copy link
Member

Eomm commented Jul 17, 2021

is there any support for discriminated union types?

As a general rule, I suggest running the debugMode whenever there is some issue to understand what is going on.

Debugging a bit your gist with this feature I found:

  • if you change the schema removing the type, the serializer function output would be fine.
    this is a bug in FJS and it would be great if you would like to fix it (ignore the type when there is the anyOf keyword)
  items: {
-    type: "object",
    anyOf: [
  • looking deeply at the schema compared to the input object, the ids field must be strings instead of integer

With these two changes, your code will works 👍🏽

@ghost
Copy link

ghost commented Jan 10, 2022

Are there any known workarounds at the moment for using the discriminator keyword? When setting strict to true, it requires a type of object at the top level, so the current state is between a rock and hard place:

  1. Remove the top-level type, set it on each oneOf schema instead, and disable strict mode type checks.
  2. Keep the top-level type but always get an empty object back.

Actually, it looks like going with option 1 allows arbitrary non-objects to pass validation and fall through, so that's no good :( It seems like the only thing that works at the moment is modifying the schema before it's used as an output schema in Fastify and setting the relevant property to an empty object.

@Eomm
Copy link
Member

Eomm commented Jan 14, 2022

discriminator keyword?

No: it is a OpenAPI keyword and fast-json-stringify requires a JSON Schema Draft 7

@ghost
Copy link

ghost commented Jan 14, 2022

discriminator keyword?

No: it is a OpenAPI keyword and fast-json-stringify requires a JSON Schema Draft 7

Sorry, to clarify, it's not that I need support for the discriminator keyword specifically, but rather that keyword uses oneOf, and since the discriminator keyword requires type set at the top level, it hits this bug where it simply returns an empty object in serialization. Bumping the type down into the oneOf subschemas fixes the serialization part, but breaks validating things properly, but then ajv throws warnings/errors by default since the top-level type keyword is missing.

@gfortaine
Copy link

@stalniy Here is another implementation based on AJV JTD Serializer that should ensure better soundness regarding the validation of the schema cc @epoberezkin

const Ajv = require("ajv/dist/jtd");

const ajv = new Ajv(); // options can be passed, e.g. {allErrors: true}

const schemaFoo = {
  properties: {
    id: { type: "string" },
    createdAt: { type: "timestamp" },
    author: {
      properties: { _id: { type: "string" }, fullName: { type: "string" } },
    },
    type: { enum: ["Test"] },
    body: { type: "string" },
  },
};

const schemaBar = {
  properties: {
    id: { type: "string" },
    createdAt: { type: "timestamp" },
    author: {
      properties: { _id: { type: "string" }, fullName: { type: "string" } },
    },
    type: { enum: ["StatusTransition"] },
    status: {
      type: "string",
    },
  },
};

const schema = {
  elements: {
    metadata: {
      union: [schemaFoo, schemaBar],
    },
  },
};

// const dataFoo = [
//   {
//     type: "Test",
//     body: "bla",
//     id: "1",
//     createdAt: new Date(),
//     author: {
//       _id: "2",
//       fullName: "John Doe",
//     },
//   },
// ];

const dataBar = [
  {
    type: "StatusTransition",
    status: "bla",
    id: "1",
    createdAt: new Date(),
    author: {
      _id: "2",
      fullName: "John Doe",
    },
  },
];

const validate = ajv.compile(schema);
const serialize = ajv.compileSerializer(schema);

function validateAndSerialize(data) {
  const valid = validate(data);

  if (valid) {
    console.log(serialize(data));
  } else {
    console.log("invalid", validate.errors);
  }
}

for (const data of [
  // dataFoo,
  dataBar,
]) {
  validateAndSerialize(data);
}

@thom4parisot
Copy link

I seem to have ran in this issue with a GeoJSON structure in which I had a oneOf of Polygon or MultiPolygon schema on a geometry property.

It scrambled the body of the request, altering the first coordinates of the geometry.
Shall I add more info here or rather open a new issue?

@mcollina
Copy link
Member

mcollina commented Aug 8, 2023

This issue needs somebody to lead its implementation, anyOf and oneOf have likely the same bugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug good first issue Good for newcomers
Projects
None yet
6 participants