0.3.20 regression with NestJS, TypeORM and Swagger

[OSA413]

Issue description

Upgrading to 0.3.20 broke NestJS + Swagger integration, downgrading to 0.3.19 fixes the issue

Expected Behavior

not regression

Actual Behavior

regression

Steps to reproduce

https://github.com/OSA413/nestjs-bug-swagger-api-property

My Environment

Dependency	Version
Operating System	Windows 10
Node.js version	18.17.1
Typescript version	not applicable i think
TypeORM version	0.3.20

Additional Context

Reposted from NestJS' Discord So, I recently updated packages and Swagger started to throw errors related to @ApiProperty (Optional) and circular dependency though a week ago it was just fine. I've managed to make an MRE from the whole project, here it is: https://github.com/OSA413/nestjs-bug-swagger-api-property

The DTO looks like this:

export class Testttttttttt {
    @ApiProperty()
    public id: string;
}

It's very interesting, because 1) it doesn't specify any type in the annotation, 2) it's a scalar type

Some other investigations:

1. The bug goes away if I remove TypeORM import from the AppModule
2. The bug may disappear if I leave only one controller by combining two of them
3. The bug may disappear if I move DTO into the same file as controller

For easier reproduction of the example I provided you'll need a Docker (because you need to connect to DB)

Package versions are listed in the repository, package.lock is provided

Here's the log

[14:50:50] Starting compilation in watch mode...

[14:50:52] Found 0 errors. Watching for file changes.

[Nest] 12856  - 31.01.2024, 14:50:53     LOG [NestFactory] Starting Nest application...
[Nest] 12856  - 31.01.2024, 14:50:53     LOG [InstanceLoader] TypeOrmModule dependencies initialized +46ms
[Nest] 12856  - 31.01.2024, 14:50:53     LOG [InstanceLoader] AppModule dependencies initialized +0ms
[Nest] 12856  - 31.01.2024, 14:50:53     LOG [InstanceLoader] TypeOrmCoreModule dependencies initialized +22ms

C:\Users\Oleg.Sokolov\Documents\GitHub\nestjs-bug-swagger-api-property\node_modules\@nestjs\swagger\dist\services\schema-object-factory.js:187
            throw new Error(`A circular dependency has been detected (property key: "${key}"). Please, make sure that each side of a bidirectional relationships are using lazy resolvers ("type: () => ClassType").`);
                  ^
Error: A circular dependency has been detected (property key: "id"). Please, make sure that each side of a bidirectional relationships are using lazy resolvers ("type: () => ClassType").
    at SchemaObjectFactory.createNotBuiltInTypeReference (C:\Users\Oleg.Sokolov\Documents\GitHub\nestjs-bug-swagger-api-property\node_modules\@nestjs\swagger\dist\services\schema-object-factory.js:187:19)
    at SchemaObjectFactory.createSchemaMetadata (C:\Users\Oleg.Sokolov\Documents\GitHub\nestjs-bug-swagger-api-property\node_modules\@nestjs\swagger\dist\services\schema-object-factory.js:297:25)
    at SchemaObjectFactory.mergePropertyWithMetadata (C:\Users\Oleg.Sokolov\Documents\GitHub\nestjs-bug-swagger-api-property\node_modules\@nestjs\swagger\dist\services\schema-object-factory.js:131:21)
    at C:\Users\Oleg.Sokolov\Documents\GitHub\nestjs-bug-swagger-api-property\node_modules\@nestjs\swagger\dist\services\schema-object-factory.js:82:35
    at Array.map (<anonymous>)
    at SchemaObjectFactory.extractPropertiesFromType (C:\Users\Oleg.Sokolov\Documents\GitHub\nestjs-bug-swagger-api-property\node_modules\@nestjs\swagger\dist\services\schema-object-factory.js:81:52)
    at SchemaObjectFactory.exploreModelSchema (C:\Users\Oleg.Sokolov\Documents\GitHub\nestjs-bug-swagger-api-property\node_modules\@nestjs\swagger\dist\services\schema-object-factory.js:103:41)
    at C:\Users\Oleg.Sokolov\Documents\GitHub\nestjs-bug-swagger-api-property\node_modules\@nestjs\swagger\dist\services\schema-object-factory.js:36:36
    at Array.map (<anonymous>)
    at SchemaObjectFactory.createFromModel (C:\Users\Oleg.Sokolov\Documents\GitHub\nestjs-bug-swagger-api-property\node_modules\@nestjs\swagger\dist\services\schema-object-factory.js:20:45)

After investigation I figured out that downgrading typeorm from 0.3.20 to 0.3.19 fixes the issue.

Relevant Database Driver(s)

• [ ] aurora-mysql
• [ ] aurora-postgres
• [ ] better-sqlite3
• [ ] cockroachdb
• [ ] cordova
• [ ] expo
• [ ] mongodb
• [ ] mysql
• [ ] nativescript
• [ ] oracle
• [ ] postgres
• [ ] react-native
• [ ] sap
• [ ] spanner
• [ ] sqlite
• [ ] sqlite-abstract
• [ ] sqljs
• [ ] sqlserver

Are you willing to resolve this issue by submitting a Pull Request?

No, I don’t have the time and I’m okay to wait for the community / maintainers to resolve this issue.

[micalevisk]

that's due to reflect-metadata being upgraded to 0.2 while others packages from nestjs are using 0.1

A quick solution is to pin the version of typeorm to v0.3.19

[OSA413]

So, this bug is not related to typeorm, closing this.

Dev response: https://github.com/nestjs/nest/issues/13107#issuecomment-1921234278

Explanation and temporal fix: https://github.com/nestjs/nest/issues/13107#issuecomment-1921364281

[kamilmysliwiec]

@OSA413 just a quick note that after walking through changes introduced in the latest version of reflect-metadata I believe typeorm should consider setting reflect-metadata as a peer dependency (otherwise similar issues might keep occurring in the future)

For example, this issue could be solved without affecting any projects using TypeORM in combination with any other libs that rely on reflect-metadata by setting reflect-metadata as a peer dependency with "^0.1.14 || ^0.2.0" constraint and removing it from deps array

[OSA413]

I believe typeorm should consider setting reflect-metadata as a peer dependency (otherwise similar issues might keep occurring in the future)

In this case I'll keep this issue open for the maintainers to decide if they want this change or not

[OSA413]

I can confirm that this is fixed after version update (and clean package install)

"@nestjs/common": "^10.3.2",
"@nestjs/core": "^10.3.2",

Source: https://github.com/nestjs/swagger/issues/2821#issuecomment-1931766882

[kamilmysliwiec]

is there any chance this is going to be reconsidered (https://github.com/typeorm/typeorm/issues/10671#issuecomment-1931628948)? We're getting more and more complaints regarding this issue (and the solution is far from obvious)

[OSA413]

@pleerock @AlexMesser

[pleerock]

If TypeORM remove reflect-metadata as a dependency and set a peer-dependency instead, it means we will have to remove "reflect-metadata" from the code imports. It will lead our users to manually install and import reflect-metadata. Those users who didn't do manual imports of reflect-metadata in their projects currently will have issues. This can be a serious breaking change for such users.

If nestjs did the upgrade to the latest version of reflect-metadata, then what can be issue?

[kamilmysliwiec]

it means we will have to remove "reflect-metadata" from the code imports.

You could still have reflect-metadata imports - peer dependencies are, by default, marked as required.

It will lead our users to manually install and import reflect-metadata.

Both npm and yarn automatically install peer dependencies now

Technically the only thing that would change is that now package managers, be it npm or yarn, would know that reflect-metadata should be automatically "hoisted" as it's supposed to be shared between packages that also rely on reflect-metadata. Currently, it's installed as an internal dependency and hoisted only in case installed versions match (deduped)

If nestjs did the upgrade to the latest version of reflect-metadata, then what can be issue?

Unfortunately, some folks still have old reflect-metadata leftovers in their lock files even after upgrading which leads to duplicated copies of reflect-metadata in the codebase (and the only solution is to remove it manually or just remove node_modules entirely and the lock file itself and reinstall packages).

Not to mention anyone who's had "^0.3.19" in their package.json and didn't change anything but reinstalled dependencies would sadly see their projects broken (as 0.3.20 matches that criterion)

Some refs:

• https://github.com/nestjs/nest/issues/13297
• https://github.com/nestjs/nest/issues/13322
• https://github.com/nestjs/nest/issues/13293
• https://github.com/nestjs/swagger/issues/2821
• https://github.com/nestjs/swagger/issues/2811

[pleerock]

Both npm and yarn automatically install peer dependencies now

the way peer deps work changed so many times that I'm not really sure how they currently work.

Technically the only thing that would change is that now package managers, be it npm or yarn, would know that reflect-metadata should be automatically "hoisted" as it's supposed to be shared between packages that also rely on reflect-metadata. Currently, it's installed as an internal dependency and hoisted only in case installed versions match (deduped)

If nothing will be broken and it's that much of the issue right now, I can apply suggested changes.

I'll wait for somebody to create a PR.

[kamilmysliwiec]

Thank you @pleerock!

[rbuckton]

I'm not clear on the issue the latest update to reflect-metadata caused, and I'd like to address it if possible. In the recent changes to reflect-metadata I was trying to make it so that the package is more reliable when different versions are installed by having them share a central metadata store. If that is breaking, I'd love to get more details as to why and a suitable repro so that I can try to address it.

[kamilmysliwiec]

Here's a pretty basic reproduction repository with instructions in the README file https://github.com/phuffer/controller-broken-example/

[OSA413]

I've done what kamilmysliwiec suggested in this PR https://github.com/typeorm/typeorm/pull/10779

[OSA413]

Any progress? A roadmap? What's current progress with the issue? Maybe it would be helpful to release a beta version of packages for testing.

[pleerock]

@OSA413 I have merged your PR, but release will be later next month. You can use beta release (made automatically after PR is merged) for now.

[kamilmysliwiec]

Ideally, we would love to see this fixed upstream in the reflect-metadata package. @rbuckton did you have a chance to investigate what caused this regression?

[OSA413]

Installing lates dev version of the package fixed the issue in the repo I provided

[rbuckton]

Any progress? A roadmap? What's current progress with the issue? Maybe it would be helpful to release a beta version of packages for testing.

I'm still looking into this, but don't yet know the cause. It does seem like NestJS is using reflect-metadata completely incorrectly, but I don't believe that's the underlying cause.

[rbuckton]

I think I have discovered the cause. I should have a fix up for reflect-metadata shortly.

[rbuckton]

This should be fixed in [email protected] and it now should be able to load side-by-side with [email protected]

[kamilmysliwiec]

Thank you @rbuckton. I just did a quick test and it seems to work as expected now (needs confirmation though).

In this case, upgrading the reflect-metadata dependency to the latest version in typeorm may be sufficient to fix this issue (without even moving it from deps to peer deps) @pleerock