Generated type of additionalProperties values should not include undefined

[dancrumb]

Description

When trying to describe a simple dictionary object in Swagger, equivalent to a TS Record<string,string>, the generated type is, instead Record<string, string | undefined>

See https://swagger.io/docs/specification/data-models/dictionaries/

I dug into the code and noticed the comment on this line:
https://github.com/drwpow/openapi-typescript/blob/main/src/transform/schema-object.ts#L194

// note: | undefined is required to mesh with possibly-undefined keys

I haven't found any kind of spec that outlines clearly how additionalProperties should be interpreted with regards to whether a value is required. I concede that this may be a matter of opinion.

However, I would contend that supporting unknown additional properties would imply that such properties must have a value; otherwise, why would that property even exist? Properties with undefined values are omitted from JSON anyway, so it's pretty hard to send them over the wire.

Therefore, I think this implementation may be flawed and should be changed.

If not, would it be possible to add a parameter to toggle this behavior to make additionalProperties with a defined type include or exclude undefined?

OpenAPI

openapi: 3.0.0
info:
  version: 0.5.0
  title: Sutro Central
  description: "These are the endpoint for the Sutro Central backend"
components:
  schemas:
    Demo:
      type: object
      required:
        - dataSchema
      properties:
        dataSchema:
          type: object
          minProperties: 1
          additionalProperties:
            type: string
            minLength: 1

paths:
  /dummy:
    get:
      responses:
        "200":
          description: OK
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/Demo"

Generated TS

/**
 * This file was auto-generated by openapi-typescript.
 * Do not make direct changes to the file.
 */


export interface paths {
  "/dummy": {
    get: {
      responses: {
        /** @description OK */
        200: {
          content: {
            "application/json": components["schemas"]["Demo"];
          };
        };
      };
    };
  };
}

export type webhooks = Record<string, never>;

export interface components {
  schemas: {
    Demo: {
      dataSchema: {
        [key: string]: string | undefined; // this is what I'd like to see changed
      };
    };
  };
  responses: never;
  parameters: never;
  requestBodies: never;
  headers: never;
  pathItems: never;
}

export type external = Record<string, never>;

export type operations = Record<string, never>;

Expected TS

/**
 * This file was auto-generated by openapi-typescript.
 * Do not make direct changes to the file.
 */


export interface paths {
  "/dummy": {
    get: {
      responses: {
        /** @description OK */
        200: {
          content: {
            "application/json": components["schemas"]["Demo"];
          };
        };
      };
    };
  };
}

export type webhooks = Record<string, never>;

export interface components {
  schemas: {
    Demo: {
      dataSchema: {
        [key: string]: string; // this is the change I'd like to see
      };
    };
  };
  responses: never;
  parameters: never;
  requestBodies: never;
  headers: never;
  pathItems: never;
}

export type external = Record<string, never>;

export type operations = Record<string, never>;

Checklist

• My OpenAPI schema passes a validator
• I’m willing to open a PR for this (see CONTRIBUTING.md)

[dancrumb]

I created a simple PR that would just remove the | undefined.

This could easily be modified to add a CLI argument to control this behaviour.

[drwpow]

Thank you for adding this issue and raising a PR! But I believe the | undefined behavior is desired for now. I don’t remember the original discussion that led to this, but TypeScript really is too lenient and trusting when it comes to untyped properties. I am of the opinion that there is no way to guarantee that a specific key exists when it’s missing from the schema, and this is the safer approach.

Anecdotally, I just fixed a bug in a production app yesterday because of this exact behavior. A map of Record<string, ProjectType> was indexed by string, and so TypeScript would let you access any key freely, with any string, without making you check if it is actually present or not. The end result is a broken app when you accidentally reference the wrong key but TypeScript raises no issues.

This wasn’t even using openapi-typescript; this was just updating our types to use Record<string, ProjectType | undefined> just for more defensive code. But when it comes to type generation, this library will always lean toward making it more “annoying” for you to access properties not explicitly outlined in your schema, and reassured they are never undefined or nullable. Because the fix is usually in the schema, and not this library.

[dancrumb]

Thanks for the explanation.

So, just to make sure I'm following your reasoning...

If we generate this:

type Foo = {
    [index: string]: string
}

const foo:Foo = {};

const bar = foo.bar;

Then bar is typed as a string, even though it's clearly undefined in this example... so your type generation is intended to make that explicit, right?

If so, I can't fault the logic there. If not, I'd appreciate the clarification, mostly for future readers of this issue :)

[drwpow]

Yes exactly! Another way of looking at it is this library tries to infer in between the lines of your schema. And there’s subtext of the assumption that you may be working with an external API you can’t manage or change. And erring on the side of safety is always better.

When writing TypeScript natively, of course, you may choose to type things however you’d like. And it’s very common to do { [index: string]: string } over { [index: string]: string | undefined }. But the key difference is you can manage it, and adjust it when you need to based on your needs. But you can’t really adjust the output of openapi-typescript easily, so this will always err on the side of caution and defensive programming.

[mitchell-merry]

To be clear - the example provided of:

type Foo = {
    [index: string]: string
}

const foo:Foo = {};

const bar = foo.bar;

will actually make bar string | undefined if you have noUncheckedIndexAccess enabled in your tsconfig. Other than that note, I have no opinion either way.