Conversation
WalkthroughThe changes introduce support for managing SQL pairs within the system. A new migration adds a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (15)
wren-ui/migrations/20250102074256_create_sql_pair_table.js (1)
6-16: Consider adding foreign key constraint for referential integrity.While the table structure is well-designed with appropriate fields and indices for the SQL pairs feature, the
project_idcolumn is only commented as a reference toproject.idwithout an actual foreign key constraint. This could lead to orphaned records if a project is deleted.table .integer('project_id') .notNullable() - .comment('Reference to project.id'); + .comment('Reference to project.id') + .references('id') + .inTable('project') + .onDelete('CASCADE');wren-ui/src/apollo/server/repositories/sqlPairRepository.ts (2)
22-27: Improve error message in transformToDBData method.While the method implementation is correct, the error message could be more descriptive to help with debugging.
- throw new Error('Unexpected dbdata'); + throw new Error('Cannot transform to DB data: input is not a plain object');
29-34: Improve error message in transformFromDBData method.Similar to the previous method, the error message could be more descriptive.
- throw new Error('Unexpected dbdata'); + throw new Error('Cannot transform from DB data: input is not a plain object');wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts (4)
22-38: Remove direct console logging in production code.
console.logcalls can clutter logs and leak potentially sensitive data. Use the existing logger or remove debug statements to maintain clean and secure logs.- console.log(sqlPairs); + // logger.debug(sqlPairs) or remove entirely
40-55: Validate user input before service calls.
The resolver accepts potentially invalid SQL or question data. Consider adding basic validation (e.g., checking if strings are empty) before calling the service.
57-68: Return a status or message upon deletion.
Currently, the method returns the result ofsqlPairService.deleteSqlPair(...), which is a boolean. If more detailed feedback (e.g., reason for failure) is needed, consider returning a custom result type.
70-85: Return an array of questions or handle empty responses.
The method currently returns only the first question fromgenerateQuestions(...). If multiple questions are possible or none are returned, consider returning a list or validating an empty result scenario.wren-ui/src/apollo/server/schema.ts (2)
929-931: Consider pagination for large lists.
sqlPairsreturns all SQL pairs, which may be costly for large projects. Implement pagination or filters as needed for scalability.
1051-1051: Return an array for question generation if needed.
generateQuestionreturns a singleString!. If the design requires multiple or zero questions, consider using[String!]!.wren-ui/src/apollo/server/services/sqlPairService.ts (4)
99-115: Distinguish between partial and complete failures.
When deploying SQL pairs in chunks, a partial deployment failure triggers a rollback for all pairs in the current transaction. If partial success is desirable, handle error cases more granularly.
166-176: Improve error context for deployment failures.
WhendeployResult.erroris present, consider logging the failing SQL or question for rapid debugging.
219-229: Limit busy-wait polling or replace it with event callbacks.
waitUntilSqlPairResultuses a 500ms polling loop. For high concurrency, polling can introduce unnecessary load. Explore an event-based approach when the adaptor supports it, or implement exponential backoff.
230-241: Consolidate polling logic.
waitUntilSqlPairResultandwaitQuestionGenerateResultcontain similar polling loops. Factor out a reusable polling helper to reduce duplication.wren-ui/src/apollo/server/models/adaptor.ts (2)
55-58: Introduce specialized types for language or timezone.
DefiningProjectConfigurationsis a good start. Consider makinglanguageandtimezonemore specific (e.g., enumerating known languages or adding additional time zone details) to reduce potential runtime errors.
146-146: Rename property for consistency.
Here, the property is namedconfiguration, whereas other interfaces useconfigurations. Consider renaming it to maintain consistent naming across the codebase.- configuration?: ProjectConfigurations; + configurations?: ProjectConfigurations;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
wren-ui/migrations/20250102074256_create_sql_pair_table.js(1 hunks)wren-ui/src/apollo/server/adaptors/wrenAIAdaptor.ts(5 hunks)wren-ui/src/apollo/server/models/adaptor.ts(8 hunks)wren-ui/src/apollo/server/repositories/index.ts(1 hunks)wren-ui/src/apollo/server/repositories/sqlPairRepository.ts(1 hunks)wren-ui/src/apollo/server/resolvers.ts(4 hunks)wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts(1 hunks)wren-ui/src/apollo/server/schema.ts(3 hunks)wren-ui/src/apollo/server/services/sqlPairService.ts(1 hunks)wren-ui/src/apollo/server/types/context.ts(4 hunks)wren-ui/src/apollo/server/utils/error.ts(3 hunks)wren-ui/src/common.ts(6 hunks)wren-ui/src/pages/api/graphql.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (47)
wren-ui/src/apollo/server/repositories/index.ts (1)
17-17: Clean integration of SQL pair repository.The export follows the established pattern in the file, maintaining consistency with other repository exports. This change correctly exposes the SQL pair repository functionality for use in the application.
wren-ui/migrations/20250102074256_create_sql_pair_table.js (2)
12-13: Appropriate field lengths for SQL and question storage.The character limits (10,000 for SQL and 1,000 for question) provide sufficient space for storing complex queries and detailed questions while preventing excessive data storage.
14-14: Good indexing practice.Adding an index on
project_idwill improve query performance when filtering SQL pairs by project, which is likely to be a common operation based on the PR objectives.wren-ui/src/apollo/server/types/context.ts (3)
19-19: Clean import of SQL pair repository interface.The import follows the established pattern in the file, maintaining consistency with other repository interface imports.
35-35: Properly imported SQL pair service interface.The import is correctly defined with a relative path to the service implementation.
55-55: Comprehensive context integration for SQL pair functionality.The SQL pair service and repository are properly added to the context interface in their respective sections, following the established pattern. This ensures they're available throughout the GraphQL resolvers for implementing the SQL pair API operations described in the PR objectives.
Also applies to: 69-69
wren-ui/src/pages/api/graphql.ts (2)
46-46: Well-structured component integration.The SQL pair repository and service are correctly destructured from the components object in their appropriate sections, maintaining the organizational structure of the file.
Also applies to: 59-59
138-138: Complete context mapping for Apollo Server.The SQL pair service and repository are properly added to the Apollo Server context, ensuring they're available to the GraphQL resolvers. This completes the integration of the SQL pair functionality into the GraphQL API.
Also applies to: 151-151
wren-ui/src/apollo/server/resolvers.ts (4)
8-8: Imported SqlPairResolver correctly.The import statement follows the existing pattern in the file and is placed in the correct location with other resolver imports.
17-17: Instantiated SqlPairResolver correctly.The sqlPairResolver instance is created consistently with the pattern used for other resolvers in the file.
60-62: SQL Pairs query implementation looks good.The sqlPairs query is correctly added to the Query section with appropriate commenting to identify the new section.
146-150: SQL Pairs mutations look well-structured.All necessary mutations for SQL pair operations (createSqlPair, editSqlPair, deleteSqlPair, generateQuestion) are correctly implemented following the project's established pattern.
wren-ui/src/common.ts (6)
17-17: SqlPairRepository import added correctly.The import is correctly placed within the repositories import group.
38-38: SqlPairService import added.Import correctly placed after the existing imports and follows the established pattern.
65-65: SqlPairRepository initialization looks good.The repository is initialized consistently with other repositories in the file.
124-127: SqlPairService initialization is correct.The service is properly initialized with the required dependencies (sqlPairRepository and wrenAIAdaptor).
160-160: SqlPairRepository correctly added to returned object.The repository is correctly added to the repositories section of the returned object.
174-174: SqlPairService correctly added to returned object.The service is properly added to the services section of the returned object.
wren-ui/src/apollo/server/utils/error.ts (3)
39-42: New error codes added properly.The DEPLOY_SQL_PAIR_ERROR and GENERATE_QUESTIONS_ERROR error codes are correctly added to the GeneralErrorCodes enum with appropriate comments.
83-86: Error messages properly defined.Error messages for the new error codes have been added with clear descriptions.
105-106: Short messages added correctly.Short versions of the error messages have been properly defined for the new error codes.
wren-ui/src/apollo/server/repositories/sqlPairRepository.ts (4)
1-4: Imports look good.Necessary imports for Knex, BaseRepository, and utility functions are properly included.
5-10: SqlPair interface is well-defined.The interface clearly defines the structure with appropriate types and includes helpful comments for each field.
12-12: ISqlPairRepository interface correctly extends IBasicRepository.The interface properly extends the base repository interface with the appropriate generic type.
14-20: SqlPairRepository class structure looks good.The class correctly extends BaseRepository and implements ISqlPairRepository. Constructor properly initializes with the Knex instance and sets the table name.
wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts (1)
4-11: Ensure project context availability.
Ifctx.projectService.getCurrentProject()fails or returnsnull, all subsequent service calls will fail. Consider adding explicit error handling or a fallback to prevent unhandled promise rejections.wren-ui/src/apollo/server/schema.ts (2)
862-867: Check ifquestionshould be optional.
If there's a use case for storing an SQL snippet without a specific question, consider markingquestionas optional. Otherwise, this is fine.
1043-1050: Validate partial updates and secure output.
The schema foreditSqlPairallows partial updates. Confirm that server-side checks prevent overwriting fields with empty or invalid data. Additionally, ensure sensitive data is not exposed in GraphQL responses.wren-ui/src/apollo/server/models/adaptor.ts (10)
64-64: Configurations property addition.
Attachingconfigurations?: ProjectConfigurations;here looks consistent with the new interface, providing contextual input for AI-driven queries.
105-105: Ensure consistent naming.
Usingconfigurations?: ProjectConfigurations;aligns with the pattern inAskInput, promoting code uniformity.
169-169: Consistent addition to TextBasedAnswerInput.
Allowing library-specific configurations is valuable; no issues found.
206-206: Extending customization.
Introducing optionalconfigurationsforChartInputis a good approach, ensuring unified handling of special preferences.
224-224: Parallel usage of the new interface.
Includingconfigurations?: ProjectConfigurationsinChartAdjustmentInputcontinues to keep the approach consistent.
239-243: New enum for SQL pair status.
DefiningSqlPairStatusprovides clarity on indexing and completion states. Nicely done.
245-248: Result object synergy.
SqlPairResultneatly encapsulates the status and optional error. This design is consistent with other result interfaces.
250-254: QuestionInput structure.
Including multiple SQL statements and an optionalconfigurationsobject is a flexible approach, aligning with project-based contexts.
256-260: Enum for question generation states.
QuestionsStatuscovers the typical pipeline states; no concerns here.
262-267: Comprehensive questions result interface.
QuestionsResulthandles status, potential error, questions array, and an optional trace ID effectively.wren-ui/src/apollo/server/adaptors/wrenAIAdaptor.ts (9)
25-29: Additional imports for SQL pair integration.
Bringing in new types (SqlPairResult,SqlPairStatus,QuestionInput, etc.) is consistent with extending the Wren AI adaptor’s functionality.
33-33: SqlPair import usage.
This import usage appears correct and is subsequently used in thedeploySqlPairmethod. No issues noted.
97-108: New methods in IWrenAIAdaptor for SQL pairs.
The JSDoc and method signatures for SQL pair operations are a clear, logical addition, matching the rest of the adaptor’s style.
116-143: Robust implementation of deploySqlPair.
Posting SQL pairs to the Wren AI endpoint, capturing theevent_idasqueryId, and error-logging in the catch block is consistent with existing patterns.
144-160: Fetching SQL pair result.
getSqlPairResultretrieves data, transforms status/error, and returns a typedSqlPairResult. Neatly done.
161-179: Deleting SQL pairs.
Implementation ofdeleteSqlPairswith an array of stringified IDs is straightforward and follows the standard approach.
506-527: Generate questions from SQL input.
generateQuestionsmethod forms the correct request body, logs potential errors, and returns a query ID. This matches other methods’ design.
529-548: Retrieve generated questions.
getQuestionsResultuses consistent status transformation and merges the response with partialQuestionsResult. Looks good.
685-690: Status union update in transformStatusAndError.
IncludingSqlPairStatusandQuestionsStatusensures coverage of newly introduced result states in the error transformation flow.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts (5)
1-3: Consider adding more specific importsThe SqlPair import is using a relative path with double dots. Consider using absolute imports with aliases for consistency (like the IContext import).
- import { SqlPair } from '../repositories'; + import { SqlPair } from '@server/repositories';
34-49: Consider extracting SQL validation to a reusable methodThe SQL validation logic is duplicated in both
createSqlPairandeditSqlPair. Extract it to a separate private method to improve maintainability.+ private async validateSql(sql: string, ctx: IContext) { + const project = await ctx.projectService.getCurrentProject(); + const lastDeployment = await ctx.deployService.getLastDeployment(project.id); + const manifest = lastDeployment.manifest; + try { + // TODO: ask engine to provide error message for dry-run, use preview for now + await ctx.queryService.preview(sql, { + manifest, + project, + }); + } catch (err) { + throw Errors.create(Errors.GeneralErrorCodes.INVALID_SQL_ERROR, { + customMessage: err.message, + }); + } + return project; + }
105-119: Consider adding input validation for SQL generationThe
generateQuestionmethod doesn't validate if the SQL is empty. Consider adding validation to ensure the SQL string is not empty before processing.public async generateQuestion( _root: any, arg: { data: { sql: string; }; }, ctx: IContext, ) { + if (!arg.data.sql || arg.data.sql.trim() === '') { + throw Errors.create(Errors.GeneralErrorCodes.INVALID_SQL_ERROR, { + customMessage: 'SQL query cannot be empty', + }); + } const project = await ctx.projectService.getCurrentProject(); const questions = await ctx.sqlPairService.generateQuestions(project, [ arg.data.sql, ]); return questions[0]; }
22-55: Add validation for empty inputs in createSqlPairThe
createSqlPairmethod should validate that both SQL and question are non-empty strings before processing.public async createSqlPair( _root: any, arg: { data: { sql: string; question: string; }; }, ctx: IContext, ) { + if (!arg.data.sql || arg.data.sql.trim() === '') { + throw Errors.create(Errors.GeneralErrorCodes.INVALID_SQL_ERROR, { + customMessage: 'SQL query cannot be empty', + }); + } + + if (!arg.data.question || arg.data.question.trim() === '') { + throw Errors.create(Errors.GeneralErrorCodes.INVALID_INPUT, { + customMessage: 'Question cannot be empty', + }); + } const project = await ctx.projectService.getCurrentProject(); // dry run the sql to check if it's valid const lastDeployment = await ctx.deployService.getLastDeployment( project.id, ); const manifest = lastDeployment.manifest; try { // TODO: ask engine to provide error message for dry-run, use preview for now await ctx.queryService.preview(arg.data.sql, { manifest, project, }); } catch (err) { throw Errors.create(Errors.GeneralErrorCodes.INVALID_SQL_ERROR, { customMessage: err.message, }); } const sqlPairs = await ctx.sqlPairService.createSqlPairs(project.id, [ arg.data, ]); return sqlPairs[0]; }
22-31: Improve type safety with more specific typesReplace the
anytypes with more specific types to improve type safety and code readability.- public async createSqlPair(_root: any, arg: { ... }, ctx: IContext) + public async createSqlPair(_root: unknown, arg: { ... }, ctx: IContext): Promise<SqlPair> - public async editSqlPair(_root: any, arg: { ... }, ctx: IContext) + public async editSqlPair(_root: unknown, arg: { ... }, ctx: IContext): Promise<SqlPair> - public async deleteSqlPair(_root: any, arg: { ... }, ctx: IContext) + public async deleteSqlPair(_root: unknown, arg: { ... }, ctx: IContext): Promise<boolean> - public async generateQuestion(_root: any, arg: { ... }, ctx: IContext) + public async generateQuestion(_root: unknown, arg: { ... }, ctx: IContext): Promise<string>Also applies to: 57-69, 92-100, 105-113
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts(1 hunks)wren-ui/src/apollo/server/utils/error.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
wren-ui/src/apollo/server/utils/error.ts (3)
38-42: Add SQL pair related error codes - looks goodThe addition of these error codes aligns well with the PR objective of implementing SQL pair API functionality.
84-87: Error messages for SQL pair operations - goodThe error messages are clear and consistent with the existing message style in the file.
106-107: Short messages for SQL pair operations - goodThe short messages match their corresponding error messages and follow the existing pattern in the codebase.
wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts (4)
4-11: Method binding looks goodGood practice binding methods in the constructor to maintain proper
thiscontext when methods are passed as callbacks.
13-20: LGTM! Well-structured method for retrieving SQL pairsThe implementation correctly gets the current project and then fetches SQL pairs for that project.
92-103:Details
❓ Verification inconclusive
Add verification that the SQL pair belongs to the current project
Currently, there's no check to ensure the SQL pair being deleted belongs to the current project. Consider adding verification to prevent unauthorized deletion.
🏁 Script executed:
#!/bin/bash # Check if sqlPairService.deleteSqlPair has project ownership verification rg -A 5 "deleteSqlPair.*project.*id" --glob "*.ts"Length of output: 1111
Verify SQL Pair Ownership Verification
- The current resolver retrieves the active project and calls
ctx.sqlPairService.deleteSqlPair(project.id, arg.where.id)without performing an explicit check that the SQL pair belongs to the current project.- Please ensure that the
deleteSqlPairmethod (or the resolver) explicitly verifies that the SQL pair is associated with the retrieved project before deletion. If this ownership verification is already implemented in the service method, add a comment or documentation to make it clear.
40-41:Details
✅ Verification successful
TODO comment should be addressed or tracked
There's a TODO comment about asking the engine to provide error messages for dry-run. Consider creating a ticket to track this improvement.
🌐 Web query:
What's the best practice for handling TODO comments in code?💡 Result:
TODO comments can be useful but require careful management to avoid technical debt. Best practices focus on making TODOs actionable, temporary, and integrated with project tracking systems. Here are key guidelines:
Make TODOs Actionable and Accountable
Link to external trackers like JIRA or GitHub Issues instead of vague notes:
//TODO: Refactor payment processor - PROJECT-456
This connects code directly to project management systems[1][4][6][8].Include context and ownership for clarity:
//TODO(@team-lead): Handle edge case for expired tokens
Specifying responsibility increases follow-through[7][8].Treat TODOs as Temporary
Remove during code review before merging to main branches
Conduct final TODO sweeps using IDE tools like:git grep -n "TODO" # Scan codebase pre-commit[1][4][6][8]
Set expiration policies
Automatically flag stale TODOs older than 2 weeks using CI/CD plugins[1][8].Use Tooling Effectively
Tool Benefit IDE integrations (IntelliJ/WebStorm) Visualize TODOs across files[2][5] imdone/Linear Kanban-style tracking[1] Static analyzers Enforce TODO patterns[2][5] Avoid Common Pitfalls
Never commit TODOs without:
- Associated ticket numbers
- Clear implementation details
- Defined ownership[3][4][6]
Convert to issues for:
- Business requirements
- Architectural changes
- Multi-step refactors[3][7][8]
When TODOs Make Sense
Short-term markers during active development:
def calculate_tax(): # TODO: Add provincial exemptions - @devA return base_rate * 1.13[4][6][8]
Document known limitations:
// TODO: Firefox drag-out bug - see #ISSUE-789
Helps maintainers understand workarounds[6][8]By treating TODOs as transient artifacts rather than permanent fixtures, teams maintain cleaner codebases while still capturing in-progress work. The key is balancing immediate developer needs with long-term code health through automation and process integration.
Citations:
- 1: https://dev.to/adammc331/todo-write-a-better-comment-4c8c
- 2: https://www.jetbrains.com/help/idea/using-todo.html
- 3: https://swimm.io/learn/code-collaboration/comments-in-code-best-practices-and-mistakes-to-avoid
- 4: https://gillesleblanc.wordpress.com/2016/02/18/todo-comments-best-practices/
- 5: https://www.jetbrains.com/help/webstorm/using-todo.html
- 6: https://stackoverflow.blog/2021/12/23/best-practices-for-writing-code-comments/
- 7: https://mitcommlab.mit.edu/broad/commkit/coding-and-comment-style/
- 8: https://www.osedea.com/insight/should-you-put-todos-in-the-source-code
Action: Link the TODO to an Issue Tracker for Dry-run Error Handling
The code in
wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts(lines 40-41) still contains a TODO regarding the engine's error handling for dry-run operations. In line with best practices, please create an issue in our external tracking system (e.g., JIRA or GitHub Issues) to address this improvement. Then update the comment to reference the ticket (e.g.,// TODO: ask engine to provide error message for dry-run; tracked in ISSUE-1234) so it becomes actionable and accountable, reducing the risk of accumulating technical debt.
onlyjackfrost
force-pushed
the
feat/sql-pair
branch
from
70bb5dc to
44825b5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts (1)
87-105: Handle null or empty SQL in validation.
Sincesqlmight be absent, consider early-returning or skipping preview if the input is falsy to prevent potential runtime errors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts (7)
1-3: Imports look correct and consistent.
No issues found with these imports.
5-11: Method binding strategy looks good.
Binding resolver methods to the class instance is a valid approach for maintaining the correct execution context.
13-21: Confirm the project is always available.
IfgetCurrentProject()may returnnullor throw an error, ensure proper handling to avoid unexpected runtime failures.
22-39: Creation logic is clear and straightforward.
The validation step before creating the SQL pair helps avoid bad data in the database.
40-56: Validate SQL conditionally to avoid null references.This was previously flagged in another review.
arg.data.sqlis optional, yet the code callsthis.validateSql(arg.data.sql, ctx)unconditionally, which may lead to a null reference error if only the question is being updated.You can fix it by validating only if
arg.data.sqlis provided:- await this.validateSql(arg.data.sql, ctx); + if (arg.data.sql) { + await this.validateSql(arg.data.sql, ctx); + }
58-69: Deletion logic looks good.
The service call is straightforward and properly references the project and SQL pair ID.
71-85: Check for empty generated questions.
Returningquestions[0]can result inundefinedif no question is generated. Consider returning a default value or throwing an error to clarify the failure mode.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts (1)
50-51:⚠️ Potential issueFix potential null reference in SQL validation for edit operation
The
updateSqlPairmethod always attempts to validatearg.data.sql, but it's marked as optional in the type definition. If only the question is being updated, this could lead to a null reference error.public async updateSqlPair( _root: unknown, arg: { data: { sql?: string; question?: string; }; where: { id: number; }; }, ctx: IContext, ): Promise<SqlPair> { const project = await ctx.projectService.getCurrentProject(); - await this.validateSql(arg.data.sql, ctx); + if (arg.data.sql) { + await this.validateSql(arg.data.sql, ctx); + } return ctx.sqlPairService.editSqlPair(project.id, arg.where.id, arg.data); }
🧹 Nitpick comments (4)
wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts (4)
68-82: Add return type and handle empty results in generateQuestion methodThe
generateQuestionmethod is missing a return type annotation, and it doesn't handle the case where no questions are generated.public async generateQuestion( _root: unknown, arg: { data: { sql: string; }; }, ctx: IContext, - ) { + ): Promise<string | null> { const project = await ctx.projectService.getCurrentProject(); const questions = await ctx.sqlPairService.generateQuestions(project, [ arg.data.sql, ]); - return questions[0]; + return questions.length > 0 ? questions[0] : null; }
84-101: Enhance error handling in validateSql methodThe
validateSqlmethod should handle the case whensqlis undefined or null, and also handle potential issues with retrieving the last deployment.- private async validateSql(sql: string, ctx: IContext) { + private async validateSql(sql: string | undefined | null, ctx: IContext) { + if (!sql) { + return; // Skip validation if sql is not provided + } const project = await ctx.projectService.getCurrentProject(); const lastDeployment = await ctx.deployService.getLastDeployment( project.id, ); + if (!lastDeployment) { + throw Errors.create(Errors.GeneralErrorCodes.DEPLOYMENT_NOT_FOUND); + } const manifest = lastDeployment.manifest; try { await ctx.queryService.preview(sql, { manifest, project, dryRun: true, }); } catch (err) { throw Errors.create(Errors.GeneralErrorCodes.INVALID_SQL_ERROR, { customMessage: err.message, }); } }
34-34: Remove unnecessary await in return statementSince the function is already marked as
async, theawaitin the return statement is redundant.- return await ctx.sqlPairService.createSqlPair(project.id, arg.data); + return ctx.sqlPairService.createSqlPair(project.id, arg.data);
32-34: Consider validating the question fieldThe
createSqlPairmethod validates the SQL field but not the question field. Consider adding validation for the question field to ensure it's not empty or malformed.public async createSqlPair( _root: unknown, arg: { data: { sql: string; question: string; }; }, ctx: IContext, ): Promise<SqlPair> { const project = await ctx.projectService.getCurrentProject(); + if (!arg.data.question || arg.data.question.trim() === '') { + throw Errors.create(Errors.GeneralErrorCodes.INVALID_INPUT, { + customMessage: 'Question cannot be empty', + }); + } await this.validateSql(arg.data.sql, ctx); return ctx.sqlPairService.createSqlPair(project.id, arg.data); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
wren-ui/migrations/20250102074256_create_sql_pair_table.js(1 hunks)wren-ui/src/apollo/server/repositories/sqlPairRepository.ts(1 hunks)wren-ui/src/apollo/server/resolvers.ts(4 hunks)wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts(1 hunks)wren-ui/src/apollo/server/schema.ts(3 hunks)wren-ui/src/apollo/server/services/sqlPairService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- wren-ui/migrations/20250102074256_create_sql_pair_table.js
- wren-ui/src/apollo/server/repositories/sqlPairRepository.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (23)
wren-ui/src/apollo/server/resolvers.ts (4)
8-8: Nicely introduced import.
The import is clear and aligns with the newly createdSqlPairResolver.
17-17: Consistent solver instantiation.
InstantiatingsqlPairResolverdirectly is straightforward and correctly follows the pattern established for other resolvers.
61-62: AddedsqlPairsquery looks good.
Providing a query for retrieving SQL pairs under the existingQueryobject is well-structured and aligns with similar existing resolver patterns.
145-151: Solid set of mutations for SQL pairs.
DefiningcreateSqlPair,updateSqlPair,deleteSqlPair, andgenerateQuestionunderMutationis consistent with your existing naming convention and ensures a more intuitive API design.wren-ui/src/apollo/server/schema.ts (7)
862-867: ConciseSqlPairtype definition.
Definingid,projectId,sql, andquestionas non-nullable ensures data integrity.
869-872:CreateSqlPairInputmatches usage.
Requiring bothsqlandquestionis sensible for creating a new pair.
874-877:UpdateSqlPairInputflexibility is good.
Making fields optional allows partial updates without rewriting unchanged fields.
879-881:SqlPairWhereUniqueInputis clear.
Using an ID-based primary key for unique identification is intuitive.
883-885:GenerateQuestionInputeffectively declares input.
Requiringsqlis all that is needed to generate relevant questions.
929-931: NewsqlPairsquery ensures clarity.
Returning a non-nullable list ofSqlPairprevents ambiguity in empty states.
1042-1050: Well-defined mutations for managing SQL pairs.
These mutations cover creation, updating, deletion, and question generation, making the API comprehensive.wren-ui/src/apollo/server/services/sqlPairService.ts (12)
1-7: Clear adaptor and model imports.
This properly surfaces the types and interfaces required by the service.
16-19: InterfaceCreateSqlPairis straightforward.
Requiringsqlandquestionis sufficient for new pair creation.
21-24: Optional fields inEditSqlPairprovide flexibility.
Your approach of selectively updating fields is minimal yet effective.
26-40: ComprehensiveISqlPairServiceinterface.
Defines clear methods for all CRUD operations plus question generation.
42-55: Class constructor: dependencies are well-injected.
Adhering to dependency injection helps testing and maintainability.
57-83: Robust error handling ingenerateQuestions.
Catching and rethrowing errors with project context is good for debugging.
85-87:getProjectSqlPairsmethod ensures data domain clarity.
Filtering byprojectIdis straightforward and consistent.
89-118: TransactionalcreateSqlPairlogic with rollback is solid.
Ensuring the pair is saved and deployed within the same transaction prevents partial failures.
120-167: Batch logic increateSqlPairsis well-structured.
UsingPromise.allSettledfor chunked parallel calls, followed by rollback on error, is robust.
169-224:editSqlPairincludes thorough pre-checks.
Verifying existence andprojectIdbelongs to this pair ensures integrity before updates.
226-251:deleteSqlPairmethod carefully validates existence.
Removing from both repository and adaptor within a transaction helps maintain data consistency.
253-282: Efficient waiting mechanisms.
Periodic polling inwaitUntilSqlPairResultandwaitQuestionGenerateResultis straightforward and well-limited, avoiding indefinite loops.
3 participants
Description
Implement sql-pari feature related APIs
GraphQL Query
Graphql Mutattion
Screenshots
Summary by CodeRabbit
Summary by CodeRabbit