fix(server): name the setting when NATS_URL cannot be parsed - #2211
fix(server): name the setting when NATS_URL cannot be parsed#2211mrautela365 wants to merge 1 commit into
Conversation
`new URL()` was unguarded, so a malformed NATS_URL threw a bare `TypeError: Invalid URL`. That happens in the NatsService constructor, which is reached through Auth0Service -> ProfileController -> server.ts during module evaluation for EVERY SSR route -- so a config typo took the whole app down and answered every page with a raw stack trace naming internal file paths. Found while pointing a local BFF at dev: omitting the `nats://` scheme is enough to trigger it. The failure stays fatal on purpose. A BFF that cannot reach NATS cannot resolve a project slug, so every authorization check would deny and serving those pages would be worse than not starting. What changes is that the error names the variable and the shape it expected. Pinned by five tests; three fail when the guard is reverted. Signed-off-by: Misha Rautela <mrautela@linuxfoundation.org>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Essentials Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
PR SummaryLow Risk Overview Adds Reviewed by Cursor Bugbot for commit 67c80c9. Bugbot is set up for automated code reviews on this repo. Configure here. |
There was a problem hiding this comment.
Pull request overview
Improves NATS configuration diagnostics during BFF initialization.
Changes:
- Converts URL parsing failures into actionable
NATS_URLerrors. - Adds malformed URL, valid URL, and default-port tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
nats.service.ts |
Adds guarded NATS URL parsing. |
nats.service.spec.ts |
Tests failure and success paths. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@mrautela365 — nice, focused fix. Wrapping the AI reconciliation: Copilot reviewed both files and raised no inline comments — I agree with that assessment. No CodeRabbit or Cursor Bot threads to reconcile. Structured issue count:
✅ Approved with minor comments |
dealako
left a comment
dealako
left a comment
There was a problem hiding this comment.
@mrautela365 — nice, focused fix. Wrapping the new URL() throw with a named NATS_URL error directly addresses the ops pain called out in the commit message: a config typo no longer surfaces as a bare TypeError: Invalid URL with internal stack paths on every SSR route. The try/catch stays fatal on purpose, which matches the BFF's dependency on NATS for project slug resolution. Five targeted tests cover the malformed, valid, and default-port paths well.
AI reconciliation: Copilot reviewed both files and raised no inline comments — I agree with that assessment. No CodeRabbit or Cursor Bot threads to reconcile.
Structured issue count:
- 🔴 Blocking: 0 issues
- 🟡 Minor: 1 issue: rejection tests use loose regexes instead of asserting the full error contract
- ⚪ Nit: 2 issues: duplicated rationale comments; optional NATS doc update
- ❔ Question: 0 items
✅ Approved with minor comments
| // Not a bare TypeError: the message must be actionable, or an operator sees only | ||
| // "Invalid URL" on every route with no indication of which setting caused it. | ||
| expect(() => new NatsService()).toThrow(/NATS_URL/); | ||
| expect(() => new NatsService()).toThrow(/nats:\/\/host:4222/); |
There was a problem hiding this comment.
[minor] Rejection tests use loose regexes instead of the full error contract
Issue: The it.each case asserts only /NATS_URL/ and /nats:\/\/host:4222/ in two separate constructor calls. A regression that throws TypeError('Invalid NATS_URL: expected nats://host:4222') or drops the received "…" clause would still pass.
Proof: Implementation at nats.service.ts:45 throws new Error(\Invalid NATS_URL: expected a URL like "nats://host:4222", received "${natsUrl}"`). Spec lines 34–35 never assert the stubbed input value or that the failure is an Errorrather than a bareTypeError`.
Why it matters: The PR's core behavior is an actionable config error naming both the setting and the bad value. Weak assertions let that contract regress while CI stays green.
Fix: Capture one thrown error per fixture and assert toBeInstanceOf(Error), not.toBeInstanceOf(TypeError), and that message includes the stubbed value (or match the full expected string per case).
| public constructor() { | ||
| const natsUrl = process.env['NATS_URL'] || NATS_CONFIG.DEFAULT_SERVER_URL; | ||
| const parsedUrl = new URL(natsUrl.replace(/^nats:/, 'http:')); | ||
| // PARSED DEFENSIVELY. `new URL()` throws a bare `TypeError: Invalid URL` on a malformed value, |
There was a problem hiding this comment.
[nit] Duplicated multi-paragraph rationale in source and spec
Issue: Lines 32–40 repeat nearly the same startup-failure narrative already documented in nats.service.spec.ts:8–18.
Proof: Both blocks describe Auth0Service → ProfileController → server.ts, fatal startup, and project-slug authorization. Comparable service specs (e.g. guild.service.spec.ts) keep long rationale in the spec while production code stays terse.
Why it matters: Two copies drift when the call chain or policy changes.
Fix: Trim the constructor comment to 1–2 lines (malformed NATS_URL is rethrown with the setting name); keep the detailed rationale in the spec.
Found while running the paid-campaign platform end-to-end against dev.
The defect
new URL()inNatsService's constructor was unguarded:A malformed
NATS_URLthrows a bareTypeError: Invalid URL. That constructor runs insideAuth0Service → ProfileController → server.tsduring module evaluation for every SSR route, so a config typo takes the whole app down and answers every page with a raw stack trace naming internal file paths.Omitting the
nats://scheme is enough to trigger it — which is exactly how I hit it.The fix
The failure stays fatal on purpose: a BFF that cannot reach NATS cannot resolve a project slug, so every authorization check would deny and serving those pages would be worse than not starting. What changes is that the error names the variable and the shape it expected:
Verification
Five new tests. Mutation-verified — reverting the guard fails three of them:
new URL(...)rejects …casesThe other two pin the success path: a well-formed URL keeps its host and port, and a port-less URL still defaults to 4222 (the
\|\| 4222fallback, which only a port-less URL reaches).100 files / 2688 server tests, typecheck and lint clean.