fix: use injected project client for programmatic table push#259
fix: use injected project client for programmatic table push#259suhasdeshpande wants to merge 1 commit intomasterfrom
Conversation
WalkthroughThis pull request introduces dependency injection of a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
lib/commands/utils/attributes.ts (1)
803-813:getDatabasesService()re-invoked per attribute increateColumns/createAttributesloops.
createColumnscallsthis.createAttributeper column in a loop (line 811);createAttributecallsthis.getDatabasesService()each time, creating a freshDatabasesinstance per attribute. The same applies tocreateAttributes→createAttribute. ForcreateIndexes(line 742), the service is correctly hoisted out of the loop.This is a pre-existing inefficiency surfaced by the refactoring, not a regression. For collections with many attributes the allocation overhead is negligible, but hoisting the service call to the loop boundary in
createColumnsandcreateAttributeswould be consistent withcreateIndexes.♻️ Proposed refactor for `createColumns`
public createColumns = async ( columns: any[], table: Collection, ): Promise<void> => { log(`Creating columns ...`); + const databasesService = await this.getDatabasesService(); for (let column of columns) { if (column.side !== "child") { - await this.createAttribute(table["databaseId"], table["$id"], column); + // call databasesService directly or pass it through to createAttribute + await this.createAttribute(table["databaseId"], table["$id"], column, databasesService); } }Alternatively, expose an overload or internal helper that accepts a pre-resolved
databasesServiceto avoid the per-call allocation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/commands/utils/attributes.ts` around lines 803 - 813, The loops in createColumns and createAttributes call this.createAttribute for each item, and createAttribute calls this.getDatabasesService() every time — hoist the databases service acquisition out of the loop (like createIndexes does) by calling this.getDatabasesService() once before the loop and reusing that instance; either modify createAttribute to accept a pre-resolved databasesService parameter (or add an internal helper overload) and pass the single service into each per-item call, or obtain the service once and call a new internal method that uses it, ensuring createColumns, createAttributes, createAttribute, and createIndexes consistently reuse the same Databases instance.lib/commands/utils/pools.ts (1)
124-132: Redundant service instantiation insidepaginatecallbacks.
this.getDatabasesService()is called on every pagination batch inside the callback forwaitForAttributeDeletion(line 126),expectAttributes(line 184), andexpectIndexes(line 252). Each call allocates a newDatabaseswrapper instance, even though the underlying client never changes within a single poll cycle.This is a pre-existing pattern, but now that the pattern is being standardized across the file, it's a good time to hoist the service creation before the
paginatecall.♻️ Example: hoist service creation in `waitForAttributeDeletion`
- const { attributes } = await paginate( - async (args: any) => { - const databasesService = await this.getDatabasesService(); - return await databasesService.listAttributes({ - databaseId: args.databaseId, - collectionId: args.collectionId, - queries: args.queries || [], - }); - }, + const databasesService = await this.getDatabasesService(); + const { attributes } = await paginate( + async (args: any) => { + return await databasesService.listAttributes({ + databaseId: args.databaseId, + collectionId: args.collectionId, + queries: args.queries || [], + }); + },Apply the same hoist for
expectAttributes(line 184) andexpectIndexes(line 252).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/commands/utils/pools.ts` around lines 124 - 132, Hoist the Databases service creation out of the paginate callbacks: before calling paginate in waitForAttributeDeletion, expectAttributes, and expectIndexes, call and await this.getDatabasesService() once (e.g., const databasesService = await this.getDatabasesService()) and then reference that variable inside the paginate callback instead of calling this.getDatabasesService() repeatedly; ensure the callback still returns the same databasesService.listAttributes/listIndexes/list... results and remove the redundant service instantiation within the callback.tests/push.programmatic.tables.test.ts (1)
66-108: Test only covers the "create new table" (404) path; the "update existing table" path is not exercised.The mock always throws
{ code: 404 }fromgetTable, sotable.isExistedis nevertrue. TheattributesToCreate→deleteAttribute→waitForAttributeDeletionflow — which also usesthis.getDatabasesService()after this PR's fix — is not covered. A regression in the injected-client path for existing tables would not be caught.Consider adding a second test case where
getTablereturns a table with a differing column or index, forcingattributesToCreateto exercise the delete/recreate path via the injected client.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/push.programmatic.tables.test.ts` around lines 66 - 108, The test only covers the create-new-table (404) path; add a second test that simulates an existing table so the update/delete-recreate flow runs. Mock the tableService.getTable to return an existing table object (with differing column or index) so table.isExisted becomes true, then call push.pushTables (same args) and assert the deleteAttribute → waitForAttributeDeletion → attributesToCreate path is invoked and that getDatabasesService()/injected client calls (databaseServiceClientArgs/tableServiceClientArgs) are used for the update flow; reference push.pushTables, getTable, attributesToCreate, deleteAttribute, waitForAttributeDeletion, and getDatabasesService in the test so the injected-client path is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/push.programmatic.tables.test.ts`:
- Around line 11-63: The mock.module call inside the test body
(mock.module("../lib/services.js")) can leak to other tests; either move this
mocking into a test-wide preload file (run with --preload) so it’s scoped before
imports, or restore the mock at the end of the test by calling mock.restore()
after the dynamic import and assertions; update the current test that references
mock.module and the dynamic import of Push to ensure mock.restore() is invoked
(or relocate the mock setup to the preload) so lib/services.js is not left
mocked for other tests.
---
Nitpick comments:
In `@lib/commands/utils/attributes.ts`:
- Around line 803-813: The loops in createColumns and createAttributes call
this.createAttribute for each item, and createAttribute calls
this.getDatabasesService() every time — hoist the databases service acquisition
out of the loop (like createIndexes does) by calling this.getDatabasesService()
once before the loop and reusing that instance; either modify createAttribute to
accept a pre-resolved databasesService parameter (or add an internal helper
overload) and pass the single service into each per-item call, or obtain the
service once and call a new internal method that uses it, ensuring
createColumns, createAttributes, createAttribute, and createIndexes consistently
reuse the same Databases instance.
In `@lib/commands/utils/pools.ts`:
- Around line 124-132: Hoist the Databases service creation out of the paginate
callbacks: before calling paginate in waitForAttributeDeletion,
expectAttributes, and expectIndexes, call and await this.getDatabasesService()
once (e.g., const databasesService = await this.getDatabasesService()) and then
reference that variable inside the paginate callback instead of calling
this.getDatabasesService() repeatedly; ensure the callback still returns the
same databasesService.listAttributes/listIndexes/list... results and remove the
redundant service instantiation within the callback.
In `@tests/push.programmatic.tables.test.ts`:
- Around line 66-108: The test only covers the create-new-table (404) path; add
a second test that simulates an existing table so the update/delete-recreate
flow runs. Mock the tableService.getTable to return an existing table object
(with differing column or index) so table.isExisted becomes true, then call
push.pushTables (same args) and assert the deleteAttribute →
waitForAttributeDeletion → attributesToCreate path is invoked and that
getDatabasesService()/injected client calls
(databaseServiceClientArgs/tableServiceClientArgs) are used for the update flow;
reference push.pushTables, getTable, attributesToCreate, deleteAttribute,
waitForAttributeDeletion, and getDatabasesService in the test so the
injected-client path is covered.
| mock.module("../lib/services.js", () => { | ||
| const databasesService = { | ||
| createStringAttribute: async () => ({}), | ||
| createIndex: async () => ({}), | ||
| listAttributes: async () => ({ | ||
| total: 1, | ||
| attributes: [{ key: "title", status: "available" }], | ||
| }), | ||
| listIndexes: async () => ({ | ||
| total: 1, | ||
| indexes: [{ key: "idx_title", status: "available" }], | ||
| }), | ||
| }; | ||
|
|
||
| const tablesService = { | ||
| getTable: async () => { | ||
| throw { code: 404 }; | ||
| }, | ||
| createTable: async () => ({}), | ||
| }; | ||
|
|
||
| const throwIfNoProjectClient = (sdk?: any) => { | ||
| if (!sdk) { | ||
| throw new Error( | ||
| "Project is not set. Please run `appwrite init project`.", | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| return { | ||
| getProxyService: async () => ({}), | ||
| getConsoleService: async () => ({}), | ||
| getFunctionsService: async () => ({}), | ||
| getSitesService: async () => ({}), | ||
| getStorageService: async () => ({}), | ||
| getMessagingService: async () => ({}), | ||
| getOrganizationsService: async () => ({}), | ||
| getTeamsService: async () => ({}), | ||
| getProjectsService: async () => ({}), | ||
| getDatabasesService: async (sdk?: any) => { | ||
| databaseServiceClientArgs.push(sdk); | ||
| throwIfNoProjectClient(sdk); | ||
| return databasesService as any; | ||
| }, | ||
| getTablesDBService: async (sdk?: any) => { | ||
| tableServiceClientArgs.push(sdk); | ||
| throwIfNoProjectClient(sdk); | ||
| return tablesService as any; | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| const { Push } = await import("../lib/commands/push.ts"); |
There was a problem hiding this comment.
mock.module inside a test() body can leak to other test files.
As of Bun v1.0.19, Bun resolves the specifier passed to mock.module() as though it were an import, using the resolved path as the module-cache key. So "../lib/services.js" from tests/ correctly resolves to lib/services.js, matching the same module imported by push.ts, attributes.ts, and pools.ts. The path and the mock.module → dynamic import() ordering are both valid.
However, there is a known Bun behavior: mock.module also mocks the functions in other test files where they aren't wanted to be mocked, and this is the case even when the function is defined inside the test block. If additional test files are added later that import (directly or transitively) from lib/services.js, this mock may interfere with them.
Consider moving the mock.module call to a --preload file or calling mock.restore() at the end of the test to limit scope.
🛡️ Suggested cleanup to prevent leakage
+import { describe, expect, test, mock, afterEach } from "bun:test";
-import { describe, expect, test, mock } from "bun:test";
describe("Push.pushTables programmatic mode", () => {
+ afterEach(() => {
+ mock.restore();
+ });
+
test("uses injected project client for table columns and indexes", async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/push.programmatic.tables.test.ts` around lines 11 - 63, The mock.module
call inside the test body (mock.module("../lib/services.js")) can leak to other
tests; either move this mocking into a test-wide preload file (run with
--preload) so it’s scoped before imports, or restore the mock at the end of the
test by calling mock.restore() after the dynamic import and assertions; update
the current test that references mock.module and the dynamic import of Push to
ensure mock.restore() is invoked (or relocate the mock setup to the preload) so
lib/services.js is not left mocked for other tests.
1 participant
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit
Tests
Chores