Skip to content

Conversation

@LucianBuzzo
Copy link
Contributor

Summary

  • run set_config commands in parallel inside dispatchBatches
  • test that multiple requests with the same context are batched into one transaction

Testing

  • npm run lint
  • npm run test:integration (fails: Environment variable not found: DATABASE_URL)

https://chatgpt.com/codex/tasks/task_e_684ff6d02784832a8567e8757367aa6c

@github-actions
Copy link
Contributor

github-actions bot commented Jun 16, 2025

Published beta package version: 3.6.3-beta.dangerous.0ae0a8a

@LucianBuzzo LucianBuzzo requested a review from Copilot June 17, 2025 09:01
@LucianBuzzo LucianBuzzo force-pushed the codex/refactor-dispatchbatches-to-use-promise.all branch from b43c6c7 to cc47b53 Compare June 17, 2025 09:01
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
47.3% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR parallelizes the set_config database calls within a transaction and adds an integration test to verify that concurrent requests sharing the same context are batched into one transaction.

  • Use Promise.all to run set_config commands in parallel rather than sequentially
  • Introduce an integration test to ensure requests with identical context are grouped

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
test/integration/middleware.spec.ts New test verifying middleware calls and batching by context
src/index.ts Changed sequential set_config loop to a parallel Promise.all
Comments suppressed due to low confidence (2)

src/index.ts:278

  • [nitpick] Consider adding a comment explaining why running set_config calls in parallel is safe and preferred over sequential execution, especially when batch.context has multiple keys.
await Promise.all(

test/integration/middleware.spec.ts:178

  • [nitpick] You may also want to assert the number of SET ROLE commands by filtering for action === 'queryRawUnsafe' to ensure role changes are batched correctly.
const queryRawCalls = middlewareSpy.mock.calls.filter(

const queryRawCalls = middlewareSpy.mock.calls.filter(
(call) => call[0].action === "queryRaw",
);
expect(queryRawCalls).toHaveLength(2);
Copy link

Copilot AI Jun 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure you disconnect the Prisma client (await prisma.$disconnect()) after the test to clean up connections and avoid hanging processes.

Suggested change
expect(queryRawCalls).toHaveLength(2);
expect(queryRawCalls).toHaveLength(2);
} finally {
await prisma.$disconnect();
}

Copilot uses AI. Check for mistakes.
@LucianBuzzo LucianBuzzo closed this Sep 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants