Adopt domain exceptions#170
Conversation
WalkthroughRefactors rate-limiting flow to throw a domain exception instead of returning an “allowed” flag, adds new domain exceptions (including OrganizationNotFound and RateLimitExceeded), and introduces a centralized Express error-handling middleware that maps DomainException instances to structured HTTP responses. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor C as Client
participant S as Express Server
participant M as RateLimitMiddleware
participant U as RateLimitUseCase
participant E as ErrorHandler
C->>S: HTTP Request
S->>M: Invoke middleware
M->>U: checkRateLimit(req)
alt Within limit
U-->>M: { remaining, retryAfter }
M-->>S: next()
S-->>C: Continue to route handler (normal flow)
else Exceeded
U--x M: throw RateLimitExceededException
M-->>S: next(err)
S->>E: errorHandler(err, req, res, next)
E-->>C: 429 JSON { success:false, errorCode, message }
end
sequenceDiagram
autonumber
actor C as Client
participant S as Express Server
participant E as ErrorHandler
C->>S: HTTP Request triggers error
S->>E: errorHandler(err)
alt err is DomainException
E-->>C: err.statusCode + JSON { success:false, errorCode, message }
else unknown error
E-->>C: 500 + JSON { success:false, errorCode:"INTERNAL_SERVER_ERROR", message:"An unexpected error occurred" }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (11)
src/modules/shared/domain/exceptions/domain.exception.ts (3)
2-2: Useoverridefor thenameproperty to align with TS best practicesSince
Erroralready definesname: string, explicitly mark the redeclaration asoverrideto avoid surprises withnoImplicitOverrideand improve intent clarity.- public readonly name: string; + public override readonly name: string;Also applies to: 16-16
6-14: Extract and reuse an options type; optionally allow acause/detailspayloadDefining an exported
DomainExceptionOptionsinterface improves reuse and consistency across all domain exceptions. Adding optionalcause(and/ordetails) enables richer diagnostics while keeping the constructor backwards-compatible.+export interface DomainExceptionOptions { + message: string; + errorCode: string; + statusCode: number; + cause?: unknown; + details?: Record<string, unknown>; +} + constructor({ message, errorCode, statusCode, - }: { - message: string; - errorCode: string; - statusCode: number; - }) { + cause, + details, + }: DomainExceptionOptions) { super(message); this.name = new.target.name; this.errorCode = errorCode; this.statusCode = statusCode; + // Optional: attach for debugging/observability + if (cause !== undefined) (this as any).cause = cause; + if (details !== undefined) (this as any).details = details;
16-18: Guard against invalid HTTP status codesMinor hardening: assert that
statusCodefalls within 400–599 to prevent accidental 2xx/3xx usage in exceptions.this.errorCode = errorCode; - this.statusCode = statusCode; + this.statusCode = statusCode; + if (this.statusCode < 400 || this.statusCode > 599) { + // Fail fast in dev; keep as-is in prod if you prefer + // eslint-disable-next-line no-console + console.warn(`[DomainException] Non-error statusCode provided: ${this.statusCode} for ${this.name}`); + }src/modules/shared/middleware/error-handler/error-handler.middleware.ts (2)
1-3: Import path hygieneCurrent file uses a relative import for
DomainException, while other new files mix relative and alias (@/…). Consider unifying to one style to reduce path-churn. If@alias is configured, you can adopt it here too for consistency.
5-25: Optional: log unknown errors here to centralize diagnosticsYou currently log in rate-limit middleware, but other routes may throw unknown errors too. Consider logging here (behind env guard) to ensure consistent observability.
src/middleware/rateLimitMiddleware.ts (2)
17-18: Remove unusedretryAfteror emit a reset header
retryAfteris destructured but unused. IfnoUnusedLocalsis enabled, this will fail compilation. Either remove it or emit a helpful header likeX-RateLimit-Reset.Option A — remove:
- const { remaining, retryAfter } = + const { remaining } = await this.rateLimitUseCase.checkRateLimit(req);Option B — emit reset timestamp (epoch seconds):
- const { remaining, retryAfter } = + const { remaining, retryAfter } = await this.rateLimitUseCase.checkRateLimit(req); res.setHeader("X-RateLimit-Remaining", remaining.toString()); + const resetEpochSeconds = Math.ceil(Date.now() / 1000) + retryAfter * 60; + res.setHeader("X-RateLimit-Reset", resetEpochSeconds.toString());
35-36: Mark the async call as intentionally fire-and-forgetUse
voidto satisfy linters (e.g., no-floating-promises) and signal intent that Express will continue oncenext()is called inside the async IIFE.- checkRateLimit(); + void checkRateLimit();src/modules/shared/middleware/rate-limit/use-cases/rate-limit-use-case.ts (4)
44-56: Use actual window TTL for retryAfter, and avoid an extra Redis round-tripCurrently retryAfter returns the full windowMinutes, which can over-warn clients. Also, getRemainingRequests adds a second Redis call that you can avoid because you already have currentCount. Consider using the key’s TTL for accurate retryAfter and compute remaining locally.
Apply this diff within this block:
- const allowed = currentCount <= routeConfig.maxRequests; - const remaining = await this.repository.getRemainingRequests( - identifier, - routeType, - routeConfig.maxRequests - ); - - if (!allowed) { - throw new RateLimitExceededException(routeConfig.windowMinutes); - } - - return { remaining, retryAfter: routeConfig.windowMinutes }; + const allowed = currentCount <= routeConfig.maxRequests; + const ttlSeconds = await this.repository.getWindowTTLSeconds( + identifier, + routeType + ); + + if (!allowed) { + throw new RateLimitExceededException( + Math.max(1, Math.ceil(ttlSeconds / 60)) + ); + } + + const remaining = Math.max(0, routeConfig.maxRequests - currentCount); + return { + remaining, + retryAfter: Math.max(1, Math.ceil(ttlSeconds / 60)), + };If RedisRateLimitRepository lacks getWindowTTLSeconds, add it (see snippet below). This keeps behavior accurate and reduces one network hop.
Additional repository helper (outside this file):
// In RedisRateLimitRepository async getWindowTTLSeconds(identifier: string, routeType: string): Promise<number> { const key = this.buildKey(identifier, routeType); // reuse your existing key builder const ttl = await this.client.ttl(key); // -2=no key, -1=no expire return ttl > 0 ? ttl : 0; }
51-53: Add targeted tests for the exception pathAdd a unit/integration test asserting that the 429 is produced and that Retry-After (or your JSON field) reflects the remaining TTL, not the static window size.
Happy to scaffold tests for:
- within-limit: returns remaining > 0 and retryAfter > 0
- at-limit: throws RateLimitExceededException with expected retryAfter
- route-specific configs (auth/wallet/email) honored
19-23: Prefer user ID over IP (and avoid raw IP storage); fall back to trusted IP/XFFComment says “Use IP or user ID” but code only uses req.ip. Prefer a stable user identifier when authenticated, and if you must use IP, pull the client IP from X-Forwarded-For when trust proxy is enabled. Consider hashing the IP to reduce PII exposure in Redis keys.
Example improvement:
- // Use IP or user ID if authenticated - return req.ip || "anonymous"; + // Prefer authenticated user identifier; fall back to client IP + const userId = (req as any)?.user?.id ?? (req as any)?.auth?.sub; + if (userId) return `user:${userId}`; + const xff = (req.headers["x-forwarded-for"] as string) || ""; + const clientIp = xff.split(",")[0]?.trim() || req.ip; + return clientIp ? `ip:${clientIp}` : "anonymous";Optional anonymization (outside this file):
import { createHmac } from "crypto"; // ... const anon = createHmac("sha256", process.env.IP_HASH_SALT || "default_salt") .update(clientIp) .digest("base64url"); return `ip:${anon}`;
24-29: Route matching by substring is brittle; use req.path and config-driven matchersUsing path.includes risks false positives (and originalUrl includes query). Prefer req.path and regex/configured matchers.
Light refactor:
- private getRouteType(path: string): keyof RateLimitConfig["routes"] { - if (path.includes("/auth")) return "auth"; - if (path.includes("/wallet")) return "wallet"; - if (path.includes("/email")) return "email"; + private getRouteType(pathname: string): keyof RateLimitConfig["routes"] { + const p = pathname.toLowerCase(); + if (/\/auth(\/|$)/i.test(p)) return "auth"; + if (/\/wallet(\/|$)/i.test(p)) return "wallet"; + if (/\/email(\/|$)/i.test(p)) return "email"; return "default"; }And call with req.path instead of req.originalUrl.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/middleware/rateLimitMiddleware.ts(2 hunks)src/modules/shared/domain/exceptions/domain.exception.ts(1 hunks)src/modules/shared/domain/exceptions/organization-not-found.exception.ts(1 hunks)src/modules/shared/middleware/error-handler/error-handler.middleware.ts(1 hunks)src/modules/shared/middleware/rate-limit/domain/exceptions/rate-limit-exceeded.exception.ts(1 hunks)src/modules/shared/middleware/rate-limit/use-cases/rate-limit-use-case.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/modules/shared/domain/exceptions/organization-not-found.exception.ts (1)
src/modules/project/domain/Project.ts (1)
id(45-47)
src/modules/shared/middleware/rate-limit/use-cases/rate-limit-use-case.ts (1)
src/modules/shared/middleware/rate-limit/domain/exceptions/rate-limit-exceeded.exception.ts (1)
RateLimitExceededException(3-11)
🔇 Additional comments (4)
src/modules/shared/domain/exceptions/organization-not-found.exception.ts (1)
3-11: LGTM: clean domain exception with specific HTTP status and codeConstructor payload is crisp and aligns with the centralized error handler contract. Message is safe to expose.
src/modules/shared/middleware/rate-limit/domain/exceptions/rate-limit-exceeded.exception.ts (1)
1-1: Alias path configuration verified
Thetsconfig.jsonclearly defines the@/*alias ("baseUrl": ".", "paths": { "@/*": ["src/*"], … }), and imports such asimport { DomainException } from "@/modules/shared/domain/exceptions/domain.exception";correctly resolve tosrc/modules/.... A repository‐wide grep also shows multiple modules using the same alias, confirming it’s supported and in use consistently.
No changes required here.src/middleware/rateLimitMiddleware.ts (1)
46-52: Global error handler registered after all routes and middlewaresVerified that in src/index.ts the application is initialized at line 31 and the custom error handler is wired up at the very end (around line 82), after all calls to app.use —including rate limiting and route registrations. No further action is needed.
src/modules/shared/middleware/rate-limit/use-cases/rate-limit-use-case.ts (1)
8-8: LGTM: adopting domain exception importImport path and usage of RateLimitExceededException are consistent with the new domain exception approach.
| export function errorHandler( | ||
| err: unknown, | ||
| req: Request, | ||
| res: Response, | ||
| next: NextFunction | ||
| ): void { | ||
| if (err instanceof DomainException) { | ||
| res.status(err.statusCode).json({ | ||
| success: false, | ||
| errorCode: err.errorCode, | ||
| message: err.message, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| res.status(500).json({ | ||
| success: false, | ||
| errorCode: "INTERNAL_SERVER_ERROR", | ||
| message: "An unexpected error occurred", | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Set Retry-After for 429 and surface rate-limit metadata
When the rate limit is exceeded, clients benefit from a standards-compliant Retry-After response header. Extend the handler to recognize RateLimitExceededException and set the header (seconds). This also requires the exception to expose retryAfterMinutes. See paired suggestion in the exception class.
import { DomainException } from "../../domain/exceptions/domain.exception";
+import { RateLimitExceededException } from "../rate-limit/domain/exceptions/rate-limit-exceeded.exception";
export function errorHandler(
err: unknown,
req: Request,
res: Response,
next: NextFunction
): void {
- if (err instanceof DomainException) {
+ if (err instanceof RateLimitExceededException) {
+ // Per RFC 6585/7231, use seconds for Retry-After
+ res.setHeader("Retry-After", String(err.retryAfterMinutes * 60));
+ res.status(err.statusCode).json({
+ success: false,
+ errorCode: err.errorCode,
+ message: err.message,
+ });
+ return;
+ } else if (err instanceof DomainException) {
res.status(err.statusCode).json({
success: false,
errorCode: err.errorCode,
message: err.message,
});
return;
}
res.status(500).json({
success: false,
errorCode: "INTERNAL_SERVER_ERROR",
message: "An unexpected error occurred",
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function errorHandler( | |
| err: unknown, | |
| req: Request, | |
| res: Response, | |
| next: NextFunction | |
| ): void { | |
| if (err instanceof DomainException) { | |
| res.status(err.statusCode).json({ | |
| success: false, | |
| errorCode: err.errorCode, | |
| message: err.message, | |
| }); | |
| return; | |
| } | |
| res.status(500).json({ | |
| success: false, | |
| errorCode: "INTERNAL_SERVER_ERROR", | |
| message: "An unexpected error occurred", | |
| }); | |
| } | |
| import { DomainException } from "../../domain/exceptions/domain.exception"; | |
| import { RateLimitExceededException } from "../rate-limit/domain/exceptions/rate-limit-exceeded.exception"; | |
| export function errorHandler( | |
| err: unknown, | |
| req: Request, | |
| res: Response, | |
| next: NextFunction | |
| ): void { | |
| if (err instanceof RateLimitExceededException) { | |
| // Per RFC 6585/7231, use seconds for Retry-After | |
| res.setHeader("Retry-After", String(err.retryAfterMinutes * 60)); | |
| res.status(err.statusCode).json({ | |
| success: false, | |
| errorCode: err.errorCode, | |
| message: err.message, | |
| }); | |
| return; | |
| } else if (err instanceof DomainException) { | |
| res.status(err.statusCode).json({ | |
| success: false, | |
| errorCode: err.errorCode, | |
| message: err.message, | |
| }); | |
| return; | |
| } | |
| res.status(500).json({ | |
| success: false, | |
| errorCode: "INTERNAL_SERVER_ERROR", | |
| message: "An unexpected error occurred", | |
| }); | |
| } |
🤖 Prompt for AI Agents
In src/modules/shared/middleware/error-handler/error-handler.middleware.ts
around lines 5 to 25, the handler doesn’t special-case rate-limit errors or set
a Retry-After header; update it to recognize RateLimitExceededException (in
addition to DomainException), respond with status 429, set the HTTP
"Retry-After" header to the number of seconds computed from the exception's
retryAfterMinutes (retryAfterMinutes * 60) before sending the JSON body, and
include any rate-limit metadata required in the response; ensure you
import/reference RateLimitExceededException and guard access to
retryAfterMinutes.
| export class RateLimitExceededException extends DomainException { | ||
| constructor(retryAfterMinutes: number) { | ||
| super({ | ||
| message: `Rate limit exceeded. Try again in ${retryAfterMinutes} minutes.`, | ||
| errorCode: "RATE_LIMIT_EXCEEDED", | ||
| statusCode: 429, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expose retryAfterMinutes on the exception to enable Retry-After header
Store the argument as a public readonly property so the error handler can set standards-compliant headers. This integrates with the earlier middleware suggestion.
export class RateLimitExceededException extends DomainException {
- constructor(retryAfterMinutes: number) {
+ public readonly retryAfterMinutes: number;
+ constructor(retryAfterMinutes: number) {
super({
message: `Rate limit exceeded. Try again in ${retryAfterMinutes} minutes.`,
errorCode: "RATE_LIMIT_EXCEEDED",
statusCode: 429,
});
+ this.retryAfterMinutes = retryAfterMinutes;
}
}🤖 Prompt for AI Agents
In
src/modules/shared/middleware/rate-limit/domain/exceptions/rate-limit-exceeded.exception.ts
around lines 3 to 11, the constructor accepts retryAfterMinutes but does not
expose it as a property; add a public readonly retryAfterMinutes: number
property on the class and assign it from the constructor argument before calling
super (or immediately after) so downstream error handlers can read this value to
set a Retry-After header; keep the existing super payload unchanged and ensure
the property is strongly typed as number.
| async checkRateLimit( | ||
| req: Request | ||
| ): Promise<{ allowed: boolean; remaining: number; retryAfter: number }> { | ||
| ): Promise<{ remaining: number; retryAfter: number }> { | ||
| const identifier = this.getIdentifier(req); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Public API shift to exception-based flow—verify all call sites and middleware
checkRateLimit now returns only remaining/retryAfter and throws on exceed. Ensure all callers stop checking an "allowed" flag and rely on exception handling (central error middleware) instead.
Run this quick scan to find impacted call sites and error handling:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Call sites of checkRateLimit:"
rg -nP -C3 '\bcheckRateLimit\s*\(' --type=ts --type=tsx
echo
echo "Usages of deprecated patterns (allowed, RATE_LIMIT_EXCEEDED handling, Retry-After headers):"
rg -nP -C2 '\ballowed\b|Retry-After|X-RateLimit-(Limit|Remaining|Reset)|RateLimitExceededException' --type=ts --type=tsx
echo
echo "Central error middleware presence (DomainException mapping):"
rg -nP -C3 '(errorHandler|DomainException|statusCode:\s*429|RATE_LIMIT_EXCEEDED)' --type=ts --type=tsxLength of output: 174
I’ll retry scanning without relying on built-in file types—using globs instead:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Call sites of checkRateLimit:"
rg -nP -C3 '\bcheckRateLimit\s*\(' --glob '*.ts' --glob '*.tsx'
echo
echo "Deprecated patterns (allowed flag, Retry-After, X-RateLimit headers, RateLimitExceededException):"
rg -nP -C2 '\ballowed\b|Retry-After|X-RateLimit-(Limit|Remaining|Reset)|RateLimitExceededException' --glob '*.ts' --glob '*.tsx'
echo
echo "Central error middleware references (errorHandler, DomainException, statusCode: 429, RATE_LIMIT_EXCEEDED):"
rg -nP -C3 '(errorHandler|DomainException|statusCode:\s*429|RATE_LIMIT_EXCEEDED)' --glob '*.ts' --glob '*.tsx'Length of output: 33729
Action Required: Migrate all callers and tests to the new exception-based rate-limit API
The scan confirms that although checkRateLimit now only returns { remaining, retryAfter } and throws a RateLimitExceededException on breach, there are still numerous call sites (especially in tests) checking an obsolete allowed flag and relying on manual header logic. You’ll need to refactor both production middleware and your tests to remove any allowed checks and lean entirely on the thrown exception (handled by your central errorHandler).
Impacted locations and recommended changes:
-
tests/rate-limit/rate-limit-use-case.test.ts
- Remove every
expect(result.allowed)…assertion. - Assert only
remainingandretryAfterfor allowed cases. - Add a test that mocks zero remaining requests and expects
checkRateLimitto throwRateLimitExceededException.
- Remove every
-
tests/rate-limit/rate-limit-middleware.test.ts
- Replace commented-out “allowed” scenarios with:
- One scenario where
checkRateLimitresolves normally (assert onX-RateLimit-Remainingandnext()being called). - One scenario where
checkRateLimitrejects withRateLimitExceededException(assert that your middleware (or centralerrorHandler) responds with status 429 and propagates the exception toerrorHandler).
- One scenario where
- Replace commented-out “allowed” scenarios with:
-
src/middleware/rateLimitMiddleware.ts
- Remove any logic that reads an
allowedflag. - In your
try { … } catch (err) { … }, simply re-throw ornext(err)forRateLimitExceededException—letting the sharederrorHandleremit the 429 response. - (Optional) If you want a
Retry-Afterheader on the 429, extend the exception or error handler to expose the retry interval and setres.setHeader("Retry-After", retryAfterInSeconds).
- Remove any logic that reads an
-
All other call sites
- Grep for any remaining
.allowedreferences in the codebase and clean up.
- Grep for any remaining
These refactors are mandatory to align with the new public API and ensure your middleware and tests remain green.
🤖 Prompt for AI Agents
In src/modules/shared/middleware/rate-limit/use-cases/rate-limit-use-case.ts
around lines 31 to 34, callers and tests still rely on an obsolete `allowed`
flag instead of the new exception-based API that returns only `{ remaining,
retryAfter }` and throws `RateLimitExceededException`; update all impacted code
by removing any `allowed` checks and header-manipulation logic, change tests
(tests/rate-limit/rate-limit-use-case.test.ts and
tests/rate-limit/rate-limit-middleware.test.ts) to assert only `remaining` and
`retryAfter` for allowed cases and add tests that expect the exception when
remaining is zero, modify src/middleware/rateLimitMiddleware.ts to stop reading
`allowed` and in catch blocks re-throw or call next(err) for
RateLimitExceededException (optionally let shared errorHandler set 429 and
Retry-After), and grep the repo to remove any remaining `.allowed` references
and update call sites accordingly.
1 participant
🚀 Volunchain Pull Request
Mark with an
xall the checkboxes that apply (like[x])📌 Type of Change
📝 Changes description
Replaced ad-hoc error handling in OrganizationController with domain exceptions (OrganizationNotFoundException).
📸 Evidence (A photo is required as evidence)
⏰ Time spent breakdown
Refactoring controller logic: 45h
Middleware and exception integration: 25m
🌌 Comments
Thank you for contributing to Volunchain, we are glad that you have chosen us as your project of choice and we hope that you continue to contribute to this great project, so that together we can make our mark at the top!
Summary by CodeRabbit
New Features
Refactor