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

toJsonSchema generates unsatisfiable schema for intersect of objects #990

Open
pokutuna opened this issue Dec 22, 2024 · 22 comments · May be fixed by #1001
Open

toJsonSchema generates unsatisfiable schema for intersect of objects #990

pokutuna opened this issue Dec 22, 2024 · 22 comments · May be fixed by #1001
Assignees
Labels
fix A smaller enhancement or bug fix priority This has priority

Comments

@pokutuna
Copy link

Problem

When using toJsonSchema with intersect of objects, it generates a JSON Schema that cannot be satisfied due to conflicting additionalProperties: false constraints.
I understand that Valibot is not primarily designed for JSON Schema compatibility, but this makes the conversion feature unusable for any JSON Schema interoperability scenarios.

Reproduction

import { object, string, number, intersect } from 'valibot';
import { toJsonSchema } from '@valibot/json-schema';

const schema = intersect([
  object({ foo: string() }),
  object({ bar: number() }),
]);
console.log(JSON.stringify(toJsonSchema(schema), null, 2));
/*
{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "allOf": [
    {
      "type": "object",
      "properties": {
        "foo": {
          "type": "string"
        }
      },
      "required": [
        "foo"
      ],
      "additionalProperties": false
    },
    {
      "type": "object",
      "properties": {
        "bar": {
          "type": "number"
        }
      },
      "required": [
        "bar"
      ],
      "additionalProperties": false
    }
  ]
}
*/

Current Behavior

The generated JSON Schema includes additionalProperties: false for each object in allOf, making it impossible to satisfy both constraints simultaneously.

Expected Behavior

The generated JSON Schema should allow objects that have both properties, matching Valibot's runtime validation behavior.
Possible solutions:

  • Remove additionalProperties: false from individual objects in allOf
  • Add additionalProperties: false only at the parent level

versions

$ npm list | grep valibot
...
├── @valibot/[email protected]
└── [email protected]
@fabian-hiller
Copy link
Owner

Thank you for creating this issue. I will investigate and fix it in the next few weeks.

@fabian-hiller fabian-hiller self-assigned this Dec 22, 2024
@fabian-hiller fabian-hiller added priority This has priority fix A smaller enhancement or bug fix labels Dec 22, 2024
@fabian-hiller
Copy link
Owner

As a workaround you can try using object merging instead of intersect:

import * as v from 'valibot';

const ObjectSchema1 = v.object({ foo: v.string(), baz: v.number() });
const ObjectSchema2 = v.object({ bar: v.string(), baz: v.boolean() });

const MergedSchema = v.object({
  ...ObjectSchema1.entries,
  ...ObjectSchema2.entries,
}); // { foo: string; bar: string; baz: boolean }

@andersk
Copy link
Contributor

andersk commented Dec 22, 2024

Possible solutions:

  • Remove additionalProperties: false from individual objects in allOf
  • Add additionalProperties: false only at the parent level

That does not work (try it). You can, however, set unevaluatedProperties to false at the parent level (try it).

@fabian-hiller
Copy link
Owner

Thank you for your feedback! If any of you have some time, feel free to research how other libraries, e.g. TypeBox, handle this case, or if there is an official recommendation on the official JSON schema website.

@43081j
Copy link

43081j commented Dec 27, 2024

fwiw that's pretty much what unevaluatedProperties is for

so when we combine multiple schemas (via allOf or similar), we can disallow additional properties to those in the combined set of sub-schemas

I suppose you could make an intersect of multiple object set unevaluatedProperties: false when converting to a schema

@fabian-hiller
Copy link
Owner

fabian-hiller commented Dec 27, 2024

It seems that TypeBox (a JSON Schema library) does exactly what we do:

Screenshot-TypeBox

@43081j
Copy link

43081j commented Dec 27, 2024

I opened #997 as one possible solution

the couple of zod JSON schema generators that exist seem to do what that PR does too

feel free to throw it away though if its not the direction you want to go

also note that we set additionalProperties: false but it doesn't look like TypeBox does that

@fabian-hiller
Copy link
Owner

Perhaps we should not add .additionalProperties at all when using v.object({...}). This will probably solve the problem.

@43081j
Copy link

43081j commented Dec 27, 2024

since we're coming from typescript types, I think we're correct in setting additionalProperties: false right now

especially since we support things like objectWithRest which do need to set additionalProperties to a non-false value

if we don't set it anymore, it means the typescript types and the JSON schema don't quite match up anymore and there'd be no difference between an object, and an object with rest

@fabian-hiller
Copy link
Owner

My idea was to add additionalProperties: true for looseObject, additionalProperties: false for strictObject and for the "normal" object schema we don't add additionalProperties. So we provide all possible three options.

@43081j
Copy link

43081j commented Dec 27, 2024

that could work

it would mean in an intersection of strict objects, we would still want to set unevaluatedProperties: false. the PR I threw together might already do that without many changes

@fabian-hiller
Copy link
Owner

Our implementation is still on Draft 07, where unevaluatedProperties is not supported. Perhaps we should ignore the strictObject problem for now, as it is probably a rare case, and upgrade to a newer draft for the v2 version of toJsonSchema.

@43081j
Copy link

43081j commented Dec 27, 2024

makes sense

it just means you wont be able to convert intersections of strict objects into a schema

validation will fail if you have properties from multiple sub-schemas

maybe we could temporarily throw in those cases until we do update to the newer draft? otherwise the schema it will produce isn't of much use

@fabian-hiller
Copy link
Owner

There is a workaround. Users could merge schemas instead of using intersect in most cases. https://valibot.dev/guides/intersections/#merge-objects

As a small bundle size is one of the main features of Valibot I am not sure if I want to add code to throw and error in this case.

@andersk
Copy link
Contributor

andersk commented Dec 27, 2024

it just means you wont be able to convert intersections of strict objects into a schema

Actually additionalProperties: false is a faithful translation for v.strictObject, even under intersections. A Valibot intersection of v.strictObjects with conflicting key requirements is just as unsatisfiable in Valibot’s runtime as the corresponding JSON schema using allOf of objects with additionalProperties: false.

@43081j
Copy link

43081j commented Dec 27, 2024

🤔 interesting

So an intersection of two strict objects:

  1. { foo: v.string() }
  2. { bar: v.string() }

Is expected to fail for the input { foo: "x", bar: "x" }?

The JSON schema will fail to validate that input. You're saying valibot will too?

@andersk
Copy link
Contributor

andersk commented Dec 27, 2024

Yes; try it.

@43081j
Copy link

43081j commented Dec 27, 2024

I'm not at a laptop right now so couldn't try it myself. Thanks for the playground 👍

In that case, object seems to be more appropriate when using intersection (forgetting the fact you can use merging for now). And intersecting strict objects is meaningless

Dumbing the schema down to always allow additional properties is the easy way out then. Then object with rest is the same as object

@andersk
Copy link
Contributor

andersk commented Dec 27, 2024

It may still be meaningful to intersect strict objects with the same set of required keys, or intersect a strict object and a non-strict object with fewer required keys.

What I’m saying is that adding additionalProperties: false for strict objects only is the correct resolution.

@43081j
Copy link

43081j commented Dec 27, 2024

Intersecting with a strict or non strict object which has extra keys can never validate in JSON schema if the input uses keys from both. That seems understood now at least

So yes, strict objects can have additional properties disabled and that won't help the OP

Going back to the OP, we could set additional properties to true on object schemas like Fabian suggested. This then makes object and objectWithRest mostly the same in JSON schema output (since we will have relaxed the type compared to the typescript type)

@andersk
Copy link
Contributor

andersk commented Dec 27, 2024

Removing additionalProperties: false for non-strict objects will help the OP. As far as JSON schema is concerned, there’s no difference between missing additionalProperties, additionalProperties: {}, and additionalProperties: true.

@43081j
Copy link

43081j commented Dec 27, 2024

Indeed it will. That is exactly what I was suggesting, the same as what Fabian suggested

we could set additional properties to true on object schemas like Fabian suggested. This then makes object and objectWithRest mostly the same in JSON schema output (since we will have relaxed the type compared to the typescript type

Good to be agreed!

andersk added a commit to andersk/valibot that referenced this issue Dec 27, 2024
`additionalProperties: true` was preventing validation of valid
intersected objects.

Fixes fabian-hiller#990.

Signed-off-by: Anders Kaseorg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix A smaller enhancement or bug fix priority This has priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants