odavid / typeorm-transactional-cls-hooked

A Transactional Method Decorator for typeorm that uses cls-hooked to handle and propagate transactions between different repositories and service methods. Inpired by Spring Trasnactional Annotation and Sequelize CLS
MIT License
524 stars 86 forks source link

ROUTE_ARGS_METADATA missing after using @Transactional decorator #15

Closed labibramadhan closed 5 years ago

labibramadhan commented 5 years ago

When @Transactional is being used on a method, the method decorator @Body(), @Param(), and @Query() documentations will be gone later on the Swagger docs.

Below is my inspections:

Here is @nestjs/common injecting ROUTE_ARGS_METADATA to controller methods by using its method name as the key, the ROUTE_ARGS_METADATA itself is mainly stored on the target.constructor which is the class where the method is defined, this is going to be used later for @nestjs/swagger to explore available methods/endpoints and its object structure as well that can be found here

The problem is that @Transactional is overwriting the method without preserving the method name, this makes the createPipesRouteParamDecorator() function of @nestjs/swagger will not find any kind of ROUTE_ARGS_METADATA that have been previously stored by @nestjs/common, because the key itself to retrieve ROUTE_ARGS_METADATA stored on the class is the method name

odavid commented 5 years ago

Hi @labibramadhan, Thank you for the contribution! I believe the issue you describe here, can happen to any decorator declared on the controller method, not just the Transactional.

So according to your suggestion, every decorator need to be changed if used together with NestJS. This does not sound reasonable to me.

Did you try to change the order of decorator declarations? Meaning the NestJS decorators close to the method and the transactional at the top - something like this:

@Transactional()
@Post() // first the nestJS decorator is fetching the metadata of the method params
async create(@Body() createCatDto: CreateCatDto) {
  this.catsService.create(createCatDto);
}
labibramadhan commented 5 years ago

Yeah i tried to reorder and put the @Transactional to the first and then everything gone on the Swagger documentation, the @Post or @Get or @Put or @Delete metadata are missing after being replaced by @Transactional decorator. So when i put the @Transactional decorator to the first (applied last), all endpoint documentations doesn't exists on the Swagger documentation, but when @Transactional being put on the last decorator (applied first), the endpoint still exists on the Swagger documentation but missing the payload @Body or @Query or @Param documentation. So the @Transactional decorator is destroying metadata everything that comes before it.

odavid commented 5 years ago

Ok, I will try to find the time to investigate that a bit more, since there is no special code here, just a simple decorator. It seems to me that no other decorator will work with NestJS and swagger, based on these findings. I am not sure the fix should be done here. But again, need to investigate that...

Cheers

joemaidman commented 5 years ago

I've also just hit this issue, looks like another 3rd party library may have faced something similar before. Found some links that may help: nestjs issue: https://github.com/nestjs/nest/issues/1180 nestjs-config 3rd party library PR linked from the nestjs issue: https://github.com/nestjs-community/nestjs-config/pull/34

joemaidman commented 5 years ago

Seems there is already a PR to fix this from @labibramadhan https://github.com/odavid/typeorm-transactional-cls-hooked/pull/16

odavid commented 5 years ago

16 Merged - Closing this issue.