Required object field with additionalProperties does not generate a required TypeScript field

[sgrimm]

Description

If a payload has a required object field whose additionalProperties specifies a schema for the values, the generated type shouldn't allow undefined values.

OpenAPI

The real schema where I'm hitting this is here but this toy schema demonstrates the problem.

openapi: 3.0.1
components:
  schemas:
    Payload:
      type: object
      required:
        - children
      properties:
        children:
          type: object
          additionalProperties:
            type: object
            required:
              - field
            properties:
              field: string

Generated TS

export type paths = Record<string, never>;

export type webhooks = Record<string, never>;

export interface components {
  schemas: {
    Payload: {
      children: {
        [key: string]: {
          field: string;
        } | undefined;
      };
    };
  };
  responses: never;
  parameters: never;
  requestBodies: never;
  headers: never;
  pathItems: never;
}

export type external = Record<string, never>;

export type operations = Record<string, never>;

Expected TS

Same as above but without the | undefined.

export type paths = Record<string, never>;

export type webhooks = Record<string, never>;

export interface components {
  schemas: {
    Payload: {
      children: {
        [key: string]: {
          field: string;
        };
      };
    };
  };
  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)

[connebs]

Any update on this? I've been bumping up against it, and right now am just grepping occurrences in the output TS, which is obviously sub-optimal.

[drwpow]

This behavior was a decision made to improve type safety because most users don’t have noUncheckedIndexAccess enabled, and this library can’t check that’s set. But when dealing with additionalProperties, their whole nature is “this key may either exist or not” so the | undefined is necessary in order to not have runtime bugs (if the key was known, and required, it would be part of the schema).

This behavior won’t be changed, but perhaps this a flag could be added if you’re assuming ownership of object index access and are confident you’re handling all the null cases properly.

[connebs]

Understood, thanks for the explanation. My issue is that I'm actually only using the resulting object as an iterator, so of course noUncheckedIndexAccess is irrelevant because I'm not trying to access by index ever.

It would be nice if Typescript was smart enough to somehow handle both (know that the value is defined when iterating over the object directly, but warn about possibly undefined when accessing by key), but I guess not?

[drwpow]

My issue is that I'm actually only using the resulting object as an iterator, so of course noUncheckedIndexAccess is irrelevant because I'm not trying to access by index ever. … It would be nice if Typescript was smart enough to somehow handle both

Oh yup—when you’re letting the object list its own keys you’re right this doesn’t affect you. But in all other instances, this is unsafe, and like you pointed out, TS can’t tell the difference. This behavior is actually my biggest gripe with TypeScript 😄.

[drwpow]

I don’t think we’d ever add a flag that disallows some part of your schema; IMO that gets into linting and the Redocly CLI is so good I’d probably just encourage users to take advantage of that if desired (you can easily write your own lint rules like this)

However, I am starting to regret departing from default TypeScript behavior just because “that’s the way it should be.” On the one hand, there is a lot of gray area when converting OpenAPI → TypeScript (more than I imagined). Because they are different languages with different rules, more is a “best approximation” than most people realize. However, generating non-standard TS like this may come with more downsides than upsides. Even if it does help type safety for some, the end result usually does not behave as expected, especially when you get into complex composition (oneOf / anyOf / allOf) it tends to backfire. Like it or hate it, the OpenAPI spec is what it is, and TS is what it is. And this library should never impose on either’s design.

Though if we did remove the | undefined behavior for additionalProperties and just let TS do its thing, would that be a breaking change? 🤔 Probably in most instances I don’t think it would “break” users’ behavior; it just may be more lax on some checks in place. But anyway, I am starting to think that changing this behavior to remove | undefined and just encouraging the use of noUncheckedIndexAccess in the docs would be a better experience for most.

[drwpow]

@RWOverdijk ah, I see. Yes this original issue was over | undefined being appended to additionalProperties. Optional properties may just be a missing required: true on your schema? Feel free to open another issue or discussion with your schema on what you’re expecting vs what you’re getting.

[RWOverdijk]

@drwpow I'll just crawl back in shame. It was a PEBKAC. I'll clear out my other issues.

[connebs]

I support the removal of | undefined and encouraging the use of noUncheckedIndexAccess, as yes that is what I need to do everywhere else for this behavior to be as correct as possible.

I don't think it's really a breaking change. Anyone that is already handling the undefined case won't be broken by this change.

[drwpow]

The new release v6.5.0 features the removal of | undefined in certain scenarios, but not all. I’m going to be revisiting the existing ones this week.

[march08]

Bump, any update on this? :)

[psychedelicious]

Revisiting upgrading to v7 today, still seems to be an issue.

[sorenhoyer]

Yes, still an issue

[psychedelicious]

@sorenhoyer This issue is resolved for me as of #1799 (v7.3.0). What version are you on and can you please elaborate on the issue?

[sorenhoyer]

@psychedelicious using 7.4.1.

      "Product": {
        "required": [
          "description",
          "details"
        ],
        "type": "object",
        "properties": {
          "sku": {
            "type": "string"
          },
          "displayName": {
            "type": "string"
          },
          "description": {
            "type": "string"
          },
          "metaTitle": {
            "type": "string",
            "nullable": true
          },
          "details": {
            "type": "array",
            "items": {
              "$ref": "#/components/schemas/ProductDetail"
            }
          }
        },
        "additionalProperties": false
      },

Only description and details will be required. But ideally all the properties that arent nullable should be required a well. Don't ask me why our devs put sku under properties :D But still... I'd expect and non-nullable property to be required.

[github-actions]

This issue is stale because it has been open for 180 days with no activity. If there is no activity in the next 7 days, the issue will be closed.

[github-actions]

This issue was closed because it has been inactive for 7 days since being marked as stale. Please open a new issue if you believe you are encountering a related problem.