Fix run replication payloads/outputs trying to send invalid types like BigInt to clickhouse #2065
Conversation
…e BigInt to clickhouse
|
WalkthroughA new asynchronous function, Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant ReplicationService as RunsReplicationService
participant IO as ioSerialization (parsePacketAsJson)
participant ClickHouse as ClickHouse DB
Test->>ReplicationService: Insert TaskRun with superjson payload
ReplicationService->>IO: parsePacketAsJson(packet)
IO-->>ReplicationService: Return plain JSON from superjson
ReplicationService->>ClickHouse: Replicate TaskRun record
Test->>ClickHouse: Query for replicated TaskRun and payload
ClickHouse-->>Test: Return record and deserialized payload
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
Scope: all 2 workspace projects This error happened while installing a direct dependency of /tmp/eslint tsx is not in the npm registry, or you have no permission to fetch it. No authorization header was set for the request. ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/v3/utils/ioSerialization.ts (1)
44-72: Good implementation for handling superjson payloads properlyThe new
parsePacketAsJsonfunction effectively addresses the issue with BigInt and other complex data types by parsing the superjson data and then immediately serializing it back to plain JSON. This ensures that complex types like BigInt, which ClickHouse doesn't support, are properly converted to string representations.Consider wrapping switch case declarations in blocks to prevent scope leakage:
case "application/super+json": - const { parse, serialize } = await loadSuperJSON(); - - const superJsonResult = parse(value.data); - - const { json } = serialize(superJsonResult); + { + const { parse, serialize } = await loadSuperJSON(); + + const superJsonResult = parse(value.data); + + const { json } = serialize(superJsonResult); + + return json; + } - return json;🧰 Tools
🪛 Biome (1.9.4)
[error] 56-56: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 58-58: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 60-60: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
apps/webapp/app/services/runsReplicationService.server.ts(2 hunks)apps/webapp/test/runsReplicationService.test.ts(2 hunks)packages/core/src/v3/utils/ioSerialization.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/webapp/app/services/runsReplicationService.server.ts (2)
packages/core/src/v3/tryCatch.ts (1)
tryCatch(8-15)packages/core/src/v3/utils/ioSerialization.ts (1)
parsePacketAsJson(44-72)
packages/core/src/v3/utils/ioSerialization.ts (1)
packages/core/src/v3/index.ts (1)
IOPacket(66-66)
🪛 Biome (1.9.4)
packages/core/src/v3/utils/ioSerialization.ts
[error] 56-56: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 58-58: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 60-60: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
apps/webapp/app/services/runsReplicationService.server.ts (1)
639-639: Updated to use the new parsePacketAsJson functionThis change correctly implements the fix for BigInt and other complex types by using the newly created
parsePacketAsJsoninstead of the previousparsePacket. This ensures that complex types in superjson payloads will be properly transformed into plain JSON before being sent to ClickHouse.apps/webapp/test/runsReplicationService.test.ts (2)
9-9: Added superjson dependency for testingCorrectly imported the superjson library which is needed for the new test.
137-275: Great comprehensive test for superjson payload replicationThis test thoroughly validates the fix by:
- Creating a TaskRun with complex data types (BigInt, Date, Map) in the payload
- Serializing it with superjson and setting the appropriate payloadType
- Verifying the data is correctly replicated in ClickHouse with the complex types properly converted to their JSON-compatible representations
The test ensures that:
- BigInt values are converted to strings (e.g., "1234")
- Date objects are converted to ISO strings
- Maps are converted to arrays of key-value pairs
This provides excellent coverage for the fix implemented in the PR.
Summary by CodeRabbit