search

lukeautry / tsoa

Build OpenAPI-compliant REST APIs using TypeScript and Node
MIT License
3.48k stars 498 forks source link

v3.0.2 pre-release: cannot generate routes or swagger doc any more #597

Closed dfrechdev closed 4 years ago

dfrechdev commented 4 years ago

Sorting

Expected Behavior

Ability to generate routes and swagger documention.

Current Behavior

Routes cannot be generated. Either an error is thrown, or the generation freezes, or completes without errors but does not generate the routes. The same happens for swagger documentation generation.

Possible Solution

Steps to Reproduce

  1. run "tsoa routes -c tsoa.json" or "tsoa swagger -c tsoa.json"

Context (Environment)

Version of the library: tested with 3.0.0, 3.0.1 and 3.0.2 Version of NodeJS: tested with 12.13.0, 12.16.1 and 13.9.0 Version of OS: Ubuntu 18.04.4 LTS

Detailed Description

We are currently using tsoa for all our services and are successfully generating routes and API doc with version 2.5.13 at the moment. Upon testing the pre-release we found out, thatwe cannot generate the route and doc any more. Our services differ in size and typescript features we are using, but none of them is currently working. Two of the services throw an error:

The other services either do not complete and do not show any error message, or do complete but do not generate anything. I do not have any additional logging for these cases, but I can provide a strace log if needed. I have tested this on a seperate machine, also with Ubuntu, and got the same results. Please let me know if you need any further information regarding our setup.

dgreene1 commented 4 years ago

I'd like to know a little more information: Are you using a custom route template?

cc: @WoH

WoH commented 4 years ago

Seems like a refObject type with undefined properties? I would not know how we would generate that, if we don't find any, it should be an empty array. Custom template seems like the most likely scenario here.

dfrechdev commented 4 years ago

No, we are not using a custom route template. Here is the tsoa.json of one of the services that throws the error:

{
    "swagger": {
        "outputDirectory": "./dist/swagger",
        "entryFile": "./src/api/index.ts",
        "noImplicitAdditionalProperties": "silently-remove-extras",
        "spec": {
            "servers": [
                {
                    "url": "https://test.project.com/project-api",
                    "description": "Test server"
                },
                {
                    "url": "https://project.com/project-api",
                    "description": "Production server"
                },
                {
                    "url": "http://localhost:3000/project-api",
                    "description": "local dev server"
                }
            ]
        },
        "specVersion": 3,
        "yaml": true
    },
    "routes": {
        "basePath": "/project-api",
        "entryFile": "./src/api/index.ts",
        "routesDir": "./src/routes",
        "middleware": "express"
    }
}
dfrechdev commented 4 years ago

Just tested building routes for our services on mac OS Catalina 10.25.3 but got the same errors.

WoH commented 4 years ago

I'm sorry this upgrade is causing issues and hope you can help us find out what's going wrong. Still however not sure how there could be Models with properties: undefined. 2 Things that could really help me track down this issue: Can you please make sure the tsoa bin you're invoking is 3.0.2 (tsoa --version). My fear is that your path may still contain a 2.x version. If that does not help and you're not able to share a minimal reproduction repo, could you add a console.log(referenceType) in at Line 209/210 in /project/node_modules/tsoa/dist/routeGeneration/routeGenerator.js, so we can find out which TS code is causing this issue?

dfrechdev commented 4 years ago

No problem, thanks for your help. The problem seems to be related to the MongoDB id fields in our interfaces:

> tsoa --version && tsoa routes -c tsoa.json
3.0.2
...
{
  dataType: 'refObject',
  refName: 'TypeObjectId<IEducationLevel>',
  description: undefined,
  properties: undefined
}

If I change the ref in the interface from "_id: TypeObjectId;" to "_id: string;" it works for that interface. This affects interfaces, that are not used in the tsoa API directly as a request or result type. In the API types, we currently only return strings for the typerefs.

This is one of the affected interfaces as an example:

export interface IEducationLevel {
    _id: TypeObjectId<IEducationLevel>;
    name: string;
    order: number;
    parent: TypeObjectId<IEducationLevel>;
}

After both replacing _id and parent with string, the type description and properties are filled.

WoH commented 4 years ago

Thank you very much for investigating. Could you share the type definition of TypeObjectId<T>?

I think the issue might be the a bug with circular references in the type argument, but it would still help to verify.

We currently use something like

/**
 * Stringified UUIDv4.
 * See [RFC 4112](https://tools.ietf.org/html/rfc4122)
 * @pattern ^[0-9A-Fa-f]{8}-[0-9A-Fa-f]{4}-4[0-9A-Fa-f]{3}-[89ABab][0-9A-Fa-f]{3}-[0-9A-Fa-f]{12}$
 */
export type UUID = string;

in tsoa@3 which validates and documents these "special" strings in a reusable manner, maybe that's something you could take advantage of aswell.

dfrechdev commented 4 years ago

sure:

export type TypeObjectId<T> = string & { type: T };
WoH commented 4 years ago

Well, string actually does not have properties, that could makes sense. tsoa should've thrown a useful error here instead of crashing.

TypeObjectId<T>: I think this type should be string | { type: T }, since the intersection of a string and an object is unassignable.

dfrechdev commented 4 years ago

We are using this to force ObjectId validation when we manuall cast from string to ObjectId. Simon already mentioned this in issue 483. For us it would be great if we could keep generating the routes and maybe only get a warning, in order to keep our validations.

WoH commented 4 years ago

Updates suggestion with 3.0 then:

export type ExposedEducationLevel = Omit<IEducationLevel, _id | parent> & { _id: string, parent: string};

//or 

export interface ExposedEducationLevel extends Omit<IEducationLevel, _id | parent> {
  _id: string,
  parent: string
};

I know this does not really solve the underlying issue, but there is no "proper" way to expose anything that can't be assigned because tsoa's validation layer wouldn't let anything pass, which is why I'm hesitant to discuss papering over the issue with a warning.

dfrechdev commented 4 years ago

I tested your alternatives from above and also with string | { type: T }, but all would require a rewrite throughout out whole codebase that would also affect other dependencies. This is just not feasible. Further I tried to rewrite the type to typeof String & { type: T }, but in that case I get the following error when generating the routes: 

Generate routes error.
 Error: Unknown type: TypeQuery
At: /project/packages/common/dist/types/services/ObjectId.d.ts:1:1.
This was caused by 'typeof String & {
    type: T;
}'

Would that however be an alternative that could pass and generate the routes?

dgreene1 commented 4 years ago

If you use the workaround and then do a runtime assertion with a type guard then you can abstract away the change to only your route controller. Then the rest of the app can stay unchanged.

However, I don’t see why you can’t just ask users to pass simply the ID instead of asking them to pass this strange nominally typed object. I used mongo for over a decade and we never asked users to be aware of the notion of an object ID. A GUID is fine in almost all cases.

But if you absolutely must nominally type your IDs then why use a fancy type when something like this would be more explicit and less strange:

export type NominalId<T extends string> = {
  type: T,
  id: string
}

Please note the T extends string> part. Otherwise you really aren’t guaranteeing that the type property is a name. The other advantage of this shape is that it doesn’t have the awkwardness of a string that is also an object (since I can’t even understand how TypeScript compiles that).

WoH commented 4 years ago

I tested your alternatives from above and also with string | { type: T }, but all would require a rewrite throughout out whole codebase that would also affect other dependencies. This is just not feasible. Further I tried to rewrite the type to typeof String & { type: T }

Well, I don't know how to best say this, but what you're essentially doing is trying to break/work around/hack past the runtime validation. By using export type TypeObjectId<T> = string & { type: T }; at the edge of the program you were actively relying on the fact that tsoa could not resolve this type correctly and generate instead generate improper type descriptions. I think you'd agree that the goal of tsoa should be to generate correct definitions whenever possible and ideally never generate improper ones, so I can't say I'm unhappy when you're telling me it's harder to trick tsoa.

I understand that this causes pains during the upgrade, that's why we were waiting for a major release to do that and why I am actively trying to push pre-releases to sort out bugs and enable everyone to be prepared.

However, I wanna emphasize that this is my personal opinion, not sure what @dgreene1 thinks, but I'd be curious to hear his.

E: If I understand correctly from the other thread, you already have to do some TS assertions to convince TS that something is of this unassignable/opaque type. I can't say without seeing the code obviously, but I'm pretty sure that this function is the one that should be adapted aswell. Guess I'm having a hard time figuring out why (from my understanding) you're exposing a type to the REST API (assuming because otherwise tsoa shouldn't break generation) which tsoa 2.x will assign but your code can't, unless it's validated through a function that the data from the web seemingly does not have to go through. For me it seems like the API should accept string and any function after the controller layer should use the unassignable/opaque type.

dfrechdev commented 4 years ago

I think there is a bit of misunderstanding of what we try to achieve and why we are doing this. Let me try to clarify: Our goal is to make it easier for consumers of the API to know, what a certain property contains. An example of how we want to use it:

postData(data: {
    id: TypeObjectID<Match>;
    job: TypeObjectID<Job>;
    user: TypeObjectID<User>;
 });

In this case, the cosumer knows, that id is a match id, job is a job id and so on. This is also displayed in the swagger docs already.

If we now change this to string:

postData(data: {
    id: string;
    job: string;
    user: string;
 });

the user does not know what kind of ID, or even if it is an ID if it is not named accordingly. The only way to add this information, as we see it currently, would be to create additional documentation. This is extra work and still makes it harder for the consumer to use the API.

Would it be a solution to allow setting "custom type transformers" or something alike, that tells tsoa to handle this custom type like a string, but still uses the type name for swagger and co.? Hope this helps to see our use case :-).

dgreene1 commented 4 years ago

Why reinvent the wheel? This is what JSDoc descriptions are for.

dfrechdev commented 4 years ago

Our current Swagger doc has everything included in one place, without having to write and build/include separate documentation. So I kindly disagree and argue, that we explicitly prefer to not reinvent that wheel.

dgreene1 commented 4 years ago

Can you please explain why JSDoc descriptions (which show up in the swagger model and in the swagger UI for consumers to see) aren’t helpful enough?

But I have to agree with my peer @WoH (and soon to be maintainer of tsoa) that you’re trying to represent a type that is not representable in TypeScript or JSON.

Here’s a link to a TypeScript playground that illustrates the problem with your type: http://www.typescriptlang.org/play/index.html?ssl=5&ssc=22&pln=1&pc=1#code/C4TwDgpgBAqgdgJwmJBnCdgEMBGAbaAXilWAQEs4BzKAMigG8pRIAuEsymgXwG4AofgGMA9nFJQIADywBbMAXbwkKCOky4CUYgCIAFhDx4ROgcLGoRBAHTGqACmlyFEAJS8gA

So even though tsoa happened to allow you to do this in v2, you were utilizing a bug in tsoa that has since been corrected in v3 as we are trying to get closer to standards. If this is too much of a burden for you to adopt then you can stay on v2, but you will miss out on substantial updates as the v2 branch will soon be left behind.

You can’t expect tsoa to add in a feature that neither JSON or TypeScript support. Simply put, a string can’t have properties.

WoH commented 4 years ago

With 3.0 you can use:

export type TypeObjectID<_T> = string;

This way, the Users can see that a thing is a TOID, maybe that helps.

In fact this is a pattern I'd like to encourage, the same way we use UUID as a type alias with a regex pattern for validation.

E: See https://github.com/lukeautry/tsoa/issues/543#issuecomment-591376212 or some of the new aliases in tests: https://github.com/lukeautry/tsoa/blob/3.x/tests/fixtures/testModel.ts#L155

dgreene1 commented 4 years ago

I stand by the above comment about how we won’t support this, but I think you might have misinterpreted the recommended solution. You put a JSDoc description above the property as opposed to a separate documentation that lives away from Swagger.

So it would look like:

export interface IBody {
  /**
   * Some special message explaining which type this Id is for
   */
  id: string;
}

This way the documentation will be generated with swagger and your consumers will know which ID type to use.

dgreene1 commented 4 years ago

Closing since you have WoH’s recommendation and mine. That’s two ways to help migrate to v3.

simllll commented 4 years ago

One more question @dgreene1 @WoH @dfrechdev , I came up with something like this to workaround it:

export type APITypeObjectId<_T extends APIObjectType> = string;

export enum APIObjectType {
       ...
        EducationLevel = 'EducationLevel',
    EducationDegree = 'EducationDegree',
    UserExperience = 'UserExperience',
       ....
}

but while rewriting and fixing some code parts (which where mostly strings before) I found some situations where it's really hard to find the source of the failing code part. Therefore my quesiton: Is it possible to debug the "root" of a type. Where it is used and how it got "included". I added already the console log for the referenceType in routeGEnerator.js, but I only get stuff like this: 

TypeObjectId<IJobField> {
  dataType: 'refObject',
  refName: 'TypeObjectId<IJobField>',
  description: undefined,
  properties: undefined
}

also debugging the "this" context, reveals some more TypeObjectIds, but not where they are coming from. Any hint would be appreicated to find out where this comes from. I'm out of ideas...

also for one project tsoa does just never finish, any idea how to track this down? (I guess it is probably related to typeobjectid, but I do not get any error trace)

Funny side fact, in the referenceTypeMap there is 'TypeObjectId<IJobField>': [Object], and 'APITypeObjectId_APIObjectType.User_': [Object], Why is the first one written with "<" and ">" and the latter one with undescores?

WoH commented 4 years ago

Anything with '<' and '>' should not be happening inside the refMap. I fixed a bug concerning that which is still unreleased, but I'll keep an eye on this.

What's pretty weird is that the alias shows up as 'refObject', should be 'refAlias'. I'll try to reproduce the issue but I may have to come back for more info.

WoH commented 4 years ago

@simllll @dfrechdev I just released a new beta, would you mind checking if 3.0.3 solves the naming issue?

simllll commented 4 years ago

Sure, what exactly you want me to look at? Right now we are already successfully deploying our test environment with tsoa@3.0.2 :-)