feat: add GET_TABLE_STRUCTURE_WITHOUT_CACHE use case and endpoint#1764
Merged
Conversation
- Introduced a new use case `GetTableStructureWithoutCacheUseCase` to fetch table structure directly from the database without using cache. - Updated `TableController` to include a new endpoint `/table/structure/no-cache/:connectionId` for accessing the new use case. - Enhanced `TableModule` to provide the new use case for dependency injection. - Updated interfaces to include `IGetTableStructureWithoutCache`. - Modified existing data access objects to support fetching table structure without cache. - Added comprehensive tests for the new endpoint in both MySQL and PostgreSQL E2E test files.
📝 WalkthroughWalkthroughThis PR adds a new uncached table-structure retrieval path throughout the stack. A new use-case endpoint ( ChangesUncached Table Structure Retrieval
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a “no-cache” variant of table-structure retrieval so callers can fetch table metadata directly from the database via a new /table/structure/no-cache/:connectionId endpoint, alongside wiring and E2E coverage.
Changes:
- Added
GetTableStructureWithoutCacheUseCaseand DI wiring (UseCaseType.GET_TABLE_STRUCTURE_WITHOUT_CACHE) plus a new controller endpoint. - Extended DAO interfaces and implementations with
getTableStructureWithoutCacheand refactored SQL DAOs so cachedgetTableStructuredelegates to the non-cached variant + cache set. - Added MySQL/Postgres E2E tests for the new endpoint.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| shared-code/src/shared/interfaces/data-access-object.interface.ts | Adds getTableStructureWithoutCache to the non-agent DAO interface. |
| shared-code/src/shared/interfaces/data-access-object-agent.interface.ts | Adds getTableStructureWithoutCache to the agent DAO interface. |
| shared-code/src/data-access-layer/data-access-objects/data-access-object-redis.ts | Adds no-cache structure method (delegates to existing implementation). |
| shared-code/src/data-access-layer/data-access-objects/data-access-object-postgres.ts | Splits cached vs non-cached structure path; caching now happens only in getTableStructure. |
| shared-code/src/data-access-layer/data-access-objects/data-access-object-oracle.ts | Same cached/non-cached split as Postgres. |
| shared-code/src/data-access-layer/data-access-objects/data-access-object-mysql.ts | Same cached/non-cached split as Postgres. |
| shared-code/src/data-access-layer/data-access-objects/data-access-object-mssql.ts | Same cached/non-cached split as Postgres. |
| shared-code/src/data-access-layer/data-access-objects/data-access-object-mongodb.ts | Adds no-cache structure method (delegates to existing implementation). |
| shared-code/src/data-access-layer/data-access-objects/data-access-object-ibmdb2.ts | Adds no-cache structure method (delegates to existing implementation). |
| shared-code/src/data-access-layer/data-access-objects/data-access-object-elasticsearch.ts | Adds no-cache structure method (delegates to existing implementation). |
| shared-code/src/data-access-layer/data-access-objects/data-access-object-dynamodb.ts | Adds no-cache structure method (delegates to existing implementation). |
| shared-code/src/data-access-layer/data-access-objects/data-access-object-clickhouse.ts | Splits cached vs non-cached structure path; caching now happens only in getTableStructure. |
| shared-code/src/data-access-layer/data-access-objects/data-access-object-cassandra.ts | Adds no-cache structure method (delegates to existing implementation). |
| shared-code/src/data-access-layer/data-access-objects/data-access-object-agent.ts | Refactors structure retrieval to add getTableStructureWithoutCache and cache population in getTableStructure. |
| backend/test/ava-tests/non-saas-tests/non-saas-table-postgres-schema-e2e.test.ts | Adds E2E tests for /table/structure/no-cache/:connectionId (Postgres). |
| backend/test/ava-tests/non-saas-tests/non-saas-table-mysql-e2e.test.ts | Adds E2E tests for /table/structure/no-cache/:connectionId (MySQL). |
| backend/src/entities/table/use-cases/table-use-cases.interface.ts | Adds IGetTableStructureWithoutCache use case interface. |
| backend/src/entities/table/use-cases/get-table-structure-without-cache.use.case.ts | New use case that fetches structure via DAO “without cache” method. |
| backend/src/entities/table/table.module.ts | Registers new use case provider and adds the new route to middleware configuration. |
| backend/src/entities/table/table.controller.ts | Adds new GET endpoint to invoke the no-cache use case. |
| backend/src/common/data-injection.tokens.ts | Adds UseCaseType.GET_TABLE_STRUCTURE_WITHOUT_CACHE. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+53
to
+65
| const userEmail = await getUserEmailForAgent(foundConnection, userId, this._dbContext.userRepository); | ||
|
|
||
| // eslint-disable-next-line prefer-const | ||
| let [tableSettings, personalTableSettings, tablePrimaryColumns, tableForeignKeys, tableStructure, tableWidgets] = | ||
| await Promise.all([ | ||
| this._dbContext.tableSettingsRepository.findTableSettings(connectionId, tableName), | ||
| this._dbContext.personalTableSettingsRepository.findUserTableSettings(userId, connectionId, tableName), | ||
| dao.getTablePrimaryColumns(tableName, userEmail), | ||
| dao.getTableForeignKeys(tableName, userEmail), | ||
| dao.getTableStructureWithoutCache(tableName, userEmail), | ||
| this._dbContext.tableWidgetsRepository.findTableWidgets(connectionId, tableName), | ||
| ]); | ||
| const foreignKeysFromWidgets = extractForeignKeysFromWidgets(tableWidgets); |
Comment on lines
+44
to
+45
| const foundTalesInConnection = await dao.getTablesFromDB(); | ||
| if (!foundTalesInConnection.find((el) => el.tableName === tableName)) { |
Comment on lines
+398
to
+401
| public async getTableStructureWithoutCache(tableName: string, userEmail: string): Promise<TableStructureDS[]> { | ||
| const jwtAuthToken = this.generateJWT(this.connection.token); | ||
| axios.defaults.headers.common.Authorization = `Bearer ${jwtAuthToken}`; | ||
|
|
Comment on lines
+2413
to
+2415
| t.is(getTableStructure.status, 403); | ||
| const { message } = JSON.parse(getTableStructure.text); | ||
| t.is(message, Messages.DONT_HAVE_PERMISSIONS); |
Comment on lines
+2414
to
+2416
| t.is(getTableStructure.status, 403); | ||
| const { message } = JSON.parse(getTableStructure.text); | ||
| t.is(message, Messages.DONT_HAVE_PERMISSIONS); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
shared-code/src/data-access-layer/data-access-objects/data-access-object-cassandra.ts (1)
548-550: 💤 Low valueThe "without cache" method doesn't bypass any cache in Cassandra DAO.
Similar to Elasticsearch, the Cassandra DAO's
getTableStructuremethod (lines 517-546) doesn't implement LRU caching. The newgetTableStructureWithoutCachesimply delegates togetTableStructure, making both methods functionally identical.This maintains interface consistency but adds no functional value for Cassandra. Consider adding a comment explaining that Cassandra table structure is never cached.
🤖 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 `@shared-code/src/data-access-layer/data-access-objects/data-access-object-cassandra.ts` around lines 548 - 550, The getTableStructureWithoutCache method currently just forwards to getTableStructure, so update getTableStructureWithoutCache in the Cassandra DAO (method getTableStructureWithoutCache) to document why—add a concise comment above the method stating that Cassandra table structure is not cached (unlike Elasticsearch) and that this method exists to satisfy the interface but does not alter behavior; reference getTableStructure in the comment for clarity.shared-code/src/data-access-layer/data-access-objects/data-access-object-elasticsearch.ts (1)
459-461: 💤 Low valueThe "without cache" method doesn't bypass any cache in Elasticsearch DAO.
Unlike SQL-based DAOs (MySQL, Postgres, MSSQL), the
getTableStructuremethod in this Elasticsearch DAO doesn't useLRUStoragecaching (lines 385-457). The newgetTableStructureWithoutCachemethod is functionally identical togetTableStructure, making the distinction unnecessary for this implementation.While this maintains interface consistency across all DAOs, consider documenting that Elasticsearch table-structure reads are always uncached to avoid confusion for future maintainers.
🤖 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 `@shared-code/src/data-access-layer/data-access-objects/data-access-object-elasticsearch.ts` around lines 459 - 461, The getTableStructureWithoutCache method in the Elasticsearch DAO simply delegates to getTableStructure, which is misleading because Elasticsearch reads are already uncached; update the method by adding a clear JSDoc comment on getTableStructureWithoutCache (and/or above getTableStructure in this DAO) stating that Elasticsearch does not use LRUStorage caching and that this method exists only for interface compatibility and therefore intentionally delegates to getTableStructure; keep the implementation unchanged to preserve the interface.shared-code/src/data-access-layer/data-access-objects/data-access-object-ibmdb2.ts (1)
501-503: 💤 Low valueMisleading method name: no cache exists to bypass.
The
getTableStructureWithoutCachemethod delegates togetTableStructure, butgetTableStructure(lines 468-499) does not use any caching mechanism (noLRUStoragecalls). This makes the "without cache" naming misleading.In contrast, ClickHouse and Oracle DAOs refactored to separate cached and uncached paths. For consistency, consider either:
- Adding caching to
getTableStructureand keeping this method as the uncached path, or- Adding a comment explaining that IBM DB2 DAO doesn't cache table structure
🤖 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 `@shared-code/src/data-access-layer/data-access-objects/data-access-object-ibmdb2.ts` around lines 501 - 503, The method getTableStructureWithoutCache currently just calls getTableStructure, but getTableStructure has no caching logic, making the "WithoutCache" name misleading; either implement a cached path (add LRUStorage-based caching into getTableStructure and keep getTableStructureWithoutCache as the direct uncached call) or explicitly mark this DAO as uncached by replacing getTableStructureWithoutCache's delegation with a clear inline comment in getTableStructure and/or getTableStructureWithoutCache stating that IBM DB2 does not use LRUStorage caching (mirroring the ClickHouse/Oracle DAO approach); update references to the unique symbols getTableStructureWithoutCache, getTableStructure, and any LRUStorage usage to keep behavior and naming consistent.shared-code/src/data-access-layer/data-access-objects/data-access-object-mongodb.ts (1)
358-360: 💤 Low valueDuplicate implementation with no cache bypass.
Identical to DynamoDB DAO: both
getTableStructure(line 355) andgetTableStructureWithoutCache(line 359) delegate to the same helper without any caching layer. The distinction exists only in the method name, not in behavior.🤖 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 `@shared-code/src/data-access-layer/data-access-objects/data-access-object-mongodb.ts` around lines 358 - 360, The method getTableStructureWithoutCache is currently identical to getTableStructure because both call getTableStructureOrReturnPrimaryKeysIfNothingToScan; change getTableStructureWithoutCache to bypass any caching by calling or implementing a cache-bypassing variant (e.g., create and call getTableStructureOrReturnPrimaryKeysIfNothingToScanNoCache or add a skipCache flag to getTableStructureOrReturnPrimaryKeysIfNothingToScan and pass true) so that getTableStructure still uses the cached path while getTableStructureWithoutCache always refreshes from the source; update any helper signatures and call sites accordingly.shared-code/src/data-access-layer/data-access-objects/data-access-object-dynamodb.ts (1)
484-486: 💤 Low valueNo cache distinction in implementation.
Both
getTableStructure(line 481) andgetTableStructureWithoutCache(line 485) call the identical helper methodgetTableStructureOrReturnPrimaryKeysIfNothingToScan. SincegetTableStructuredoesn't implement caching, the "without cache" variant provides no functional difference.This differs from the ClickHouse/Oracle pattern where caching logic was extracted. Consider adding a clarifying comment or implementing caching in
getTableStructureto justify the distinction.🤖 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 `@shared-code/src/data-access-layer/data-access-objects/data-access-object-dynamodb.ts` around lines 484 - 486, getTableStructure and getTableStructureWithoutCache both call getTableStructureOrReturnPrimaryKeysIfNothingToScan, so the "without cache" method is misleading; either implement caching in getTableStructure or collapse/remove the duplicate method. To fix, add a cache layer inside getTableStructure: check a table-structure cache (e.g., a Map or existing cache helper) and return cached value if present, otherwise call getTableStructureOrReturnPrimaryKeysIfNothingToScan, store the result in the cache, and return it; keep getTableStructureWithoutCache calling getTableStructureOrReturnPrimaryKeysIfNothingToScan unchanged. Alternatively, if you prefer not to add caching, remove getTableStructureWithoutCache and add a clarifying comment on getTableStructure explaining there's no caching.backend/src/entities/table/use-cases/get-table-structure-without-cache.use.case.ts (1)
56-64: 💤 Low valueClarify partial cache bypass behavior.
The use case fetches most data with caching still enabled:
- Line 60:
dao.getTablePrimaryColumns()– uses cache- Line 61:
dao.getTableForeignKeys()– uses cache- Line 62:
dao.getTableStructureWithoutCache()– bypasses cacheWhile this aligns with the PR's focus on "table structure", the endpoint name
/table/structure/no-cache/:connectionIdmay lead developers to expect all related data (primary keys, foreign keys, widgets, settings) to bypass cache. Consider documenting this behavior in the API docs or the use case itself to clarify that only the core column structure bypasses cache.🤖 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/src/entities/table/use-cases/get-table-structure-without-cache.use.case.ts` around lines 56 - 64, The use-case GetTableStructureWithoutCache mixes a no-cache call (dao.getTableStructureWithoutCache) with cached calls (dao.getTablePrimaryColumns, dao.getTableForeignKeys, tableWidgetsRepository.findTableWidgets, tableSettingsRepository.findTableSettings, personalTableSettingsRepository.findUserTableSettings), which can confuse consumers of the /table/structure/no-cache/:connectionId endpoint; either (A) document the intended partial-bypass behavior by adding a clear comment above the get-table-structure-without-cache.use.case.ts logic and update the API docs for /table/structure/no-cache/:connectionId to state that only dao.getTableStructureWithoutCache bypasses cache and other related data remains cached, or (B) if the endpoint should truly bypass all caches, replace the cached calls with their no-cache equivalents (call the DAO/repository methods that explicitly bypass cache for primary keys, foreign keys, widgets, and settings) so dao.getTablePrimaryColumns, dao.getTableForeignKeys, tableWidgetsRepository.findTableWidgets, tableSettingsRepository.findTableSettings and personalTableSettingsRepository.findUserTableSettings behave consistently with dao.getTableStructureWithoutCache.
🤖 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/src/entities/table/table.module.ts`:
- Around line 122-123: The route ordering in the routes array registers { path:
'/table/structure/:connectionId', method: RequestMethod.GET } before the more
specific { path: '/table/structure/no-cache/:connectionId', method:
RequestMethod.GET }, causing "no-cache" to be treated as a connectionId; fix
this by swapping their order so '/table/structure/no-cache/:connectionId'
appears before '/table/structure/:connectionId' in the module's route
registrations (the array containing these path entries) so the specific route
matches first.
In `@backend/test/ava-tests/non-saas-tests/non-saas-table-mysql-e2e.test.ts`:
- Around line 2392-2416: The test incorrectly expects a 403 and
DONT_HAVE_PERMISSIONS when an empty connection ID is supplied; the
TableReadGuard validates the connection ID and returns a 400 with
Messages.CONNECTION_ID_MISSING. Update the assertions in the test case (the one
calling
get(`/table/structure/no-cache/${createConnectionRO.id}?tableName=${testTableName}`)
after setting createConnectionRO.id = '') to expect status 400 and to assert the
returned message equals Messages.CONNECTION_ID_MISSING; also apply the same
change to the cached-endpoint test that currently expects 404.
In
`@backend/test/ava-tests/non-saas-tests/non-saas-table-postgres-schema-e2e.test.ts`:
- Around line 2385-2391: The parity test currently only compares top-level keys
and column names; extend it to assert deep equality of nested collections by
adding normalized (sorted) comparisons for noCacheRO.primaryColumns vs
cachedRO.primaryColumns and noCacheRO.foreignKeys vs cachedRO.foreignKeys (use
stable sorting or canonicalization for order and then deepEqual), and also check
lengths where appropriate (e.g., primaryColumns.length and foreignKeys.length)
so divergences in those nested structures are detected; locate and update the
assertions around the existing noCacheRO and cachedRO comparisons in the test
block that contains the current t.deepEqual and t.is calls.
- Around line 2408-2416: The test is simulating a missing connectionId by
setting createConnectionRO.id = '' which can hit an unintended route; instead
remove the id overwrite and call the no-cache endpoint without the path param
(use /table/structure/no-cache?tableName=${testTableName}) when building the
request to app.getHttpServer(), keeping the same headers (.set('Cookie',
firstUserToken), Content-Type, Accept) and the same assertions
(t.is(getTableStructure.status, 403) and t.is(message,
Messages.DONT_HAVE_PERMISSIONS)) so the test validates the explicit "missing
:connectionId" contract; update references to createConnectionRO.id usage
accordingly (remove or stop interpolating it into the URL).
---
Nitpick comments:
In
`@backend/src/entities/table/use-cases/get-table-structure-without-cache.use.case.ts`:
- Around line 56-64: The use-case GetTableStructureWithoutCache mixes a no-cache
call (dao.getTableStructureWithoutCache) with cached calls
(dao.getTablePrimaryColumns, dao.getTableForeignKeys,
tableWidgetsRepository.findTableWidgets,
tableSettingsRepository.findTableSettings,
personalTableSettingsRepository.findUserTableSettings), which can confuse
consumers of the /table/structure/no-cache/:connectionId endpoint; either (A)
document the intended partial-bypass behavior by adding a clear comment above
the get-table-structure-without-cache.use.case.ts logic and update the API docs
for /table/structure/no-cache/:connectionId to state that only
dao.getTableStructureWithoutCache bypasses cache and other related data remains
cached, or (B) if the endpoint should truly bypass all caches, replace the
cached calls with their no-cache equivalents (call the DAO/repository methods
that explicitly bypass cache for primary keys, foreign keys, widgets, and
settings) so dao.getTablePrimaryColumns, dao.getTableForeignKeys,
tableWidgetsRepository.findTableWidgets,
tableSettingsRepository.findTableSettings and
personalTableSettingsRepository.findUserTableSettings behave consistently with
dao.getTableStructureWithoutCache.
In
`@shared-code/src/data-access-layer/data-access-objects/data-access-object-cassandra.ts`:
- Around line 548-550: The getTableStructureWithoutCache method currently just
forwards to getTableStructure, so update getTableStructureWithoutCache in the
Cassandra DAO (method getTableStructureWithoutCache) to document why—add a
concise comment above the method stating that Cassandra table structure is not
cached (unlike Elasticsearch) and that this method exists to satisfy the
interface but does not alter behavior; reference getTableStructure in the
comment for clarity.
In
`@shared-code/src/data-access-layer/data-access-objects/data-access-object-dynamodb.ts`:
- Around line 484-486: getTableStructure and getTableStructureWithoutCache both
call getTableStructureOrReturnPrimaryKeysIfNothingToScan, so the "without cache"
method is misleading; either implement caching in getTableStructure or
collapse/remove the duplicate method. To fix, add a cache layer inside
getTableStructure: check a table-structure cache (e.g., a Map or existing cache
helper) and return cached value if present, otherwise call
getTableStructureOrReturnPrimaryKeysIfNothingToScan, store the result in the
cache, and return it; keep getTableStructureWithoutCache calling
getTableStructureOrReturnPrimaryKeysIfNothingToScan unchanged. Alternatively, if
you prefer not to add caching, remove getTableStructureWithoutCache and add a
clarifying comment on getTableStructure explaining there's no caching.
In
`@shared-code/src/data-access-layer/data-access-objects/data-access-object-elasticsearch.ts`:
- Around line 459-461: The getTableStructureWithoutCache method in the
Elasticsearch DAO simply delegates to getTableStructure, which is misleading
because Elasticsearch reads are already uncached; update the method by adding a
clear JSDoc comment on getTableStructureWithoutCache (and/or above
getTableStructure in this DAO) stating that Elasticsearch does not use
LRUStorage caching and that this method exists only for interface compatibility
and therefore intentionally delegates to getTableStructure; keep the
implementation unchanged to preserve the interface.
In
`@shared-code/src/data-access-layer/data-access-objects/data-access-object-ibmdb2.ts`:
- Around line 501-503: The method getTableStructureWithoutCache currently just
calls getTableStructure, but getTableStructure has no caching logic, making the
"WithoutCache" name misleading; either implement a cached path (add
LRUStorage-based caching into getTableStructure and keep
getTableStructureWithoutCache as the direct uncached call) or explicitly mark
this DAO as uncached by replacing getTableStructureWithoutCache's delegation
with a clear inline comment in getTableStructure and/or
getTableStructureWithoutCache stating that IBM DB2 does not use LRUStorage
caching (mirroring the ClickHouse/Oracle DAO approach); update references to the
unique symbols getTableStructureWithoutCache, getTableStructure, and any
LRUStorage usage to keep behavior and naming consistent.
In
`@shared-code/src/data-access-layer/data-access-objects/data-access-object-mongodb.ts`:
- Around line 358-360: The method getTableStructureWithoutCache is currently
identical to getTableStructure because both call
getTableStructureOrReturnPrimaryKeysIfNothingToScan; change
getTableStructureWithoutCache to bypass any caching by calling or implementing a
cache-bypassing variant (e.g., create and call
getTableStructureOrReturnPrimaryKeysIfNothingToScanNoCache or add a skipCache
flag to getTableStructureOrReturnPrimaryKeysIfNothingToScan and pass true) so
that getTableStructure still uses the cached path while
getTableStructureWithoutCache always refreshes from the source; update any
helper signatures and call sites accordingly.
🪄 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: 55c2e85d-fb9d-41ba-9e9c-81e65b280494
📒 Files selected for processing (21)
backend/src/common/data-injection.tokens.tsbackend/src/entities/table/table.controller.tsbackend/src/entities/table/table.module.tsbackend/src/entities/table/use-cases/get-table-structure-without-cache.use.case.tsbackend/src/entities/table/use-cases/table-use-cases.interface.tsbackend/test/ava-tests/non-saas-tests/non-saas-table-mysql-e2e.test.tsbackend/test/ava-tests/non-saas-tests/non-saas-table-postgres-schema-e2e.test.tsshared-code/src/data-access-layer/data-access-objects/data-access-object-agent.tsshared-code/src/data-access-layer/data-access-objects/data-access-object-cassandra.tsshared-code/src/data-access-layer/data-access-objects/data-access-object-clickhouse.tsshared-code/src/data-access-layer/data-access-objects/data-access-object-dynamodb.tsshared-code/src/data-access-layer/data-access-objects/data-access-object-elasticsearch.tsshared-code/src/data-access-layer/data-access-objects/data-access-object-ibmdb2.tsshared-code/src/data-access-layer/data-access-objects/data-access-object-mongodb.tsshared-code/src/data-access-layer/data-access-objects/data-access-object-mssql.tsshared-code/src/data-access-layer/data-access-objects/data-access-object-mysql.tsshared-code/src/data-access-layer/data-access-objects/data-access-object-oracle.tsshared-code/src/data-access-layer/data-access-objects/data-access-object-postgres.tsshared-code/src/data-access-layer/data-access-objects/data-access-object-redis.tsshared-code/src/shared/interfaces/data-access-object-agent.interface.tsshared-code/src/shared/interfaces/data-access-object.interface.ts
Comment on lines
122
to
+123
| { path: '/table/structure/:connectionId', method: RequestMethod.GET }, | ||
| { path: '/table/structure/no-cache/:connectionId', method: RequestMethod.GET }, |
There was a problem hiding this comment.
Fix route ordering to prevent path conflict.
The route /table/structure/no-cache/:connectionId (line 123) should be registered before /table/structure/:connectionId (line 122). With the current order, requests to /table/structure/no-cache/{id} will match the more generic pattern first, treating "no-cache" as a connection ID.
🔧 Proposed fix
.forRoutes(
{ path: '/table/columns/:connectionId', method: RequestMethod.GET },
{ path: '/table/rows/:connectionId', method: RequestMethod.GET },
{ path: '/table/:connectionId', method: RequestMethod.GET },
{ path: '/connection/tables/:connectionId', method: RequestMethod.GET },
{ path: '/connection/tables/v2/:connectionId', method: RequestMethod.GET },
{ path: '/table/rows/:connectionId', method: RequestMethod.GET },
{ path: '/table/rows/find/:connectionId', method: RequestMethod.POST },
- { path: '/table/structure/:connectionId', method: RequestMethod.GET },
{ path: '/table/structure/no-cache/:connectionId', method: RequestMethod.GET },
+ { path: '/table/structure/:connectionId', method: RequestMethod.GET },
{ path: '/table/row/:connectionId', method: RequestMethod.POST },📝 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.
Suggested change
| { path: '/table/structure/:connectionId', method: RequestMethod.GET }, | |
| { path: '/table/structure/no-cache/:connectionId', method: RequestMethod.GET }, | |
| .forRoutes( | |
| { path: '/table/columns/:connectionId', method: RequestMethod.GET }, | |
| { path: '/table/rows/:connectionId', method: RequestMethod.GET }, | |
| { path: '/table/:connectionId', method: RequestMethod.GET }, | |
| { path: '/connection/tables/:connectionId', method: RequestMethod.GET }, | |
| { path: '/connection/tables/v2/:connectionId', method: RequestMethod.GET }, | |
| { path: '/table/rows/:connectionId', method: RequestMethod.GET }, | |
| { path: '/table/rows/find/:connectionId', method: RequestMethod.POST }, | |
| { path: '/table/structure/no-cache/:connectionId', method: RequestMethod.GET }, | |
| { path: '/table/structure/:connectionId', method: RequestMethod.GET }, | |
| { path: '/table/row/:connectionId', method: RequestMethod.POST }, |
🤖 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/src/entities/table/table.module.ts` around lines 122 - 123, The route
ordering in the routes array registers { path: '/table/structure/:connectionId',
method: RequestMethod.GET } before the more specific { path:
'/table/structure/no-cache/:connectionId', method: RequestMethod.GET }, causing
"no-cache" to be treated as a connectionId; fix this by swapping their order so
'/table/structure/no-cache/:connectionId' appears before
'/table/structure/:connectionId' in the module's route registrations (the array
containing these path entries) so the specific route matches first.
Comment on lines
+2392
to
+2416
| test.serial(`${currentTest} should reject access when connection id is not passed in request`, async (t) => { | ||
| const connectionToTestDB = getTestData(mockFactory).connectionToMySQL; | ||
| const firstUserToken = (await registerUserAndReturnUserInfo(app)).token; | ||
| const { testTableName } = await createTestTable(connectionToTestDB); | ||
|
|
||
| testTables.push(testTableName); | ||
|
|
||
| const createConnectionResponse = await request(app.getHttpServer()) | ||
| .post('/connection') | ||
| .send(connectionToTestDB) | ||
| .set('Cookie', firstUserToken) | ||
| .set('Content-Type', 'application/json') | ||
| .set('Accept', 'application/json'); | ||
| const createConnectionRO = JSON.parse(createConnectionResponse.text); | ||
| t.is(createConnectionResponse.status, 201); | ||
| createConnectionRO.id = ''; | ||
| const getTableStructure = await request(app.getHttpServer()) | ||
| .get(`/table/structure/no-cache/${createConnectionRO.id}?tableName=${testTableName}`) | ||
| .set('Cookie', firstUserToken) | ||
| .set('Content-Type', 'application/json') | ||
| .set('Accept', 'application/json'); | ||
| t.is(getTableStructure.status, 403); | ||
| const { message } = JSON.parse(getTableStructure.text); | ||
| t.is(message, Messages.DONT_HAVE_PERMISSIONS); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare routing and guard setup for cached vs no-cache endpoints
echo "=== Cached endpoint ==="
rg -A 10 "table/structure/:connectionId" backend/src/entities/table/table.controller.ts
echo -e "\n=== No-cache endpoint ==="
rg -A 10 "table/structure/no-cache/:connectionId" backend/src/entities/table/table.controller.ts
echo -e "\n=== Check for different guards ==="
rg -B 5 -A 15 "@Get.*structure.*no-cache" backend/src/entities/table/table.controller.tsRepository: rocket-admin/rocketadmin
Length of output: 1505
🏁 Script executed:
rg -A 30 "async getTableStructureWithoutCache" backend/src/entities/table/table.controller.tsRepository: rocket-admin/rocketadmin
Length of output: 917
🏁 Script executed:
# Get full decorator context for both endpoints
rg -B 10 "async getTableStructure\(" backend/src/entities/table/table.controller.ts | head -40
echo "---"
rg -B 10 "async getTableStructureWithoutCache" backend/src/entities/table/table.controller.ts | head -40Repository: rocket-admin/rocketadmin
Length of output: 789
🏁 Script executed:
# Check what the cached endpoint test expects at line 2212
sed -n '2190,2213p' backend/test/ava-tests/non-saas-tests/non-saas-table-mysql-e2e.test.ts
echo "---"
# Check what the no-cache endpoint test expects at line 2413
sed -n '2392,2416p' backend/test/ava-tests/non-saas-tests/non-saas-table-mysql-e2e.test.tsRepository: rocket-admin/rocketadmin
Length of output: 2335
🏁 Script executed:
# Check if there's exception handling middleware that transforms 400 errors
rg -B 5 -A 10 "HttpStatus.BAD_REQUEST|HttpException" backend/src/common/exceptions/ 2>/dev/null | head -60
echo "---"
# Check for filters that might transform 400 to other status codes
rg -B 5 -A 10 "catch.*HttpException|ExceptionFilter" backend/src/ -t ts 2>/dev/null | head -80Repository: rocket-admin/rocketadmin
Length of output: 1910
🏁 Script executed:
# Get full AllExceptionsFilter implementation
cat backend/src/exceptions/all-exceptions.filter.tsRepository: rocket-admin/rocketadmin
Length of output: 2337
🏁 Script executed:
# Check TableReadGuard implementation
rg -B 5 -A 30 "class TableReadGuard" backend/src/Repository: rocket-admin/rocketadmin
Length of output: 2866
🏁 Script executed:
# Check what `@SlugUuid` decorator does
rg -B 5 -A 20 "export.*SlugUuid|function SlugUuid" backend/src/Repository: rocket-admin/rocketadmin
Length of output: 3084
🏁 Script executed:
# Verify the complete flow by checking the guard reject behavior
cat backend/src/guards/table-read.guard.ts | tail -20Repository: rocket-admin/rocketadmin
Length of output: 425
Fix incorrect test expectations for empty connection ID handling.
Both the cached and no-cache endpoints use the same TableReadGuard, which validates the connection ID before execution. When an empty connection ID is passed, the guard rejects with BadRequestException(Messages.CONNECTION_ID_MISSING), returning HTTP 400, not 404 or 403.
The test at line 2413 expects status 403 with message Messages.DONT_HAVE_PERMISSIONS, but the actual response will be 400 with Messages.CONNECTION_ID_MISSING. Update the assertion to match the actual guard behavior:
- Change
t.is(getTableStructure.status, 403)tot.is(getTableStructure.status, 400) - Change the message assertion from
Messages.DONT_HAVE_PERMISSIONStoMessages.CONNECTION_ID_MISSING
(Note: The cached endpoint test at line 2212 also has an incorrect expectation of 404—it should also be 400.)
🤖 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/non-saas-tests/non-saas-table-mysql-e2e.test.ts`
around lines 2392 - 2416, The test incorrectly expects a 403 and
DONT_HAVE_PERMISSIONS when an empty connection ID is supplied; the
TableReadGuard validates the connection ID and returns a 400 with
Messages.CONNECTION_ID_MISSING. Update the assertions in the test case (the one
calling
get(`/table/structure/no-cache/${createConnectionRO.id}?tableName=${testTableName}`)
after setting createConnectionRO.id = '') to expect status 400 and to assert the
returned message equals Messages.CONNECTION_ID_MISSING; also apply the same
change to the cached-endpoint test that currently expects 404.
Comment on lines
+2385
to
+2391
| t.deepEqual( | ||
| noCacheRO.structure.map((column) => column.column_name).sort(), | ||
| cachedRO.structure.map((column) => column.column_name).sort(), | ||
| ); | ||
| t.is(noCacheRO.structure.length, cachedRO.structure.length); | ||
| t.deepEqual(Object.keys(noCacheRO).sort(), Object.keys(cachedRO).sort()); | ||
| }); |
There was a problem hiding this comment.
Parity test is too shallow for nested structure fields
This check can pass even if primaryColumns or foreignKeys diverge between cached and no-cache responses. Add direct equality checks for those collections (normalized for order) to make this contract test meaningful.
✅ Suggested assertion extension
t.deepEqual(
noCacheRO.structure.map((column) => column.column_name).sort(),
cachedRO.structure.map((column) => column.column_name).sort(),
);
t.is(noCacheRO.structure.length, cachedRO.structure.length);
+t.deepEqual(
+ noCacheRO.primaryColumns.map((column) => column.column_name).sort(),
+ cachedRO.primaryColumns.map((column) => column.column_name).sort(),
+);
+t.deepEqual(
+ noCacheRO.foreignKeys
+ .map((key) => `${key.constraint_name}:${key.column_name}:${key.referenced_table_name}:${key.referenced_column_name}`)
+ .sort(),
+ cachedRO.foreignKeys
+ .map((key) => `${key.constraint_name}:${key.column_name}:${key.referenced_table_name}:${key.referenced_column_name}`)
+ .sort(),
+);
t.deepEqual(Object.keys(noCacheRO).sort(), Object.keys(cachedRO).sort());📝 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.
Suggested change
| t.deepEqual( | |
| noCacheRO.structure.map((column) => column.column_name).sort(), | |
| cachedRO.structure.map((column) => column.column_name).sort(), | |
| ); | |
| t.is(noCacheRO.structure.length, cachedRO.structure.length); | |
| t.deepEqual(Object.keys(noCacheRO).sort(), Object.keys(cachedRO).sort()); | |
| }); | |
| t.deepEqual( | |
| noCacheRO.structure.map((column) => column.column_name).sort(), | |
| cachedRO.structure.map((column) => column.column_name).sort(), | |
| ); | |
| t.is(noCacheRO.structure.length, cachedRO.structure.length); | |
| t.deepEqual( | |
| noCacheRO.primaryColumns.map((column) => column.column_name).sort(), | |
| cachedRO.primaryColumns.map((column) => column.column_name).sort(), | |
| ); | |
| t.deepEqual( | |
| noCacheRO.foreignKeys | |
| .map((key) => `${key.constraint_name}:${key.column_name}:${key.referenced_table_name}:${key.referenced_column_name}`) | |
| .sort(), | |
| cachedRO.foreignKeys | |
| .map((key) => `${key.constraint_name}:${key.column_name}:${key.referenced_table_name}:${key.referenced_column_name}`) | |
| .sort(), | |
| ); | |
| t.deepEqual(Object.keys(noCacheRO).sort(), Object.keys(cachedRO).sort()); | |
| }); |
🤖 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/non-saas-tests/non-saas-table-postgres-schema-e2e.test.ts`
around lines 2385 - 2391, The parity test currently only compares top-level keys
and column names; extend it to assert deep equality of nested collections by
adding normalized (sorted) comparisons for noCacheRO.primaryColumns vs
cachedRO.primaryColumns and noCacheRO.foreignKeys vs cachedRO.foreignKeys (use
stable sorting or canonicalization for order and then deepEqual), and also check
lengths where appropriate (e.g., primaryColumns.length and foreignKeys.length)
so divergences in those nested structures are detected; locate and update the
assertions around the existing noCacheRO and cachedRO comparisons in the test
block that contains the current t.deepEqual and t.is calls.
Comment on lines
+2408
to
+2416
| createConnectionRO.id = ''; | ||
| const getTableStructure = await request(app.getHttpServer()) | ||
| .get(`/table/structure/no-cache/${createConnectionRO.id}?tableName=${testTableName}`) | ||
| .set('Cookie', firstUserToken) | ||
| .set('Content-Type', 'application/json') | ||
| .set('Accept', 'application/json'); | ||
| t.is(getTableStructure.status, 403); | ||
| const { message } = JSON.parse(getTableStructure.text); | ||
| t.is(message, Messages.DONT_HAVE_PERMISSIONS); |
There was a problem hiding this comment.
Missing-connection-id case is not validating the intended scenario
Setting id = '' and interpolating it into the URL makes this assertion brittle and can validate a different routing path than the intended “missing :connectionId on no-cache endpoint” case. Please call the endpoint without the path param explicitly and assert that contract.
✅ Suggested test adjustment
- createConnectionRO.id = '';
- const getTableStructure = await request(app.getHttpServer())
- .get(`/table/structure/no-cache/${createConnectionRO.id}?tableName=${testTableName}`)
+ const getTableStructure = await request(app.getHttpServer())
+ .get(`/table/structure/no-cache?tableName=${testTableName}`)
.set('Cookie', firstUserToken)
.set('Content-Type', 'application/json')
.set('Accept', 'application/json');
- t.is(getTableStructure.status, 403);
- const { message } = JSON.parse(getTableStructure.text);
- t.is(message, Messages.DONT_HAVE_PERMISSIONS);
+ t.is(getTableStructure.status, 404);📝 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.
Suggested change
| createConnectionRO.id = ''; | |
| const getTableStructure = await request(app.getHttpServer()) | |
| .get(`/table/structure/no-cache/${createConnectionRO.id}?tableName=${testTableName}`) | |
| .set('Cookie', firstUserToken) | |
| .set('Content-Type', 'application/json') | |
| .set('Accept', 'application/json'); | |
| t.is(getTableStructure.status, 403); | |
| const { message } = JSON.parse(getTableStructure.text); | |
| t.is(message, Messages.DONT_HAVE_PERMISSIONS); | |
| const getTableStructure = await request(app.getHttpServer()) | |
| .get(`/table/structure/no-cache?tableName=${testTableName}`) | |
| .set('Cookie', firstUserToken) | |
| .set('Content-Type', 'application/json') | |
| .set('Accept', 'application/json'); | |
| t.is(getTableStructure.status, 404); |
🤖 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/non-saas-tests/non-saas-table-postgres-schema-e2e.test.ts`
around lines 2408 - 2416, The test is simulating a missing connectionId by
setting createConnectionRO.id = '' which can hit an unintended route; instead
remove the id overwrite and call the no-cache endpoint without the path param
(use /table/structure/no-cache?tableName=${testTableName}) when building the
request to app.getHttpServer(), keeping the same headers (.set('Cookie',
firstUserToken), Content-Type, Accept) and the same assertions
(t.is(getTableStructure.status, 403) and t.is(message,
Messages.DONT_HAVE_PERMISSIONS)) so the test validates the explicit "missing
:connectionId" contract; update references to createConnectionRO.id usage
accordingly (remove or stop interpolating it into the URL).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
GetTableStructureWithoutCacheUseCaseto fetch table structure directly from the database without using cache.TableControllerto include a new endpoint/table/structure/no-cache/:connectionIdfor accessing the new use case.TableModuleto provide the new use case for dependency injection.IGetTableStructureWithoutCache.Summary by CodeRabbit