feat: support compacted destination transformation payloads#4220
feat: support compacted destination transformation payloads#4220krishna2020 merged 3 commits intodevelopfrom
Conversation
Summary by CodeRabbit
WalkthroughThis pull request introduces a new feature flag in the configuration interface to indicate support for a specific destination transformation payload (compacted payload v1). It adds two new middleware functions along with their associated unit tests for processing and transforming compacted payloads in both destination and router requests. The update adjusts routing to integrate these middleware functions, expands type definitions to support the new payload structure, and includes a minor update to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DestMiddleware as DestTransformCompactedPayloadV1Middleware
participant Lookup as Lookup Service (Destinations/Connections)
participant Next as Next Middleware
Client->>DestMiddleware: Send request with 'json+compactedv1' header
DestMiddleware->>Lookup: Retrieve destination & connection details
Lookup-->>DestMiddleware: Return lookup data
DestMiddleware->>Next: Pass transformed payload
sequenceDiagram
participant Client
participant RouterMiddleware as RouterTransformCompactedPayloadV1Middleware
participant Lookup as Lookup Service (Destinations/Connections)
participant Next as Next Middleware
Client->>RouterMiddleware: Send request with 'json+compactedv1' header
RouterMiddleware->>Lookup: Extract destination & connection info
Lookup-->>RouterMiddleware: Return details
RouterMiddleware->>Next: Pass modified request body
Suggestions for Improvements:
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
Allure Test reports for this run are available at:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4220 +/- ##
===========================================
+ Coverage 91.05% 91.06% +0.01%
===========================================
Files 627 630 +3
Lines 33049 33048 -1
Branches 7818 7825 +7
===========================================
+ Hits 30092 30095 +3
- Misses 2710 2736 +26
+ Partials 247 217 -30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
atzoum
force-pushed
the
feat.dtransformCompaction
branch
3 times, most recently
from
5a1ff29 to
c5b87a1
Compare
|
Allure Test reports for this run are available at:
|
atzoum
force-pushed
the
feat.dtransformCompaction
branch
from
c5b87a1 to
98687d1
Compare
|
Allure Test reports for this run are available at:
|
atzoum
force-pushed
the
feat.dtransformCompaction
branch
from
98687d1 to
b63382d
Compare
|
Allure Test reports for this run are available at:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (14)
src/middlewares/destTransformCompactedPayloadV1.ts (1)
11-23: Consider error handling and refactoring to a function.The middleware implementation correctly transforms compacted payloads based on the presence of the
x-content-formatheader. However, there are two potential improvements:
- Add error handling for cases where the expected keys in
body.connectionsorbody.destinationsmight not exist.- Consider refactoring to a simple function instead of a class with static methods, as suggested by the static analysis.
-export class DestTransformCompactedPayloadV1Middleware { - public static async handle(ctx: Context, next: Next): Promise<void> { +export async function handleDestTransformCompactedPayloadV1(ctx: Context, next: Next): Promise<void> { if (ctx.get('x-content-format') === 'json+compactedv1') { const body = ctx.request.body as ProcessorTransformationRequestV2; + try { ctx.request.body = body.input.map((input) => ({ ...input, connection: body.connections[`${input.metadata.sourceId}:${input.metadata.destinationId}`], destination: body.destinations[input.metadata.destinationId], })) as ProcessorTransformationRequest[]; + } catch (error) { + ctx.throw(400, 'Invalid compacted payload format'); + } } return next(); - } }🧰 Tools
🪛 Biome (1.9.4)
[error] 11-23: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/middlewares/routerTransformCompactedPayloadV1.ts (1)
11-27: Consider error handling and function-based approach.The implementation correctly transforms compacted payloads for router transformations. As with the previous middleware, consider these improvements:
- Add error handling for missing keys in the connections or destinations objects
- Refactor to a simple function rather than a class with static methods
-export class RouterTransformCompactedPayloadV1Middleware { - public static async handle(ctx: Context, next: Next): Promise<void> { +export async function handleRouterTransformCompactedPayloadV1(ctx: Context, next: Next): Promise<void> { if (ctx.get('x-content-format') === 'json+compactedv1') { const body = ctx.request.body as RouterTransformationRequestV2; + try { ctx.request.body = { input: body.input.map((input) => ({ ...input, connection: body.connections[`${input.metadata.sourceId}:${input.metadata.destinationId}`], destination: body.destinations[input.metadata.destinationId], })), destType: body.destType, } as RouterTransformationRequest; + } catch (error) { + ctx.throw(400, 'Invalid compacted payload format'); + } } return next(); - } }🧰 Tools
🪛 Biome (1.9.4)
[error] 11-27: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/middlewares/__tests__/routerTransformCompactedPayloadV1.test.ts (4)
14-14: Rename describe block to match the middleware nameThe describe block name
RouterTransformV2PayloadMiddlewaredoesn't match the middleware being tested (RouterTransformCompactedPayloadV1Middleware). This could lead to confusion when reading the tests.-describe('RouterTransformV2PayloadMiddleware', () => { +describe('RouterTransformCompactedPayloadV1Middleware', () => {
68-70: Add await for async function callThe middleware's handle method is being called with an async callback, but there's no await. This could lead to test failures or race conditions if the assertion completes before the middleware finishes.
- RouterTransformCompactedPayloadV1Middleware.handle(ctx, async () => { - expect(ctx.request.body).toEqual(originalBody); - }); + await RouterTransformCompactedPayloadV1Middleware.handle(ctx, async () => { + expect(ctx.request.body).toEqual(originalBody); + });
101-103: Add await for async function callSimilar to the previous test, this async function call should also have an await.
- RouterTransformCompactedPayloadV1Middleware.handle(ctx, async () => { - expect(ctx.request.body).toEqual(expectedBody); - }); + await RouterTransformCompactedPayloadV1Middleware.handle(ctx, async () => { + expect(ctx.request.body).toEqual(expectedBody); + });
73-104: Consider adding test for malformed input dataThe tests cover the happy path scenarios when headers are present or absent, but don't test how the middleware handles malformed or incomplete input data. Adding a test for these edge cases would improve robustness.
it('should handle malformed input data gracefully', async () => { const ctx = { request: { body: { input: [{ metadata: { /* missing properties */ } }], destinations: {}, connections: {}, destType: 'testDestination', } as RouterTransformationRequestV2, }, } as Koa.Context; ctx.get = jest.fn(() => 'json+compactedv1'); await RouterTransformCompactedPayloadV1Middleware.handle(ctx, async () => { // Check that the middleware doesn't throw an error expect(ctx.request.body).toBeDefined(); }); });src/types/destinationTransformation.ts (3)
9-13: Add documentation for the new typeThis new type lacks documentation explaining its purpose, usage, and relationship to the original
ProcessorTransformationRequesttype. Adding JSDoc comments would improve code clarity and maintainability.+/** + * Processor transformation request structure for compacted payloads (v2) + * Used when the 'x-content-format' header is set to 'json+compactedv1' + * This format removes redundancy by storing destinations and connections as lookup tables + */ export type ProcessorTransformationRequestV2<M = RudderMessage, MD = Metadata> = { input: Omit<ProcessorTransformationRequest<M, MD>, 'connection' | 'destination'>[]; destinations: Record<string, Destination>; connections: Record<string, Connection>; };
107-112: Add documentation for the new typeSimilar to
ProcessorTransformationRequestV2, this type also needs documentation to explain its purpose and relationship toRouterTransformationRequest.+/** + * Router transformation request structure for compacted payloads (v2) + * Used when the 'x-content-format' header is set to 'json+compactedv1' + * This format removes redundancy by storing destinations and connections as lookup tables + */ export type RouterTransformationRequestV2<M = RudderMessage, MD = Metadata> = { input: Omit<RouterTransformationRequestData<M, MD>, 'destination' | 'connection'>[]; destType: string; connections: Record<string, Connection>; destinations: Record<string, Destination>; };
107-112: Consider adding key structure information for the connection recordThe key structure for connections (
${sourceId}:${destinationId}) is implementation-specific and not enforced in the type. Consider adding a comment or using a more specific type definition.export type RouterTransformationRequestV2<M = RudderMessage, MD = Metadata> = { input: Omit<RouterTransformationRequestData<M, MD>, 'destination' | 'connection'>[]; destType: string; - connections: Record<string, Connection>; + // Keys are in the format "${sourceId}:${destinationId}" + connections: Record<string, Connection>; destinations: Record<string, Destination>; };src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts (5)
9-9: Rename describe block to match the middleware nameThe describe block name
DestTransformV2PayloadMiddlewaredoesn't match the middleware being tested (DestTransformCompactedPayloadV1Middleware). This mismatch could cause confusion.-describe('DestTransformV2PayloadMiddleware', () => { +describe('DestTransformCompactedPayloadV1Middleware', () => {
60-62: Add await for async function callThe middleware's handle method is being called with an async callback, but there's no await. This could lead to test failures or race conditions if the assertion completes before the middleware finishes.
- DestTransformCompactedPayloadV1Middleware.handle(ctx, async () => { - expect(ctx.request.body).toEqual(originalBody); - }); + await DestTransformCompactedPayloadV1Middleware.handle(ctx, async () => { + expect(ctx.request.body).toEqual(originalBody); + });
76-93: Reuse existing test object to reduce duplicationThe test is reconstructing the input object instead of reusing the one defined earlier. This creates unnecessary duplication.
body: { input: [ - { - message: { type: 'track' }, - metadata: { - sourceId: 'sourceId', - destinationId: 'destinationId', - messageId: 'messageId', - workspaceId: 'workspaceId', - sourceType: 'sourceType', - sourceCategory: 'sourceCategory', - jobId: 1, - destinationType: 'destinationType', - }, - libraries: [], - credentials: [], - }, + input, ],
105-107: Add await for async function callSimilar to the previous test, this async function call should also have an await.
- DestTransformCompactedPayloadV1Middleware.handle(ctx, async () => { - expect(ctx.request.body).toEqual(expectedBody); - }); + await DestTransformCompactedPayloadV1Middleware.handle(ctx, async () => { + expect(ctx.request.body).toEqual(expectedBody); + });
45-108: Consider adding test for edge casesThe tests only cover the happy path scenarios. Consider adding tests for edge cases such as:
- Missing metadata properties in the input
- Missing entries in the destinations or connections lookup tables
- Malformed connection keys
This would ensure the middleware gracefully handles incomplete or incorrect data.
it('should handle missing connection gracefully', async () => { const ctx = { request: { body: { input: [input], destinations: { destinationId: destination, }, connections: { // Missing connection for 'sourceId:destinationId' }, } as ProcessorTransformationRequestV2, }, } as Koa.Context; ctx.get = jest.fn(() => 'json+compactedv1'); await DestTransformCompactedPayloadV1Middleware.handle(ctx, async () => { // Check that the middleware doesn't throw and sets connection to undefined expect(ctx.request.body[0].connection).toBeUndefined(); }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d3c6937 and b63382d2a8c8a0120fb22e0accf8caac70c5439f.
📒 Files selected for processing (7)
src/features.ts(2 hunks)src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts(1 hunks)src/middlewares/__tests__/routerTransformCompactedPayloadV1.test.ts(1 hunks)src/middlewares/destTransformCompactedPayloadV1.ts(1 hunks)src/middlewares/routerTransformCompactedPayloadV1.ts(1 hunks)src/routes/destination.ts(2 hunks)src/types/destinationTransformation.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
src/routes/destination.ts (2)
src/middlewares/destTransformCompactedPayloadV1.ts (1)
DestTransformCompactedPayloadV1Middleware(11-23)src/middlewares/routerTransformCompactedPayloadV1.ts (1)
RouterTransformCompactedPayloadV1Middleware(11-27)
src/features.ts (2)
src/sources/adjust/transform.js (1)
process(37-43)src/v0/destinations/topsort/transform.js (1)
process(92-114)
src/middlewares/destTransformCompactedPayloadV1.ts (1)
src/types/destinationTransformation.ts (2)
ProcessorTransformationRequestV2(9-13)ProcessorTransformationRequest(18-26)
src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts (2)
src/types/destinationTransformation.ts (2)
ProcessorTransformationRequest(18-26)ProcessorTransformationRequestV2(9-13)src/middlewares/destTransformCompactedPayloadV1.ts (1)
DestTransformCompactedPayloadV1Middleware(11-23)
src/middlewares/__tests__/routerTransformCompactedPayloadV1.test.ts (2)
src/types/destinationTransformation.ts (2)
RouterTransformationRequest(102-105)RouterTransformationRequestV2(107-112)src/middlewares/routerTransformCompactedPayloadV1.ts (1)
RouterTransformCompactedPayloadV1Middleware(11-27)
src/types/destinationTransformation.ts (2)
src/types/rudderEvents.ts (2)
RudderMessage(42-42)Metadata(89-89)src/types/controlPlaneConfig.ts (2)
Destination(11-22)Connection(28-34)
🪛 Biome (1.9.4)
src/middlewares/routerTransformCompactedPayloadV1.ts
[error] 11-27: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/middlewares/destTransformCompactedPayloadV1.ts
[error] 11-23: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ingestion-service-test
🔇 Additional comments (9)
src/features.ts (2)
7-7: Feature flag addition looks good.The new feature flag
supportDestTransformCompactedPayloadV1follows the established naming convention and clearly communicates its purpose. This correctly supports the compacted payload feature as described in the PR objectives.
118-119: Feature flag configuration is properly implemented.The default value for
supportDestTransformCompactedPayloadV1is set totrue, which means the feature will be enabled by default. This aligns with the PR objective to introduce the compacted payload support. The trailing comma added to the previous property is also good for consistency.src/routes/destination.ts (4)
6-7: Clean import statements for the new middleware components.The imports follow the project's standard import pattern and correctly reference the newly created middleware classes.
11-18: Middleware correctly added to destination transformation route.The
DestTransformCompactedPayloadV1Middleware.handleis correctly inserted in the middleware chain after the feature flag check but before the controller method. This ensures that the payload transformation happens at the right time in the request processing flow.
19-26: Appropriate middleware addition for router transform endpoint.The
RouterTransformCompactedPayloadV1Middleware.handleis properly integrated into the/routerTransformroute, allowing compacted payloads to be processed correctly at this endpoint.
27-34: Batch route correctly updated with compacted payload support.The same middleware is appropriately added to the
/batchendpoint, maintaining consistency across the relevant endpoints that need to support this feature.src/middlewares/destTransformCompactedPayloadV1.ts (1)
1-10: Well-documented middleware implementation.The import statements and JSDoc comments provide clear context about the purpose of this middleware: transforming compacted payloads by reconstructing connection and destination details. This aligns with the PR objectives.
src/middlewares/routerTransformCompactedPayloadV1.ts (2)
1-10: Well-documented middleware with clear purpose.The import statements and JSDoc comments provide good context about the purpose of this middleware for router transformations. The documentation effectively explains how compacted payloads are structured and processed.
13-24:Details
✅ Verification successful
Verify header name case sensitivity.
The code checks for the header
x-content-formatwhile the PR description mentionsX-Content-Format. HTTP headers are generally case-insensitive, but it's worth verifying that Koa'sctx.get()method handles case-insensitivity correctly.
🏁 Script executed:
#!/bin/bash # Check if there are any other places in the codebase where header case conventions are established rg -i "content.?format" --type ts --no-filenameLength of output: 341
Header Name Consistency Verified
The verification confirms that the codebase consistently uses the lower-case header
'x-content-format', which is appropriate since Koa normalizes HTTP headers to lower-case. Therefore, despite the PR description mentioningX-Content-Format, the current implementation usingctx.get('x-content-format')correctly handles header case sensitivity.No modifications are required in this area.
atzoum
force-pushed
the
feat.dtransformCompaction
branch
2 times, most recently
from
f606aba to
99d2b50
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/middlewares/routerTransformCompactedPayloadV1.ts (2)
11-27: Consider refactoring the class to a simple functionThe middleware implementation is clean and clear, but it's implemented as a class with only static members. Consider refactoring to a simple function to follow best practices and simplify the code.
-export class RouterTransformCompactedPayloadV1Middleware { - public static async handle(ctx: Context, next: Next): Promise<void> { +export async function handleRouterTransformCompactedPayloadV1(ctx: Context, next: Next): Promise<void> { if (ctx.get('x-content-format') === 'json+compactedv1') { const body = ctx.request.body as RouterTransformationRequestV2; ctx.request.body = { input: body.input.map((input) => ({ ...input, connection: body.connections[`${input.metadata.sourceId}:${input.metadata.destinationId}`], destination: body.destinations[input.metadata.destinationId], })), destType: body.destType, } as RouterTransformationRequest; } return next(); - } -} +}🧰 Tools
🪛 Biome (1.9.4)
[error] 11-27: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
13-24: Add error handling for missing or malformed dataThe middleware currently assumes the input data has the expected structure. Consider adding error handling to manage cases where
metadata.sourceId,metadata.destinationId, or the corresponding lookup values inconnectionsanddestinationsare missing.if (ctx.get('x-content-format') === 'json+compactedv1') { const body = ctx.request.body as RouterTransformationRequestV2; + try { ctx.request.body = { input: body.input.map((input) => ({ ...input, connection: body.connections[`${input.metadata.sourceId}:${input.metadata.destinationId}`], destination: body.destinations[input.metadata.destinationId], })), destType: body.destType, } as RouterTransformationRequest; + } catch (error) { + console.error('Error processing compacted payload:', error); + // Maintain original request body or set appropriate error status + } }src/middlewares/destTransformCompactedPayloadV1.ts (2)
11-23: Consider refactoring the class to a simple functionSimilar to the router middleware, this class contains only static members. Consider refactoring to a simple function to follow best practices and simplify the code.
-export class DestTransformCompactedPayloadV1Middleware { - public static async handle(ctx: Context, next: Next): Promise<void> { +export async function handleDestTransformCompactedPayloadV1(ctx: Context, next: Next): Promise<void> { if (ctx.get('x-content-format') === 'json+compactedv1') { const body = ctx.request.body as ProcessorTransformationRequestV2; ctx.request.body = body.input.map((input) => ({ ...input, connection: body.connections[`${input.metadata.sourceId}:${input.metadata.destinationId}`], destination: body.destinations[input.metadata.destinationId], })) as ProcessorTransformationRequest[]; } return next(); - } -} +}🧰 Tools
🪛 Biome (1.9.4)
[error] 11-23: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
13-19: Add error handling for missing or malformed dataThe middleware should handle cases where the expected structure is not present, especially when accessing nested properties like
input.metadata.sourceIdandinput.metadata.destinationId.if (ctx.get('x-content-format') === 'json+compactedv1') { const body = ctx.request.body as ProcessorTransformationRequestV2; + try { ctx.request.body = body.input.map((input) => ({ ...input, connection: body.connections[`${input.metadata.sourceId}:${input.metadata.destinationId}`], destination: body.destinations[input.metadata.destinationId], })) as ProcessorTransformationRequest[]; + } catch (error) { + console.error('Error processing compacted payload:', error); + // Maintain original request body or set appropriate error status + } }src/types/destinationTransformation.ts (2)
9-13: Add documentation on the purpose and usage of the compacted structureWhile the type definition is clear, adding more detailed documentation would help future developers understand the purpose and benefits of this compacted structure.
+/** + * Processor transformation request with compacted payload structure. + * This removes connection and destination details from individual input events + * and stores them in separate lookup tables for improved performance. + */ export type ProcessorTransformationRequestV2<M = RudderMessage, MD = Metadata> = { input: Omit<ProcessorTransformationRequest<M, MD>, 'connection' | 'destination'>[]; destinations: Record<string, Destination>; connections: Record<string, Connection>; };
107-112: Add documentation on the purpose and usage of the compacted structureSimilar to the ProcessorTransformationRequestV2 type, adding more detailed documentation would improve clarity.
+/** + * Router transformation request with compacted payload structure. + * This removes connection and destination details from individual input events + * and stores them in separate lookup tables for improved performance. + */ export type RouterTransformationRequestV2<M = RudderMessage, MD = Metadata> = { input: Omit<RouterTransformationRequestData<M, MD>, 'destination' | 'connection'>[]; destType: string; connections: Record<string, Connection>; destinations: Record<string, Destination>; };src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts (2)
65-108: Add test cases for error handling and edge casesThe current test verifies the happy path, but consider adding tests for error scenarios. For example, test what happens when:
- The input metadata is missing sourceId or destinationId
- The connections or destinations lookup tables don't contain expected keys
- The input structure is malformed
This would ensure the middleware handles unexpected inputs gracefully.
9-44: Consider using more descriptive test data variablesThe test data setup is comprehensive, but could benefit from more descriptive variable names or comments explaining the structure of the test data. This would make it easier to understand the test scenarios without having to analyze the data structure.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between b63382d2a8c8a0120fb22e0accf8caac70c5439f and f606aba0a8087498bba446f896b221c314dd9841.
📒 Files selected for processing (7)
src/features.ts(2 hunks)src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts(1 hunks)src/middlewares/__tests__/routerTransformCompactedPayloadV1.test.ts(1 hunks)src/middlewares/destTransformCompactedPayloadV1.ts(1 hunks)src/middlewares/routerTransformCompactedPayloadV1.ts(1 hunks)src/routes/destination.ts(2 hunks)src/types/destinationTransformation.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/features.ts
- src/routes/destination.ts
- src/middlewares/tests/routerTransformCompactedPayloadV1.test.ts
🧰 Additional context used
🧬 Code Definitions (2)
src/types/destinationTransformation.ts (2)
src/types/rudderEvents.ts (2)
RudderMessage(42-42)Metadata(89-89)src/types/controlPlaneConfig.ts (2)
Destination(11-22)Connection(28-34)
src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts (2)
src/types/destinationTransformation.ts (2)
ProcessorTransformationRequest(18-26)ProcessorTransformationRequestV2(9-13)src/middlewares/destTransformCompactedPayloadV1.ts (1)
DestTransformCompactedPayloadV1Middleware(11-23)
🪛 Biome (1.9.4)
src/middlewares/destTransformCompactedPayloadV1.ts
[error] 11-23: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/middlewares/routerTransformCompactedPayloadV1.ts
[error] 11-27: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build User Transformer Docker Image - PR / Build Transformer Docker Image AMD64
- GitHub Check: Build User Transformer Docker Image - PR / Build Transformer Docker Image ARM64
- GitHub Check: Build Transformer Docker Image - PR / Build Transformer Docker Image AMD64
- GitHub Check: Build Transformer Docker Image - PR / Build Transformer Docker Image ARM64
- GitHub Check: Code Coverage
- GitHub Check: ingestion-service-test
- GitHub Check: test_and_publish
- GitHub Check: UT Tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts (1)
45-63: LGTM!The test case correctly verifies that the middleware doesn't alter the body when the header is not present.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/middlewares/routerTransformCompactedPayloadV1.ts (1)
11-27: Consider replacing class with a standalone functionThe middleware is implemented as a class with only static methods. This approach adds unnecessary complexity. Consider refactoring to a simple function for better maintainability.
-export class RouterTransformCompactedPayloadV1Middleware { - public static async handle(ctx: Context, next: Next): Promise<void> { +export async function handleRouterTransformCompactedPayloadV1(ctx: Context, next: Next): Promise<void> { if (ctx.get('x-content-format') === 'json+compactedv1') { const body = ctx.request.body as RouterCompactedTransformationRequest; ctx.request.body = { input: body.input.map((input) => ({ ...input, connection: body.connections[`${input.metadata.sourceId}:${input.metadata.destinationId}`], destination: body.destinations[input.metadata.destinationId], })), destType: body.destType, } as RouterTransformationRequest; } return next(); - } -} +}🧰 Tools
🪛 Biome (1.9.4)
[error] 11-27: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/middlewares/destTransformCompactedPayloadV1.ts (1)
11-23: Consider replacing class with a standalone functionSimilar to the router middleware, this class contains only static methods. Refactor to a simple function for better maintainability and consistency with best practices.
-export class DestTransformCompactedPayloadV1Middleware { - public static async handle(ctx: Context, next: Next): Promise<void> { +export async function handleDestTransformCompactedPayloadV1(ctx: Context, next: Next): Promise<void> { if (ctx.get('x-content-format') === 'json+compactedv1') { const body = ctx.request.body as ProcessorCompactedTransformationRequest; ctx.request.body = body.input.map((input) => ({ ...input, connection: body.connections[`${input.metadata.sourceId}:${input.metadata.destinationId}`], destination: body.destinations[input.metadata.destinationId], })) as ProcessorTransformationRequest[]; } return next(); - } -} +}🧰 Tools
🪛 Biome (1.9.4)
[error] 11-23: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts (1)
12-47: Add test for error handling scenariosThe tests cover the happy path scenarios but lack tests for error cases like missing connection or destination keys in the lookup tables.
Consider adding tests for error scenarios to ensure robust error handling:
it('should handle missing connection in lookup table', async () => { const ctx = { request: { body: { input: [ { message: { type: 'track' }, metadata: { sourceId: 'unknownSource', destinationId: 'destinationId', // other metadata }, // other properties }, ], destinations: { destinationId: destination, }, connections: { // missing connection for 'unknownSource:destinationId' }, } as ProcessorCompactedTransformationRequest, }, } as Koa.Context; ctx.get = jest.fn(() => 'json+compactedv1'); const consoleSpy = jest.spyOn(console, 'warn').mockImplementation(); await DestTransformCompactedPayloadV1Middleware.handle(ctx, async () => { expect(consoleSpy).toHaveBeenCalled(); expect(ctx.request.body[0].connection).toBeUndefined(); }); consoleSpy.mockRestore(); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between f606aba0a8087498bba446f896b221c314dd9841 and 99d2b500c4658be212cea52fc31a135282f0c628.
📒 Files selected for processing (7)
src/features.ts(2 hunks)src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts(1 hunks)src/middlewares/__tests__/routerTransformCompactedPayloadV1.test.ts(1 hunks)src/middlewares/destTransformCompactedPayloadV1.ts(1 hunks)src/middlewares/routerTransformCompactedPayloadV1.ts(1 hunks)src/routes/destination.ts(2 hunks)src/types/destinationTransformation.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/features.ts
- src/middlewares/tests/routerTransformCompactedPayloadV1.test.ts
- src/routes/destination.ts
🧰 Additional context used
🧬 Code Definitions (2)
src/types/destinationTransformation.ts (2)
src/types/rudderEvents.ts (2)
RudderMessage(42-42)Metadata(89-89)src/types/controlPlaneConfig.ts (2)
Destination(11-22)Connection(28-34)
src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts (2)
src/types/destinationTransformation.ts (2)
ProcessorTransformationRequest(18-26)ProcessorCompactedTransformationRequest(9-13)src/middlewares/destTransformCompactedPayloadV1.ts (1)
DestTransformCompactedPayloadV1Middleware(11-23)
🪛 Biome (1.9.4)
src/middlewares/routerTransformCompactedPayloadV1.ts
[error] 11-27: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/middlewares/destTransformCompactedPayloadV1.ts
[error] 11-23: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Code Coverage
- GitHub Check: Build User Transformer Docker Image - PR / Build Transformer Docker Image ARM64
- GitHub Check: Build User Transformer Docker Image - PR / Build Transformer Docker Image AMD64
- GitHub Check: Build Transformer Docker Image - PR / Build Transformer Docker Image AMD64
- GitHub Check: Build Transformer Docker Image - PR / Build Transformer Docker Image ARM64
- GitHub Check: ingestion-service-test
- GitHub Check: UT Tests
- GitHub Check: test_and_publish
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
src/types/destinationTransformation.ts (2)
9-13: LGTM! Type definition for ProcessorCompactedTransformationRequestThe type properly defines the structure for compacted transformation requests, utilizing the
Omitutility type to derive from existing types while excluding specific properties.
107-112: LGTM! Type definition for RouterCompactedTransformationRequestThe type correctly defines the structure for router compacted transformation requests, maintaining consistency with the processor compacted type pattern.
src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts (3)
8-10: LGTM! Proper test cleanupGood practice to clear mocks after each test to prevent test pollution.
48-66: LGTM! Comprehensive test for unaltered bodyThe test correctly verifies that the middleware doesn't modify the request body when the header is not present.
68-111: LGTM! Thorough test for payload transformationThe test correctly verifies that the middleware properly transforms the compacted payload when the header is present. The test data is comprehensive and matches the expected transformation outcome.
|
Allure Test reports for this run are available at:
|
atzoum
force-pushed
the
feat.dtransformCompaction
branch
from
99d2b50 to
1c58d87
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/middlewares/destTransformCompactedPayloadV1.ts (1)
12-22:⚠️ Potential issueAdd error handling for missing lookup keys
The middleware doesn't handle cases where lookup keys don't exist in the connections or destinations records. If a key is missing, it will assign
undefinedto the corresponding property without any warning or error handling.Consider adding validation and error handling:
public static async handle(ctx: Context, next: Next): Promise<void> { if (ctx.get('x-content-format') === 'json+compactedv1') { const body = ctx.request.body as ProcessorCompactedTransformationRequest; + + // Validate the compacted payload structure + if (!body.input || !body.connections || !body.destinations) { + ctx.status = 400; + ctx.body = { error: 'Invalid compacted payload structure' }; + return next(); + } + ctx.request.body = body.input.map((input) => ({ ...input, - connection: body.connections[`${input.metadata.sourceId}:${input.metadata.destinationId}`], - destination: body.destinations[input.metadata.destinationId], + connection: body.connections[`${input.metadata.sourceId}:${input.metadata.destinationId}`] || + (console.warn(`Missing connection for sourceId:${input.metadata.sourceId}, destinationId:${input.metadata.destinationId}`), undefined), + destination: body.destinations[input.metadata.destinationId] || + (console.warn(`Missing destination for destinationId:${input.metadata.destinationId}`), undefined), })) as ProcessorTransformationRequest[]; } return next(); }src/middlewares/routerTransformCompactedPayloadV1.ts (1)
12-26:⚠️ Potential issueAdd error handling for missing lookup keys
The middleware doesn't handle cases where lookup keys don't exist in the connections or destinations records. If a key is missing, it will assign
undefinedto the corresponding property without any warning or error handling.Consider adding validation and error handling:
public static async handle(ctx: Context, next: Next): Promise<void> { if (ctx.get('x-content-format') === 'json+compactedv1') { const body = ctx.request.body as RouterCompactedTransformationRequest; + + // Validate the compacted payload structure + if (!body.input || !body.connections || !body.destinations) { + ctx.status = 400; + ctx.body = { error: 'Invalid compacted payload structure' }; + return next(); + } + ctx.request.body = { input: body.input.map((input) => ({ ...input, - connection: - body.connections[`${input.metadata.sourceId}:${input.metadata.destinationId}`], - destination: body.destinations[input.metadata.destinationId], + connection: body.connections[`${input.metadata.sourceId}:${input.metadata.destinationId}`] || + (console.warn(`Missing connection for sourceId:${input.metadata.sourceId}, destinationId:${input.metadata.destinationId}`), undefined), + destination: body.destinations[input.metadata.destinationId] || + (console.warn(`Missing destination for destinationId:${input.metadata.destinationId}`), undefined), })), destType: body.destType, } as RouterTransformationRequest; } return next(); }
🧹 Nitpick comments (2)
src/middlewares/destTransformCompactedPayloadV1.ts (1)
11-23: Convert static class to a simple functionThe middleware is implemented as a class with only static members, which is discouraged by static analysis tools.
Consider refactoring to use a simple function instead:
-export class DestTransformCompactedPayloadV1Middleware { - public static async handle(ctx: Context, next: Next): Promise<void> { +export async function handleDestTransformCompactedPayloadV1(ctx: Context, next: Next): Promise<void> { if (ctx.get('x-content-format') === 'json+compactedv1') { const body = ctx.request.body as ProcessorCompactedTransformationRequest; ctx.request.body = body.input.map((input) => ({ ...input, connection: body.connections[`${input.metadata.sourceId}:${input.metadata.destinationId}`], destination: body.destinations[input.metadata.destinationId], })) as ProcessorTransformationRequest[]; } return next(); - } }🧰 Tools
🪛 Biome (1.9.4)
[error] 11-23: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/middlewares/routerTransformCompactedPayloadV1.ts (1)
11-27: Convert static class to a simple functionThe middleware is implemented as a class with only static members, which is discouraged by static analysis tools.
Consider refactoring to use a simple function instead:
-export class RouterTransformCompactedPayloadV1Middleware { - public static async handle(ctx: Context, next: Next): Promise<void> { +export async function handleRouterTransformCompactedPayloadV1(ctx: Context, next: Next): Promise<void> { if (ctx.get('x-content-format') === 'json+compactedv1') { const body = ctx.request.body as RouterCompactedTransformationRequest; ctx.request.body = { input: body.input.map((input) => ({ ...input, connection: body.connections[`${input.metadata.sourceId}:${input.metadata.destinationId}`], destination: body.destinations[input.metadata.destinationId], })), destType: body.destType, } as RouterTransformationRequest; } return next(); - } }🧰 Tools
🪛 Biome (1.9.4)
[error] 11-27: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 99d2b500c4658be212cea52fc31a135282f0c628 and 1c58d875195ae45e0598ee2229723ba2da196b3e.
📒 Files selected for processing (7)
src/features.ts(2 hunks)src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts(1 hunks)src/middlewares/__tests__/routerTransformCompactedPayloadV1.test.ts(1 hunks)src/middlewares/destTransformCompactedPayloadV1.ts(1 hunks)src/middlewares/routerTransformCompactedPayloadV1.ts(1 hunks)src/routes/destination.ts(2 hunks)src/types/destinationTransformation.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/middlewares/tests/routerTransformCompactedPayloadV1.test.ts
- src/routes/destination.ts
- src/features.ts
- src/middlewares/tests/destTransformCompactedPayloadV1.test.ts
🧰 Additional context used
🧬 Code Definitions (2)
src/types/destinationTransformation.ts (2)
src/types/rudderEvents.ts (2)
RudderMessage(42-42)Metadata(89-89)src/types/controlPlaneConfig.ts (2)
Destination(11-22)Connection(28-34)
src/middlewares/destTransformCompactedPayloadV1.ts (1)
src/types/destinationTransformation.ts (2)
ProcessorCompactedTransformationRequest(9-13)ProcessorTransformationRequest(18-26)
🪛 Biome (1.9.4)
src/middlewares/destTransformCompactedPayloadV1.ts
[error] 11-23: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
src/middlewares/routerTransformCompactedPayloadV1.ts
[error] 11-27: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Build Transformer Docker Image - PR / Build Transformer Docker Image AMD64
- GitHub Check: Build Transformer Docker Image - PR / Build Transformer Docker Image ARM64
- GitHub Check: Build User Transformer Docker Image - PR / Build Transformer Docker Image AMD64
- GitHub Check: Build User Transformer Docker Image - PR / Build Transformer Docker Image ARM64
- GitHub Check: Code Coverage
- GitHub Check: test_and_publish
- GitHub Check: ingestion-service-test
- GitHub Check: UT Tests
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
src/types/destinationTransformation.ts (2)
9-13: Type definitions look goodThe
ProcessorCompactedTransformationRequesttype is well-structured for the compacted payload format, properly omitting theconnectionanddestinationfields from individual items and providing them as lookup tables instead.
107-112: Type definitions look goodThe
RouterCompactedTransformationRequesttype is properly structured for the router transformation context, correctly omitting thedestinationandconnectionfields from individual input items and providing them as lookup records instead.
|
Allure Test reports for this run are available at:
|
atzoum
force-pushed
the
feat.dtransformCompaction
branch
from
35cda1e to
479db27
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/middlewares/destTransformCompactedPayloadV1.ts (1)
11-24: 🛠️ Refactor suggestionAdd error handling for missing lookup data
Similar to the router middleware, this implementation doesn't validate that connection and destination keys exist in the lookup tables, which could lead to undefined properties and errors downstream.
export async function DestTransformCompactedPayloadV1Middleware( ctx: Context, next: Next, ): Promise<void> { if (ctx.get('x-content-format') === 'json+compactedv1') { const body = ctx.request.body as ProcessorCompactedTransformationRequest; + + // Basic validation of the compacted payload structure + if (!body.input || !Array.isArray(body.input)) { + throw new Error('Invalid compacted payload: missing or invalid input array'); + } + + if (!body.connections || typeof body.connections !== 'object') { + throw new Error('Invalid compacted payload: missing or invalid connections lookup'); + } + + if (!body.destinations || typeof body.destinations !== 'object') { + throw new Error('Invalid compacted payload: missing or invalid destinations lookup'); + } + + try { ctx.request.body = body.input.map((input) => { + const connectionKey = `${input.metadata.sourceId}:${input.metadata.destinationId}`; + const connection = body.connections[connectionKey]; + const destination = body.destinations[input.metadata.destinationId]; + + if (!destination) { + throw new Error(`Missing destination for destinationId:${input.metadata.destinationId}`); + } + return { ...input, - connection: body.connections[`${input.metadata.sourceId}:${input.metadata.destinationId}`], - destination: body.destinations[input.metadata.destinationId], + connection, + destination, }; })) as ProcessorTransformationRequest[]; + } catch (error) { + throw error; + } } await next(); }
🧹 Nitpick comments (2)
src/middlewares/routerTransformCompactedPayloadV1.ts (1)
1-27: Add logging for errors and operationsAdd logging statements to track the middleware's operations and any errors that occur. This will help with debugging and monitoring.
import { Context, Next } from 'koa'; import { RouterTransformationRequest, RouterCompactedTransformationRequest } from '../types'; + import logger from '../logger'; /** * Middleware to transform a compacted payload for destination transformations * to its decompacted form, as it is expected by the destination transformers. * Compacted payloads are used to reduce the size of the payload by removing * connection and destination details from each input and instead providing them * in separate lookup tables. */ export async function RouterTransformCompactedPayloadV1Middleware( ctx: Context, next: Next, ): Promise<void> { if (ctx.get('x-content-format') === 'json+compactedv1') { + logger.debug('Processing compacted payload format (router transform)'); const body = ctx.request.body as RouterCompactedTransformationRequest; // ... validation code from previous suggestion ... try { ctx.request.body = { input: body.input.map((input) => { // ... connection and destination validation from previous suggestion ... return { ...input, connection, destination, }; })), destType: body.destType, } as RouterTransformationRequest; + logger.debug('Successfully transformed compacted payload format'); + } catch (error) { + logger.error(`Error transforming compacted payload: ${error.message}`); + throw error; + } } await next(); }src/middlewares/destTransformCompactedPayloadV1.ts (1)
1-24: Add logging for better observabilityAdd logging to track the middleware's operations and any errors that occur, which will be helpful for debugging and monitoring.
import { Context, Next } from 'koa'; import { ProcessorCompactedTransformationRequest, ProcessorTransformationRequest } from '../types'; + import logger from '../logger'; /** * Middleware to transform a compacted payload for destination transformations * to its decompacted form, as it is expected by the destination transformers. * Compacted payloads are used to reduce the size of the payload by removing * connection and destination details from each input and instead providing them * in separate lookup tables. */ export async function DestTransformCompactedPayloadV1Middleware( ctx: Context, next: Next, ): Promise<void> { if (ctx.get('x-content-format') === 'json+compactedv1') { + logger.debug('Processing compacted payload format (destination transform)'); const body = ctx.request.body as ProcessorCompactedTransformationRequest; // ... validation code from previous suggestion ... try { ctx.request.body = body.input.map((input) => { // ... connection and destination validation ... return { ...input, connection, destination, }; })) as ProcessorTransformationRequest[]; + logger.debug('Successfully transformed compacted payload format'); } catch (error) { + logger.error(`Error transforming compacted payload: ${error.message}`); throw error; } } await next(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 35cda1ed1b49eee6548a55d04d0711dff83055a6 and 479db27e2ab4c62debf5cb515580ea80611ab65a.
📒 Files selected for processing (7)
src/features.ts(2 hunks)src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts(1 hunks)src/middlewares/__tests__/routerTransformCompactedPayloadV1.test.ts(1 hunks)src/middlewares/destTransformCompactedPayloadV1.ts(1 hunks)src/middlewares/routerTransformCompactedPayloadV1.ts(1 hunks)src/routes/destination.ts(2 hunks)src/types/destinationTransformation.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/features.ts
- src/middlewares/tests/routerTransformCompactedPayloadV1.test.ts
- src/routes/destination.ts
🧰 Additional context used
🧠 Learnings (1)
src/middlewares/routerTransformCompactedPayloadV1.ts (1)
Learnt from: vinayteki95
PR: rudderlabs/rudder-transformer#4220
File: src/middlewares/routerTransformCompactedPayloadV1.ts:13-24
Timestamp: 2025-04-03T04:15:13.615Z
Learning: In the Rudder Transformer codebase, use generic Error objects instead of TransformationError when error monitoring with Bugsnag is needed, as TransformationError is configured to not trigger Bugsnag alerts.
🧬 Code Definitions (3)
src/middlewares/routerTransformCompactedPayloadV1.ts (1)
src/types/destinationTransformation.ts (2)
RouterCompactedTransformationRequest(107-115)RouterTransformationRequest(102-105)
src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts (2)
src/types/destinationTransformation.ts (2)
ProcessorTransformationRequest(18-26)ProcessorCompactedTransformationRequest(9-13)src/middlewares/destTransformCompactedPayloadV1.ts (1)
DestTransformCompactedPayloadV1Middleware(11-24)
src/types/destinationTransformation.ts (2)
src/types/rudderEvents.ts (2)
RudderMessage(42-42)Metadata(89-89)src/types/controlPlaneConfig.ts (2)
Destination(11-22)Connection(28-34)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Build User Transformer Docker Image - PR / Build Transformer Docker Image ARM64
- GitHub Check: Build User Transformer Docker Image - PR / Build Transformer Docker Image AMD64
- GitHub Check: Build Transformer Docker Image - PR / Build Transformer Docker Image ARM64
- GitHub Check: Build Transformer Docker Image - PR / Build Transformer Docker Image AMD64
- GitHub Check: Code Coverage
- GitHub Check: UT Tests
- GitHub Check: test_and_publish
- GitHub Check: ingestion-service-test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
src/types/destinationTransformation.ts (2)
9-13: LGTM: Good type definition for processor compacted transformation requestThe type definition for
ProcessorCompactedTransformationRequestcorrectly usesOmitto exclude the connection and destination properties from each input item while providing them as separate lookup tables.
107-115: LGTM: Good type definition for router compacted transformation requestThe type definition for
RouterCompactedTransformationRequestcorrectly usesOmitwith the proper generic parameter order to exclude the connection and destination properties from each input item while providing them as separate lookup tables.src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts (2)
8-10: Good practice using afterEach to clear mocksUsing
afterEachto clear mocks is a good practice for maintaining test isolation.
12-112: Add test cases for error scenariosThe tests only cover the happy path but don't test any error scenarios. Add test cases for situations where destinations or connections are missing from the lookup tables.
it('should throw error when destination is missing from lookup', async () => { const ctx = { request: { body: { input: [ { message: { type: 'track' }, metadata: { sourceId: 'sourceId', destinationId: 'missingDestinationId', // This ID doesn't exist in the destinations lookup messageId: 'messageId', workspaceId: 'workspaceId', sourceType: 'sourceType', sourceCategory: 'sourceCategory', jobId: 1, destinationType: 'destinationType', }, libraries: [], credentials: [], }, ], destinations: { destinationId: destination, // Note: No 'missingDestinationId' key }, connections: { 'sourceId:destinationId': connection, }, } as ProcessorCompactedTransformationRequest, }, } as Koa.Context; ctx.get = jest.fn((v) => (v === 'x-content-format' ? 'json+compactedv1' : '')); await expect( DestTransformCompactedPayloadV1Middleware(ctx, async () => {}) ).rejects.toThrow(/Missing destination/); }); it('should throw error when connections object is missing', async () => { const ctx = { request: { body: { input: [ { message: { type: 'track' }, metadata: { sourceId: 'sourceId', destinationId: 'destinationId', messageId: 'messageId', workspaceId: 'workspaceId', sourceType: 'sourceType', sourceCategory: 'sourceCategory', jobId: 1, destinationType: 'destinationType', }, libraries: [], credentials: [], }, ], destinations: { destinationId: destination, }, // Connections object is missing } as ProcessorCompactedTransformationRequest, }, } as Koa.Context; ctx.get = jest.fn((v) => (v === 'x-content-format' ? 'json+compactedv1' : '')); await expect( DestTransformCompactedPayloadV1Middleware(ctx, async () => {}) ).rejects.toThrow(/missing or invalid connections lookup/); });
|
Allure Test reports for this run are available at:
|
atzoum
force-pushed
the
feat.dtransformCompaction
branch
from
479db27 to
c138ea2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/middlewares/destTransformCompactedPayloadV1.ts (2)
20-21: Enhance validation for destinationIdThe current check
!destination && input.metadata.destinationIdwill pass ifdestinationIdis an empty string, which may not be intended. Consider strengthening this validation.-if (!destination && input.metadata.destinationId) { +if (!destination && input.metadata.destinationId && input.metadata.destinationId.trim() !== '') { throw new PlatformError(`no destination found for id ${input.metadata.destinationId}`, 500); }
19-22: Add validation for metadata propertiesThe code assumes that
input.metadata.sourceIdandinput.metadata.destinationIdalways exist. Adding validation for these properties would make the middleware more robust.const destination = body.destinations[input.metadata.destinationId]; +if (!input.metadata || !input.metadata.sourceId) { + throw new PlatformError('Missing required metadata.sourceId in input', 500); +} if (!destination && input.metadata.destinationId) { throw new PlatformError(`no destination found for id ${input.metadata.destinationId}`, 500); }src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts (2)
114-154: Consider using Jest's expect().toThrow() patternThe current try-catch error testing could be simplified using Jest's built-in
toThrow()matcher for a more concise test.- try { - await DestTransformCompactedPayloadV1Middleware(ctx, async () => { - fail('should not call next middleware'); - }); - } catch (error) { - expect(error).toBeInstanceOf(PlatformError); - expect((error as Error).message).toBe(`no destination found for id destinationId`); - } + await expect( + DestTransformCompactedPayloadV1Middleware(ctx, async () => { + fail('should not call next middleware'); + }) + ).rejects.toThrow(new PlatformError(`no destination found for id destinationId`, 500));
13-197: Consider adding tests for additional edge casesThe tests cover the basic scenarios, but consider adding tests for edge cases such as:
- Empty input arrays
- Missing metadata properties
- Invalid destinationId formats
These additional tests would enhance the robustness of your test suite.
src/middlewares/__tests__/routerTransformCompactedPayloadV1.test.ts (2)
10-45: Well-structured test fixturesThe test fixture data provides comprehensive mock objects for destination, connection, and input, which makes the tests clear and representative of real-world scenarios.
Consider extracting these fixtures to a separate helper file if they'll be reused across multiple test files.
102-127: Good error testing for missing destinationThis test correctly validates the error scenario when a destination is missing from the lookup. However, consider using Jest's
expect().toThrow()pattern for more concise error testing.- try { - await RouterTransformCompactedPayloadV1Middleware(ctx, async () => { - fail('should not call next middleware'); - }); - } catch (error) { - expect(error).toBeInstanceOf(PlatformError); - expect((error as Error).message).toBe(`no destination found for id destinationId`); - } + await expect(RouterTransformCompactedPayloadV1Middleware(ctx, async () => { + fail('should not call next middleware'); + })).rejects.toThrow(new PlatformError(`no destination found for id destinationId`, 500));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 479db27e2ab4c62debf5cb515580ea80611ab65a and c138ea2cba9f5362f8d39e4f5ea41780082831e6.
📒 Files selected for processing (7)
src/features.ts(2 hunks)src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts(1 hunks)src/middlewares/__tests__/routerTransformCompactedPayloadV1.test.ts(1 hunks)src/middlewares/destTransformCompactedPayloadV1.ts(1 hunks)src/middlewares/routerTransformCompactedPayloadV1.ts(1 hunks)src/routes/destination.ts(2 hunks)src/types/destinationTransformation.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/routes/destination.ts
- src/types/destinationTransformation.ts
- src/features.ts
🧰 Additional context used
🧬 Code Definitions (2)
src/middlewares/__tests__/routerTransformCompactedPayloadV1.test.ts (2)
src/types/destinationTransformation.ts (2)
RouterTransformationRequest(102-105)RouterCompactedTransformationRequest(107-115)src/middlewares/routerTransformCompactedPayloadV1.ts (1)
RouterTransformCompactedPayloadV1Middleware(12-42)
src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts (2)
src/types/destinationTransformation.ts (2)
ProcessorTransformationRequest(18-26)ProcessorCompactedTransformationRequest(9-13)src/middlewares/destTransformCompactedPayloadV1.ts (1)
DestTransformCompactedPayloadV1Middleware(12-36)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Build User Transformer Docker Image - PR / Build Transformer Docker Image AMD64
- GitHub Check: Build Transformer Docker Image - PR / Build Transformer Docker Image AMD64
- GitHub Check: Code Coverage
- GitHub Check: ingestion-service-test
- GitHub Check: UT Tests
- GitHub Check: test_and_publish
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (13)
src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts (3)
49-67: LGTM - Test correctly verifies middleware behavior without headerThe test properly verifies that the middleware doesn't modify the request body when the header is absent. Good job using
awaitwith the async middleware function.
69-112: LGTM - Test correctly verifies the transformation logicThis test effectively checks that the middleware transforms the compacted payload correctly when the header is present. The test data is comprehensive and the assertions are appropriate.
156-196: LGTM - Connection error test is comprehensiveThe test effectively verifies that an error is thrown when a connection is missing from the lookup. Good job on checking both the error type and message.
src/middlewares/routerTransformCompactedPayloadV1.ts (6)
1-4: Good organization of importsClean import structure with appropriate modules and types. The use of
PlatformErrorfrom the integrations library is a good choice for error handling in this middleware.
5-11: Well-documented middleware purposeExcellent JSDoc comments explaining the purpose and behavior of the middleware. The description clearly explains what compacted payloads are and why they're beneficial (reducing payload size by using lookup tables).
20-26: Proper error handling for missing destinationGood implementation of error handling for missing destinations, throwing a PlatformError with appropriate message and status code.
27-31: Proper error handling for missing connectionGood implementation of error handling for missing connections, using a consistent error handling approach with the destination check above.
32-37: Clean transformation of the input objectThe transformation logic is clean and preserves all original properties from the input while adding the connection and destination properties.
41-42: Correct middleware chain continuationThe
await next()ensures proper middleware chaining in Koa, allowing subsequent middleware to process the request.src/middlewares/__tests__/routerTransformCompactedPayloadV1.test.ts (4)
6-8: Good test cleanup practiceUsing
afterEachto clear mocks ensures that each test starts with a clean state, preventing potential side effects between tests.
46-67: Good test for unmodified behavior without headerThis test correctly verifies that the middleware doesn't alter the request body when the header is not present, ensuring backward compatibility.
69-100: Comprehensive test for transformation with headerThis test thoroughly validates that the middleware correctly transforms the compacted payload when the header is present, ensuring the correct mapping of connections and destinations.
129-154: Good error testing for missing connectionThis test correctly validates the error scenario when a connection is missing from the lookup. Similar to the previous comment, consider using Jest's
expect().toThrow()pattern.
atzoum
force-pushed
the
feat.dtransformCompaction
branch
from
c138ea2 to
b073158
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/types/destinationTransformation.ts (3)
9-13: Add JSDoc Comments for the New TypeThe
ProcessorCompactedTransformationRequesttype is well-structured for handling compacted payloads, but lacks documentation explaining its purpose and usage. Consider adding JSDoc comments similar to those for other types in this file.Add descriptive documentation:
+/** + * Processor compacted transformation request structure + * Used for optimized payloads where destination and connection fields + * are moved from individual events to lookup tables + */ export type ProcessorCompactedTransformationRequest<M = RudderMessage, MD = Metadata> = { input: Omit<ProcessorTransformationRequest<M, MD>, 'connection' | 'destination'>[]; destinations: Record<string, Destination>; connections: Record<string, Connection>; };
107-115: Add Documentation for RouterCompactedTransformationRequestThe
RouterCompactedTransformationRequesttype is correctly defined with proper generic parameters, but lacks documentation explaining its purpose and how it relates to the performance improvements mentioned in the PR description.Add JSDoc comments to improve code readability:
+/** + * Router compacted transformation request structure + * Optimizes payload size by moving destination and connection objects + * out of individual events and into lookup tables, improving + * marshalling/unmarshalling performance and network transfer times + */ export type RouterCompactedTransformationRequest<M = RudderMessage, MD = Metadata> = { input: Omit< RouterTransformationRequestData<M, Destination, Connection, MD>, 'destination' | 'connection' >[]; destType: string; connections: Record<string, Connection>; destinations: Record<string, Destination>; };
9-13: Consider Type Organization for Better ReadabilityBoth new compacted types are defined separately from their standard counterparts. For improved code organization and readability, consider grouping related types together.
Rearrange the types so that each compacted type follows immediately after its standard version. For example, place
ProcessorCompactedTransformationRequestafterProcessorTransformationRequestandRouterCompactedTransformationRequestafterRouterTransformationRequest.Also applies to: 107-115
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between c138ea2cba9f5362f8d39e4f5ea41780082831e6 and b0731586798059c0e7bfd8e6b77a44f9a2f41ffb.
📒 Files selected for processing (7)
src/features.ts(2 hunks)src/middlewares/__tests__/destTransformCompactedPayloadV1.test.ts(1 hunks)src/middlewares/__tests__/routerTransformCompactedPayloadV1.test.ts(1 hunks)src/middlewares/destTransformCompactedPayloadV1.ts(1 hunks)src/middlewares/routerTransformCompactedPayloadV1.ts(1 hunks)src/routes/destination.ts(2 hunks)src/types/destinationTransformation.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/middlewares/destTransformCompactedPayloadV1.ts
- src/features.ts
- src/middlewares/routerTransformCompactedPayloadV1.ts
- src/routes/destination.ts
- src/middlewares/tests/routerTransformCompactedPayloadV1.test.ts
- src/middlewares/tests/destTransformCompactedPayloadV1.test.ts
🧰 Additional context used
🧬 Code Definitions (1)
src/types/destinationTransformation.ts (2)
src/types/rudderEvents.ts (2)
RudderMessage(42-42)Metadata(89-89)src/types/controlPlaneConfig.ts (2)
Destination(11-22)Connection(28-34)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Code Coverage
- GitHub Check: Build User Transformer Docker Image - PR / Build Transformer Docker Image AMD64
- GitHub Check: Build Transformer Docker Image - PR / Build Transformer Docker Image AMD64
- GitHub Check: test_and_publish
- GitHub Check: UT Tests
- GitHub Check: ingestion-service-test
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
atzoum
force-pushed
the
feat.dtransformCompaction
branch
from
b073158 to
fc4d4d6
Compare
atzoum
force-pushed
the
feat.dtransformCompaction
branch
from
fc4d4d6 to
0711e78
Compare
|
Allure Test reports for this run are available at:
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/routes/destination.ts(3 hunks)src/routes/destination.ts.orig(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/routes/destination.ts
🧰 Additional context used
🪛 GitHub Actions: Verify
src/routes/destination.ts.orig
[error] 1-1: Parsing error: ESLint was configured to run on '/src/routes/destination.ts.orig' using 'parserOptions.project': /tsconfig.json. The extension for the file (.orig) is non-standard. It should be added to your existing parserOptions.extraFileExtensions.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Build Transformer Docker Image - PR / Build Transformer Docker Image AMD64
- GitHub Check: Build User Transformer Docker Image - PR / Build Transformer Docker Image AMD64
- GitHub Check: Build User Transformer Docker Image - PR / Build Transformer Docker Image ARM64
- GitHub Check: Code Coverage
- GitHub Check: ingestion-service-test
- GitHub Check: UT Tests
- GitHub Check: test_and_publish
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (2)
src/routes/destination.ts.orig (2)
15-22: Looks good for destination transformation route.
The route definition and middleware usage are correct.
23-44: Batch and delete routes appear properly structured.
The usage ofbatchProcessanddeleteUsersis appropriately routed. No issues found.
|
Allure Test reports for this run are available at:
|
|
# Description Related to rudderlabs/rudder-transformer#4220 From now on, as long as transformer service supports it and the relevant feature flag is enabled, rudder-server is going to send compacted transformation request payloads by making use of lookup tables instead of repeating the same destination and connection information in every event. The feature can be enabled through the following configuration options ```yaml Transformer: compactionEnabled: true # enables compaction in both processor and router as long as the remote transformer service supports it forceCompactionEnabled: true # enables compaction in both processor and router regardless if the remote transformer service supports it or not Router: DestinationTransformer: compactionEnabled: true # enables compaction in router as long as the remote transformer service supports it forceCompactionEnabled: true # enables compaction in router regardless if the remote transformer service supports it or not Processor: DestinationTransformer: compactionEnabled: true # enables it in processor as long as the remote transformer service supports it forceCompactionEnabled: true # enables compaction in processor regardless if the remote transformer service supports it or not ``` All three transformer endpoints are being tested for proper handling of compacted payloads through integration tests: - `POST /:version/destinations/:destination` in `processor_isolation_test.go` using a `WEBHOOK` destination - `POST /routerTransform` in `router_isolation_test.go` using a `WEBHOOK` destination - `POST /batch` in `kafka_batching_test.go` using a `KAFKA` destination We are making use of `RSERVER_TRANSFORMER_FORCE_COMPACTION_ENABLED` flag in our tests for ensuring that the `rudder-transformer` container we are using is indeed able to understand compacted payloads. _**Note:** tests are expected to fail until the required feature gets released in rudder-transformer._ (update: all ✅ now) ## Additional items - Trimming down user transformation payloads by stripping out empty metadata fields and zero destination and connection structs ## Linear Ticket Resolves PIPE-1992 ## Security - [x] The code changed/added as part of this pull request won't create any security issues with how the software is being used.
6 participants
What are the changes introduced in this PR?
Transformer now advertises support for a new feature, through which it supports compacted payloads for the following endpoints:
Clients wanting to take advantage of this feature, e.g. rudder-server, need to use a special HTTP request header
X-Content-Format: json+compactedv1along with the compacted payload in the request's body.Compacted payloads, omit the
destinationandconnectionfields from each individual event and include a lookup table instead, e.g.{ "input": [ { "message": {...}, "metadata": { "sourceId": "source-1", "destinationId": "destination-1" } }, { "message": {...}, "metadata": { "sourceId": "source-1", "destinationId": "destination-1" } },{ "message": {...}, "metadata": { "sourceId": "source-1", "destinationId": "destination-1" } },{ "message": {...}, "metadata": { "sourceId": "source-1", "destinationId": "destination-2" } } ], "destinations": { "destination-1": {...}, "destination-2": {...} }, "connections": { "source-1:destination-1": {...}, "source-1:destination-2": {...} } }This compaction can provide significant benefits, taking into consideration that destination objects are quite big and heavy, usually bigger than the messages themselves:
What is the related Linear task?
Resolves PIPE-1992