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

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

Merged
merged 5 commits into from
May 22, 2024

Conversation

nicobuzeta
Copy link
Contributor

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

Copy link
Owner

@Papooch Papooch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Please fix the issue I mentioned and then I'll go ahead and merge it.

If you feel like it, you could have a go at writing tests for the decorator that it correctly preserves metadata (If not, I'll add them later)

packages/transactional/src/lib/transactional.decorator.ts Outdated Show resolved Hide resolved
@nicobuzeta
Copy link
Contributor Author

nicobuzeta commented May 12, 2024

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.

@Papooch Papooch self-requested a review May 13, 2024 09:35
Copy link
Owner

@Papooch Papooch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, thanks for the fix and the tests.

However, mixing actual transaction tests with metadata checking tests is not very transparent, so I'd like to ask you to put the decorator tests into a separate file transactional.decorator.spec.ts (together with the mock decorators).

Apart from that, everything looks fine to me.

@nicobuzeta
Copy link
Contributor Author

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
Copy link
Owner

Papooch commented May 14, 2024

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.

@nicobuzeta nicobuzeta requested a review from Papooch May 15, 2024 13:24
Copy link
Owner

@Papooch Papooch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, this looks alright to me :)

@Papooch Papooch merged commit 52b067e into Papooch:main May 22, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@Transactional decorator breaking swagger documentation
2 participants