Backend test improvements#1765
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a timing wrapper and worker DB setup, refactors test seeding to bulk/batch operations, adjusts test lifecycle and short initialization delays, updates test scripts to use the wrapper and AVA concurrency, and configures many test DB services to use tmpfs/in-memory storage. ChangesTest Performance & Parallelization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Pull request overview
This PR focuses on speeding up and stabilizing backend E2E tests by reducing database I/O overhead, batching seed inserts, and enabling higher test concurrency with AVA, including per-worker DB setup and timing output.
Changes:
- Mount DB data directories on
tmpfsin compose files to reduce disk I/O during tests. - Batch/parallelize test data seeding across multiple DB backends (Knex batch inserts, ES bulk, DynamoDB batch write, Redis pipeline, Cassandra concurrency).
- Add AVA parallel test scripts + worker DB bootstrap + timing wrapper for test commands.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
docker-compose.yml |
Uses tmpfs mounts for multiple DB services to speed up local/E2E runs. |
docker-compose.tst.yml |
Runs tests via test-all-parallel and adds tmpfs mounts for test DB services. |
backend/test/utils/create-test-table.ts |
Major seeding performance refactor: batch inserts, ES bulk, DynamoDB batch writes, Redis pipeline, Cassandra concurrency. |
backend/test/ava-tests/saas-tests/table-redis-agent-e2e.test.ts |
Adds a fixed delay after creating a connection. |
backend/test/ava-tests/saas-tests/table-postgres-agent-e2e.test.ts |
Uses bulk insert when seeding rows in beforeEach. |
backend/test/ava-tests/saas-tests/table-oracle-agent-e2e.test.ts |
Uses bulk insert when seeding rows in beforeEach. |
backend/test/ava-tests/saas-tests/table-mysql-agent-e2e.test.ts |
Uses bulk insert when seeding rows in beforeEach. |
backend/test/ava-tests/saas-tests/table-mssql-agent-e2e.test.ts |
Uses bulk insert when seeding rows in beforeEach (and removes commented code). |
backend/test/ava-tests/saas-tests/table-clickhouse-agent-e2e.test.ts |
Adds a fixed delay after creating a connection. |
backend/test/ava-tests/saas-tests/table-cassandra-agent.e2e.test.ts |
Adds a fixed delay after creating a connection. |
backend/test/ava-tests/non-saas-tests/non-saas-user-e2e.test.ts |
Moves app setup from beforeEach to before to reduce per-test overhead. |
backend/package.json |
Wraps AVA commands with timing script; adds parallel/fast test scripts. |
backend/ava.config.mjs |
Adds per-worker DB setup hook; makes concurrency configurable via env. |
backend/_setup-worker-db.mjs |
New: creates/uses per-worker PostgreSQL DBs for parallel test isolation. |
backend/_run-with-timing.mjs |
New: wrapper to run a command and print elapsed time on completion/failure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /* eslint-disable @typescript-eslint/no-unused-vars */ | ||
|
|
||
| import { DynamoDB, PutItemCommand, PutItemCommandInput } from '@aws-sdk/client-dynamodb'; | ||
| import { BatchWriteItemCommand, DynamoDB, PutItemCommand, PutItemCommandInput } from '@aws-sdk/client-dynamodb'; |
| const bulkItems = (bulkResponse as { items: Array<{ index?: { _id?: string } }> }).items; | ||
| for (let i = 0; i < testEntitiesSeedsCount; i++) { | ||
| insertedSearchedIds.push({ | ||
| number: i, | ||
| _id: bulkItems[i]?.index?._id ?? '', | ||
| }); | ||
| } |
| id: { N: i }, | ||
| name: { S: isSearchedUser ? testSearchedUserName : faker.person.firstName() }, | ||
| email: { S: faker.internet.email() }, | ||
| age: { | ||
| N: !isSearchedUser | ||
| ? faker.number.int({ min: 16, max: 80 }) | ||
| : i === 0 | ||
| ? 14 | ||
| : i === testEntitiesSeedsCount - 21 | ||
| ? 90 | ||
| : 95, |
| const batchSize = 25; | ||
| for (let start = 0; start < writeRequests.length; start += batchSize) { | ||
| let unprocessed: any = { | ||
| [testTableName]: writeRequests.slice(start, start + batchSize), | ||
| }; | ||
| await documentClient.send(new PutItemCommand(params)); | ||
| while (unprocessed && Object.keys(unprocessed).length > 0) { | ||
| const result = await documentClient.send(new BatchWriteItemCommand({ RequestItems: unprocessed })); | ||
| unprocessed = | ||
| result.UnprocessedItems && Object.keys(result.UnprocessedItems).length > 0 ? result.UnprocessedItems : null; | ||
| } |
| const workerId = process.pid; | ||
| const pgLiteFolderPath = process.env.PGLITE_FOLDER_PATH; | ||
|
|
||
| if (pgLiteFolderPath && pgLiteFolderPath.length > 0) { | ||
| process.env.PGLITE_FOLDER_PATH = path.join(pgLiteFolderPath, `worker-${workerId}`); | ||
| } else if (process.env.DATABASE_URL) { | ||
| const url = new URL(process.env.DATABASE_URL); | ||
| const sourceDb = url.pathname.replace(/^\//, '') || 'postgres'; | ||
| const workerDbName = `rocketadmin_test_w${workerId}`; | ||
| const baseConnection = { |
| .set('Accept', 'application/json'); | ||
| const createConnectionRO = JSON.parse(createConnectionResponse.text); | ||
| createdConnectionId = createConnectionRO.id; | ||
| await _testUtils.sleep(2000); | ||
| }); |
| .set('Accept', 'application/json'); | ||
| const createConnectionRO = JSON.parse(createConnectionResponse.text); | ||
| createdConnectionId = createConnectionRO.id; | ||
| await _testUtils.sleep(2000); | ||
| }); |
| .set('Accept', 'application/json'); | ||
| const createConnectionRO = JSON.parse(createConnectionResponse.text); | ||
| createdConnectionId = createConnectionRO.id; | ||
| await _testUtils.sleep(2000); | ||
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
backend/test/ava-tests/saas-tests/table-redis-agent-e2e.test.ts (1)
74-74: ⚡ Quick winReplace fixed sleep with readiness polling in setup.
Line 74 adds a hard 2s delay, which can still be flaky under load and always slows the suite. Prefer waiting on a concrete readiness condition with timeout/backoff instead of unconditional sleep.
backend/_setup-worker-db.mjs (1)
50-54: ⚖️ Poor tradeoffConsider cleanup of orphaned worker databases.
The script only drops the current worker's database before recreating it. If test processes crash or are killed abruptly, their databases (
rocketadmin_test_w<pid>) will remain in PostgreSQL, accumulating over time.Consider adding a cleanup mechanism (perhaps in a separate maintenance script or at the start of test runs) that drops worker databases older than a threshold or whose PIDs no longer exist.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/_setup-worker-db.mjs` around lines 50 - 54, Current setup only drops the current workerDbName, leaving orphaned databases like rocketadmin_test_w<pid>; modify this script (or add a separate maintenance function) to list databases via admin.raw querying pg_database for names matching 'rocketadmin_test_w%', parse out the PID/timestamp from the name, and for each candidate either: (a) check if the PID process exists on the host and drop the DB if the PID is gone, or (b) check the database creation/age and drop if older than a configurable threshold; implement this as a pre-create cleanup routine invoked before the existing try/catch that uses admin.raw to execute DROP DATABASE IF EXISTS for each stale DB.backend/test/ava-tests/saas-tests/table-clickhouse-agent-e2e.test.ts (1)
80-80: ⚡ Quick winReplace hard-coded sleep with condition-based waiting.
The 2-second hard-coded delay masks a timing dependency. If the ClickHouse connection or agent needs time to become ready, polling for a specific ready condition would be more reliable and potentially faster.
Consider replacing with a retry loop that checks connection health or waits for a specific agent/database state.
♻️ Example condition-based wait pattern
const createConnectionRO = JSON.parse(createConnectionResponse.text); createdConnectionId = createConnectionRO.id; -await _testUtils.sleep(2000); +// Wait for agent connection to be ready +let retries = 20; +while (retries-- > 0) { + const healthCheck = await request(app.getHttpServer()) + .get(`/connection/tables/${createdConnectionId}`) + .set('Cookie', firstUserToken); + if (healthCheck.status === 200) break; + await _testUtils.sleep(100); +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/test/ava-tests/saas-tests/table-clickhouse-agent-e2e.test.ts` at line 80, Replace the hard-coded await _testUtils.sleep(2000); with a condition-based retry/polling loop that waits until the ClickHouse connection or agent is actually ready (or times out). Implement the loop in the test (or reuse a helper) to repeatedly call a readiness check such as a ClickHouse ping/health call or agentClient.isReady()/getStatus() and break when it returns success, sleeping briefly between attempts and failing the test after a configurable timeout; replace the single sleep call in table-clickhouse-agent-e2e.test.ts with this polling logic so the test proceeds as soon as readiness is confirmed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/test/utils/create-test-table.ts`:
- Around line 192-202: The loop that builds insertedSearchedIds currently
iterates over all testEntitiesSeedsCount and thus records every inserted ID;
change it to only collect IDs for the searched fixtures (e.g., restrict the
iteration or filter bulkItems to entries that belong to the three
testSearchedUserName rows) so insertedSearchedIds only contains the searched
documents; apply the same narrowing to the MongoDB insertedIds collection loop
(the insertedIds variable/loop) so both Elasticsearch and MongoDB ID lists only
include the searched fixtures.
- Around line 651-695: The test table seeding constructs low-level DynamoDB
AttributeValue objects but sends them through documentClient.send() and uses
numeric N values; change the caller to use the low-level client (dynamoDb.send)
when sending BatchWriteItemCommand and ensure all AttributeValue N fields are
strings (e.g., convert id: { N: i } -> id: { N: String(i) } and age: { N:
String(...) } ) inside the loop that builds item and in any other places where {
N: ... } is created so the BatchWriteItemCommand receives the correct low-level
format.
- Around line 191-202: After calling client.bulk in create-test-table.ts, detect
per-item failures and fail fast: inspect bulkResponse.errors and each element of
bulkResponse.items (referencing bulkResponse, bulkItems, and the loop that
builds insertedSearchedIds using testEntitiesSeedsCount) for an error property
on index/create/update operations, and if any exist throw an informative Error
(including the failing item's _id and error reason) instead of continuing to
push partial IDs; this ensures the test setup fails immediately on bulk item
errors rather than producing a partially seeded index.
---
Nitpick comments:
In `@backend/_setup-worker-db.mjs`:
- Around line 50-54: Current setup only drops the current workerDbName, leaving
orphaned databases like rocketadmin_test_w<pid>; modify this script (or add a
separate maintenance function) to list databases via admin.raw querying
pg_database for names matching 'rocketadmin_test_w%', parse out the
PID/timestamp from the name, and for each candidate either: (a) check if the PID
process exists on the host and drop the DB if the PID is gone, or (b) check the
database creation/age and drop if older than a configurable threshold; implement
this as a pre-create cleanup routine invoked before the existing try/catch that
uses admin.raw to execute DROP DATABASE IF EXISTS for each stale DB.
In `@backend/test/ava-tests/saas-tests/table-clickhouse-agent-e2e.test.ts`:
- Line 80: Replace the hard-coded await _testUtils.sleep(2000); with a
condition-based retry/polling loop that waits until the ClickHouse connection or
agent is actually ready (or times out). Implement the loop in the test (or reuse
a helper) to repeatedly call a readiness check such as a ClickHouse ping/health
call or agentClient.isReady()/getStatus() and break when it returns success,
sleeping briefly between attempts and failing the test after a configurable
timeout; replace the single sleep call in table-clickhouse-agent-e2e.test.ts
with this polling logic so the test proceeds as soon as readiness is confirmed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98a0512f-e380-4601-bf5c-17c913388cc5
📒 Files selected for processing (15)
backend/_run-with-timing.mjsbackend/_setup-worker-db.mjsbackend/ava.config.mjsbackend/package.jsonbackend/test/ava-tests/non-saas-tests/non-saas-user-e2e.test.tsbackend/test/ava-tests/saas-tests/table-cassandra-agent.e2e.test.tsbackend/test/ava-tests/saas-tests/table-clickhouse-agent-e2e.test.tsbackend/test/ava-tests/saas-tests/table-mssql-agent-e2e.test.tsbackend/test/ava-tests/saas-tests/table-mysql-agent-e2e.test.tsbackend/test/ava-tests/saas-tests/table-oracle-agent-e2e.test.tsbackend/test/ava-tests/saas-tests/table-postgres-agent-e2e.test.tsbackend/test/ava-tests/saas-tests/table-redis-agent-e2e.test.tsbackend/test/utils/create-test-table.tsdocker-compose.tst.ymldocker-compose.yml
| const bulkResponse = await client.bulk({ refresh: true, operations: bulkOperations }); | ||
| const insertedSearchedIds: Array<{ | ||
| number: number; | ||
| _id: string; | ||
| }> = []; | ||
| const bulkItems = (bulkResponse as { items: Array<{ index?: { _id?: string } }> }).items; | ||
| for (let i = 0; i < testEntitiesSeedsCount; i++) { | ||
| insertedSearchedIds.push({ | ||
| number: i, | ||
| _id: bulkItems[i]?.index?._id ?? '', | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd 'create-test-table.ts' --type fRepository: rocket-admin/rocketadmin
Length of output: 108
🏁 Script executed:
wc -l backend/test/utils/create-test-table.tsRepository: rocket-admin/rocketadmin
Length of output: 112
🏁 Script executed:
sed -n '185,210p' backend/test/utils/create-test-table.tsRepository: rocket-admin/rocketadmin
Length of output: 810
🏁 Script executed:
rg 'client\.bulk' backend/test/utils/create-test-table.ts -A 10 -B 2Repository: rocket-admin/rocketadmin
Length of output: 474
🏁 Script executed:
rg '@elastic/elasticsearch' backend/test/utils/create-test-table.ts -B 5 -A 5Repository: rocket-admin/rocketadmin
Length of output: 693
🏁 Script executed:
rg 'bulkResponse\.' backend/ -A 2 -B 2Repository: rocket-admin/rocketadmin
Length of output: 50
🌐 Web query:
Elasticsearch @elastic/elasticsearch client bulk API errors partial failure
💡 Result:
In Elasticsearch bulk indexing, the response is “partially successful”: Elasticsearch returns a per-item result for every operation, and you must inspect those item results to detect which documents failed. A bulk call may still return HTTP 200 even when some items failed. How to detect partial failures with @elastic/elasticsearch (JavaScript) 1) Check the top-level bulk response flag errors. If errors is false, then no operations failed.[1][2] 2) If errors is true, iterate bulkResponse.items. Each element corresponds to one action in your request, and the specific per-item result contains an error object when that operation failed.[1][2] 3) For each failed item, read item[operation].status and item[operation].error (where operation is index, create, update, or delete). The Elastic JS client documentation example shows this pattern and also notes that status 429 indicates you can retry the document, while other errors (e.g., mapping errors) usually require fixing the document before retrying.[2] Example pattern (as shown in Elastic’s JS client docs) - Call bulk and check bulkResponse.errors - If true, collect/handle items where action[operation].error is present Relevant official details from Elastic - The Bulk API response includes an errors boolean: “If true, one or more of the operations in the bulk request did not complete successfully.”[1][3] - Elastic’s JS client example states: “The presence of the error key indicates that the operation that we did for the document has failed,” and it shows how to build erroredDocuments from bulkResponse.items.[2] If you want, paste a sample bulkResponse you’re getting (or the shape of bulkRows you send), and I can show you exactly how to map each failed item back to your original document/action and decide whether it’s retryable (e.g., 429) vs. a mapping/data issue.
Citations:
- 1: https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-bulk.html
- 2: https://elastic.co/guide/en/elasticsearch/client/javascript-api/current/bulk_examples.html
- 3: https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-bulk
Fail fast on Elasticsearch bulk item errors.
client.bulk() can succeed at the request level while individual index operations fail. Right now this helper will keep going with a partially seeded index, which turns setup problems into flaky downstream assertions instead of a clear test setup failure.
Proposed fix
const bulkResponse = await client.bulk({ refresh: true, operations: bulkOperations });
+if (bulkResponse.errors) {
+ const failedItems = bulkResponse.items.filter((item) => item.index?.error);
+ throw new Error(`Failed to seed Elasticsearch test index: ${JSON.stringify(failedItems)}`);
+}
const insertedSearchedIds: Array<{
number: number;
_id: string;
}> = [];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const bulkResponse = await client.bulk({ refresh: true, operations: bulkOperations }); | |
| const insertedSearchedIds: Array<{ | |
| number: number; | |
| _id: string; | |
| }> = []; | |
| const bulkItems = (bulkResponse as { items: Array<{ index?: { _id?: string } }> }).items; | |
| for (let i = 0; i < testEntitiesSeedsCount; i++) { | |
| insertedSearchedIds.push({ | |
| number: i, | |
| _id: bulkItems[i]?.index?._id ?? '', | |
| }); | |
| } | |
| const bulkResponse = await client.bulk({ refresh: true, operations: bulkOperations }); | |
| if (bulkResponse.errors) { | |
| const failedItems = bulkResponse.items.filter((item) => item.index?.error); | |
| throw new Error(`Failed to seed Elasticsearch test index: ${JSON.stringify(failedItems)}`); | |
| } | |
| const insertedSearchedIds: Array<{ | |
| number: number; | |
| _id: string; | |
| }> = []; | |
| const bulkItems = (bulkResponse as { items: Array<{ index?: { _id?: string } }> }).items; | |
| for (let i = 0; i < testEntitiesSeedsCount; i++) { | |
| insertedSearchedIds.push({ | |
| number: i, | |
| _id: bulkItems[i]?.index?._id ?? '', | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/test/utils/create-test-table.ts` around lines 191 - 202, After
calling client.bulk in create-test-table.ts, detect per-item failures and fail
fast: inspect bulkResponse.errors and each element of bulkResponse.items
(referencing bulkResponse, bulkItems, and the loop that builds
insertedSearchedIds using testEntitiesSeedsCount) for an error property on
index/create/update operations, and if any exist throw an informative Error
(including the failing item's _id and error reason) instead of continuing to
push partial IDs; this ensures the test setup fails immediately on bulk item
errors rather than producing a partially seeded index.
| const insertedSearchedIds: Array<{ | ||
| number: number; | ||
| _id: string; | ||
| }> = []; | ||
| const bulkItems = (bulkResponse as { items: Array<{ index?: { _id?: string } }> }).items; | ||
| for (let i = 0; i < testEntitiesSeedsCount; i++) { | ||
| insertedSearchedIds.push({ | ||
| number: i, | ||
| _id: bulkItems[i]?.index?._id ?? '', | ||
| }); | ||
| } |
There was a problem hiding this comment.
Keep insertedSearchedIds scoped to the searched fixtures.
Both new loops append every inserted document ID, but the rest of this helper only records IDs for the three testSearchedUserName rows. That contract drift will make callers pick non-searched records on Elasticsearch and MongoDB.
Proposed fix
- for (let i = 0; i < testEntitiesSeedsCount; i++) {
- insertedSearchedIds.push({
- number: i,
- _id: bulkItems[i]?.index?._id ?? '',
- });
- }
+ for (const i of [0, testEntitiesSeedsCount - 21, testEntitiesSeedsCount - 5]) {
+ insertedSearchedIds.push({
+ number: i,
+ _id: bulkItems[i]?.index?._id ?? '',
+ });
+ }Apply the same narrowing to the MongoDB insertedIds loop.
Also applies to: 336-343
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/test/utils/create-test-table.ts` around lines 192 - 202, The loop
that builds insertedSearchedIds currently iterates over all
testEntitiesSeedsCount and thus records every inserted ID; change it to only
collect IDs for the searched fixtures (e.g., restrict the iteration or filter
bulkItems to entries that belong to the three testSearchedUserName rows) so
insertedSearchedIds only contains the searched documents; apply the same
narrowing to the MongoDB insertedIds collection loop (the insertedIds
variable/loop) so both Elasticsearch and MongoDB ID lists only include the
searched fixtures.
| const writeRequests: Array<{ PutRequest: { Item: any } }> = []; | ||
| for (let i = 0; i < testEntitiesSeedsCount; i++) { | ||
| const isSearchedUser = i === 0 || i === testEntitiesSeedsCount - 21 || i === testEntitiesSeedsCount - 5; | ||
| const item = { | ||
| id: { N: i }, | ||
| name: { S: isSearchedUser ? testSearchedUserName : faker.person.firstName() }, | ||
| email: { S: faker.internet.email() }, | ||
| age: { | ||
| N: !isSearchedUser | ||
| ? faker.number.int({ min: 16, max: 80 }) | ||
| : i === 0 | ||
| ? 14 | ||
| : i === testEntitiesSeedsCount - 21 | ||
| ? 90 | ||
| : 95, | ||
| }, | ||
| created_at: { S: new Date().toISOString() }, | ||
| updated_at: { S: new Date().toISOString() }, | ||
| list_column: { L: [{ S: 'value1' }, { S: 'value2' }] }, | ||
| set_column: { SS: ['value1', 'value2'] }, | ||
| map_column: { M: { key1: { S: 'value1' }, key2: { S: 'value2' } } }, | ||
| binary_column: { B: Buffer.from('hello') }, | ||
| binary_set_column: { BS: [Buffer.from('value1'), Buffer.from('value2')] }, | ||
| }; | ||
|
|
||
| if (isSearchedUser) { | ||
| insertedSearchedIds.push({ | ||
| number: i, | ||
| id: String(item.id.N), | ||
| }); | ||
| } | ||
| if (isSearchedUser) { | ||
| insertedSearchedIds.push({ | ||
| number: i, | ||
| id: String(item.id.N), | ||
| }); | ||
| } | ||
|
|
||
| const params: PutItemCommandInput = { | ||
| TableName: testTableName, | ||
| Item: item as any, | ||
| writeRequests.push({ PutRequest: { Item: item } }); | ||
| } | ||
| try { | ||
| const batchSize = 25; | ||
| for (let start = 0; start < writeRequests.length; start += batchSize) { | ||
| let unprocessed: any = { | ||
| [testTableName]: writeRequests.slice(start, start + batchSize), | ||
| }; | ||
| await documentClient.send(new PutItemCommand(params)); | ||
| while (unprocessed && Object.keys(unprocessed).length > 0) { | ||
| const result = await documentClient.send(new BatchWriteItemCommand({ RequestItems: unprocessed })); | ||
| unprocessed = | ||
| result.UnprocessedItems && Object.keys(result.UnprocessedItems).length > 0 ? result.UnprocessedItems : null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file to understand the client initialization and full context
head -n 700 backend/test/utils/create-test-table.ts | tail -n 100Repository: rocket-admin/rocketadmin
Length of output: 3173
🏁 Script executed:
# Also check the imports and initial setup of the file
head -n 50 backend/test/utils/create-test-table.tsRepository: rocket-admin/rocketadmin
Length of output: 2281
🏁 Script executed:
# Search for where documentClient and dynamoDb are initialized
rg -n "documentClient\|dynamoDb.*=" backend/test/utils/create-test-table.ts -A 2Repository: rocket-admin/rocketadmin
Length of output: 50
🌐 Web query:
DynamoDB BatchWriteItemCommand N attribute string format requirement
💡 Result:
In DynamoDB BatchWriteItem (including DynamoDB BatchWriteItemCommand), the AttributeValue for type N (“Number”) must be provided as a string, e.g. { "N": "123.45" }. [1][2] So the correct JSON shape is: RequestItems: { "TableName": [ { "PutRequest": { "Item": { "myNumber": { "N": "5" } } } } ] } Notes: - BatchWriteItem’s API schema defines N as a string value. [2] - AWS CLI examples for batch-write-item also show Number attributes formatted as strings (e.g., "N": "601"). [1] - If you’re using the AWS SDK “document client” (e.g., DynamoDBDocument), you can supply native numbers, because it marshals them to the required AttributeValue form internally; but for the low-level BatchWriteItemCommand you still must use {"N":"..."} when providing AttributeValues. [3][4]
Citations:
- 1: https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/WorkingWithItems.html
- 2: https://docs.aws.amazon.com/amazondynamodb/latest/APIReference/API_BatchWriteItem.html
- 3: https://docs.aws.amazon.com/goto/SdkForJavaScriptV3/dynamodb-2012-08-10/BatchWriteItem
- 4: https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/Package/-aws-sdk-lib-dynamodb/Class/DynamoDBDocument/
🌐 Web query:
DynamoDBDocumentClient vs low-level DynamoDB client mixing compatibility issues
💡 Result:
The primary compatibility issue when mixing DynamoDBDocumentClient (high-level, from @aws-sdk/lib-dynamodb) and the low-level DynamoDBClient (from @aws-sdk/client-dynamodb) in AWS SDK v3 is related to shared configuration and state when creating multiple DocumentClients from the same underlying DynamoDBClient instance[1][2][3]. DynamoDBDocumentClient is constructed from a DynamoDBClient instance and wraps it to handle native JS types (marshalling/unmarshalling AttributeValues automatically)[1][4][5][6]. Both clients support standard CRUD operations (GetCommand/PutCommand/etc. for DocumentClient; GetItemCommand/PutItemCommand for low-level), batch operations, and transactions like TransactWriteItems via TransactWriteCommand[4][7][8]. No evidence of fundamental incompatibility in mixing operations across clients for the same table, as both ultimately call the same DynamoDB APIs. You can use DocumentClient for most app logic and low-level client for admin tasks (e.g., CreateTable), sharing the same DynamoDBClient[1][9]. Known issues: - Marshalling options (e.g., removeUndefinedValues): Creating multiple DynamoDBDocumentClients from one DynamoDBClient causes later ones to override earlier ones' options by mutating shared config[2]. AWS recommends separate DynamoDBClient instances for isolation, sharing credentials/handlers manually for performance[2]. - Middleware stack sharing: Previously shared by reference (mutations affected both), but recent changes clone it for independence[3]. - Undefined values: v3 does not auto-remove them (unlike v2); set marshallOptions explicitly[1]. - Transactions: Use native JS types with DocumentClient's TransactWriteCommand; mixing low-level AttributeValues requires correct formatting to avoid ValidationException[10][7][11]. Best practice: Create one DynamoDBClient, derive one DynamoDBDocumentClient for app use. For multiple isolated DocumentClients, use separate DynamoDBClients[2]. Official examples consistently pair them without mixing in single transactions[4].
Citations:
- 1: https://docs.aws.amazon.com/sdk-for-javascript/v3/developer-guide/migrate-dynamodb-doc-client.html
- 2: DynamoDBDocumentClients override other DynamoDBDocumentClients' marshalling options aws/aws-sdk-js-v3#6545
- 3: fix(lib-dynamodb): clone middleware stack to support cacheMiddleware aws/aws-sdk-js-v3#7762
- 4: https://docs.aws.amazon.com/sdk-for-javascript/v3/developer-guide/javascript_dynamodb_code_examples.html
- 5: https://www.npmjs.com/package/@aws-sdk/lib-dynamodb
- 6: https://github.com/aws/aws-sdk-js-v3/blob/main/lib/lib-dynamodb/README.md
- 7: https://oneuptime.com/blog/post/2026-02-02-dynamodb-transactions/view
- 8: https://www.mintlify.com/aws/aws-sdk-js-v3/api/dynamodb
- 9: https://stackoverflow.com/questions/57804745/difference-between-aws-sdk-dynamodb-client-and-documentclient
- 10: DynamoDBDocument transactWrite() errors when the TransactItems object contains Put objects aws/aws-sdk-js-v3#6007
- 11: https://stackoverflow.com/questions/78118560/dynamodb-documentclient-transactwrite-throws-validationexception-but-the-dataty
Use the low-level DynamoDB client for low-level AttributeValue format, and convert numeric N values to strings.
The code builds DynamoDB AttributeValue objects (with explicit N, S, L, M keys) but sends them through DynamoDBDocumentClient.send(), which expects native JavaScript types. Additionally, the N attribute values are numbers (id: { N: i }), but DynamoDB's BatchWriteItemCommand requires them as strings ({ N: "123" }). This combination causes runtime failures.
Use dynamoDb.send() instead of documentClient.send(), and wrap all numeric N values with String().
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/test/utils/create-test-table.ts` around lines 651 - 695, The test
table seeding constructs low-level DynamoDB AttributeValue objects but sends
them through documentClient.send() and uses numeric N values; change the caller
to use the low-level client (dynamoDb.send) when sending BatchWriteItemCommand
and ensure all AttributeValue N fields are strings (e.g., convert id: { N: i }
-> id: { N: String(i) } and age: { N: String(...) } ) inside the loop that
builds item and in any other places where { N: ... } is created so the
BatchWriteItemCommand receives the correct low-level format.
… docker-compose files
…s and improve connection retry logic
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docker-compose.tst.yml (1)
227-237: ⚡ Quick winConsider adding tmpfs to DynamoDB service for consistency with other test databases; verify DB2 intentionally omits tmpfs due to PERSISTENT_HOME=true.
The DynamoDB service (lines 227-237) lacks a tmpfs mount, unlike MySQL, Postgres, Mongo, Redis, Cassandra, ClickHouse, Elasticsearch, Oracle, and MSSQL in this configuration. This could impact test performance.
DB2 (lines 198-225) also lacks tmpfs but explicitly sets
PERSISTENT_HOME=true, which suggests the omission is intentional for data preservation across restarts. For DynamoDB, no such persistence flag is set, making the tmpfs omission less clearly justified. Consider either adding tmpfs to DynamoDB or documenting why it differs from the pattern used by other in-memory/ephemeral test databases.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose.tst.yml` around lines 227 - 237, The DynamoDB test service "test-dynamodb-e2e-testing" currently lacks a tmpfs mount while other ephemeral DB services use tmpfs; update the docker-compose service definition for test-dynamodb-e2e-testing to add a tmpfs entry (matching the pattern used by other services) so it runs in-memory for tests, or alternatively explicitly set and document a PERSISTENT_HOME=true flag for that service if persistence is intended (mirror the DB2 approach); locate the service block named test-dynamodb-e2e-testing and either add the tmpfs configuration or add a PERSISTENT_HOME=true environment variable and a comment explaining the intentional difference.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker-compose.tst.yml`:
- Around line 166-168: Cassandra service in docker-compose.tst.yml is missing a
tmpfs mount which causes slower DB test performance; update the Cassandra
service block (the section containing MAX_HEAP_SIZE, HEAP_NEWSIZE and mem_limit)
to add the same tmpfs configuration used by other DB services (e.g., a tmpfs
entry like "/tmp:rw,nosuid,nodev" or the project's canonical tmpfs options) so
Cassandra uses an in-memory filesystem for temp files; ensure the tmpfs entry
follows the same formatting and location as the other services' tmpfs entries.
---
Nitpick comments:
In `@docker-compose.tst.yml`:
- Around line 227-237: The DynamoDB test service "test-dynamodb-e2e-testing"
currently lacks a tmpfs mount while other ephemeral DB services use tmpfs;
update the docker-compose service definition for test-dynamodb-e2e-testing to
add a tmpfs entry (matching the pattern used by other services) so it runs
in-memory for tests, or alternatively explicitly set and document a
PERSISTENT_HOME=true flag for that service if persistence is intended (mirror
the DB2 approach); locate the service block named test-dynamodb-e2e-testing and
either add the tmpfs configuration or add a PERSISTENT_HOME=true environment
variable and a comment explaining the intentional difference.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 97ce3eac-9f55-4cf6-9a87-0e0cbfbb7843
📒 Files selected for processing (4)
.github/workflows/code-quality.ymlbackend/test/utils/create-test-table.tsdocker-compose.tst.ymldocker-compose.yml
💤 Files with no reviewable changes (1)
- .github/workflows/code-quality.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/test/utils/create-test-table.ts
| - MAX_HEAP_SIZE=768M | ||
| - HEAP_NEWSIZE=128M | ||
| mem_limit: 3g |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any comments or documentation about Cassandra tmpfs decisions
rg -i "cassandra.*tmpfs|tmpfs.*cassandra" -C3Repository: rocket-admin/rocketadmin
Length of output: 50
🏁 Script executed:
cat docker-compose.tst.yml | head -200Repository: rocket-admin/rocketadmin
Length of output: 4960
🏁 Script executed:
wc -l docker-compose.tst.ymlRepository: rocket-admin/rocketadmin
Length of output: 95
🏁 Script executed:
# Search for all tmpfs configurations in the file
rg "tmpfs:" docker-compose.tst.yml -A2 -B2Repository: rocket-admin/rocketadmin
Length of output: 1080
🏁 Script executed:
# Find the Cassandra service section and surrounding context
rg -A20 "cassandra:" docker-compose.tst.ymlRepository: rocket-admin/rocketadmin
Length of output: 517
🏁 Script executed:
# List all services to understand the database landscape
rg "^ [a-z_]+:" docker-compose.tst.yml | head -30Repository: rocket-admin/rocketadmin
Length of output: 91
Add tmpfs mount for Cassandra to match other database services.
The Cassandra service is the only database missing a tmpfs mount, while all other DB services (MySQL, PostgreSQL, MongoDB, MSSQL, Oracle, Redis, ClickHouse, Elasticsearch) use tmpfs for performance. This inconsistency will cause Cassandra tests to run slower than others.
Add:
Proposed tmpfs mount for Cassandra
- MAX_HEAP_SIZE=768M
- HEAP_NEWSIZE=128M
mem_limit: 3g
+ tmpfs:
+ - /var/lib/cassandra
restart: always📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - MAX_HEAP_SIZE=768M | |
| - HEAP_NEWSIZE=128M | |
| mem_limit: 3g | |
| - MAX_HEAP_SIZE=768M | |
| - HEAP_NEWSIZE=128M | |
| mem_limit: 3g | |
| tmpfs: | |
| - /var/lib/cassandra | |
| restart: always |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docker-compose.tst.yml` around lines 166 - 168, Cassandra service in
docker-compose.tst.yml is missing a tmpfs mount which causes slower DB test
performance; update the Cassandra service block (the section containing
MAX_HEAP_SIZE, HEAP_NEWSIZE and mem_limit) to add the same tmpfs configuration
used by other DB services (e.g., a tmpfs entry like "/tmp:rw,nosuid,nodev" or
the project's canonical tmpfs options) so Cassandra uses an in-memory filesystem
for temp files; ensure the tmpfs entry follows the same formatting and location
as the other services' tmpfs entries.
Summary by CodeRabbit
Tests
Chores