unlock unlimited code runs for $79, limited time →
NOSSLETTER.techLIVE
back to OSSpedia
nestjs/nest
mid8/10
refactoringtesting-strategy

fix(core): correctly discover value providers in discovery service

Project: nestjs/nest | Language: TypeScript | Concepts: refactoring, testing-strategy | Difficulty: mid
Link: https://github.com/nestjs/nest/pull/17618

The problem

Providers registered with useValue and decorated with a custom decorator via DiscoveryService.createDecorator() were not being discovered by DiscoveryService.getProviders({ metadataKey }). Only useClass-style providers were found, so value providers with custom decorators were invisible to the discovery mechanism.

Why it's non-trivial

A naive fix might just try to add value providers to the discovery set, but the bug was due to a subtle logic error: the code that determines the "type" of a provider (for metadata lookup) had an incorrect condition. This logic is performance-sensitive and interacts with how provider wrappers store metadata, so changing it risks breaking discovery for other provider types or degrading performance by eagerly instantiating providers.

The approach

Key fix in the provider type resolution logic:

-      instanceWrapper.metatype || instanceWrapper.inject
+      !instanceWrapper.metatype || instanceWrapper.inject
         ? (instanceWrapper.instance?.constructor ?? instanceWrapper.metatype)
         : instanceWrapper.metatype,

What it does:
This change adds a negation (!) to the check for metatype.

  • Before: For value providers (metatype === null), the condition was falsy, so it didn't fall back to using the instance's constructor, and the lookup failed.
  • After: Now, when metatype is null (as with useValue), it uses the instance's constructor for the lookup, enabling discovery.

Test update to reflect the corrected behavior:

-    it('should not add provider when metatype is null and inject is not provided (useValue without inject)', () => {
+    it('should add provider when metatype is null and inject is not provided (useValue without inject)', () => {
...
-      expect(result.size).toBe(0);
+      expect(result.size).toBe(1);
+      expect(result.has(instanceWrapper)).toBe(true);

What it does:
Changes a test that previously asserted value providers were not discovered (the buggy behavior) to now assert they are discovered (the correct behavior).

New regression test for DiscoveryService:

+    it('should discover useValue providers decorated with custom discovery decorator', () => {
+      const TestDecorator = DiscoveryService.createDecorator();
+      @TestDecorator()
+      class TargetService {}
+      ...
+      const providers = discoveryService.getProviders({
+        metadataKey: TestDecorator.KEY,
+      });
+      expect(providers).toHaveLength(1);
+      expect(providers).toContain(instanceWrapper);
+    });

What it does:
Adds a new test that registers a useValue provider with a custom decorator and asserts that DiscoveryService.getProviders can now find it.

From the review thread

johnnyhuirilef:
I ran into the same issue and arrived at the same change, so I'll leave this one to you — yours covers more ground with the DiscoveryService test. Some context I picked up along the way, in case it's useful if anyone asks:

The comment and the typo landed together. Both come from 7e3da2253 (2023-07-24), and the note has said !wrapper.metatype from the first day while the code never had the negation. git log -S '!wrapper.metatype' returns only that one commit, so the note was never edited — it isn't documentation that drifted, it's the original intent.

The spec you changed is a characterization test. It was added in 5ea3988a8 (2025-12-13), 2.5 years later, in a coverage pass that touched no source files. Inverting it turns it from pinning the bug into guarding against it, which is what you did.

On the performance note, which is what I'd expect to come up: it says wrapper.instance should be deferred, which makes negating the condition sound like it would make that worse. It goes the other way —

| provider kind | old condition reads .instance | new | | --- | --- | --- | | class / useClass | yes | no | | useFactory | yes | yes | | useValue | no | yes |

The old condition was truthy for every class provider, which is the dominant case in an app; the new one is truthy only for useValue and factories. The change reduces reads of that getter rather than adding them.

Happy to help verify if that's useful.

kamilmysliwiec: lgtm

Takeaways

  • Always align code with documented intent: If a code comment describes a condition, ensure the implementation matches—especially for subtle logic like negations.
  • Characterization tests can pin bugs: Tests that lock in buggy behavior should be inverted (not deleted) when fixing the bug, to prevent regressions.
  • Performance-sensitive logic deserves careful review: Even small condition changes can impact performance, especially when they control expensive operations like instance creation. Analyze the before/after impact, not just correctness.