Intersection schema generation is order-dependent #603

altearius · 2024-06-23T02:09:14Z

Addresses #602 and #597

bcherny

Thank you for the contribution!

Your approach looks good to me. I tried a more surgical fix in https://github.com/bcherny/json-schema-to-typescript/pull/605/files, but like that your approach is more generic and cleaner.

A few minor things before we merge this:

Would you mind splitting your PR into two commits: one to add the new test cases, and one to make code changes and change the output of your test cases? That would make it more clear how your changes change the output of the test cases.
Would you mind simplifying the naming in your cases (eg. A instead of Level2A)? (example: 1252285)

bcherny · 2024-06-24T10:37:01Z

src/applySchemaTyping.ts

+  schema.$types = types
+
+  if (types.length > 1) {
+    schema.$intersection = {


$intersection and $types may conflict with schema properties (in JSON-Schema, users can define custom properties). Instead, can you use Symbols (similar to Parent)? This name collision could also be good to add as a test case.

json-schema-to-typescript/src/types/JSONSchema.ts

Line 43 in 31993de

export const Parent = Symbol('Parent')

This suggestion significantly improved the code quality, thank you.

bcherny · 2024-06-24T10:38:45Z

src/applySchemaTyping.ts

+
+    delete schema.$id
+    delete schema.description
+    delete schema.name


What about title?

I missed title as it was not handled by the original maybeStripNameHints function, though I have now added it.

bcherny · 2024-06-24T10:39:26Z

src/parser.ts

@@ -54,13 +55,13 @@ export function parse(

  // Be careful to first process the intersection before processing its params,
  // so that it gets first pick for standalone name.
+  const intersectionSchema = schema.$intersection
+  if (!intersectionSchema) {
+    throw new Error('Expected intersection schema')


Suggested change

throw new Error('Expected intersection schema')

throw new ReferenceError('Expected intersection schema. Please file an issue on GitHub.')

bcherny · 2024-06-24T10:40:49Z

src/types/JSONSchema.ts

@@ -71,6 +72,8 @@ export interface LinkedJSONSchema extends JSONSchema {
 }

 export interface NormalizedJSONSchema extends LinkedJSONSchema {
+  $intersection?: NormalizedJSONSchema
+  $types: ReturnType<typeof typesOfSchema>


Nit: please type this explicitly to keep it simple.

Suggested change

$types: ReturnType<typeof typesOfSchema>

$types: readonly [SchemaType, ...SchemaType[]]

- Given a schema that contains a named definition (`B`), - And that named definition is referenced in multiple locations, - And that named schema is also an intersection type (`allOf` in this example), Then when parsed, the generated TypeScript will contain the correct reference only for the _first_ location in which the named schema is encountered, during a depth-first traversal. Subsequent references to the same schema will be generated as though they were only the intersection type, and not the named schema. Example Given the following schema: ```yaml $id: Intersection type: object oneOf: - $ref: '#/definitions/A' - $ref: '#/definitions/B' definitions: A: type: object additionalProperties: false allOf: [$ref: '#/definitions/Base'] properties: b: {$ref: '#/definitions/B'} B: type: object additionalProperties: false, allOf: [$ref: '#/definitions/Base'] properties: x: {type: string} Base: type: object additionalProperties: false, properties: y: {type: string} ``` The current resulting TypeScript will be (comments adjusted for clarity): ```ts // Incorrect: should be `type Intersection = A | B` // Note that the B type at this location is the _second_ reference to // B during a depth-first traversal. export type Intersection = A | B1; export type A = A1 & { b?: B; }; export type A1 = Base; export type B = B1 & { x?: string; }; export type B1 = Base; export interface Base { y?: string; } ``` Root Cause In `parser.ts`, [lines 57 - 75][1], when schema that matches multiple "types" is encountered, the parser generates a new `ALL_OF` intersection schema to contain each sub-type, then adds each sub-type to the new `ALL_OF` schema. Each sub-type is then parsed sequentially. During this process, `maybeStripNameHints` is called, which mutates the schema by removing the `$id`, `description`, and `name` properties. Notably, these properties are used by `typesOfSchema` to detect the `NAMED_SCHEMA` type. As a result, this schema object will never again be detected as a `NAMED_SCHEMA` type. Therefore, the _first_ instance of the schema object is correctly handled as an intersection schema **and** a named schema, but all subsequent instances are treated as though they are **only** an intersection schema. Proposed Solution - The call to `typesOfSchema` is moved from `parser.ts` to `normalizer.ts`, with the goal of avoiding confusion due to a mutated schema object. The resulting list of schema types is persisted on the schema using a newly-introduced `Types` symbol. - The generated intersection schema is _also_ moved from `parser.ts` to `normalizer.ts`. This is because it is advantageous to let the generated intersection schema participate in the caching mechanism (which it could not previously do, since it was generated dynamically during each encounter). Without this, multiple instances of the same schema are generated. Related Issues - bcherny#597 [1]: https://github.com/bcherny/json-schema-to-typescript/blob/31993def993b610ba238d3024260129e31ddc371/src/parser.ts#L57-L75 'parser.ts, lines 57 - 75'

altearius · 2024-06-24T15:27:39Z

Thank you for the code review! This is ready for a second pass.

bcherny

Looking better!

The output is a nice improvement across a number of tests. I wonder if there might be a way to improve the output more for the test cases you added.

To help debug, try adding export const only = true to your test case, then running VERBOSE=1 npm test to enable debug tracing.

bcherny · 2024-06-24T16:52:29Z

test/__snapshots__/test/test.ts.md

+     */␊
+    ␊
+    export type Intersection = A | B;␊
+    export type A = A1 & {␊


I think I missed this earlier -- this is a case where the output in #605 is better than #603. I wonder if there's a way to improve this output, so we don't generate A1 and B1, and instead just use Base directly.

Like I did here: https://github.com/bcherny/json-schema-to-typescript/pull/605/files#diff-34615f003eb49f90c3044b8fafebe3fe6b162e290b9b724082a6d72f58cfa4afR1876

Thanks! I took a try at that in 7d3f495 by pulling schema's pre-existing allOf rule into the intersection. Would you mind taking a third pass and let me know if I missed anything?

bcherny · 2024-06-24T16:53:53Z

test/__snapshots__/test/test.ts.md

+     */␊
+    ␊
+    export type Intersection = Car | Truck;␊
+    export type Car = Car1 & {␊


And here. This output is cleaner, if you can achieve something similar with your approach: https://github.com/bcherny/json-schema-to-typescript/pull/605/files#diff-34615f003eb49f90c3044b8fafebe3fe6b162e290b9b724082a6d72f58cfa4afR1927

bcherny · 2024-06-24T16:55:12Z

test/__snapshots__/test/test.ts.md

@@ -448968,8 +448968,7 @@ Generated by [AVA](https://avajs.dev).
     * and run json-schema-to-typescript to regenerate this file.␊
     */␊
    ␊
-    export type UnionWithProps = UnionWithProps1 & UnionWithProps2;␊
-    export type UnionWithProps1 =␊
+    export type UnionWithProps = (␊


bcherny · 2024-06-25T08:55:25Z

test/__snapshots__/test/test.ts.md

      /**␊
       * Any property starting with _ is valid.␊
       *␊
-       * This interface was referenced by \`JSONSchemaForNPMPackageJsonFiles2\`'s JSON-Schema definition␊
+       * This interface was referenced by \`undefined\`'s JSON-Schema definition␊


This seems to have regressed

Just a heads up: I haven't lost interest in moving this forward. I've had some deadlines come up, but I plan to return to this over the weekend.

Hello! Thank you for your patience. I've taken a look at this regression and I'm not sure what the expected behavior should be. Could you advise?

This is what I observe, simplified to the relevant bits.

Given this schema:

title: JSON schema for NPM package.json files type: object patternProperties: '^_': description: Any property starting with _ is valid. properties: # etc, etc, etc, actual properties clipped

The original code produced this structure:

export type JSONSchemaForNPMPackageJsonFiles = JSONSchemaForNPMPackageJsonFiles1 & JSONSchemaForNPMPackageJsonFiles2; export type JSONSchemaForNPMPackageJsonFiles1 = { [k: string]: unknown; }; export interface JSONSchemaForNPMPackageJsonFiles2 {␊ /** * Any property starting with _ is valid. * * This interface was referenced by \`JSONSchemaForNPMPackageJsonFiles2\`'s * JSON-Schema definition via the \`patternProperty\` "^_". */ [k: string]: any; }

This branch produces a different structure:

export type JSONSchemaForNPMPackageJsonFiles = { [k: string]: unknown; } & { /** * Any property starting with _ is valid. * * This interface was referenced by \`undefined\`'s JSON-Schema definition * via the \`patternProperty\` "^_". */ [k: string]: any; };

While I agree that the change in the comment from JSONSchemaForNPMPackageJsonFiles2 to undefined is unfortunate, this behavior is in line with other instances in which this comment is generated due to the presence of patternProperties on the root schema.

For instance, here is the current output of the intersection.2.ts test on the master branch, which has a similar scenario:

export type Intersection = (A & B) & { c: string; d: string; /** * This interface was referenced by \`undefined\`'s JSON-Schema definition * via the \`patternProperty\` "^x-". */ [k: string]: unknown; }; export interface A { a?: string; [k: string]: unknown; } export interface B { b?: string; [k: string]: unknown; }

I would argue that neither behavior is ideal, and an improvement might be to suppress the comment entirely if the interface is not named.

I can make this change, but I worry that it may not be what you have in mind, and that it feels like something that should be a separate PR.

Could you please provide direction on how this might be resolved?

That seems reasonable -- we can improve this as a follow-up.

bcherny · 2024-06-25T08:56:06Z

test/__snapshots__/test/test.ts.md

@@ -225147,7 +225124,7 @@ Generated by [AVA](https://avajs.dev).
    /**␊
     * The list of custom alert time-window rules.␊
     */␊
-    timeWindowRules?: ((ActiveConnectionsNotInAllowedRange | AmqpC2DMessagesNotInAllowedRange | MqttC2DMessagesNotInAllowedRange | HttpC2DMessagesNotInAllowedRange | AmqpC2DRejectedMessagesNotInAllowedRange | MqttC2DRejectedMessagesNotInAllowedRange | HttpC2DRejectedMessagesNotInAllowedRange | AmqpD2CMessagesNotInAllowedRange | MqttD2CMessagesNotInAllowedRange | HttpD2CMessagesNotInAllowedRange | DirectMethodInvokesNotInAllowedRange | FailedLocalLoginsNotInAllowedRange | FileUploadsNotInAllowedRange | QueuePurgesNotInAllowedRange | TwinUpdatesNotInAllowedRange | UnauthorizedOperationsNotInAllowedRange)[] | string)␊
+    timeWindowRules?: (TimeWindowCustomAlertRule[] | string)␊


This looks like a regression. Asked about it here to understand the expected behavior: json-schema-org/json-schema-spec#1529

Given this schema:

type: object required: [ruleType, timeWindowSize] oneOf: - $ref: '#/definitions/ActiveConnectionsNotInAllowedRange' # ...etc... - $ref: '#/definitions/UnauthorizedOperationsNotInAllowedRange' properties: ruleType: type: string enum: [ TimeWindowCustomAlertRule ] timeWindowSize: type: string

The old behavior is:

export interface DeviceSecurityGroupProperties { // Note: does **not** include the ruleType or timeWindowSize properties timeWindowRules?: | ( | ActiveConnectionsNotInAllowedRange // ...etc... | UnauthorizedOperationsNotInAllowedRange )[] | string; [k: string]: unknown; } export type TimeWindowCustomAlertRule = { ruleType: "TimeWindowCustomAlertRule"; timeWindowSize: string; [k: string]: unknown; } & ( | ActiveConnectionsNotInAllowedRange // ...etc... | UnauthorizedOperationsNotInAllowedRange );

The new behavior is:

export interface DeviceSecurityGroupProperties { // Note: Now includes ruleType and timeWindowSize. timeWindowRules?: TimeWindowCustomAlertRule[] | string; } export type TimeWindowCustomAlertRule = { ruleType: "TimeWindowCustomAlertRule"; timeWindowSize: string; [k: string]: unknown; } & ( | ActiveConnectionsNotInAllowedRange // ...etc... | UnauthorizedOperationsNotInAllowedRange );

I would argue that the new behavior is more nearly correct. My reasoning is based on the definition of the oneOf keyword in the JSON Schema spec:

10.2.1.3. oneOf

This keyword's value MUST be a non-empty array. Each item of the array MUST be a valid JSON Schema.

An instance validates successfully against this keyword if it validates successfully against exactly one schema defined by this keyword's value.

This definition does not address the possibility that oneOf may be combined with additional keywords, such as properties. However, one reasonable interpretation is that the properties keyword should be applied in addition to the oneOf keyword. In such a case, both oneOf and properties (or other keywords) would all apply.

This interpretation aligns with section 10.1:

10.1 Keyword Independence

Schema keywords typically operate independently, without affecting each other's outcomes.

This suggests that oneOf and properties should both affect the output.

This interpretation is also in line with the Factoring Schemas example, which demonstrates how one schema:

oneOf: - { type: number, multipleOf: 5 } - { type: number, multipleOf: 3 }

Can be factored into a schema that combines oneOf with additional keywords (type, in this case, but I presume an example with properties would be equally valid):

type: number oneOf: - { multipleOf: 5 } - { multipleOf: 3 }

I realize that other interpretations of the oneOf schema are possible, but this is what feels most intuitive to me. Please let me know if other changes need to be made.

I think that's a reasonable interpretation, but as you said, there may be other interpretations. Implicit intersection schemas like this make me scratch my head sometimes. I think your interpretation is correct, and the new behavior is correct, but since it's a subtle breaking change that will affect the largest consumers of JSTT, I want to double check with JSON-Schema maintainers first.

I bumped the Github issue to see is someone is willing to clarify.

bcherny · 2024-07-22T10:32:33Z

Looking good. Thank you for the contribution, and for iterating!

bcherny · 2024-07-22T10:36:03Z

Published as v15.0.0.

bcherny requested changes Jun 24, 2024

View reviewed changes

bcherny mentioned this pull request Jun 24, 2024

602 #605

Closed

altearius added 2 commits June 24, 2024 10:58

Add new test cases.

fb9fc26

altearius force-pushed the intersections branch from 7118862 to f604d66 Compare June 24, 2024 15:22

bcherny requested changes Jun 24, 2024

View reviewed changes

Additionally hoist allOf behavior.

7d3f495

bcherny reviewed Jun 25, 2024

View reviewed changes

altearius added 2 commits July 2, 2024 11:25

Merge branch 'master' into intersections

6abd384

Traverse the generated intersection schema.

ae98923

bcherny mentioned this pull request Jul 21, 2024

How to interpret schema with both properties and oneOf? json-schema-org/json-schema-spec#1529

Closed

bcherny merged commit 62cc052 into bcherny:master Jul 22, 2024
18 checks passed

This was referenced Jul 22, 2024

Generation for intersection/named schemas is order-dependent #602

Closed

$ref in allOf with depth of 2 is not correctly converted when multi-referenced #597

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Intersection schema generation is order-dependent #603

Intersection schema generation is order-dependent #603

altearius commented Jun 23, 2024

bcherny left a comment

bcherny Jun 24, 2024

altearius Jun 24, 2024

bcherny Jun 24, 2024

altearius Jun 24, 2024

bcherny Jun 24, 2024 •

edited

Loading

bcherny Jun 24, 2024

altearius commented Jun 24, 2024

bcherny left a comment

bcherny Jun 24, 2024

altearius Jun 24, 2024

bcherny Jun 24, 2024

bcherny Jun 24, 2024

bcherny Jun 25, 2024

altearius Jun 28, 2024

altearius Jul 2, 2024

bcherny Jul 21, 2024

bcherny Jun 25, 2024

altearius Jul 2, 2024

bcherny Jul 21, 2024

bcherny commented Jul 22, 2024

bcherny commented Jul 22, 2024

	throw new Error('Expected intersection schema')
	throw new ReferenceError('Expected intersection schema. Please file an issue on GitHub.')

	$types: ReturnType<typeof typesOfSchema>
	$types: readonly [SchemaType, ...SchemaType[]]

Intersection schema generation is order-dependent #603

Intersection schema generation is order-dependent #603

Conversation

altearius commented Jun 23, 2024

bcherny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcherny Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

altearius commented Jun 24, 2024

bcherny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcherny commented Jul 22, 2024

bcherny commented Jul 22, 2024

bcherny Jun 24, 2024 •

edited

Loading