Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Swagger not showing intersection model properly #391

Closed
avizcaino opened this issue Aug 8, 2019 · 17 comments · Fixed by #461
Closed

Swagger not showing intersection model properly #391

avizcaino opened this issue Aug 8, 2019 · 17 comments · Fixed by #461

Comments

@avizcaino
Copy link

I have the following model:

export interface IIQCommentRecord {
  id: string;
  comment: string;
}

export interface DomainCommand{
  aggregateId: string;
}

export type AddCommentCommand extends DomainCommand & {[key: string] : IIQCommentRecord};

Where key can be any key since we are working with a quite dynamic interface.

Build works great but the model shown in Swagger UI is the following:
image

Is there any way to show this intersecction properly? I would expect both aggregateId and the dictionary to show on the model.

@dgreene1
Copy link
Collaborator

dgreene1 commented Aug 8, 2019

@avizcaino there are a couple of things currently going on here.

  1. You're trying to represent a dictionary (in C# terms) or a Record (in TypeScript terms). The way that swagger represents dictionaries is actually as an object but with additional properties allowed. Read more here: https://swagger.io/docs/specification/data-models/dictionaries/.
  2. As far as the &, we don't currently support aggregate types (that enhancement is tracked under Seeking contributions to add anyOf and oneOf #393). We don't currently support mapped types (although mapped types might be added under Support for mapped types (including Omit) #372).

Workaround:

Until we add support for aggregate types (#393), please consider creating your input type tool look as follows:

export interface IIQCommentRecord {
    id: string;
    comment: string;
}

export interface DomainCommand{
    aggregateId: string;
}

export interface ICommandDictionary {
    [key: string] : IIQCommentRecord
}

export interface AddCommentCommand {
    command: DomainCommand,
    commentsForCommand: ICommandDictionary,
}

This will create swagger models that look like:

    DomainCommand:
        properties:
            aggregateId:
                type: string
        required:
            - aggregateId
        type: object
    IIQCommentRecord:
        properties:
            id:
                type: string
            comment:
                type: string
        required:
            - id
            - comment
        type: object
    ICommandDictionary:
        properties: {}
        type: object
        additionalProperties:
            $ref: '#/definitions/IIQCommentRecord'
    AddCommentCommand:
        properties:
            command:
                $ref: '#/definitions/DomainCommand'
            commentsForCommand:
                $ref: '#/definitions/ICommandDictionary'
        required:
            - command
            - commentsForCommand
        type: object

I think those models will be much more helpful. I'm closing this issue so we can track the larger feature in #393.

@dgreene1
Copy link
Collaborator

@avizcaino I'm happy to report that the we added our first draft of unions and intersections in v2.4.11. Please consider upgrading to it and let us know how it works for you. :)

@avizcaino
Copy link
Author

Hi! I'm trying this new version but I'm encountering new errors that did not occur previously...

There was a problem resolving type of 'T'.
There was a problem resolving type of 'IIQEventBase'.
There was a problem resolving type of 'IIQSingleEvent'.
There was a problem resolving type of 'IIQEvent'.
There was a problem resolving type of 'IPatientIQ'.
Generate routes error.
 Error: No matching model found for referenced type T. If T comes from a dependency, please create an interface in your own code that has the same structure. Tsoa can not utilize interfaces from external dependencies. Read more at https://github.com/lukeautry/tsoa/blob/master/ExternalInterfacesExplanation.MD
    at new GenerateMetadataError (/Users/alexvizcaino/Documents/uxland/projects/ics/etc/ics-etc-api/node_modules/tsoa/dist/metadataGeneration/exceptions.js:20:28)
    at TypeResolver.getModelTypeDeclaration (/Users/alexvizcaino/Documents/uxland/projects/ics/etc/ics-etc-api/node_modules/tsoa/dist/metadataGeneration/typeResolver.js:458:19)
    at TypeResolver.getReferenceType (/Users/alexvizcaino/Documents/uxland/projects/ics/etc/ics-etc-api/node_modules/tsoa/dist/metadataGeneration/typeResolver.js:299:34)
    at TypeResolver.resolve (/Users/alexvizcaino/Documents/uxland/projects/ics/etc/ics-etc-api/node_modules/tsoa/dist/metadataGeneration/typeResolver.js:147:34)
    at /Users/alexvizcaino/Documents/uxland/projects/ics/etc/ics-etc-api/node_modules/tsoa/dist/metadataGeneration/typeResolver.js:81:103
    at Array.map (<anonymous>)
    at TypeResolver.resolve (/Users/alexvizcaino/Documents/uxland/projects/ics/etc/ics-etc-api/node_modules/tsoa/dist/metadataGeneration/typeResolver.js:80:49)
    at /Users/alexvizcaino/Documents/uxland/projects/ics/etc/ics-etc-api/node_modules/tsoa/dist/metadataGeneration/typeResolver.js:538:80
    at Array.map (<anonymous>)
    at TypeResolver.getModelProperties (/Users/alexvizcaino/Documents/uxland/projects/ics/etc/ics-etc-api/node_modules/tsoa/dist/metadataGeneration/typeResolver.js:502:18)

It looks like it is caused by a generic type in an interface

export interface IIQEventBase<T> {
  id: string;
  value: T | string;
  description?: string;
  owner: IIQEventOwner;
  comment: string;
  unit?: string;
  abnormality?: Abnormality;
}

But this was working before... has something changed in the last release?

@dgreene1
Copy link
Collaborator

dgreene1 commented Sep 4, 2019

@avizcaino yes, we added the ability to calculate swagger docs from intersection and aggregate types.

Can you please copy here (1) your controller file and (2) the file that contains your interfaces?

I will try to fix the error, but I am fairly confident that the error is due to you using an interface that lives in node_modules. It’s regrettable that the error is only manifesting after we introduced the new feature that allows unions and intersections, but tsoa has never been able to use interfaces in node_modules (that’s why the error links to a document I wrote that explains this in detail). I believe you weren’t seeing this previously due to the fact that unions were essentially ignored before. Now that they’re not being ignored... you get the true tsoa reaction which is to point out “hey we couldn’t find this interface.”

So please confirm to me that the interface you’re passing in as the type parameter to IIQEventBase is in an interface living in node modules. If so, the workaround is described in the link that the error threw.

@WoH
Copy link
Collaborator

WoH commented Sep 4, 2019

@dgreene1 I can explain what happened (since I introduced the relevant code):
Previously, that union would've automatically been any.
Now that we actually try to resolve it, we run into this issue: #27 (comment)

This happens because tsoa doesn't really pass down the type arguments as parameters, but substitutes Twhen it sees a property of type T (without transformations of T) when there is a declared parameter T with the type argument.

@dgreene1
Copy link
Collaborator

dgreene1 commented Sep 4, 2019

@WoH is there a way to preserve the original interface name for T? I tried debugging the code in .getModelProperties and .resolve and I wasn't able to find anything on the typeReference object.

@dgreene1 dgreene1 reopened this Sep 4, 2019
@WoH
Copy link
Collaborator

WoH commented Sep 4, 2019

Yes, there is. Whenever there is a TypeReference to a generic Interface (in this context), the types that should be passed down are on the TypeReference as TypeReference.typeArguments.

I think I have an approach to solve this issue, which would hopefully fix generic interfaces with an arbitrary amount of type parameters and handle propagating the arguments down the type resolver "right" in terms of correctness, but that'll take some time to do. I am very sorry @avizcaino for exposing you to this.

@WoH
Copy link
Collaborator

WoH commented Sep 4, 2019

@dgreene1 I have the code, no tests no nothing, but I'd like to open a draft PR so we can spec this together, are you ok with that?

@dgreene1
Copy link
Collaborator

dgreene1 commented Sep 4, 2019

Yes please. Thank you for looking into it @WoH.

@WoH WoH mentioned this issue Sep 4, 2019
6 tasks
@dgreene1
Copy link
Collaborator

dgreene1 commented Sep 5, 2019

@avizcaino this fix/feature was just released in v2.5.1. Can you let us know how it worked out for you?

Side note: we're looking for companies that want to be featured in tsoa's readme: #464

@avizcaino
Copy link
Author

Hi! I'm sorry to say that we still have the same problem...

@dgreene1, as you requested, here is the controller and interface (summarized):

export class PatientIQController extends Controller {
  @inject(TYPES.IPatientIQProxy) private proxy: IPatientIQProxy;

  /**
   * Returns patient's IQ
   * @param request ApiRequest
   */
  @Get()
  getPatientIQ(@Request() request: ApiRequest): Promise<IPatientIQ> {
    return getIQ(request, this.proxy);
  }
}
export interface IPatientIQ {
  events?: IIQEvent[];
}
export interface IIQEvent extends IIQSingleEvent<Event> {}
export interface IIQSingleEvent<T = any> extends IIQEventBase<T> {
  timestamp: Date;
}
export interface IIQEventBase<T = any> {
  value: T | string;
}
export const enum Event {
  Dummy = 'DM'
}

Not sure what we are doing wrong... This works on 2.4.7.

@WoH don't worry, we still can fix to previous versions in the meantime. Thanks to you for your work and ultra-fast answer!

@dgreene1
Copy link
Collaborator

dgreene1 commented Sep 9, 2019

I think it might be the extends keyword. I don’t believe we have a unit test for that. I’ll reopen.

WoH has something to test below.

@dgreene1 dgreene1 reopened this Sep 9, 2019
@WoH
Copy link
Collaborator

WoH commented Sep 9, 2019

We do not check the heritage clauses for type arguments, that's the issue here. Duplicate of #467.

Edit. Can you try this? :

export interface IPatientIQ {
  events?:  IIQSingleEvent<Event>[];
}

export interface IIQSingleEvent<T = any> extends IIQEventBase<T> {
  timestamp: Date;
}
export interface IIQEventBase<T = any> {
  value: T | string;
}

export const enum Event {
  Dummy = 'DM'
}

@avizcaino
Copy link
Author

yep, that solution works, although I would like to maintain the original solution since the model is likely to grow in future iterations.

@WoH
Copy link
Collaborator

WoH commented Sep 10, 2019

File a PR for #467 and you can 😉

We get the properties here:
https://github.com/lukeautry/tsoa/blob/master/src/metadataGeneration/typeResolver.ts#L721

However, you need a new context that only applies during resolution of the inherited properties. In this context, you should use the type arguments or forward from the context:

I.e.

let a: ThingContainerWithTitle<string, string>;

interface ThingContainerWithTitle<T, U> extends GenericContainer<number, U> {
  // T is string here
  t: T;
  title: string;
}

interface GenericContainer<T, U> {
  // U is string
  id: U;
  // T is number here
  list: T[];
}

@WoH
Copy link
Collaborator

WoH commented Sep 10, 2019

@dgreene1 Can we close this in favor of #467 (Duplicate + has workaround).

@mrgoonie
Copy link

I'm not sure if this issue is related

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants