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

getFromContainer is called too early #328

Closed
satanTime opened this issue Mar 10, 2019 · 11 comments · Fixed by #335
Closed

getFromContainer is called too early #328

satanTime opened this issue Mar 10, 2019 · 11 comments · Fixed by #335

Comments

@satanTime
Copy link
Contributor

satanTime commented Mar 10, 2019

When I want to use own container, I call it in my index.ts

import {useContainer} from 'class-validator';
import {Container} from 'typedi';

useContainer(Container);

but the problem is, that MetadataStorage in this case will be replaced with a brand new instance of MetadataStorage, because the call of useContainer is made after all annotations have been parsed and my Container from typedi doesn't them yet and wasn't used to collect them. Therefore the conclusion that we can't use MetadataStorage and getContainer, and should exclude it from the getFromContainer .

The problem is caused by decorators from src/decorator/decorators.ts because they use getFromContainer(MetadataStorage) too early, before the very first place where we could call useContainer.

Currently I investigated class-transformer because it doesn't have this issue, looks like it happens because it uses own MetadataStorage without container.

I've created a pull request to fix the issue, please check it and let me know what you think.

Thank you in advance.

@humbertowoody
Copy link

Same problem here! Without calling useContainer everything works just fine.

@satanTime
Copy link
Contributor Author

Possible fix: #335

@jotamorais
Copy link
Member

jotamorais commented Feb 20, 2020

@satanTime and @humbertowoody, can you please test again using the pre-release of the routing-controllers, version 0.8.1-alpha.2 and let me know how it goes?

I've published the pre-release to NPM to make it easier to test.

@satanTime
Copy link
Contributor Author

Hi @jotamorais, the problem is not with the routing-controllers but with the class-validator. If you check how routing-controllers and and class-validator gather metadata from decorators you can see that actually routing-controllers does it directly to the MetadataArgsStorage (without getFromContainer call) and class-validator uses own getFromContainer to get MetadataStorage object. More info you can find in the #335. The pull request simply changes behaviour of gathering data into MetadataStorage to the same way as routing-controllers does, directly without getFromContainer.

@satanTime
Copy link
Contributor Author

Hi @vlapo,

could you leave any thoughts or to point to the right person for the review?

Implementation in routing-controllers was changed a bit, you can find it here:
https://github.com/typestack/routing-controllers/blob/master/src/index.ts#L103-L108

Implementation in typeorm is quite the same:
https://github.com/typeorm/typeorm/blob/master/src/index.ts#L176-L190

Please let me know which way you prefer and I'll update my PR.

@satanTime
Copy link
Contributor Author

satanTime commented Mar 18, 2020

Hi @vlapo, any update?

I've updated the PR to follow routing-controllers's style.

@satanTime
Copy link
Contributor Author

Hi @jotamorais,

do you know anyone from class-validator I could ping for the review?

@satanTime
Copy link
Contributor Author

Hi, any update on this?

vlapo pushed a commit that referenced this issue Mar 24, 2020
@vlapo
Copy link
Contributor

vlapo commented Mar 24, 2020

Happy testing https://www.npmjs.com/package/class-validator/v/0.12.0-rc.0. Please give me a feedback.

@satanTime
Copy link
Contributor Author

On my project it works well now with useContainer(container); without any extra magic.

@lock
Copy link

lock bot commented Apr 2, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants