xddq / schema2typebox

Creating TypeBox code from JSON schemas
MIT License
54 stars 12 forks source link

feat: :sparkles: handle patternProperties and create record type #37

Open HannesOberreiter opened 4 months ago

HannesOberreiter commented 4 months ago

Summary

Handle JSON Schema patternProperties^1 to be able to create a type of Record.

xddq commented 4 months ago

@HannesOberreiter hey

first of, thanks for the PR! I have a couple of problems. I don't understand your PR as it is right now. Could you take the time to update your PR and add some more information? E.g:

  1. Why do you want to make this change?
  2. Did you post a discussion or issue somewhere that I missed?
  3. What is the current behaviour (actual code output vs. expected code output), why should it be adapted to the new one?
  4. Is this change JSON schema draft 07 (the main and supported draft) compatible?

Feel free to add more infos. If the PR stays in the current state I will not go out of my way to investigate it, sorry!

HannesOberreiter commented 4 months ago

Hi @xddq

Thank you for your feedback on my PR. I apologize for any confusion. Let me provide some additional context:

  1. Why do you want to make this change? I noticed that the current implementation does not handle JSON Schema's patternProperties at all. This PR aims to add support for patternProperties to improve the library's compatibility with JSON Schema, as there is no option to create a Record type.

  2. Did you post a discussion or issue somewhere that I missed? I have not posted a separate discussion or issue for this. I found this issue while working on a project and thought it would be beneficial to contribute the fix.

  3. What is the current behaviour (actual code output vs. expected code output), why should it be adapted to the new one? Currently, the library does not create a type of Record for patternProperties you will get a Type.Unknown. With this PR, when the schema has patternProperties, it will generate a type of Record which is more accurate according to the JSON Schema specification. In the TypeBox Workbench you can have a look how the JSON Schema and accordingly the TypeBox Variant would look like for export type T = Record<string, string>: https://sinclairzx81.github.io/typebox-workbench/

  4. Is this change JSON Schema draft 07 (the main and supported draft) compatible? The patternProperties keyword was introduced in JSON Schema Draft 3. It allows you to specify rules for property names that match a regular expression. The matching properties will need to validate against the schema associated with the regular expression.

I hope this clarifies the purpose and impact of my PR. I'm happy to make any necessary adjustments or provide further information if needed.

Best, Hannes

PS: Before this version I tried a more strict approach and add the pattern to the Type.String({pattern="XX"}). But it seems TypeBox ignores the string pattern for the key of a record, alt-ought I did not deep digger in this issue. Therefore, I opt-in for the solution without pattern.

      for (const [key, value] of Object.entries(schema.patternProperties)) {
        if (typeof value === "object") {
          records.push(
            `Type.Record(Type.String({pattern:"${key}"}), ${collect(value)})`
          );
        }
      }
xddq commented 4 months ago

@HannesOberreiter Hey

thanks for the information! I would like to avoid having a mismatch in JSON schema validation and the generated typebox code validation. Which we would have when we just use Record<string,...>. Although it would be better than Type.Unknown, for sure.. 😅

Did you fiddle around with Record<Type.Regex(/dummyRegex/),...> yet?

To me it looks like it could work. See https://github.com/sinclairzx81/typebox/issues/746 for the Type.Regex usage example. Also the template literal usage example (although I think it is wrong and it should say propA, propB, etc.. in the typebox code) seems to indicate to me that this approach could work https://github.com/sinclairzx81/typebox?tab=readme-ov-file#template-literal-types

HannesOberreiter commented 4 months ago

@xddq I'm with you that there should be no mismatch but I only tested a little bit and it seems to not work. At least with the integrated TypeCompiler, never tested with Ajv.

(Added the Type.Regrex to the test, same problem.)

You can run following test case:

import { Type } from "@sinclair/typebox";
import { TypeCompiler } from "@sinclair/typebox/compiler";

const T = Type.Record(
  Type.String({ pattern: "^A_" }),
  Type.String({ pattern: "^A_" })
);
const C = TypeCompiler.Compile(T);

const TR = Type.Record(Type.RegExp("^A_"), Type.RegExp("^A_"));
const CR = TypeCompiler.Compile(TR);

function test(o: unknown) {
  console.log(o);
  console.log(C.Check(o));
  console.log(CR.Check(o));
  console.log(...C.Errors(o));
  console.log(...CR.Errors(o));
}

test({
  B_: "A_",
  A_: "A_",
});

test({
  B_: "B_",
  A_: "C_",
});
sinclairzx81 commented 4 months ago

@HannesOberreiter Hi, try the following.

const T = Type.Record(
  Type.String({ pattern: "^A_" }),
  Type.String({ pattern: "^A_" }), {
  additionalProperties: false
});

Note, when using regular expressions for record keys, you will need to specify additionalProperties: false. This is because non-matching keys are treated as additionalProperties (which are allowed by default). This rule is true for object types also, noting that the following two types would be equivalent.

const T = Type.Record(
  Type.String({ pattern: "(x|y|z)" }), 
  Type.Number(), 
  { additionalProperties: false }
)

// is the same as

const T = Type.Partial(Type.Object({
  x: Type.Number(),
  y: Type.Number(),
  z: Type.Number(),
}, {
  additionalProperties: false
}))

In terms of code generation, it may be a good idea to only emit the additionalProperties constraint if it's expressed in the input schema (as this constraint will prevent extending the type in composition). There's a bit of additional detail on this aspect found at the following URL

https://json-schema.org/understanding-json-schema/reference/object#extending-closed-schemas

Hope this helps S

HannesOberreiter commented 4 months ago

@sinclairzx81 thank you very much for the in depth-explanation. Makes sense to me how it works.

Forwarding the additionalProperties should be no problem. Anyway reading the given link I need to adapt the code. As currently my code would not be correct for following case, as I only use the patternProperties if there is no properties property. Did not know this was possible, but of course why not.

{
  "type": "object",
  "properties": {
    "builtin": { "type": "number" }
  },
  "patternProperties": {
    "^S_": { "type": "string" },
    "^I_": { "type": "integer" }
  },
  "additionalProperties": { "type": "string" }
}

Cheers Hannes

HannesOberreiter commented 4 months ago

@xddq added a new commit but this one is broken. I'm not enough TypeScript wizard to know how to handle this problem. This is my test case:

const dummySchema: ObjectSchema = {
        type: "object",
        properties: {
          builtin: { type: "number" },
          nested: {
            type: "object",
            properties: {
              a: { type: "string" },
              b: { type: "number" },
            },
          },
        },
        patternProperties: {
          "^S_": { type: "string" },
          "^I_": { type: "integer" },
        },
      };

And this is the broken result:

Type.Object({
    builtin: Type.Number(),
    nested: Type.Object({a: Type.String(),b: Type.Number()}),
    Type.Union([
      Type.Record(Type.String({pattern:"^S_"}), Type.String()), 
      Type.Record(Type.String({pattern:"^I_"}), Type.Number())
    ]),
})

Cheers Hannes

xddq commented 4 months ago

Mhh, I see that you wrote "dont know what to expect" in the test case. Did just quickly look over it. I think it should/could be

schema:


const dummySchema: ObjectSchema = {
        type: "object",
        properties: {
          builtin: { type: "number" },
        },
        patternProperties: {
          "^S_": { type: "string" },
          "^I_": { type: "integer" },
        },
      };

typebox:


Type.Composite(
Type.Object(
builtin: Type.Number(),
),
Type.Intersect(Type.Record(Type.String(^S_"), Type.String()), Type.Record(Type.String("I_"))
)

Did just type it of my head, there could be typos and wrong commas etc.. But this should get the point across.

Do you also think that this is the expected result?

Also @sinclairzx81 thanks for hopping in! Hope your are doing good! :) Sorry for the unnecessary @ :p

HannesOberreiter commented 4 months ago

@xddq, I'm not on my computer but the Intersect of two Records will be no Object, which would be needed for a Composite.

We would need something like a TypeScript equivalent for a regEx in the key, which I believe is not possible or I'm not aware of it.

type O = {
 /^A_/: string
}
xddq commented 4 months ago

@HannesOberreiter We don't have to specify it using Typebox, not "onky Typescript". And for that we could use Type.Regex or Type.String with pattern, no..?

Anyway, I will have a look at this in the next couple of weeks. Or will you have another look at it?

HannesOberreiter commented 4 months ago

@xddq, I'm afraid I don't have a solution for this issue at the moment. The intersection of a Record and Object is (for me) complex and could potentially disrupt the IDE, integrated compile checking, and possibly Ajv.

Here are some related discussions:

For instance, generating an intersection of an object and record (composite is not feasible) is possible in both TypeScript and TypeBox.

const BoxIntersect = Type.Intersect([
  Type.Object(
    {
      A: Type.Number(),
    },
  ),
  Type.Record(Type.String(), Type.String()),
]);
type TypescriptIntersect = {
    A: number;
    } & Record<string, string>;

However, both will trigger the same TypeScript Error:

  /*  Type '{ A: number; B_: string; }' is not assignable to type 'Record<string, string>'.
    Property 'A' is incompatible with index signature.
      Type 'number' is not assignable to type 'string'. */
  const BoxTest: BoxIntersect = {
    A: 1,
    B_: "A_",
  };
  const TypeTest: TypescriptIntersect = {
    A: 1,
    B_: "A_",
  };

Moreover, type checking with TypeBox's integrated module will fail due to the discrepancy in expected values value.A === 'number' VS ((typeof value === 'string')) : true)). A union could be an alternative here, but it wouldn't be accurate

const local_0 = new RegExp(/^(.*)$/)
return function check(value) {
  return (
    (((typeof value === 'object' && value !== null && !Array.isArray(value)) && (typeof value.A === 'number' && Number.isFinite(value.A))) && ((typeof value === 'object' && value !== null && !Array.isArray(value) && !(value instanceof Date) && !(value instanceof Uint8Array)) && (Object.entries(value).every(([key, value]) => (local_0.test(key) ? ((typeof value === 'string')) : true)))))
  )
}

Given my current understanding, I see two potential approaches:

  1. We could focus on patternProperties only when there's no properties property on the object and note this exception in the readme.
  2. Similar to the first approach, but if both patternProperties and properties are present, we could log a warning and automatically add additionalProperties= [Type of first value in patternProperties] to the object.

In this case I would prefer the first option, as it is less magic behaviour.

Cheers Hannes