Bugfix for discriminators using allOf

[mzronek]

Changes

I noticed that the current implementation for discriminators inherited from a base class is not working when the final objects are used in anyOf or oneOf. These combinations will omit the discriminator property from the final object and result in an invalid type:

{
  "oneOf": [
    { "$ref": "#/components/schemas/Cat" },
    { "$ref": "#/components/schemas/Dog" },
  ]
}

type AnimalSighting = Omit<components["schemas"]["Cat"], "petType"> | Omit<components["schemas"]["Dog"], "petType">;

const sighting: AnimalSighting = {
  petType: 'Cat'; // <= without petType we can't use the discriminator in Typescript
}

The OpenAPI specification on discriminators states the following:

In both the oneOf and anyOf use cases, all possible schemas MUST be listed explicitly. To avoid redundancy, the discriminator MAY be added to a parent schema definition, and all schemas comprising the parent schema in an allOf construct may be used as an alternate schema.

This is very confusing in my opinion. My interpretation is as follows: A discriminator from one of the base classes will not be automatically inherited unless it uses allOf. This makes sense looking at the result above, that never results in something useful (I can think of) in my opinion.

The anyOf case is currently not handled, but I can have a look as well. anyOf + discriminator is a rare combination, but I think it can be handled exactly like we handle oneOf.

Bonus: I improved Windows compatibility and error handling for updating the examples.

How to Review

The appropriate unit tests were adapted to test the changes. The DigitalOcean schema shows, that responses with discriminators now produce valid types.

Checklist

• Unit tests updated
• docs/ updated (if necessary)
• pnpm run update:examples run (only applicable for openapi-typescript)

[changeset-bot]

⚠️ No Changeset found

Latest commit: df9e8b2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[mzronek]

@drwpow I was unsure why there is already an object here for the test of a oneOf discriminator, so I removed it. Maybe the implicit allOf merging here should be in a separate unit test?

[drwpow]

Yes this actually should be its own test, you’re right, but it’s important we keep this “wrong” testcase. We still get so many bug reports and code examples of people combining type: "object" with its own properties + oneOf (even though in the docs it suggests not doing that). You’re right—it shouldn’t be the ideal case, but we should keep it as a test case so we can just be sure that it generates something (logical collisions are the responsibility of the schema author, but openapi-typescript should still attempt to generate something that reflects what they’ve authored)

[mzronek]

I just ran into another problem with multiple discriminator property values assigned to the same base class. Let me work it out as well, but please feel free to discuss the new PR.

@drwpow

[mzronek]

First try using the same discriminator property replacement as I used in my oneOf PR.

[drwpow]

This is great! I have not taken a look yet but will later today.

[mzronek]

I think we should decide on a consistent output of the discriminator patterns. Right now there are two patterns for allOf inheritance compositions:

// discriminator property prepended + Omit
Dog: {
  petType: "dog";
} & (Omit<components["schemas"]["Pet"], "petType"> & {
  bark?: string;
});

// discriminator property appended + Omit
Dog: Omit<components["schemas"]["Pet"], "petType"> & {
  bark?: string;
} & {
  /**
   * @description discriminator enum property added by openapi-typescript
   * @enum {string}
   */
   petType: "dog" | "poodle";
};

I implemented the second one because it was easier to cover all edge cases and it is more robust. I don't have to fiddle around with omitting and requiring discriminator properties:

// discriminator property append + no omit because of type narrowing
export interface components {
  schemas: {
    baseObject: {
      type: components["schemas"]["type"];
    };
    simpleObject: components["schemas"]["baseObject"] & {
      simple?: boolean;
    } & {
      /**
       * @description discriminator enum property added by openapi-typescript
       * @enum {string}
       */
      type: "simple";
    };
    complexObject: components["schemas"]["baseObject"] & {
      complex?: boolean;
    } & {
      /**
       * @description discriminator enum property added by openapi-typescript
       * @enum {string}
       */
      type: "complex" | "tooComplex";
    };
    /** @enum {string} */
    type: "simple" | "complex" | "tooComplex";
  };
}

The existing code for that was harder to understand and already part of the transformation to Typescript, so I decided to go the other route for my oneOf implementation and don't omit the existing petType property (even if I can't replace it). This works because each appended discriminator property is narrowing the full union type. Fun fact: This fails if the appended discriminator is not part of the previously declared union! The property would then be evaluated by Typescript as never in the resulting type. I think this is a positive side-effect because it indicates an invalid schema.

[mzronek]

The last comment was meant as a future opportunity btw. I think the important part of this PR is that it fixes the allOf inheritance cases, so if this looks good to you, we should probably merge it.

I will continue to look into it and I still owe you the backport to 6.x, which definitely takes longer than anticipated. A lot of code has changed for version 7 - for the better!

[drwpow]

Nice addition 👍

[drwpow]

Sorry for the delay in reviewing! I am happy with this, and this is a fantastic fix!

See the comment on the modified testcase of type: "object" + oneOf: we should keep this as a testcase somewhere. And for now I’m less concerned about what it generates, more so than just guaranteeing that it generates something (i.e. doesn’t produce invalid TypeScript, but if it produces logically-invalid types that’s OK).

[mzronek]

See the comment on the modified testcase of type: "object" + oneOf: we should keep this as a testcase somewhere. And for now I’m less concerned about what it generates, more so than just guaranteeing that it generates something (i.e. doesn’t produce invalid TypeScript, but if it produces logically-invalid types that’s OK).

Thanks for the good input. I know bad schemas very well and if this gives the users more wiggle room, so be it. :)
I moved this case into its own unit test now (discriminator > oneOf inside object). As we no longer omit the discriminator enum property for oneOfs this case now works without adding it to the parent object. The resulting type is the same as long as each oneOf object has the discriminator enum as property.
I personally think this is better for this outlier case. It is less magic (no omit, no auto adding of the enum), less code for the type and the type directly reflects the schema itself.

Please let me know if there is still something I have missed.

[drwpow]

This is great, thank you! Sorry for the delay in approving. Agree with your perspective; thanks for clarifying! 🙂 Discriminators just leave so much room open for interpretation, it’s fine if we have some minor breaking changes in 7.x for discriminators because it’s an ambiguous part of the spec (especially for generating deterministic TypeScript code). As long as we’re interpreting it in good faith as best we can, that’s all anyone can ask for. And your fix makes our support even better! Thank you.