feat(wren-ui): support generate wren sql from dialect sql#1539
feat(wren-ui): support generate wren sql from dialect sql#1539onlyjackfrost merged 8 commits intomainfrom
Conversation
… model substitution functionality to convert dialect SQL to Wren SQL - Introduce branded types DialectSQL and WrenSQL for type safety - Centralize error handling in IbisAdaptor with enhanced error messages - Add GraphQL scalar type for DialectSQL - Update test cases for new functionality and error handling
WalkthroughThe changes extend the application's SQL handling and error management by introducing a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQL
participant Resolver
participant SQLPairService
participant IbisAdaptor
Client->>GraphQL: Send mutation modelSubstitute(sql)
GraphQL->>Resolver: Call modelSubstitute resolver
Resolver->>SQLPairService: Invoke modelSubstitute(sql, options)
SQLPairService->>IbisAdaptor: Call modelSubstitute(data, connectionInfo, mdl, sql)
IbisAdaptor-->>SQLPairService: Return substituted WrenSQL
SQLPairService-->>Resolver: Return WrenSQL result
Resolver-->>GraphQL: Pass WrenSQL back
GraphQL-->>Client: Respond with WrenSQL
Suggested reviewers
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 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: 1
🔭 Outside diff range comments (1)
wren-ui/src/apollo/server/services/modelService.ts (1)
214-218:⚠️ Potential issueInconsistent error handling approach
While
safeParseJsonis used in thecreateCalculatedFieldmethod, this method still uses rawJSON.parsewhich could throw an uncaught exception iferrorMessagecontains invalid JSON.Apply this change for consistency and to avoid potential exceptions:
- const error = JSON.parse(errorMessage); + const parsedErrorMessage = safeParseJson(errorMessage); throw Errors.create(Errors.GeneralErrorCodes.INVALID_CALCULATED_FIELD, { - customMessage: error?.message, - originalError: error, + customMessage: parsedErrorMessage?.message || errorMessage, + originalError: parsedErrorMessage || null, });
🧹 Nitpick comments (3)
wren-ui/src/apollo/server/utils/helper.ts (1)
13-22: New safeStringify function implements robust error handlingThe implementation is clean and consistent with the error-handling pattern of
safeParseJson. The function intelligently handles different data types by first checking if data is already a string, and then gracefully handles potential stringify errors.Consider adding TypeScript types for improved type safety:
-export const safeStringify = (data) => { +export const safeStringify = (data: unknown): string | unknown => {wren-ui/src/apollo/server/scalars.ts (1)
1-25: Well-implemented GraphQL scalar type for DialectSQLThe
DialectSQLScalarimplementation follows GraphQL best practices with proper validation in all three required methods:serialize,parseValue, andparseLiteral. The error handling ensures type safety by validating input values.Two minor improvements could make the code more robust:
- Use a more specific type for the AST parameter:
- parseLiteral(ast: any): DialectSQL { + parseLiteral(ast: import('graphql').ValueNode): DialectSQL {
- Extract the error message to a constant to ensure consistency:
+const DIALECT_SQL_TYPE_ERROR = 'DialectSQL must be a string'; serialize(value: unknown): string { if (typeof value !== 'string') { - throw new Error('DialectSQL must be a string'); + throw new Error(DIALECT_SQL_TYPE_ERROR); } // ... rest of the codewren-ui/src/apollo/server/schema.ts (1)
1145-1145: New modelSubstitute mutation addedThe mutation is correctly added to the schema and takes the ModelSubstituteInput type as its parameter.
Consider using a more specific return type like a custom scalar for WrenSQL instead of a generic String for better type safety:
- modelSubstitute(data: ModelSubstituteInput!): String! + modelSubstitute(data: ModelSubstituteInput!): WrenSQL!This would require creating a WrenSQL scalar similar to the DialectSQL scalar.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts(12 hunks)wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts(7 hunks)wren-ui/src/apollo/server/models/adaptor.ts(1 hunks)wren-ui/src/apollo/server/resolvers.ts(3 hunks)wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts(2 hunks)wren-ui/src/apollo/server/scalars.ts(1 hunks)wren-ui/src/apollo/server/schema.ts(3 hunks)wren-ui/src/apollo/server/services/modelService.ts(2 hunks)wren-ui/src/apollo/server/services/sqlPairService.ts(3 hunks)wren-ui/src/apollo/server/utils/helper.ts(1 hunks)wren-ui/src/common.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
wren-ui/src/apollo/server/services/modelService.ts (1)
wren-ui/src/apollo/server/utils/helper.ts (1)
safeParseJson(5-11)
wren-ui/src/apollo/server/resolvers.ts (1)
wren-ui/src/apollo/server/scalars.ts (1)
DialectSQLScalar(4-25)
wren-ui/src/apollo/server/scalars.ts (1)
wren-ui/src/apollo/server/models/adaptor.ts (1)
DialectSQL(8-8)
wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts (2)
wren-ui/src/apollo/server/models/adaptor.ts (2)
DialectSQL(8-8)WrenSQL(9-9)wren-ui/src/apollo/server/types/context.ts (1)
IContext(39-78)
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts (2)
wren-ui/src/apollo/server/repositories/projectRepository.ts (1)
WREN_AI_CONNECTION_INFO(76-84)wren-ui/src/apollo/server/models/adaptor.ts (2)
DialectSQL(8-8)WrenSQL(9-9)
wren-ui/src/apollo/server/services/sqlPairService.ts (5)
wren-ui/src/apollo/server/repositories/projectRepository.ts (1)
Project(92-107)wren-ui/src/apollo/server/mdl/type.ts (1)
Manifest(70-78)wren-ui/src/apollo/server/models/adaptor.ts (2)
DialectSQL(8-8)WrenSQL(9-9)wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts (2)
WrenSQL(23-23)IIbisAdaptor(125-155)wren-ui/src/apollo/server/adaptors/wrenAIAdaptor.ts (1)
IWrenAIAdaptor(52-132)
wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts (2)
wren-ui/migrations/20240531085916_transfer_model_properties.js (2)
catalog(14-14)schema(13-13)wren-ui/src/apollo/server/models/adaptor.ts (1)
DialectSQL(8-8)
wren-ui/src/apollo/server/models/adaptor.ts (1)
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts (1)
WrenSQL(23-23)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (32)
wren-ui/src/common.ts (1)
138-142: SqlPairService initialization updated to include ibisAdaptor dependencyThe change adds
ibisAdaptoras a parameter to the SqlPairService constructor, enabling the service to utilize this adaptor for the new model substitution functionality mentioned in the PR objectives.wren-ui/src/apollo/server/services/modelService.ts (2)
12-14: Improved error handling with safeParseJsonThe import of
safeParseJsoninstead of using rawJSON.parseis a good defensive programming approach that prevents potential exceptions when parsing invalid JSON.
147-152: Good use of safeParseJson for error handlingThe switch to
safeParseJsonwith fallback handling for invalid JSON improves the robustness of the error handling in this method.wren-ui/src/apollo/server/models/adaptor.ts (1)
5-9: Enhanced type safety with branded types for SQL stringsThe addition of branded types
DialectSQLandWrenSQLis an excellent approach to improve type safety in the codebase. This technique helps prevent accidental mixing of different SQL string types at compile time without adding any runtime overhead.The
Brand<T, B>generic type is a clean implementation of the branded type pattern in TypeScript. This change aligns well with the PR objective of enhancing type safety for SQL strings.wren-ui/src/apollo/server/resolvers.ts (3)
11-11: Adding support for DialectSQL scalar typeImporting the custom GraphQL scalar for dialect SQL strings to enable proper type handling in the GraphQL API.
23-23: Registered DialectSQL scalar in GraphQL resolversAdding the DialectSQL scalar to the resolvers map allows GraphQL to properly serialize, parse and validate SQL strings in a specific dialect.
167-167: New modelSubstitute mutation addedThe addition of the modelSubstitute mutation provides the GraphQL API endpoint for converting dialect SQL to Wren SQL, which is the core functionality described in the PR objectives.
wren-ui/src/apollo/server/utils/helper.ts (1)
5-5: Function renamed for better clarityThe function has been appropriately renamed from
parseJsontosafeParseJson, which better reflects its error-handling behavior.wren-ui/src/apollo/server/schema.ts (2)
5-5: Added scalar DialectSQL to GraphQL schemaSuccessfully added the new scalar type to the schema, which allows for type-safe SQL handling in GraphQL operations.
933-935: New ModelSubstituteInput type with DialectSQL fieldThe input type correctly uses the new DialectSQL scalar type for the sql field, maintaining type safety.
wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts (2)
5-5: Import branded types for SQL handlingCorrectly importing the necessary branded types
DialectSQLandWrenSQLfor type safety.
14-14: Bind new modelSubstitute method in constructorMethod binding in the constructor is consistent with the existing pattern in the class.
wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts (7)
11-11: Good addition of the branded type import.The import of
DialectSQLfrom'../../models/adaptor'is appropriate for adding type safety to SQL strings in the test file.
261-271: Properly structured Trino connection information.The connection info for Trino has been updated to use individual fields instead of a connection URL, which aligns with changes in the implementation file. The destructuring is clean and the object construction follows best practices.
401-401: API version update correctly implemented.The validation API endpoint has been updated from
/v2/to/v3/across all test cases, which is consistent with the implementation changes that support new features like model substitution.Also applies to: 433-433
564-589: Well-structured test for successful model substitution.This test case properly validates that the
modelSubstitutemethod correctly:
- Makes the API call with the right parameters
- Processes the response
- Returns the expected result
The mock setup and assertions are comprehensive.
591-623: Thorough error handling test for MODEL_NOT_FOUND case.The test correctly verifies that when a model is not found:
- The error includes appropriate correlation ID and process time
- The error message is enhanced with a helpful suggestion about adding catalog and schema
This matches the expected behavior described in the PR objectives.
625-657: Good coverage of PARSING_EXCEPTION error case.This test properly verifies the enhanced error message for SQL parsing exceptions, checking that it includes a helpful suggestion about column quoting for non-alphanumeric characters.
659-689: Generic error handling properly tested.The test ensures that when an unrecognized error occurs, the original error message is preserved and metadata like correlation ID and process time are still included.
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts (8)
21-23: Good type imports and exports for model substitution.The file correctly imports and re-exports the branded types
DialectSQLandWrenSQL, which enhances type safety when working with different SQL formats.
149-154: Well-defined interface method for model substitution.The
modelSubstitutemethod signature is clear, with appropriate parameters and return type. Using branded types (DialectSQLandWrenSQL) provides strong type safety for SQL string handling.
177-177: Appropriate API type addition.Adding
MODEL_SUBSTITUTEto theIBIS_API_TYPEenum ensures the new functionality integrates well with the existing API version handling mechanism.
359-389: Well-implemented model substitution method.The
modelSubstituteimplementation follows the same pattern as other API methods:
- Updates connection info for Docker compatibility
- Prepares the request body with proper encoding
- Makes the API call with appropriate endpoint and parameters
- Returns typed results or handles errors appropriately
This implementation aligns well with the PR objectives for model substitution.
444-444: Correct API version handling for new functionality.Including
IBIS_API_TYPE.MODEL_SUBSTITUTEin the list of API types that use v3 ensures consistent versioning across related endpoints.
450-470: Excellent centralized error handling implementation.The
throwErrormethod is a great improvement that:
- Centralizes error handling logic across methods
- Consistently includes correlation IDs and process times
- Supports custom error message builders for specific error types
- Creates standardized error structures for better client-side handling
This implementation addresses the PR objective of improving error handling.
472-494: Helpful error message enhancement for model substitution.The
modelSubstituteErrorMessageBuildermethod improves user experience by:
- Detecting specific error types (MODEL_NOT_FOUND, PARSING_EXCEPTION)
- Adding actionable suggestions to error messages
- Preserving original error details for other error types
This implementation directly addresses the PR objective of providing better error messages for model substitution.
199-200: Consistent error handling refactoring across methods.The changes to error handling in existing methods consistently apply the new centralized
throwErrorapproach, which:
- Maintains consistent logging
- Standardizes error structures
- Ensures correlation IDs and process times are included in all errors
This refactoring improves maintainability and consistency.
Also applies to: 232-233, 261-262, 305-306, 327-328, 381-386
wren-ui/src/apollo/server/services/sqlPairService.ts (5)
1-19: Well-structured imports for new functionality.The imports are organized by source, with appropriate grouping of related types and dependencies. The branded types and required interfaces are properly imported to support the new model substitution functionality.
33-37: Clear interface definition for model substitution options.The
ModelSubstituteOptionsinterface is well-defined with:
- A required
projectproperty for context- A required
manifestproperty for model definitions- A helpful comment about the manifest parameter
This provides a clear contract for using the model substitution service.
53-56: Appropriate service interface extension.The
modelSubstitutemethod is well-defined in theISqlPairServiceinterface with:
- Clear parameter names and types
- Use of branded types for type safety
- Proper return type using Promise for asynchronous operation
This maintains the service's contract with its consumers.
62-62: Proper dependency injection for the ibisAdaptor.The
ibisAdaptoris correctly:
- Added as a private property
- Included in the constructor parameter list
- Type-checked with the
IIbisAdaptorinterface- Assigned in the constructor
This follows good dependency injection practices.
Also applies to: 67-67, 71-71, 75-75
78-96: Well-implemented model substitution method.The
modelSubstituteimplementation:
- Properly destructures options to extract needed values
- Checks for unsupported data sources (DuckDB) with a clear error message
- Delegates to the ibisAdaptor with appropriate parameters
- Maintains type safety throughout the implementation
This implementation completes the feature described in the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts (1)
91-114: 🛠️ Refactor suggestionAdd error handling for modelSubstitute method.
The implementation follows the project pattern correctly, but is missing error handling that is present in other methods like
validateSql. There's also an unnecessary type assertion.Add error handling and remove the unnecessary type assertion:
public async modelSubstitute( _root: unknown, arg: { data: { sql: DialectSQL; }; }, ctx: IContext, ): Promise<WrenSQL> { const project = await ctx.projectService.getCurrentProject(); const lastDeployment = await ctx.deployService.getLastDeployment( project.id, ); const manifest = lastDeployment.manifest; - const wrenSQL = await ctx.sqlPairService.modelSubstitute( - arg.data.sql as DialectSQL, - { - project, - manifest, - }, - ); - return wrenSQL; + try { + const wrenSQL = await ctx.sqlPairService.modelSubstitute( + arg.data.sql, + { + project, + manifest, + }, + ); + return wrenSQL; + } catch (err) { + throw Errors.create(Errors.GeneralErrorCodes.INVALID_SQL_ERROR, { + customMessage: err.message, + }); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts(12 hunks)wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts(7 hunks)wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts(2 hunks)wren-ui/src/apollo/server/services/sqlPairService.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts (2)
wren-ui/src/apollo/server/models/adaptor.ts (2)
DialectSQL(8-8)WrenSQL(9-9)wren-ui/src/apollo/server/types/context.ts (1)
IContext(39-78)
wren-ui/src/apollo/server/services/sqlPairService.ts (6)
wren-ui/src/apollo/server/repositories/projectRepository.ts (1)
Project(92-107)wren-ui/src/apollo/server/mdl/type.ts (1)
Manifest(70-78)wren-ui/src/apollo/server/models/adaptor.ts (2)
DialectSQL(8-8)WrenSQL(9-9)wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts (2)
WrenSQL(23-23)IIbisAdaptor(125-159)wren-ui/src/apollo/server/repositories/sqlPairRepository.ts (1)
ISqlPairRepository(13-13)wren-ui/src/apollo/server/adaptors/wrenAIAdaptor.ts (1)
IWrenAIAdaptor(52-132)
🪛 Biome (1.9.4)
wren-ui/src/apollo/server/services/sqlPairService.ts
[error] 91-91: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (20)
wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts (2)
5-5: Good addition of branded types for type safety.The addition of
DialectSQLandWrenSQLtypes provides stronger type checking, making the codebase more robust by ensuring type safety during SQL dialect conversions.
14-14: LGTM - Proper method binding in constructor.Correctly binding the new
modelSubstitutemethod ensuresthiscontext is maintained when the method is invoked.wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts (8)
11-11: Good import for type safety.Importing
DialectSQLtype ensures proper typing for SQL strings in tests, matching the implementation in the adapter.
261-271: LGTM - Updated Trino connection structure.The connection information for Trino has been properly restructured to use separate fields instead of a connection URL, which aligns with the connection handling approach used by the adapter.
401-401: API version update to v3.Correctly updated the PostgreSQL validation API endpoint from
/v2/to/v3/, maintaining consistency with versioning changes.Also applies to: 433-433
564-597: Good test for successful model substitution.This test effectively verifies that the
modelSubstitutemethod works correctly for the happy path scenario, checking both the correct return value and appropriate API call parameters.
599-630: Good error handling test for MODEL_NOT_FOUND.The test properly validates the enhanced error message includes helpful suggestions for the user when a model is not found, and verifies that correlation ID and process time are included in the error metadata.
632-663: Good error handling test for PARSING_EXCEPTION.This test effectively verifies that parsing exceptions include user-friendly suggestions about column quoting, making debugging easier for users.
665-695: Good test for generic error handling.This test confirms proper handling of generic errors, ensuring the original error message is preserved and metadata is included.
697-735: Good test for catalog and schema inclusion.Test effectively verifies that catalog and schema parameters are correctly passed in headers, which is important for context-aware SQL processing.
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts (7)
21-23: Good type handling for SQL representation.Importing and re-exporting the branded types improves type safety across the application while maintaining a clean API.
149-159: Well-defined interface for model substitution.The
modelSubstitutemethod signature in the interface is well-defined with appropriate parameter types and return type, providing a clear contract for implementations.
181-181: Good addition to enum for API type.Adding
MODEL_SUBSTITUTEto theIBIS_API_TYPEenum ensures consistent API versioning handling.
357-361: Good update to validation error handling.Changing the error handling in
validateto return a validation response with details instead of throwing an error provides better flexibility for callers.
363-406: Good implementation of modelSubstitute method.The implementation follows the same pattern as other methods in the class, adding appropriate logging, error handling, and parameter processing. It correctly includes catalog and schema in headers when provided.
467-487: Excellent centralization of error handling.Creating a centralized
throwErrormethod improves error handling consistency across methods and makes the code more maintainable. The inclusion of correlation ID and process time in errors will help with debugging.
489-511: Good error message builder for model substitution.The
modelSubstituteErrorMessageBuilderprovides helpful, context-specific error messages that guide users to resolve common issues like missing models or parsing exceptions.wren-ui/src/apollo/server/services/sqlPairService.ts (3)
33-37: Good interface definition for options.
ModelSubstituteOptionsinterface is well-defined with appropriate documentation comments for optional parameters.
53-57: Clear interface method definition.The
modelSubstitutemethod is well-defined in the interface with appropriate parameter types and return type.
62-76: Good dependency injection in constructor.The addition of
ibisAdaptoras a dependency is properly injected through the constructor, following the dependency injection pattern used in the rest of the service.
| manifest, | ||
| }, | ||
| ); | ||
| return wrenSQL; |
There was a problem hiding this comment.
suggest to format sql for better reading
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts (3)
121-122: Consider implementing the TODOs to fully leverage the branded types.These TODOs indicate future work to replace SQL string types with branded types. Consider implementing these changes in a follow-up PR to fully leverage the type safety benefits across the codebase.
Also applies to: 127-128
377-379: Consider defining header keys as constants.The header keys 'X-User-CATALOG' and 'X-User-SCHEMA' are used as magic strings. Consider defining them as constants at the class or module level to avoid typos and improve maintainability.
+const HEADER_KEYS = { + USER_CATALOG: 'X-User-CATALOG', + USER_SCHEMA: 'X-User-SCHEMA', +}; // Then use them in the headers object: const headers = { - 'X-User-CATALOG': catalog, - 'X-User-SCHEMA': schema, + [HEADER_KEYS.USER_CATALOG]: catalog, + [HEADER_KEYS.USER_SCHEMA]: schema, };
498-504: Improve model name extraction in error message builder.The current approach to extract the model name using string splitting is fragile and might break if the error message format changes. Consider a more robust approach.
if (ModelSubstituteErrorEnum.MODEL_NOT_FOUND()) { - const modelName = message.split(': ')[1]; + // More robust pattern matching to extract model name + const modelNameMatch = message.match(/Model not found: (.*)/); + const modelName = modelNameMatch ? modelNameMatch[1] : 'your_table'; return ( message + `. Try to add catalog and schema in front of your table. eg: my_database.public.${modelName}` ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts(12 hunks)wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts (2)
wren-ui/migrations/20240531085916_transfer_model_properties.js (2)
catalog(14-14)schema(13-13)wren-ui/src/apollo/server/models/adaptor.ts (1)
DialectSQL(8-8)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts (7)
11-11: Good addition of the DialectSQL type import.The addition of the branded type import supports the type safety improvements mentioned in the PR objectives.
261-271: Improved Trino connection info handling.The destructuring approach makes the code more readable and explicit about which properties are being used. The code correctly extracts and maps the properties from the Trino connection info to the expected format.
564-597: Good test coverage for successful model substitution.This test case verifies the basic functionality of model substitution, including checking that the API is called with the correct parameters and headers. The test structure follows the existing patterns in the file.
599-630: Well-implemented error handling test for MODEL_NOT_FOUND.This test properly checks the error message formatting for the MODEL_NOT_FOUND scenario, ensuring it includes the helpful suggestion to add catalog and schema information.
632-663: Good test coverage for PARSING_EXCEPTION error handling.The test confirms that parsing exceptions include helpful guidance about quoting columns with non-alphanumeric characters.
665-695: Proper handling of generic errors.The test verifies that generic errors are properly passed through without additional formatting, maintaining the original error message.
697-735: Good test for catalog and schema header inclusion.This test ensures that when catalog and schema are provided, they are correctly included in the request headers, which is critical for the model substitution functionality.
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts (7)
21-23: Good use of branded types for improved type safety.The import and re-export of the
WrenSQLtype, along with the use ofDialectSQL, enhances type safety as mentioned in the PR objectives.
149-159: Well-designed interface for model substitution.The
modelSubstitutemethod is well-defined with appropriate parameter and return types. The optional catalog and schema parameters align well with the PR objectives.
180-182: IBIS_API_TYPE enum appropriately extended.Adding
MODEL_SUBSTITUTEto the enum is a clean way to extend the API types.
363-406: Well-implemented modelSubstitute method.The method follows the established pattern of other methods in the class and properly handles headers for catalog and schema information. The error handling uses the centralized throwError method with a custom error message builder for improved user guidance.
461-462: Proper API version selection for model substitution.Including
IBIS_API_TYPE.MODEL_SUBSTITUTEin the list of API types that use v3, ensures the correct API version is used for the new functionality.
467-487: Good centralization of error handling logic.The
throwErrormethod centralizes error handling, ensuring consistent error creation with correlation IDs and process times, improving debugging capabilities.
489-511: Well-designed error message builder for model substitution.The error message builder provides helpful guidance for specific error types, helping users troubleshoot issues more effectively. The MODEL_NOT_FOUND handler suggests adding catalog and schema information, while the PARSING_EXCEPTION handler suggests quoting columns with non-alphanumeric characters.
| } catch (e) { | ||
| logger.debug(`Validation error: ${e.response?.data || e.message}`); | ||
| return { valid: false, message: e.response?.data || e.message }; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent error handling approach in validate method.
The validate method returns an error response instead of throwing exceptions, which is inconsistent with other methods that use the throwError function. Consider making the error handling approach consistent across all methods.
public async validate(
dataSource: DataSourceName,
validationRule: ValidationRules,
connectionInfo: WREN_AI_CONNECTION_INFO,
mdl: Manifest,
parameters: Record<string, any>,
): Promise<ValidationResponse> {
connectionInfo = this.updateConnectionInfo(connectionInfo);
const ibisConnectionInfo = toIbisConnectionInfo(dataSource, connectionInfo);
const body = {
connectionInfo: ibisConnectionInfo,
manifestStr: Buffer.from(JSON.stringify(mdl)).toString('base64'),
parameters,
};
try {
logger.debug(`Run validation rule "${validationRule}" with ibis`);
await axios.post(
`${this.ibisServerEndpoint}/${this.getIbisApiVersion(IBIS_API_TYPE.VALIDATION)}/connector/${dataSourceUrlMap[dataSource]}/validate/${snakeCase(validationRule)}`,
body,
);
return { valid: true, message: null };
- } catch (e) {
- logger.debug(`Validation error: ${e.response?.data || e.message}`);
- return { valid: false, message: e.response?.data || e.message };
- }
+ } catch (e) {
+ logger.debug(`Validation error: ${e.response?.data || e.message}`);
+ // Either convert all methods to return errors like this, or make this one throw like the others
+ return { valid: false, message: e.response?.data || e.message };
+ }
}📝 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.
| } catch (e) { | |
| logger.debug(`Validation error: ${e.response?.data || e.message}`); | |
| return { valid: false, message: e.response?.data || e.message }; | |
| } | |
| } | |
| public async validate( | |
| dataSource: DataSourceName, | |
| validationRule: ValidationRules, | |
| connectionInfo: WREN_AI_CONNECTION_INFO, | |
| mdl: Manifest, | |
| parameters: Record<string, any>, | |
| ): Promise<ValidationResponse> { | |
| connectionInfo = this.updateConnectionInfo(connectionInfo); | |
| const ibisConnectionInfo = toIbisConnectionInfo(dataSource, connectionInfo); | |
| const body = { | |
| connectionInfo: ibisConnectionInfo, | |
| manifestStr: Buffer.from(JSON.stringify(mdl)).toString('base64'), | |
| parameters, | |
| }; | |
| try { | |
| logger.debug(`Run validation rule "${validationRule}" with ibis`); | |
| await axios.post( | |
| `${this.ibisServerEndpoint}/${this.getIbisApiVersion(IBIS_API_TYPE.VALIDATION)}/connector/${dataSourceUrlMap[dataSource]}/validate/${snakeCase(validationRule)}`, | |
| body, | |
| ); | |
| return { valid: true, message: null }; | |
| } catch (e) { | |
| logger.debug(`Validation error: ${e.response?.data || e.message}`); | |
| // Either convert all methods to return errors like this, or make this one throw like the others | |
| return { valid: false, message: e.response?.data || e.message }; | |
| } | |
| } |
onlyjackfrost
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
wren-ui/src/apollo/client/graphql/sql.ts (1)
15-19: Fix typo in constant name for model substitution mutationThere's a typo in the constant name
MODEL_SUBSTITUDE(should beMODEL_SUBSTITUTE) while the mutation itself is correctly namedModelSubstitute.-export const MODEL_SUBSTITUDE = gql` +export const MODEL_SUBSTITUTE = gql` mutation ModelSubstitute($data: ModelSubstituteInput!) { modelSubstitute(data: $data) } `;wren-ui/src/components/modals/ImportDataSourceSQLModal.tsx (3)
43-44: Typo in variable name 'modelSubstitudeResult'.There's a spelling error in the variable name. It should be 'modelSubstituteResult' to maintain naming consistency.
- const [substituteDialectSQL, modelSubstitudeResult] = + const [substituteDialectSQL, modelSubstituteResult] = useModelSubstituteMutation();
63-74: Handle error logging more gracefully in form submission.The current implementation logs validation errors to the console, which is fine for development but may expose sensitive information in production environments.
const submit = async () => { form .validateFields() .then(async (values) => { const response = await substituteDialectSQL({ variables: { data: { sql: values.dialectSql } }, }); await onSubmit(response.data?.modelSubstitute); onClose(); }) - .catch(console.error); + .catch(() => { + // Form validation failed, the form will display validation errors + }); };
87-87: Consider updating 'visible' prop to 'open' for Ant Design v4 compatibility.The
visibleprop is deprecated in Ant Design v4 in favor ofopen. Consider updating it if you're using Ant Design v4.- visible={visible} + open={visible}wren-ui/src/hooks/useAutoComplete.tsx (1)
39-52: Consider adding type safety fornodeType.The
convertMentionfunction is well-implemented, but thenodeTypeis used without type checking. Consider creating a union type for possible node types to improve type safety.+type NodeType = 'model' | 'view' | 'field'; + // For mention usage -export const convertMention = (item: (Model | Field) & { parent?: Model }) => { +export const convertMention = (item: (Model | Field) & { parent?: Model, nodeType: NodeType }) => { return { id: `${item.id}-${item.referenceName}`, label: item.displayName, value: item.referenceName, nodeType: capitalize(item.nodeType), meta: item.parent ? `${item.displayName}.${item.displayName}` : undefined, icon: getNodeTypeIcon( { nodeType: item.nodeType, type: (item as Field).type }, { className: 'gray-8 mr-2' }, ), }; };wren-ui/src/components/modals/QuestionSQLPairModal.tsx (2)
67-77: Consider adding loading state for settings.The component correctly uses the settings query to determine if substitution is supported, but there's no handling for the loading state of the query, which could lead to UI flicker.
- const { data: settingsResult } = useGetSettingsQuery(); + const { data: settingsResult, loading: settingsLoading } = useGetSettingsQuery(); const settings = settingsResult?.settings; const dataSource = useMemo( () => ({ isSupportSubstitute: isSupportSubstitute(settings?.dataSource), type: settings?.dataSource?.type, + loading: settingsLoading, }), [settings?.dataSource], );Then update the condition for rendering the toolbar:
- toolbar={ - dataSource.isSupportSubstitute && ( - <Toolbar - dataSource={dataSource.type} - onClick={() => - importDataSourceSQLModal.openModal({ - dataSource: dataSource.type, - }) - } - /> - ) - } + toolbar={ + !dataSource.loading && dataSource.isSupportSubstitute && ( + <Toolbar + dataSource={dataSource.type} + onClick={() => + importDataSourceSQLModal.openModal({ + dataSource: dataSource.type, + }) + } + /> + ) + }
333-341: Consider adding error handling for import submission.When submitting the imported SQL, there's no error handling. Consider adding a try-catch block to handle potential errors during form state updates.
<ImportDataSourceSQLModal {...importDataSourceSQLModal.state} onClose={importDataSourceSQLModal.closeModal} - onSubmit={async (convertedSql: string) => { - form.setFieldsValue({ sql: convertedSql }); + onSubmit={async (convertedSql: string) => { + try { + form.setFieldsValue({ sql: convertedSql }); + } catch (error) { + console.error('Error updating SQL field:', error); + setError({ + message: String(error), + shortMessage: 'Failed to update SQL field' + }); + } }} />wren-ui/src/components/editor/SQLEditor.tsx (2)
50-53: Improve type safety for Ace editor access.The current implementation uses type casting, but you could improve type safety with a more specific interface.
+interface WindowWithAce extends Window { + ace?: { + require: (module: string) => any; + }; +} const getLangTools = () => { - const { ace } = window as any; + const { ace } = window as WindowWithAce; return ace ? ace.require('ace/ext/language_tools') : null; };
110-127: Consider forwarding additional editor props.The component now supports autocomplete and toolbar, but it would be beneficial to allow forwarding other Ace editor props for more customization.
-interface Props { +interface Props extends Omit<React.ComponentProps<typeof AceEditor>, 'onChange' | 'value' | 'mode' | 'theme'> { value?: string; onChange?: (value: string) => void; autoFocus?: boolean; autoComplete?: boolean; toolbar?: React.ReactNode; } // Then in the component: const SQLEditor = (props: Props) => { - const { value, onChange, autoFocus, autoComplete, toolbar } = props; + const { value, onChange, autoFocus, autoComplete, toolbar, ...restProps } = props; // Later in the render: <AceEditor mode="sql" width="100%" height="300px" fontSize={14} theme="tomorrow" value={value || sql} onChange={change} onFocus={() => setFocused(true)} onBlur={() => setFocused(false)} name="sql_editor" editorProps={{ $blockScrolling: true }} enableLiveAutocompletion enableBasicAutocompletion showPrintMargin={false} focus={autoFocus} + {...restProps} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
CONTRIBUTING.md(1 hunks)wren-ui/src/apollo/client/graphql/__types__.ts(4 hunks)wren-ui/src/apollo/client/graphql/sql.generated.ts(2 hunks)wren-ui/src/apollo/client/graphql/sql.ts(1 hunks)wren-ui/src/components/editor/MarkdownEditor.tsx(1 hunks)wren-ui/src/components/editor/SQLEditor.tsx(1 hunks)wren-ui/src/components/modals/AdjustReasoningStepsModal.tsx(2 hunks)wren-ui/src/components/modals/AdjustSQLModal.tsx(1 hunks)wren-ui/src/components/modals/FixSQLModal.tsx(1 hunks)wren-ui/src/components/modals/ImportDataSourceSQLModal.tsx(1 hunks)wren-ui/src/components/modals/QuestionSQLPairModal.tsx(4 hunks)wren-ui/src/components/pages/setup/ConnectDataSource.tsx(1 hunks)wren-ui/src/components/pages/setup/utils.tsx(3 hunks)wren-ui/src/hooks/useAutoComplete.tsx(1 hunks)wren-ui/src/hooks/useMentions.tsx(0 hunks)wren-ui/src/utils/dataSourceType.ts(1 hunks)wren-ui/src/utils/enum/dataSources.ts(1 hunks)wren-ui/src/utils/error/dictionary.ts(1 hunks)
💤 Files with no reviewable changes (1)
- wren-ui/src/hooks/useMentions.tsx
✅ Files skipped from review due to trivial changes (5)
- wren-ui/src/components/editor/MarkdownEditor.tsx
- wren-ui/src/components/modals/FixSQLModal.tsx
- wren-ui/src/utils/enum/dataSources.ts
- wren-ui/src/utils/error/dictionary.ts
- CONTRIBUTING.md
🧰 Additional context used
🧬 Code Graph Analysis (6)
wren-ui/src/components/modals/AdjustSQLModal.tsx (1)
wren-ui/src/components/editor/SQLEditor.tsx (1)
SQLEditor(55-130)
wren-ui/src/components/modals/ImportDataSourceSQLModal.tsx (7)
wren-ui/src/utils/dataSourceType.ts (2)
getDataSourceImage(11-32)getDataSourceName(34-55)wren-ui/src/apollo/client/graphql/__types__.ts (1)
DataSource(227-232)wren-ui/src/apollo/client/graphql/sql.generated.ts (1)
useModelSubstituteMutation(114-117)wren-ui/src/utils/errorHandler.tsx (1)
parseGraphQLError(451-461)wren-ui/src/utils/error/dictionary.ts (1)
ERROR_TEXTS(1-141)wren-ui/src/components/editor/SQLEditor.tsx (1)
SQLEditor(55-130)wren-ui/src/components/ErrorCollapse.tsx (1)
ErrorCollapse(33-56)
wren-ui/src/components/pages/setup/utils.tsx (1)
wren-ui/src/utils/dataSourceType.ts (2)
getDataSourceConfig(80-86)getDataSourceFormComponent(88-92)
wren-ui/src/components/modals/AdjustReasoningStepsModal.tsx (1)
wren-ui/src/hooks/useAutoComplete.tsx (2)
useAutoComplete(75-96)convertMention(40-52)
wren-ui/src/apollo/client/graphql/sql.generated.ts (1)
wren-ui/src/apollo/client/graphql/__types__.ts (2)
Exact(4-4)ModelSubstituteInput(555-557)
wren-ui/src/utils/dataSourceType.ts (6)
wren-ui/src/components/pages/setup/dataSources/PostgreSQLProperties.tsx (1)
PostgreSQLProperties(10-97)wren-ui/src/components/pages/setup/dataSources/MySQLProperties.tsx (1)
MySQLProperties(10-94)wren-ui/src/components/pages/setup/dataSources/SQLServerProperties.tsx (1)
SQLServerProperties(10-103)wren-ui/src/components/pages/setup/dataSources/ClickHouseProperties.tsx (1)
ClickHouseProperties(10-100)wren-ui/src/components/pages/setup/dataSources/TrinoProperties.tsx (1)
TrinoProperties(10-97)wren-ui/src/components/pages/setup/dataSources/SnowflakeProperties.tsx (1)
SnowflakeProperties(9-96)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (25)
wren-ui/src/components/modals/AdjustSQLModal.tsx (1)
181-181: Enhanced SQL editor with autocomplete functionalityThe addition of the
autoCompleteprop to the SQLEditor component enables SQL autocompletion functionality, improving the user experience when writing SQL queries.wren-ui/src/components/pages/setup/ConnectDataSource.tsx (1)
92-92: Updated PostgreSQL data source referenceThe condition has been updated to use
DATA_SOURCES.POSTGRESinstead ofDATA_SOURCES.PG_SQL, aligning with the standardized naming conventions for data sources throughout the codebase.wren-ui/src/components/modals/AdjustReasoningStepsModal.tsx (2)
7-7: Updated import to use new autocomplete hookReplaced the import of the deprecated
useMentionshook with the more versatileuseAutoCompletehook and itsconvertMentionfunction.
39-43: Updated mention generation to use the enhanced autocomplete hookThe component now uses
useAutoCompletewith theconvertMentionconvertor instead of the deprecateduseMentionshook, while maintaining the same functionality. This aligns with the broader refactoring effort to standardize autocomplete functionality across the application.wren-ui/src/apollo/client/graphql/__types__.ts (4)
14-14: Added new DialectSQL scalar type for type safety.This new GraphQL scalar type will be used for the model substitution feature to provide better type checking for SQL in specific dialects.
555-557: Added ModelSubstituteInput type with appropriate typing.The input type correctly uses the new DialectSQL scalar type for the SQL field, ensuring type safety for the model substitution feature.
600-600: Added modelSubstitute mutation that returns SQL string.This mutation will handle the conversion of dialect SQL to Wren SQL as described in the PR objectives.
782-784: Proper argument typing for the modelSubstitute mutation.The arguments are correctly typed to use the ModelSubstituteInput type, maintaining type safety throughout the GraphQL schema.
wren-ui/src/components/modals/ImportDataSourceSQLModal.tsx (3)
17-30: Clean implementation of the Toolbar component with data source branding.The Toolbar component effectively utilizes the utility functions from dataSourceType.ts to display the appropriate logo and name for each data source, providing a consistent user experience.
32-37: Appropriate check for supported data sources in isSupportSubstitute function.The function correctly excludes DuckDB and sample datasets from the model substitution feature, which aligns with the backend implementation constraints.
110-117: Effective error handling with clear user feedback.The error display implementation using Alert and ErrorCollapse components provides good user feedback with both a concise error message and detailed error information when needed.
wren-ui/src/components/pages/setup/utils.tsx (4)
12-15: Good refactoring to use centralized data source utilities.Importing utility functions from dataSourceType.ts improves code organization and maintainability by centralizing data source management logic.
60-64: Simplified data source options with centralized configuration.Using the
getDataSourceConfigutility function for all data sources creates a more consistent and maintainable codebase, reducing duplication and the potential for errors.Also applies to: 65-69, 70-74, 75-79, 80-84, 85-89, 90-94, 95-99
115-117: Simplified getDataSources function with direct object values.The refactored function is cleaner and more straightforward, directly returning the object values without additional mapping.
119-124: Simplified getDataSource function with utility function.The updated function uses the new utility function to get form components dynamically rather than using a hardcoded mapping, making it easier to maintain and extend.
wren-ui/src/utils/dataSourceType.ts (5)
1-9: Good organization of imports for data source property components.The imports are logically organized, bringing in all necessary components for different data sources, which will be referenced in the utility functions.
11-32: Well-structured getDataSourceImage function with comprehensive data source coverage.The function handles all supported data sources with appropriate image paths and includes a default case to handle unexpected inputs gracefully.
34-55: Comprehensive getDataSourceName function with consistent naming conventions.The function provides human-readable names for all supported data sources and includes a default case returning an empty string for unsupported types.
57-78: Centralized component retrieval with getDataSourceProperties function.This function provides a single point of access for all data source property components, making it easier to maintain and extend the system with new data sources.
88-92: Elegant fallback in getDataSourceFormComponent for unsupported data sources.The function gracefully handles cases where a data source doesn't have a properties component by providing a no-op component as a fallback.
wren-ui/src/hooks/useAutoComplete.tsx (1)
35-37: Great regex-based identifier quoting check.The
shouldQuoteIdentifierfunction correctly identifies SQL identifiers that need quoting, which is important for generating valid SQL in autocomplete suggestions.wren-ui/src/components/modals/QuestionSQLPairModal.tsx (2)
37-52: Well-structured toolbar component.The Toolbar component is nicely implemented with clear responsibilities and good integration with the Logo component. It provides a clear UI for importing SQL from the data source.
285-301: Good integration of SQLEditor with new features.The SQLEditor is well integrated with the new features, including conditional toolbar rendering and autocomplete functionality. Excellent use of the new toolbar and autoComplete props.
wren-ui/src/apollo/client/graphql/sql.generated.ts (2)
20-25: Correct type definition for ModelSubstitute mutation.The TypeScript types for the mutation variables and result are correctly defined, ensuring type safety for the model substitution functionality.
90-94: Consistent GraphQL mutation definition.The ModelSubstitute mutation is consistently defined following the same pattern as other mutations in the file. Good job on maintaining code consistency.
| export const getDataSourceConfig = (dataSource: DATA_SOURCES | string) => { | ||
| return { | ||
| label: getDataSourceName(dataSource), | ||
| logo: getDataSourceImage(dataSource), | ||
| value: DATA_SOURCES[dataSource], | ||
| }; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential type safety issue in getDataSourceConfig.
When dataSource is a string that's not a key in DATA_SOURCES, the expression DATA_SOURCES[dataSource] could result in undefined values, which might cause unexpected behavior.
export const getDataSourceConfig = (dataSource: DATA_SOURCES | string) => {
return {
label: getDataSourceName(dataSource),
logo: getDataSourceImage(dataSource),
- value: DATA_SOURCES[dataSource],
+ value: typeof dataSource === 'string' && dataSource in DATA_SOURCES
+ ? DATA_SOURCES[dataSource]
+ : dataSource,
};
};📝 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.
| export const getDataSourceConfig = (dataSource: DATA_SOURCES | string) => { | |
| return { | |
| label: getDataSourceName(dataSource), | |
| logo: getDataSourceImage(dataSource), | |
| value: DATA_SOURCES[dataSource], | |
| }; | |
| }; | |
| export const getDataSourceConfig = (dataSource: DATA_SOURCES | string) => { | |
| return { | |
| label: getDataSourceName(dataSource), | |
| logo: getDataSourceImage(dataSource), | |
| value: typeof dataSource === 'string' && dataSource in DATA_SOURCES | |
| ? DATA_SOURCES[dataSource] | |
| : dataSource, | |
| }; | |
| }; |
| label: item.displayName, | ||
| value: item.referenceName, | ||
| nodeType: capitalize(item.nodeType), | ||
| meta: item.parent ? `${item.displayName}.${item.displayName}` : undefined, |
There was a problem hiding this comment.
Fix metadata concatenation logic.
The current implementation concatenates the display name with itself. This appears to be incorrect - you likely want to show the parent's display name followed by the item's display name.
- meta: item.parent ? `${item.displayName}.${item.displayName}` : undefined,
+ meta: item.parent ? `${item.parent.displayName}.${item.displayName}` : undefined,📝 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.
| meta: item.parent ? `${item.displayName}.${item.displayName}` : undefined, | |
| meta: item.parent ? `${item.parent.displayName}.${item.displayName}` : undefined, |
| export default function useAutoComplete<T = Completer>(props: Props<T>) { | ||
| const { includeColumns, skip } = props; | ||
| const { data } = useDiagramQuery({ skip }); | ||
|
|
||
| // Defined convertor | ||
| const convertor = (props.convertor || convertCompleter) as Convertor<T>; | ||
|
|
||
| return useMemo(() => { | ||
| const models = data?.diagram.models || []; | ||
| const views = data?.diagram.views || []; | ||
|
|
||
| return [...models, ...views].reduce((result, item) => { | ||
| result.push(convertor(item)); | ||
| if (includeColumns) { | ||
| item.fields.forEach((field) => { | ||
| result.push(convertor({ ...field, parent: item })); | ||
| }); | ||
| } | ||
| return result; | ||
| }, [] as T[]); | ||
| }, [data?.diagram, includeColumns]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for missing diagram data.
The hook implementation is clean and follows React best practices with memoization. Consider adding null checking or error handling for cases where the diagram query fails.
export default function useAutoComplete<T = Completer>(props: Props<T>) {
const { includeColumns, skip } = props;
- const { data } = useDiagramQuery({ skip });
+ const { data, error } = useDiagramQuery({ skip });
// Defined convertor
const convertor = (props.convertor || convertCompleter) as Convertor<T>;
return useMemo(() => {
+ if (error) {
+ console.error('Error fetching diagram data for autocomplete:', error);
+ return [];
+ }
+
const models = data?.diagram.models || [];
const views = data?.diagram.views || [];
return [...models, ...views].reduce((result, item) => {
result.push(convertor(item));
if (includeColumns) {
item.fields.forEach((field) => {
result.push(convertor({ ...field, parent: item }));
});
}
return result;
}, [] as T[]);
}, [data?.diagram, includeColumns, error]);
}📝 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.
| export default function useAutoComplete<T = Completer>(props: Props<T>) { | |
| const { includeColumns, skip } = props; | |
| const { data } = useDiagramQuery({ skip }); | |
| // Defined convertor | |
| const convertor = (props.convertor || convertCompleter) as Convertor<T>; | |
| return useMemo(() => { | |
| const models = data?.diagram.models || []; | |
| const views = data?.diagram.views || []; | |
| return [...models, ...views].reduce((result, item) => { | |
| result.push(convertor(item)); | |
| if (includeColumns) { | |
| item.fields.forEach((field) => { | |
| result.push(convertor({ ...field, parent: item })); | |
| }); | |
| } | |
| return result; | |
| }, [] as T[]); | |
| }, [data?.diagram, includeColumns]); | |
| export default function useAutoComplete<T = Completer>(props: Props<T>) { | |
| const { includeColumns, skip } = props; | |
| const { data, error } = useDiagramQuery({ skip }); | |
| // Defined convertor | |
| const convertor = (props.convertor || convertCompleter) as Convertor<T>; | |
| return useMemo(() => { | |
| if (error) { | |
| console.error('Error fetching diagram data for autocomplete:', error); | |
| return []; | |
| } | |
| const models = data?.diagram.models || []; | |
| const views = data?.diagram.views || []; | |
| return [...models, ...views].reduce((result, item) => { | |
| result.push(convertor(item)); | |
| if (includeColumns) { | |
| item.fields.forEach((field) => { | |
| result.push(convertor({ ...field, parent: item })); | |
| }); | |
| } | |
| return result; | |
| }, [] as T[]); | |
| }, [data?.diagram, includeColumns, error]); | |
| } |
| export function useModelSubstituteMutation(baseOptions?: Apollo.MutationHookOptions<ModelSubstituteMutation, ModelSubstituteMutationVariables>) { | ||
| const options = {...defaultOptions, ...baseOptions} | ||
| return Apollo.useMutation<ModelSubstituteMutation, ModelSubstituteMutationVariables>(ModelSubstituteDocument, options); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify DialectSQL scalar type implementation.
This hook correctly uses the mutation with the new DialectSQL scalar type. Make sure the scalar implementation correctly validates the input SQL to prevent security issues like SQL injection.
🏁 Script executed:
#!/bin/bash
# Check the scalar implementation for DialectSQL
# Look for sanitization and validation logic
echo "Searching for DialectSQL scalar implementation..."
rg --type ts "DialectSQL" -A 10 -B 10
# Looking for security validation in scalar implementation
echo "Checking for validation or sanitization in the scalar implementation..."
rg --type ts "DialectSQL.*validateValue|DialectSQL.*serialize|sanitize|validate" -A 5Length of output: 84758
Verify SQL scalar security and validation
The DialectSQLScalar in wren-ui/src/apollo/server/scalars.ts currently only checks for a string type and does not enforce any SQL syntax validation or sanitization. As a result, malicious payloads could reach your resolver and AI adaptor unfiltered. To mitigate injection risks, please:
- Enhance
DialectSQLScalar(or upstream input handling) to parse and validate the SQL string (e.g. with a safe‑SQL parser or whitelist of allowed statements). - Ensure the
modelSubstituteresolver inwren-ui/src/apollo/server/resolvers/sqlPairResolver.tscalls your existingvalidateSqllogic before forwarding the query. - Add or update tests to cover invalid or dangerous SQL inputs for both the scalar and the resolver.
| const Wrapper = styled.div` | ||
| transition: all 0.3s cubic-bezier(0.645, 0.045, 0.355, 1); | ||
|
|
||
| &:hover { | ||
| border-color: var(--geekblue-5) !important; | ||
| } | ||
|
|
||
| &.adm-markdown-editor-error { | ||
| border-color: var(--red-5) !important; | ||
|
|
||
| .adm-markdown-editor-length { | ||
| color: var(--red-5) !important; | ||
| } | ||
| } | ||
| &:not(.adm-markdown-editor-error).adm-markdown-editor-focused { | ||
| border-color: var(--geekblue-5) !important; | ||
| box-shadow: 0 0 0 2px rgba(47, 84, 235, 0.2); | ||
| } | ||
|
|
||
| &.adm-markdown-editor-focused.adm-markdown-editor-error { | ||
| borer-color: var(--red-4) !important; | ||
| box-shadow: 0 0 0 2px rgba(255, 77, 79, 0.2); | ||
| } | ||
| `; |
There was a problem hiding this comment.
Fix typo in styled component CSS.
There's a typo in the CSS for the error focus state.
&.adm-markdown-editor-focused.adm-markdown-editor-error {
- borer-color: var(--red-4) !important;
+ border-color: var(--red-4) !important;
box-shadow: 0 0 0 2px rgba(255, 77, 79, 0.2);
}📝 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.
| const Wrapper = styled.div` | |
| transition: all 0.3s cubic-bezier(0.645, 0.045, 0.355, 1); | |
| &:hover { | |
| border-color: var(--geekblue-5) !important; | |
| } | |
| &.adm-markdown-editor-error { | |
| border-color: var(--red-5) !important; | |
| .adm-markdown-editor-length { | |
| color: var(--red-5) !important; | |
| } | |
| } | |
| &:not(.adm-markdown-editor-error).adm-markdown-editor-focused { | |
| border-color: var(--geekblue-5) !important; | |
| box-shadow: 0 0 0 2px rgba(47, 84, 235, 0.2); | |
| } | |
| &.adm-markdown-editor-focused.adm-markdown-editor-error { | |
| borer-color: var(--red-4) !important; | |
| box-shadow: 0 0 0 2px rgba(255, 77, 79, 0.2); | |
| } | |
| `; | |
| const Wrapper = styled.div` | |
| transition: all 0.3s cubic-bezier(0.645, 0.045, 0.355, 1); | |
| &:hover { | |
| border-color: var(--geekblue-5) !important; | |
| } | |
| &.adm-markdown-editor-error { | |
| border-color: var(--red-5) !important; | |
| .adm-markdown-editor-length { | |
| color: var(--red-5) !important; | |
| } | |
| } | |
| &:not(.adm-markdown-editor-error).adm-markdown-editor-focused { | |
| border-color: var(--geekblue-5) !important; | |
| box-shadow: 0 0 0 2px rgba(47, 84, 235, 0.2); | |
| } | |
| &.adm-markdown-editor-focused.adm-markdown-editor-error { | |
| border-color: var(--red-4) !important; | |
| box-shadow: 0 0 0 2px rgba(255, 77, 79, 0.2); | |
| } | |
| `; |
| useEffect(() => { | ||
| resetCompleters(); | ||
| if (!autoComplete || completers.length === 0) return; | ||
|
|
||
| const langTools = getLangTools(); | ||
| const customCompleter = { | ||
| getCompletions: (_editor, _session, _pos, _prefix, callback) => { | ||
| callback(null, completers); | ||
| }, | ||
| }; | ||
| langTools?.addCompleter(customCompleter); | ||
|
|
||
| return () => resetCompleters(); | ||
| }, [focused, autoComplete, completers]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure proper cleanup of Ace completer.
The cleanup function resets completers, but there might be a race condition if the component unmounts during initialization. Consider adding a cleanup flag.
useEffect(() => {
+ let isActive = true;
resetCompleters();
if (!autoComplete || completers.length === 0) return;
const langTools = getLangTools();
const customCompleter = {
getCompletions: (_editor, _session, _pos, _prefix, callback) => {
+ if (!isActive) return;
callback(null, completers);
},
};
langTools?.addCompleter(customCompleter);
- return () => resetCompleters();
+ return () => {
+ isActive = false;
+ resetCompleters();
+ };
}, [focused, autoComplete, completers]);📝 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.
| useEffect(() => { | |
| resetCompleters(); | |
| if (!autoComplete || completers.length === 0) return; | |
| const langTools = getLangTools(); | |
| const customCompleter = { | |
| getCompletions: (_editor, _session, _pos, _prefix, callback) => { | |
| callback(null, completers); | |
| }, | |
| }; | |
| langTools?.addCompleter(customCompleter); | |
| return () => resetCompleters(); | |
| }, [focused, autoComplete, completers]); | |
| useEffect(() => { | |
| let isActive = true; | |
| resetCompleters(); | |
| if (!autoComplete || completers.length === 0) return; | |
| const langTools = getLangTools(); | |
| const customCompleter = { | |
| getCompletions: (_editor, _session, _pos, _prefix, callback) => { | |
| if (!isActive) return; | |
| callback(null, completers); | |
| }, | |
| }; | |
| langTools?.addCompleter(customCompleter); | |
| return () => { | |
| isActive = false; | |
| resetCompleters(); | |
| }; | |
| }, [focused, autoComplete, completers]); |
3 participants
Description
feat: Add model substitution support and improve error handling
Key changes:
DialectSQLandWrenSQLfor type safetyTechnical details:
Screenshot
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests
Refactor