refactor(dal): update analitycs repository to use zod for schema generation#10581
refactor(dal): update analitycs repository to use zod for schema generation#10581djabarovgeorge wants to merge 2 commits intonextfrom
Conversation
…ositoryV2 and modify requestLogSchema to use Zod for validation
✅ Deploy Preview for dashboard-v2-novu-staging canceled.
|
|
Hey there and thank you for opening this pull request! 👋 We require pull request titles to follow specific formatting rules and it looks like your proposed title needs to be adjusted. Your PR title is: Requirements:
Expected format: Details: PR title must end with 'fixes TICKET-ID' (e.g., 'fixes NOV-123') or include ticket ID in branch name |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA new Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant QB as QueryBuilder
participant LogRepV2 as LogRepositoryV2
participant CH as ClickHouse
App->>QB: new QueryBuilder()
App->>QB: whereEquals('status', 'active')<br/>.whereGreaterThan('count', 10)
QB->>QB: Store typed conditions
App->>LogRepV2: find(qb.build())
LogRepV2->>LogRepV2: Validate conditions<br/>(operators, columns)
LogRepV2->>LogRepV2: Add environment_id<br/>enforcement
LogRepV2->>LogRepV2: Build WHERE clause<br/>with parameter map
LogRepV2->>CH: Execute: SELECT ... WHERE<br/>environment_id = ? AND ...<br/>ORDER BY ... FINAL
CH-->>LogRepV2: Result rows
LogRepV2-->>App: Typed result[]
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested Labels
Suggested Reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/application-generic/src/services/analytic-logs/request-log/request-log.repository.ts (1)
15-24:⚠️ Potential issue | 🟠 MajorPass the batch writer through this V2 migration.
After switching this repository to
LogRepositoryV2, the constructor still only wiresClickHouseService.shouldUseBatching()will now always short-circuit on!this.batchService, socreateMany()never reaches the feature-flagged batch path.♻️ Suggested wiring
import { ClickHouseService, InsertOptions } from '../clickhouse.service'; +import { ClickHouseBatchService } from '../clickhouse-batch.service'; ... constructor( protected readonly clickhouseService: ClickHouseService, + protected readonly clickhouseBatchService: ClickHouseBatchService, protected readonly logger: PinoLogger, protected readonly featureFlagsService: FeatureFlagsService ) { - super(clickhouseService, logger, requestLogSchema, ORDER_BY, featureFlagsService); + super(clickhouseService, logger, requestLogSchema, ORDER_BY, featureFlagsService, clickhouseBatchService);As per coding guidelines, "Use
ClickHouseServicefor single queries/inserts andClickHouseBatchServicefor high-throughput writes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/application-generic/src/services/analytic-logs/request-log/request-log.repository.ts` around lines 15 - 24, The constructor of RequestLogRepository was not wired with a ClickHouseBatchService so LogRepositoryV2's shouldUseBatching() short-circuits (this.batchService is undefined) and createMany() never uses the batch path; update the constructor signature to accept a protected readonly batchService: ClickHouseBatchService (alongside ClickHouseService, PinoLogger, FeatureFlagsService) and ensure you pass or assign that batchService into the base class (LogRepositoryV2) / this.batchService so shouldUseBatching() and createMany() can use batching; look for symbols RequestLogRepository, constructor, LogRepositoryV2, shouldUseBatching(), createMany(), and ClickHouseBatchService when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/application-generic/src/services/analytic-logs/v2/log.repository.ts`:
- Around line 51-53: EnforcedContext lost organization scoping; add
organizationId: string back to the EnforcedContext type and update
buildEnforcedConditions() to inject both environment_id and organization_id (map
to the expected ClickHouse parameters _environmentId and _organizationId or the
existing parameter naming used elsewhere) so all V2 safe-read queries include
both tenant boundaries; update any call sites that construct EnforcedContext to
supply organizationId accordingly.
- Around line 92-103: The new abstract class LogRepositoryV2 is defined as a
separate base instead of extending the shared LogRepository contract; update the
class so it either extends LogRepository or move the Zod-specific behavior into
the existing LogRepository. Concretely, change the declaration of
LogRepositoryV2 to extend LogRepository and keep its constructor signature
(clickhouseService, logger, schema: ZodChSchema, schemaOrderBy,
featureFlagsService, batchService) while delegating common fields/methods to the
parent; alternatively, fold schema/schemaOrderBy handling (ZodChSchema and
SchemaKeys) into the existing LogRepository implementation so all ClickHouse
repositories continue to inherit from LogRepository per guidelines. Ensure
references to LogRepositoryV2, LogRepository, schema, and schemaOrderBy are
updated accordingly.
- Around line 464-489: The findOne overloads (the generic version and the '*'
version) currently return result.data[0], which can be undefined on no-match and
violates their promised non-null return types; update both implementations (the
findOne method and the similar block around lines referenced for 492-523) to
explicitly handle empty results by either returning a nullable type (e.g.,
Promise<... | null>) or throwing a NotFound/Error when result.data.length === 0,
and update the function signatures to reflect the chosen contract; ensure you
adjust callers/types accordingly so the behaviors of findOne and the analogous
method are consistent and no longer return { data: undefined, rows: 0 } while
claiming a non-null row.
---
Outside diff comments:
In
`@libs/application-generic/src/services/analytic-logs/request-log/request-log.repository.ts`:
- Around line 15-24: The constructor of RequestLogRepository was not wired with
a ClickHouseBatchService so LogRepositoryV2's shouldUseBatching() short-circuits
(this.batchService is undefined) and createMany() never uses the batch path;
update the constructor signature to accept a protected readonly batchService:
ClickHouseBatchService (alongside ClickHouseService, PinoLogger,
FeatureFlagsService) and ensure you pass or assign that batchService into the
base class (LogRepositoryV2) / this.batchService so shouldUseBatching() and
createMany() can use batching; look for symbols RequestLogRepository,
constructor, LogRepositoryV2, shouldUseBatching(), createMany(), and
ClickHouseBatchService when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7fafc5b0-a2c1-48f1-97f9-db89b6397910
📒 Files selected for processing (5)
libs/application-generic/src/services/analytic-logs/request-log/request-log.repository.tslibs/application-generic/src/services/analytic-logs/request-log/request-log.schema.tslibs/application-generic/src/services/analytic-logs/v2/clickhouse-types.tslibs/application-generic/src/services/analytic-logs/v2/index.tslibs/application-generic/src/services/analytic-logs/v2/log.repository.ts
| export type EnforcedContext = { | ||
| environmentId: string; | ||
| }; |
There was a problem hiding this comment.
Restore organization scoping in enforced queries.
EnforcedContext no longer carries organizationId, and buildEnforcedConditions() only injects environment_id. That means every safe read path in V2 is missing one tenant boundary.
As per coding guidelines, "Always include _environmentId and _organizationId in ClickHouse queries for tenant isolation".
Also applies to: 181-188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/application-generic/src/services/analytic-logs/v2/log.repository.ts`
around lines 51 - 53, EnforcedContext lost organization scoping; add
organizationId: string back to the EnforcedContext type and update
buildEnforcedConditions() to inject both environment_id and organization_id (map
to the expected ClickHouse parameters _environmentId and _organizationId or the
existing parameter naming used elsewhere) so all V2 safe-read queries include
both tenant boundaries; update any call sites that construct EnforcedContext to
supply organizationId accordingly.
| export abstract class LogRepositoryV2<TSchema extends ZodChSchema> { | ||
| readonly table: string; | ||
| readonly identifierPrefix: string; | ||
|
|
||
| constructor( | ||
| protected readonly clickhouseService: ClickHouseService, | ||
| protected readonly logger: PinoLogger, | ||
| protected readonly schema: TSchema, | ||
| protected readonly schemaOrderBy: SchemaKeys<TSchema>[], | ||
| protected readonly featureFlagsService: FeatureFlagsService, | ||
| protected readonly batchService?: ClickHouseBatchService | ||
| ) {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Keep the new base chained to LogRepository.
This starts a parallel analytic-logs repository hierarchy instead of extending the shared base contract the rest of the layer is expected to use. Please fold the Zod-specific behavior into LogRepository, or make LogRepositoryV2 extend it.
As per coding guidelines, "All ClickHouse repositories must extend LogRepository from libs/application-generic/src/services/analytic-logs/".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/application-generic/src/services/analytic-logs/v2/log.repository.ts`
around lines 92 - 103, The new abstract class LogRepositoryV2 is defined as a
separate base instead of extending the shared LogRepository contract; update the
class so it either extends LogRepository or move the Zod-specific behavior into
the existing LogRepository. Concretely, change the declaration of
LogRepositoryV2 to extend LogRepository and keep its constructor signature
(clickhouseService, logger, schema: ZodChSchema, schemaOrderBy,
featureFlagsService, batchService) while delegating common fields/methods to the
parent; alternatively, fold schema/schemaOrderBy handling (ZodChSchema and
SchemaKeys) into the existing LogRepository implementation so all ClickHouse
repositories continue to inherit from LogRepository per guidelines. Ensure
references to LogRepositoryV2, LogRepository, schema, and schemaOrderBy are
updated accordingly.
| async findOne<T extends readonly (keyof InferZodChSchema<TSchema>)[]>(options: { | ||
| where: Where<InferZodChSchema<TSchema>>; | ||
| limit?: number; | ||
| offset?: number; | ||
| orderBy?: SchemaKeys<TSchema>; | ||
| orderDirection?: 'ASC' | 'DESC'; | ||
| useFinal?: boolean; | ||
| select: T; | ||
| }): Promise<{ | ||
| data: Pick<InferZodChSchema<TSchema>, T[number]>; | ||
| rows: number; | ||
| }>; | ||
|
|
||
| // Overload for "*" all columns selection | ||
| async findOne(options: { | ||
| where: Where<InferZodChSchema<TSchema>>; | ||
| limit?: number; | ||
| offset?: number; | ||
| orderBy?: SchemaKeys<TSchema>; | ||
| orderDirection?: 'ASC' | 'DESC'; | ||
| useFinal?: boolean; | ||
| select: '*'; | ||
| }): Promise<{ | ||
| data: InferZodChSchema<TSchema>; | ||
| rows: number; | ||
| }>; |
There was a problem hiding this comment.
Handle the empty-result path in findOne.
Both branches return result.data[0], so a miss becomes { data: undefined, rows: 0 } even though the overloads promise a row. Make the contract explicit (null/undefined) or throw when nothing matches.
Also applies to: 492-523
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@libs/application-generic/src/services/analytic-logs/v2/log.repository.ts`
around lines 464 - 489, The findOne overloads (the generic version and the '*'
version) currently return result.data[0], which can be undefined on no-match and
violates their promised non-null return types; update both implementations (the
findOne method and the similar block around lines referenced for 492-523) to
explicitly handle empty results by either returning a nullable type (e.g.,
Promise<... | null>) or throwing a NotFound/Error when result.data.length === 0,
and update the function signatures to reflect the chosen contract; ensure you
adjust callers/types accordingly so the behaviors of findOne and the analogous
method are consistent and no longer return { data: undefined, rows: 0 } while
claiming a non-null row.
1 participant
What changed? Why was the change needed?
What changed
The analytics repository's data access layer was refactored to migrate from a ClickHouse schema builder pattern to Zod-based schema validation with integrated ClickHouse type descriptors. A new
LogRepositoryV2abstract class was introduced as the base for analytics repositories, centralizing tenant/environment enforcement, feature-flag-driven log expiration, and type-safe query condition building with runtime validation. TheRequestLogRepositorynow extendsLogRepositoryV2instead ofLogRepository, and its schema definition was migrated to use Zod validators paired with ClickHouse type metadata.Affected areas
RequestLogRepositoryfromLogRepositorytoLogRepositoryV2with schema validation now driven by Zod validators instead of ClickHouse schema builders. New v2 module introduces reusable typed query conditions, tenant enforcement, and batch insert capabilities.Key technical decisions
{ zod, ch }) for runtime safety and type inference, replacing the previousClickhouseSchemawrapper class.__unsafeescape hatch), feature-flag-driven log expiration, and query construction with automatic column name and operator validation.QueryBuilder<T>provides fluent API for constructingWHEREclauses with type-checked columns, operators (equals,in,like,between), and array/null handling.idgeneration andexpires_atcomputation.Testing
No test files were included in the summary. Verification of schema validation behavior, tenant enforcement, and query builder correctness through existing or new unit tests is recommended.
Screenshots
Expand for optional sections
Related enterprise PR
Special notes for your reviewer