spot icon indicating copy to clipboard operation
spot copied to clipboard

Allow usage for indexed types on body elements

Open mahirk opened this issue 4 years ago • 11 comments

Is your feature request related to a problem? Please describe. The JSON specification allows an indexed type by defining it as an object which can have any number of strings in it, for example: the following type definition:

metadata?: {
        [key: string]: string;
    };

Becomes:

        "metadata": {
          "additionalProperties": {
            "type": "string"
          },
          "type": "object"
        },

Describe the solution you'd like The following spec fails:

class Api {}

@endpoint({
  method: "POST",
  path: "/users/:someOtherString"
})
class CreateUser {
  @request
  request(
    @headers
    headers: HeaderAlias,
    @pathParams
    pathParams: PathParams,
    @body body: CreateUserRequest
  ) {}

  @response({ status: 201 })
  successResponse(@body body: CreateUserResponse) {}
}


interface CreateUserResponse {
  firstName: string;
  lastName: string;
  role: string;
  [key: string]: string
}

with the error as indexed types are not allowed.

The proposal would be to update the type-parser to allow indexed types and define them using the guidelines provided above.

mahirk avatar Sep 25 '20 23:09 mahirk

@lfportal Happy to work on it, and get any implementation ideas you may have on the same. Any motivation on why it was initially not permitted would be beneficial!

mahirk avatar Sep 25 '20 23:09 mahirk

I also noticed the following which capability matrix: union type -> ✅ inheritance type -> ✅ recursive -> 🔴 generics -> 🔴

Would love to hear any thoughts you have on implementing these.

mahirk avatar Sep 25 '20 23:09 mahirk

Any motivation on why it was initially not permitted would be beneficial!

This was considered, but was not a priority use case for us. Ultimately we deemed TypeScript's indexed types not flexible enough to support this feature in a sufficiently generic way. Where an indexed type is present, defined sibling properties must also be assignable to the index type definition:

interface CreateUserResponse {
  firstName: string; // Property 'firstName' of type 'string' is not assignable to string index type 'boolean'.
  [key: string]: boolean;
}

lfportal avatar Sep 25 '20 23:09 lfportal

recursive -> 🔴

The type parser would need to be modified to pass around some sort "type parse stack" structure to keep track of types currently being parsed as it traverses the types to short circuit reference type parsing. I'm not too familiar with recursive type representation with the OpenAPI spec. Are you aware of any particular limitations @mahirk?

generics -> 🔴

interface Generic<T> {
  genericProp: T;
}

type A = Generic<String>

Off the top of my head:

  1. Parse type parameters (T)
  2. Parse the generic interface, passing in the resolved type parameters

Questions that need answering:

  • Do we store the resulting type in the typeTable?
    • I'm thinking we don't, and treat it as if it were defined inline (i.e. type A = { genericProp: String; }) to keep it simple
    • If we do, the need some way to name the type. GenericString, GenericInt? But this could lead to type name clashes

Thoughts @mahirk?

lfportal avatar Sep 26 '20 01:09 lfportal

Apologies, @lfportal got pulled off to something else:

Ultimately we deemed TypeScript's indexed types not flexible enough to support this feature in a sufficiently generic way

Absolutely, I am curious though, what about scenarios which could enable an open-ended pair of single level JSONs? Although I do understand the type safety aspect of it, I was thinking more along the lines of the metadata which is available here: https://stripe.com/docs/api/metadata. You can see the definition here: https://raw.githubusercontent.com/stripe/openapi/master/openapi/spec3.yaml

metadata:
          additionalProperties:
            maxLength: 500
            type: string
          description: Set of [key-value pairs](https://stripe.com/docs/api/metadata)
            that you can attach to an object. This can be useful for storing additional
            information about the object in a structured format.
          type: object

recursive -> 🔴 ....Are you aware of any particular limitations @mahirk?

Swagger UI seems to have trouble, and potentially the model generation might have issues. Let me check those and then evaluate if it would be okay. As far as representation, that's possible using the specification https://github.com/swagger-api/swagger-ui/issues/3325

Questions that need answering

I agree with that, it may be something the system calls out to the developers. I don't forsee any issues with generating the type in that way however.

mahirk avatar Oct 06 '20 21:10 mahirk

Absolutely, I am curious though, what about scenarios which could enable an open-ended pair of single level JSONs?

Do you mean to allow index signatures only where no sibling properties are defined? i.e:

// allowed
interface A { 
  [key: string]: string;
}

// not allowed
interface B {
  [key: string]: string;
  a: string
}

lfportal avatar Oct 08 '20 11:10 lfportal

Yup @lfportal i.e. if it has sibling properties, then spot can log an error stating that sibling properties on index types are not permitted. That should satisfy the use case and maybe also force better design of open-ended structs?

mahirk avatar Oct 09 '20 16:10 mahirk

I'm thinking a simpler way to constrain the usage could be TypeScript's Record<string, *> type. The parser would only need to constrain the first type argument to string - disallowing number and literals "a" | "b" | "c".

lfportal avatar Oct 10 '20 10:10 lfportal

Hi @mahirk @lfportal

Catching up with the discussion.

generics -> 🔴

interface Generic<T> {
  genericProp: T;
}

type A = Generic<String>

Off the top of my head:

  1. Parse type parameters (T)
  2. Parse the generic interface, passing in the resolved type parameters

Questions that need answering:

  • Do we store the resulting type in the typeTable?

Just to confirm we will store Generic in the typeTable regardless of we storing the resulting types, right?

  • I'm thinking we don't, and treat it as if it were defined inline (i.e. type A = { genericProp: String; }) to keep it simple
  • If we do, the need some way to name the type. GenericString, GenericInt? But this could lead to type name clashes

inline type works great when the Generic object is small, but when its big and frequently referenced, the cost of expanding it every where goes up very quickly. The name clashes should not be a problem if we just use the full name Generic<String>. Maybe we can have a threshold on when we should put it into typeTable?

Also, are we only considering handle custom generic type, or we will also support typescript utility types? https://www.typescriptlang.org/docs/handbook/utility-types.html

As a side note on typescript supported types, I noticed: undefined, object, unknown are giving unknown type error at the moment, is it expected?

generics/recursive/index-type/util-type It feels like we should create few separate issue to address each of the problem and discuss them separately? thoughts?

Noeyfan avatar Oct 21 '20 05:10 Noeyfan

inline type works great when the Generic object is small, but when its big and frequently referenced, the cost of expanding it every where goes up very quickly. The name clashes should not be a problem if we just use the full name Generic<String>

The type table is used to generate the components section of OpenAPI documents. We would require some strategy to convert Generic<String> into something OpenAPI friendly. Whatever that becomes will be subject to name clashing with user defined types (this will be true for typescript's utility types too). I think we should aim to avoid any implicit naming, and stick with a clear and predictable syntax. Currently the type table only stores types declared using type aliases and interfaces. I don't see any need to complicate this:

body: {
  a: Generic<String>; // inline type, rendered as an inline schema in OpenAPI
  b: MyType; // reference type, rendered as a $ref schema in OpenAPI
}

type MyType = Generic<String> // stored in type table

As a side note on typescript supported types, I noticed: undefined, object, unknown are giving unknown type error at the moment, is it expected?

This is currently expected. However, undefined is indirectly supported using the question token to indicate optionality myOptionalField?: string. Do you have a use case for using these types? Perhaps we can discuss them in a separate issue/s.

generics/recursive/index-type/util-type It feels like we should create few separate issue to address each of the problem and discuss them separately? thoughts?

Yup, agreed, I'll leave this issue to focus on indexed types.

lfportal avatar Oct 22 '20 09:10 lfportal

This feature would be really useful for us as well. Allowing indexed types without explicitly listed siblings is more than enough.

jankuca avatar Nov 26 '20 16:11 jankuca