feat: tanent caching #437
Merged
feat: tanent caching #437
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary by CodeRabbit
WalkthroughRemoved TENANT_BY_ORIGIN_PATH; adjusted authentication whitelist to a string literal. Refactored TenantMiddleware to cache Tenant objects, expose a get-by-origin FQN handler, and drop the constructor argument. Updated the tenant middleware export accordingly. Simplified tearDownWorker by removing event broker and notification service disconnections. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Runtime as Runtime
participant TenantMW as TenantMiddleware
participant Cache as Tenant Cache
participant Conv as getByOrigin converter
participant Next as Next Handler
rect rgb(245,248,255)
note right of TenantMW: Normal request flow
Client->>Runtime: Request (fqn != getByOrigin)
Runtime->>TenantMW: handle(request)
TenantMW->>Cache: lookup(origin)
alt cache miss
TenantMW->>Conv: getByOrigin(origin)
Conv-->>TenantMW: Tenant
TenantMW->>Cache: cache Tenant
end
TenantMW->>Next: set TENANT_PARAMETER, next()
Next-->>Runtime: Response
Runtime-->>Client: Response
end
rect rgb(243,255,245)
note right of TenantMW: getByOrigin FQN path
Client->>Runtime: Request (fqn == getByOrigin)
Runtime->>TenantMW: handle(request)
TenantMW->>Cache: lookup(origin)
alt cache miss
TenantMW->>Conv: getByOrigin(origin)
Conv-->>TenantMW: Tenant
TenantMW->>Cache: cache Tenant
end
TenantMW-->>Runtime: 200 Response(Tenant)
Runtime-->>Client: 200 Response(Tenant)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/integrations/runtime/authenticationMiddleware.ts (1)
14-16: Avoid string literal drift: import a shared FQN constant instead of hardcoding.Hardcoding 'domain/tenant/getByOriginConverted' risks divergence from the middleware comparison. Centralize the FQN and import it here.
Apply this diff locally (uses a shared constant GET_BY_ORIGIN_FQN):
+import { GET_BY_ORIGIN_FQN } from '^/domain/tenant/fqns'; -const whiteList: string[] = [ - 'domain/tenant/getByOriginConverted' -]; +const whiteList: string[] = [GET_BY_ORIGIN_FQN];New shared constant (add this file):
// src/domain/tenant/fqns.ts export const GET_BY_ORIGIN_FQN = 'domain/tenant/getByOriginConverted';src/integrations/runtime/middlewares/TenantMiddleware.ts (3)
8-8: Typo and constant reuse: rename GEY_BY_ORIGIN_FQN → GET_BY_ORIGIN_FQN and reuse shared constant.Minor typo; also prefer importing the shared FQN to keep it in sync with the auth whitelist.
Apply this diff:
-import getByOrigin from '^/domain/tenant/getByOriginConverted'; - -const GEY_BY_ORIGIN_FQN = 'domain/tenant/getByOriginConverted'; +import getByOrigin from '^/domain/tenant/getByOriginConverted'; +import { GET_BY_ORIGIN_FQN } from '^/domain/tenant/fqns'; @@ - return request.fqn === GEY_BY_ORIGIN_FQN + return request.fqn === GET_BY_ORIGIN_FQNAlso applies to: 17-17
27-34: Shadowed variable 'tenant' reduces clarity; rename inner binding.The inner const tenant masks the outer variable. Rename to avoid shadowing.
- if (tenant === undefined) - { - const tenant = await getByOrigin(origin); - - this.#tenants.set(origin, tenant); - - return tenant; - } + if (tenant === undefined) { + const resolvedTenant = await getByOrigin(origin); + this.#tenants.set(origin, resolvedTenant); + return resolvedTenant; + }
22-36: Optional: dedupe concurrent lookups to the same origin.Two simultaneous cache misses for the same origin will trigger duplicate getByOrigin calls. Track in‑flight promises per origin to coalesce.
Sketch:
// field #inflight = new Map<string, Promise<Tenant>>(); async #resolveTenant(request: Request): Promise<Tenant> { const origin = /* validated/normalized origin */; const cached = this.#tenants.get(origin); if (cached) return cached; const existing = this.#inflight.get(origin); if (existing) return existing; const p = getByOrigin(origin).then(t => { this.#tenants.set(origin, t); this.#inflight.delete(origin); return t; }).catch(err => { this.#inflight.delete(origin); throw err; }); this.#inflight.set(origin, p); return p; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/domain/definitions.ts(0 hunks)src/integrations/runtime/authenticationMiddleware.ts(1 hunks)src/integrations/runtime/middlewares/TenantMiddleware.ts(1 hunks)src/integrations/runtime/tearDownWorker.ts(0 hunks)src/integrations/runtime/tenantMiddleware.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/domain/definitions.ts
- src/integrations/runtime/tearDownWorker.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: basmasking
PR: MaskingTechnology/comify#428
File: segments/bff.json:48-48
Timestamp: 2025-07-22T21:11:15.729Z
Learning: The tenant domain uses a two-tier data model: the database `DataModel` contains an `id` and an array of valid `origins`, while the public `Tenant` type returned by `getByOriginConverted` contains only `id` and `origin` (singular). This conversion serves as data minimization, where `id` is needed for stylesheet retrieval and `origin` is the validated browser-sent origin. This architecture implements proper security through data filtering at the public interface level.
📚 Learning: 2025-07-22T21:11:15.729Z
Learnt from: basmasking
PR: MaskingTechnology/comify#428
File: segments/bff.json:48-48
Timestamp: 2025-07-22T21:11:15.729Z
Learning: The tenant domain uses a two-tier data model: the database `DataModel` contains an `id` and an array of valid `origins`, while the public `Tenant` type returned by `getByOriginConverted` contains only `id` and `origin` (singular). This conversion serves as data minimization, where `id` is needed for stylesheet retrieval and `origin` is the validated browser-sent origin. This architecture implements proper security through data filtering at the public interface level.
Applied to files:
src/integrations/runtime/tenantMiddleware.tssrc/integrations/runtime/middlewares/TenantMiddleware.ts
📚 Learning: 2025-07-22T20:29:33.605Z
Learnt from: basmasking
PR: MaskingTechnology/comify#428
File: src/webui/features/hooks/usePostsFollowing.ts:6-6
Timestamp: 2025-07-22T20:29:33.605Z
Learning: In Jitar applications, static imports of context objects like `tenant` from `^/domain/tenant` and `requester` from `^/domain/authentication` are intentionally static placeholders that serve as type-safe function signature matches. At runtime, these are dynamically replaced through Jitar's parameter injection mechanism via middleware (TenantMiddleware and AuthenticationMiddleware) using special parameter keys like `*tenant`. This allows for compile-time type safety while providing runtime flexibility for multi-tenant and authentication contexts.
Applied to files:
src/integrations/runtime/middlewares/TenantMiddleware.ts
🧬 Code graph analysis (2)
src/integrations/runtime/tenantMiddleware.ts (1)
src/integrations/runtime/middlewares/TenantMiddleware.ts (1)
TenantMiddleware(11-54)
src/integrations/runtime/middlewares/TenantMiddleware.ts (1)
src/domain/tenant/getByOrigin/getByOrigin.ts (1)
getByOrigin(9-24)
🔇 Additional comments (1)
src/integrations/runtime/tenantMiddleware.ts (1)
4-4: Constructor change looks good — no callers rely on the old signature.
Search found only the self-export at src/integrations/runtime/tenantMiddleware.ts:4 — export default new TenantMiddleware();
Comment on lines
+22
to
26
| async #resolveTenant(request: Request): Promise<Tenant> | ||
| { | ||
| const origin = this.#getOrigin(request); | ||
| const cached = this.#cache.get(origin); | ||
| const origin = request.getHeader('origin') as string; | ||
| const tenant = this.#tenants.get(origin); | ||
|
|
There was a problem hiding this comment.
Guard and normalize the Origin header; current cast may pass undefined through.
request.getHeader('origin') can be missing or 'null'. The forced cast risks passing undefined into the cache and data layer.
Apply this minimal guard (optional: add normalization):
- const origin = request.getHeader('origin') as string;
+ const rawOrigin = request.getHeader('origin');
+ if (typeof rawOrigin !== 'string' || rawOrigin.trim() === '' || rawOrigin === 'null') {
+ // Consider mapping to a 400 response upstream or throwing a typed error the global handler maps to 400.
+ throw new Error('Missing or invalid Origin header');
+ }
+ const origin = rawOrigin.trim();Optional normalization helper (to avoid case/port variance):
// add as a private static method if desired
static #normalizeOrigin(raw: string): string {
try {
const u = new URL(raw);
const isDefault = (u.protocol === 'https:' && (u.port === '' || u.port === '443')) ||
(u.protocol === 'http:' && (u.port === '' || u.port === '80'));
const host = isDefault ? u.hostname : `${u.hostname}:${u.port}`;
return `${u.protocol}//${host}`.toLowerCase();
} catch {
return raw.toLowerCase();
}
}🤖 Prompt for AI Agents
In src/integrations/runtime/middlewares/TenantMiddleware.ts around lines 22-26,
the code force-casts request.getHeader('origin') to string which may yield
undefined or 'null' and pollute the tenant cache; update the method to first
guard against missing/invalid origin (e.g., check for falsy or the literal
'null' and handle by throwing or returning a default tenant), then normalize the
origin before using it as a cache key (add the provided #normalizeOrigin helper
as a private static method or inline equivalent), and finally use the
normalizedOrigin when calling this.#tenants.get(...) so only valid, normalized
origin strings reach the cache and data layer.
basmasking
approved these changes
Sep 19, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #436
Changes proposed in this pull request:
@MaskingTechnology/comify