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

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

Closed
altearius opened this issue Jun 23, 2024 · 3 comments
Closed

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

altearius opened this issue Jun 23, 2024 · 3 comments

Comments

@altearius
Copy link
Contributor

  • Given a schema that contains a named definition (Level2B),
  • 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:

description: Top Level
type: object
oneOf:
  - $ref: '#/definitions/Level2A'
  - $ref: '#/definitions/Level2B'

definitions:
  Level2A:
    description: Level2A
    type: object
    allOf: [$ref: '#/definitions/Base']
    properties:
      level_2A_ref: { $ref: '#/definitions/Level2B' }
  Level2B:
    description: Level2B
    type: object
    allOf: [$ref: '#/definitions/Base']
    properties:
      level_2B_prop: { const: xyzzy }
  Base:
    description: Base
    type: object
    properties:
      base_prop: { type: string }

The current resulting TypeScript will be (comments adjusted for clarity):

// Incorrect: should be `type Demo = Level2A | Level2B`
// Note that the Level2B type at this location is the _second_ instance to
// Level2B during a depth-first traversal.
export type Demo = Level2A | Level2B1;

export type Level2A = Level2A1 & {
  // Correct in this location because this property is reached first in a
  // depth-first traversal.
  level_2A_ref?: Level2B;
  [k: string]: unknown;
};

export type Level2A1 = Base;

export type Level2B = Level2B1 & {
  level_2B_prop?: "xyzzy";
  [k: string]: unknown;
};

export type Level2B1 = Base;

export interface Base {
  base_prop?: string;
  [k: string]: unknown;
}

Root Cause

In parser.ts, lines 57 - 75, 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

I have completed a proof-of-concept fork that addresses this:

  • 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 as a $types property on the schema.

  • 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.

PR to be submitted shortly.

Related Issues

@altearius
Copy link
Contributor Author

Hello @bcherny ! Is there anything I can do to help make progress on this? I can make changes to the PR or take a completely different approach if you prefer. I have a project that is blocked by this and I have time this week to devote to a resolution.

@bcherny
Copy link
Owner

bcherny commented Jul 22, 2024

Merged #603

@bcherny bcherny closed this as completed Jul 22, 2024
@bcherny
Copy link
Owner

bcherny commented Jul 22, 2024

Fix included in v15.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants