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

Combining 'allOf' with 'If' / 'then' checks Do not resolve correctly in TS #167

Closed
jfarre90 opened this issue Sep 19, 2023 · 2 comments
Closed
Labels
wontfix This will not be worked on

Comments

@jfarre90
Copy link

jfarre90 commented Sep 19, 2023

The following schema:

const configTest = {
  type: 'object',
  properties: {
    animal: { enum: ['cat', 'dog'] },
    dynamicProperty: {},
  },
  additionalProperties: false,

  allOf: [
    {
      if: {
        properties: {
          animal: { const: 'cat' },
        },
      },
      then: {
        properties: {
          dynamicProperty: {
            type: 'object',
            properties: {
              key1: { type: 'string' },
            },
            required: ['key1'],
            additionalProperties: false,
          },
        },
      },
    },
    {
      if: {
        properties: {
          animal: { const: 'dog' },
        },
      },
      then: {
        properties: {
          dynamicProperty: {
            type: 'null',
          },
        },
      },
    },
  ],
  required: ['animal', 'dynamicProperty'],
} as const

Generates this type:

type CONFIGTEST = FromSchema<
  typeof configTest,
  { parseIfThenElseKeywords: true }
>
// type CONFIGTEST = {
//   animal: "dog";
//   dynamicProperty: null;
// } | {
//   animal: "cat";
//   dynamicProperty: {
//       key1: string;
//   };
// } | {
//   animal: "cat" | "dog";
//   dynamicProperty: unknown;
// }
}

I would expect this to generate a type that looks something like this:

type CONFIGTEST = 
   | {animal: "cat", dynamicProperty: {key1: string}}
   | {animal: "dog", dynamicProperty: null}

Why does the logic include the last optional type? Is this a bug?

@jfarre90 jfarre90 changed the title Combining allOf''If' / 'then' / 'else' checks not resolved correctly to TS Combining 'allOf' with 'If' / 'then' / 'else' checks Do not resolve correctly in TS Sep 19, 2023
@jfarre90 jfarre90 changed the title Combining 'allOf' with 'If' / 'then' / 'else' checks Do not resolve correctly in TS Combining 'allOf' with 'If' / 'then' checks Do not resolve correctly in TS Sep 19, 2023
@ThomasAribart ThomasAribart added the wontfix This will not be worked on label Dec 18, 2023
@ThomasAribart
Copy link
Owner

Hi @jfarre90 and thanks for the reach out !

This is a tricky one: properties / additionalProperties / required from parent & child schemas behave separately and are not to be merged. See https://json-schema.org/understanding-json-schema/reference/object#extending

What FromSchema does is actually to intersect the two allOf cases but with those keywords reset to their default values (see this line of code), and then to the rest of the parent schema.

So it compute the intersection of those schemas:

  • 1st child schema:
{
  type: "object",
  properties: {},
  required: [],
  additionalProperties: true,
  if: ... (cat),
  then ... (cat)
}
  • 2nd child schema:
{
  type: "object",
  properties: {},
  required: [],
  additionalProperties: true,
  if: ... (dog),
  then ... (dog)
}
  • Parent schema:
{
  type: "object",
  properties: {
    animal,
    ...
  },
  required: ["animal",...]
}

The pb is that the first two schemas, taken independently, are not mutually exclusive because the animal property can have any value.

If you add it to the child schemas, it will actually work!

const configTest = {
  type: "object",
  properties: {
    animal: { $ref: "#/$defs/animal" },
    dynamicProperty: {},
  },
  additionalProperties: false,
  allOf: [
    {
      properties: {
        animal: { $ref: "#/$defs/animal" },
      },
      if: {
        properties: {
          animal: { const: "cat" },
        },
      },
      then: {
        properties: {
          dynamicProperty: {
            type: "object",
            properties: {
              key1: { type: "string" },
            },
            required: ["key1"],
            additionalProperties: false,
          },
        },
      },
    },
    {
      properties: {
        animal: { $ref: "#/$defs/animal" },
      },
      if: {
        properties: {
          animal: { const: "dog" },
        },
      },
      then: {
        properties: {
          dynamicProperty: {
            type: "null",
          },
        },
      },
    },
  ],
  required: ["animal", "dynamicProperty"],
  $defs: {
    animal: { enum: ["cat", "dog"] },
  },
} as const;

type CONFIGTEST = FromSchema<
  typeof configTest,
  { parseIfThenElseKeywords: true }
>;

What I'm sure of is that properties cannot naively be merged from child to parent schemas. Still, I'm not sure resetting all properties is the best approach 🤷‍♂️ WDYT ?

@jfarre90
Copy link
Author

jfarre90 commented Dec 19, 2023

Hi @ThomasAribart! Thanks for the reply and info shared, much appreciated. I agree with your points and it makes sense.

In our use case we ended up defining 2 separate schemas, one for the "dog" and another for the "cat" in this example. And within our logic we had a switch/case that used one schema or the other for some validation. As for the type generated, we just applied the condition from our end like...

type FinalType =
  | FromSchema<typeof dogSchema>
  | FromSchema<typeof catSchema>

Which for our use case ended up working nicely, and we didn't have to use the parseIfThenElseKeywords which was causing some TS compilation issues in some of our larger schemas (as you mention in your docs, it sometimes got aborted due to complexity).

However, we are still refactoring a big part of our application to TS, so the info above is helpful in case we face a similar scenario down the line.

Thanks again for the great work! Finding this library very useful 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants