-
-
Notifications
You must be signed in to change notification settings - Fork 18
Enhance AI query validation by adding support for query refinement and explanation handling #1628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,8 @@ | ||||||||||||||||||||||||||||||||
| import { BadRequestException, Inject, Injectable, NotFoundException, Scope } from '@nestjs/common'; | ||||||||||||||||||||||||||||||||
| import { BadRequestException, Inject, Injectable, Logger, NotFoundException, Scope } from '@nestjs/common'; | ||||||||||||||||||||||||||||||||
| import { getDataAccessObject } from '@rocketadmin/shared-code/dist/src/data-access-layer/shared/create-data-access-object.js'; | ||||||||||||||||||||||||||||||||
| import { ConnectionTypesEnum } from '@rocketadmin/shared-code/dist/src/shared/enums/connection-types-enum.js'; | ||||||||||||||||||||||||||||||||
| import { IDataAccessObject } from '@rocketadmin/shared-code/dist/src/shared/interfaces/data-access-object.interface.js'; | ||||||||||||||||||||||||||||||||
| import { IDataAccessObjectAgent } from '@rocketadmin/shared-code/dist/src/shared/interfaces/data-access-object-agent.interface.js'; | ||||||||||||||||||||||||||||||||
| import AbstractUseCase from '../../../../common/abstract-use.case.js'; | ||||||||||||||||||||||||||||||||
| import { IGlobalDatabaseContext } from '../../../../common/application/global-database-context.interface.js'; | ||||||||||||||||||||||||||||||||
| import { BaseType } from '../../../../common/data-injection.tokens.js'; | ||||||||||||||||||||||||||||||||
|
|
@@ -36,11 +38,29 @@ interface AIGeneratedWidgetResponse { | |||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| const MAX_FEEDBACK_ITERATIONS = 3; | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| const EXPLAIN_SUPPORTED_TYPES: ReadonlySet<ConnectionTypesEnum> = new Set([ | ||||||||||||||||||||||||||||||||
| ConnectionTypesEnum.postgres, | ||||||||||||||||||||||||||||||||
| ConnectionTypesEnum.agent_postgres, | ||||||||||||||||||||||||||||||||
| ConnectionTypesEnum.mysql, | ||||||||||||||||||||||||||||||||
| ConnectionTypesEnum.agent_mysql, | ||||||||||||||||||||||||||||||||
| ConnectionTypesEnum.clickhouse, | ||||||||||||||||||||||||||||||||
| ConnectionTypesEnum.agent_clickhouse, | ||||||||||||||||||||||||||||||||
|
Comment on lines
+43
to
+49
|
||||||||||||||||||||||||||||||||
| const EXPLAIN_SUPPORTED_TYPES: ReadonlySet<ConnectionTypesEnum> = new Set([ | |
| ConnectionTypesEnum.postgres, | |
| ConnectionTypesEnum.agent_postgres, | |
| ConnectionTypesEnum.mysql, | |
| ConnectionTypesEnum.agent_mysql, | |
| ConnectionTypesEnum.clickhouse, | |
| ConnectionTypesEnum.agent_clickhouse, | |
| // Only direct DB connections are treated as safely supporting EXPLAIN. | |
| // Agent connection types (agent_postgres, agent_mysql, agent_clickhouse) are | |
| // intentionally excluded because their EXPLAIN behavior may differ when | |
| // routed through the agent proxy and has not been explicitly validated here. | |
| const EXPLAIN_SUPPORTED_TYPES: ReadonlySet<ConnectionTypesEnum> = new Set([ | |
| ConnectionTypesEnum.postgres, | |
| ConnectionTypesEnum.mysql, | |
| ConnectionTypesEnum.clickhouse, |
Copilot
AI
Feb 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logger is instantiated using NestJS's built-in Logger class, which deviates from the codebase convention. Other use cases in the codebase (e.g., find-tables-in-connection.use.case.ts:38) use WinstonLogger injected via constructor dependency injection. This pattern should be followed for consistency and to ensure proper logging configuration across the application.
Copilot
AI
Feb 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When validateQuerySafety fails on line 278, it will throw an exception that propagates up without any specific handling. This means that if the AI-generated correction introduces unsafe SQL (e.g., contains forbidden keywords), the entire operation fails. Consider catching this exception and either breaking the loop to return the previous safe query, or adding retry logic with a different prompt to the AI. The current behavior could be confusing to users since the EXPLAIN-based refinement is meant to improve the query, but could cause the operation to fail when the original query was already safe.
Copilot
AI
Feb 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for breaking out of the refinement loop may be flawed. On line 288-290, the loop breaks if explainResult.success is true, even though the AI just corrected the query. This means if the original query fails EXPLAIN, the AI corrects it, and the corrected query succeeds EXPLAIN, the loop breaks immediately without verifying if the AI made additional changes. The break should occur after checking if the query is unchanged AND the EXPLAIN succeeded, or consider removing the second break condition entirely since the unchanged check on line 280-283 already provides a termination condition.
| if (explainResult.success) { | |
| break; | |
| } |
Copilot
AI
Feb 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running EXPLAIN queries up to MAX_FEEDBACK_ITERATIONS times (3) with AI correction calls in between could significantly increase response time and AI token usage. Each iteration involves: 1) executing an EXPLAIN query, 2) calling the AI service with a potentially large EXPLAIN result, and 3) parsing and validating the AI response. Consider adding timeout handling or making the iteration count configurable based on the complexity of the operation. Additionally, consider logging execution time for each iteration to help monitor performance.
Copilot
AI
Feb 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new validateAndRefineQueryWithExplain method adds significant complexity and multiple code paths (success/failure EXPLAIN, iterations, AI corrections) but appears to lack test coverage. Given that other similar use-cases in the codebase (request-info-from-table-with-ai-v5/v6/v7) have comprehensive test coverage, tests should be added to verify: 1) behavior for supported vs unsupported connection types, 2) handling of EXPLAIN failures vs successes, 3) iteration limits, 4) AI correction acceptance/rejection logic, and 5) error handling for unsafe queries generated by AI.
Copilot
AI
Feb 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The EXPLAIN query is constructed by simply prepending "EXPLAIN " to the original query. However, different database types have different EXPLAIN syntax. For example, MySQL uses "EXPLAIN" but ClickHouse might use "EXPLAIN PLAN" or have different options. PostgreSQL supports extended EXPLAIN options like "EXPLAIN ANALYZE". The implementation should account for database-specific EXPLAIN syntax differences based on the connectionType parameter.
Copilot
AI
Feb 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method assumes that calling JSON.stringify on the EXPLAIN result will always produce a useful string for the AI to analyze. However, executeRawQuery can return different data structures depending on the connection type (as seen in execute-panel.use.case.ts lines 67-86 where postgres returns {rows: []}, mysql returns [[]], etc.). The result should be normalized or processed based on connectionType before stringifying to ensure the AI receives consistent, meaningful EXPLAIN output format.
Copilot
AI
Feb 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The executeRawQuery method requires a userEmail parameter (third parameter) for agent connection types (agent_postgres, agent_mysql, agent_clickhouse). However, this implementation casts dao to IDataAccessObject and only passes two parameters. This will cause the executeRawQuery call to fail for agent connections that are included in EXPLAIN_SUPPORTED_TYPES. The userId should be extracted from inputData in the implementation method, converted to userEmail using isConnectionTypeAgent check and getUserEmailOrReturnNull, and then passed to validateAndRefineQueryWithExplain and ultimately to executeRawQuery.
| const result = await (dao as IDataAccessObject).executeRawQuery(explainQuery, tableName); | |
| return { success: true, result: JSON.stringify(result, null, 2) }; | |
| } catch (error) { | |
| return { success: false, error: error.message }; | |
| const result = await (dao as IDataAccessObject).executeRawQuery(explainQuery, tableName, null); | |
| return { success: true, result: JSON.stringify(result, null, 2) }; | |
| } catch (error) { | |
| return { success: false, error: (error as Error).message }; |
Copilot
AI
Feb 24, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error.message access is unsafe. If the error object doesn't have a message property or is not an Error instance, this will cause a runtime error. Consider using a type guard or optional chaining: error?.message || 'Unknown error'
| return { success: false, error: error.message }; | |
| const errorMessage = | |
| error instanceof Error | |
| ? error.message | |
| : (typeof error === 'string' ? error : undefined) || 'Unknown error'; | |
| return { success: false, error: errorMessage }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The constant naming convention appears inconsistent with the rest of the codebase. Based on common TypeScript/NestJS conventions and the SCREAMING_SNAKE_CASE pattern used for EXPLAIN_SUPPORTED_TYPES, the constant should be named MAX_REFINEMENT_ITERATIONS or MAX_EXPLAIN_ITERATIONS instead of MAX_FEEDBACK_ITERATIONS to better describe its purpose in the context of query refinement with EXPLAIN.