search

Papooch / nestjs-cls

A continuation-local storage (async context) module compatible with NestJS's dependency injection.
https://papooch.github.io/nestjs-cls/
MIT License
453 stars 29 forks source link

fix(transactional): use proxy on descriptor.value #150

Closed nicobuzeta closed 6 months ago

nicobuzeta commented 6 months ago

Fixes #149 by using a Proxy object to avoid overiding descriptor.value inside the decorator.

nicobuzeta commented 6 months ago

I investigated a little further, and it's a little puzzling as to why the original method didn't work since it already copies over the necessary metadata. I have to assume that swagger must add some property to the method outside of metadata as that is already being copied over correctly. This is quite strange as I looked over the ApiResponse code and I could only see metadata being defined. Nevertheless, I've already confirmed the code is working in a project, so the changes seem necessary.

I've added two tests, one to check that the metadata is being passed over correctly, which passes with the previous code, and one to check that properties are accessible through the proxy.

nicobuzeta commented 6 months ago

How do I handle checking if adding metadata breaks the transaction decorator? I added it to the original tests to verify that adding other decorators doesn't break the transactions. Should I add some simple transaction test to the new spec file or just check the metadata and assume adding new decorators doesn't affect @Transactional?

Papooch commented 6 months ago

How do I handle checking if adding metadata breaks the transaction decorator

Personally, I would add a simple transaction test (using the mock adapter) for a method that accepts multiple parameters and asserts that it received them unchanged.