openapi-ts / openapi-typescript

Generate TypeScript types from OpenAPI 3 specs
https://openapi-ts.dev
MIT License
5.93k stars 471 forks source link

Property without type should be declared as unknown #1039

Closed ggrossetie closed 1 year ago

ggrossetie commented 1 year ago

Description

Name Version
openapi-typescript 6.1.1
Node.js 18.12.2
OS + version Ubuntu 22.04.2

Reproduction

  1. Create a file named api.json with the following content:

api.json

{
  "openapi": "3.0.0",
  "components": {
    "schemas": {
      "User": {
        "type": "object",
        "additionalProperties": false,
        "properties": {
          "Info": {
            "nullable": false
          }
        }
      }
    }
  }
}
  1. Run openapi-typescript:
$ npx openapi-typescript api.json --output type.ts
✨ openapi-typescript 6.1.1
🚀 api.json → file:///path/to/type.ts [12ms]
  1. You should get the following result:

type.ts

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

export type paths = Record<string, never>;

export type webhooks = Record<string, never>;

export interface components {
  schemas: {
    User: {
      Info?: Record<string, never>;
    };
  };
  responses: never;
  parameters: never;
  requestBodies: never;
  headers: never;
  pathItems: never;
}

export type external = Record<string, never>;

export type operations = Record<string, never>;

As you can see, schemas.User.Info is defined as Record<string, never> which is incorrect because Info might contain a string, number or boolean and not necessarily an object.

Expected result

export interface components {
  schemas: {
    User: {
      Info?: unknown;
    };
  };
  responses: never;
  parameters: never;
  requestBodies: never;
  headers: never;
  pathItems: never;
}

Checklist

ggrossetie commented 1 year ago

It might be the same as https://github.com/drwpow/openapi-typescript/issues/1031 and might be resolved by https://github.com/drwpow/openapi-typescript/pull/1032

@duncanbeevers could you please confirm (or not) that this is the same issue?

duncanbeevers commented 1 year ago

Yes, I can confirm that #1032 would resolve this issue.

image

The type of Info is still a Record, which is a little odd; as you pointed-out @ggrossetie, Info could be a string, a boolean, a number, etc…

I think your proposal that Info should be typed as unknown is correct.

ggrossetie commented 1 year ago

Thanks! I will keep this issue open since it goes a bit further and suggest to use unknown instead of Record<string, unknown> when the type is unknown.

drwpow commented 1 year ago

I believe this is taken care of by the --empty-objects-unknown flag introduced in #1032. I’m still somewhat against loosening up the output types to unknown or even Record<string, unknown> as default behavior (explanation).

I also don’t want to debate this topic too much more, because I think no matter how you answer the question “what types should be generated when my schema doesn’t specify a type” isn’t a very productive conversation. No matter what the decision is, the fact is that your schema is lacking critical information needed to type it safely. And I’d rather err on the side of generating types to encourage you to fix it at the schema level, not the typegen level.

duncanbeevers commented 1 year ago

And I’d rather err on the side of generating types to encourage you to fix it at the schema level, not the typegen level.

I agree with this stance. Strict-by-default guides API designers towards better schemas.

Importantly though, this tool is used to generate types for APIs outside of one's own control, so having controls for loosening the generated types is key.

mitchell-merry commented 1 year ago

Importantly though, this tool is used to generate types for APIs outside of one's own control, so having controls for loosening the generated types is key.

Exactly.

Besides, I don't see why a discussion is necessary, since the spec is well-defined for these cases as I've demonstrated. Specs of this form - without type specified - are perfectly legal according to the spec, but you're choosing here to not support them correctly. Why? What if we genuinely want to accept an unknown type? (that is, any value? number, string, boolean, array, object, whatever)

Realistically, if this doesn't make it into project, I'm just going to have to complicate generation on my side as I'll need to use the Node API and specify a custom transform, and then implement this logic myself.

drwpow commented 1 year ago

@mitchell-merry Sorry—just for clarification, is the --empty-objects-unknown flag not generating what you’d like? It would be helpful for me to clearly see the differences between a) the default generation, b) --empty-objects-unknown, and c) what your proposal is. Just so we’re all on the same page.

drwpow commented 1 year ago

Oh, sorry—I understand now. Yes that doesn’t conflict and this should be fixed (rubber ducking works!). I’ll re-open your PR.

ggrossetie commented 1 year ago

It would be helpful for me to clearly see the differences between a) the default generation, b) --empty-objects-unknown, and c) what your proposal is. Just so we’re all on the same page.

Sure, I will do that tomorrow but I believe it's:

a)

Record<string, never>

b)

Record<string, unknown>

c)

unknown

(as mentioned by @duncanbeevers in https://github.com/drwpow/openapi-typescript/issues/1039#issuecomment-1450479055) But I will confirm tomorrow using the latest release.

Oh, sorry—I understand now. Yes that doesn’t conflict and this should be fixed (rubber ducking works!). I’ll re-open your PR.

No worries 😉