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: made metadata global to prevent issues with multiple package installations #1440

Closed
wants to merge 1 commit into from

Conversation

jtimmons
Copy link

@jtimmons jtimmons commented Jan 24, 2023

Description

The MetadataStorage object currently acts as a singleton within the module by using the defaultMetadataStorage constant. However, this doesn't account for the scenario in which there are multiple instances of the class-transformer library installed; this seems to be common in two cases:

  1. Shared libraries using class-transformer as a prod dependency. If the library and its consumer are on two different versions of class-validator then we end up with a MetadataStorage object for each version installed
  2. Shared libraries which are symlinked (common in monorepos using workspaces, lerna, etc). Regardless of version, if there's no dep hoisting happening you'll have an instance installed in each package

Having duplicate metadata stores can have some really subtle behavior for users. In our case we were using class-transformer alongside class-validator to validate incoming messages; because our validators were coming in from a shared library the @Type() decorator was silently failing its lookup, resulting in all of our nested validators failing to run.

We were made aware of this issue by the breaking change in [email protected] (changelog) to forbid unknown values by default. Because our objects were silently being only partially transformed to DTOs by this bug, they were being considered as an "unknown value". I imagine that this may be affecting other users as well since there's been a decent amount of chatter about this on the NestJS discord and issue boards (examples: typestack/class-validator#1873, nestjs/nest#10683)

This implementation is lifted from what was done in typestack/class-validator#265 to use a global store to avoid these conflicts.

Checklist

  • the pull request title describes what this PR does (not a vague title like Update index.md)
  • the pull request targets the default branch of the repository (develop)
  • the code follows the established code style of the repository
    • npm run prettier:check passes
    • npm run lint:check passes
  • [n/a] tests are added for the changes I made (if any source code was modified)
  • [n/a] documentation added or updated
  • I have run the project locally and verified that there are no errors

Fixes

fixes #1043, fixes #928

Update: found PR #929 from 2021 which is similar but contains a lot of automatic dependency updates. This change might be easier to merge at this point so going to leave it up for one of the maintainers to decide between

@y0gzah
Copy link

y0gzah commented Feb 1, 2023

This is still not fixed.
I'm using class-transformer class-validator class-sanitizier and routing-controllers in different packages of my project.
And decorators just don't exist at all from one project to the other.

@jtimmons
Copy link
Author

jtimmons commented Feb 1, 2023

This is still not fixed. I'm using class-transformer class-validator class-sanitizier and routing-controllers in different packages of my project. And decorators just don't exist at all from one project to the other.

hey @y0gzah! Do you mean that this is not fixed by this pull request when testing it? Or that the bug still exists in the library?

@nocive
Copy link

nocive commented Feb 16, 2023

ping pong!

@dabernathy89
Copy link

Running into this issue today. Similar to @jtimmons, I'm attempting to use a shared library of class-validator models between our frontend (Vue/Vite) repo and our backend (Nest.js) repo. Would love to see this prioritized.

@tetofonta
Copy link

+1

@NoNameProvided
Copy link
Member

Fixed in #929.

Copy link

github-actions bot commented Jun 4, 2024

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

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
6 participants