feat: complete adding support for parameterized and registered queries #45
Conversation
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
WalkthroughThis pull request introduces enhancements to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant QueryEngine
participant QueryStore
User->>QueryEngine: Execute query
QueryEngine->>QueryStore: Check if query is registered
alt Query is registered
QueryStore-->>QueryEngine: Return query
QueryEngine-->>User: Execute and return results
else Query is not registered
alt Allow unregistered queries
QueryEngine-->>User: Execute and return results
else Do not allow
QueryEngine-->>User: Throw error
end
end
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (7)
packages/queries/src/util/iterateRecursively.ts (1)
21-22: Consider more descriptive names or specific wrappers.The TODO comment raises a valid point about the clarity of the function name
iterateRecursively. Here are some suggestions:
More descriptive names:
traverseNestedStructuredeepTraversalrecursiveVisitorSpecific named wrappers:
findNestedValue(traversable, predicate)transformNestedStructure(traversable, transformer)flattenNestedStructure(traversable)These wrappers can provide a more intuitive API for common use cases while using
iterateRecursivelyinternally.packages/backend/src/QueryEngine.test.ts (1)
1-9: LGTM!The imports and test case setup look good. The test case is properly defined using the Vitest functions.
Please address the TODO comment in the future to complete the tests and consider moving them to an appropriate location if necessary.
packages/backend/src/QueryEngine.ts (4)
90-93: LGTM!The new property
dangerouslyAllowUnregisteredQueriesis used to allow the execution of unregistered queries and the TODO comment indicates that the callback return type of thequeriesproperty needs to be fixed.Do you want me to open a GitHub issue to track the TODO comment?
151-159: LGTM!The conditional check ensures that only registered queries are executed if
dangerouslyAllowUnregisteredQueriesisfalse, which is a reasonable safety measure. Thequeriesmap is used to check if the query has been registered, which is a reasonable approach.Do you want me to open a GitHub issue to track the TODO comment?
188-196: LGTM!The conditional check ensures that only registered queries are executed if
dangerouslyAllowUnregisteredQueriesisfalse, which is a reasonable safety measure. Thequeriesmap is used to check if the query has been registered, which is a reasonable approach.Do you want me to open a GitHub issue to track the TODO comment?
Line range hint
208-301: LGTM!The new
registerQueriesmethod provides a way to register query functions, which is a useful feature. The logic of iterating over the provided query functions, executing each function to get the query, checking if the query has a hash, and adding the query function to thequeriesmap using the query hash as the key is a reasonable approach to register query functions.Do you want me to open GitHub issues to track the TODO comments?
packages/queries/src/query.ts (1)
Line range hint
66-100: Excellent addition for handling parameterized queries!The new code block introduces a robust mechanism to identify and assign identifiers to query parameters within the constructed query object. The use of
iterateRecursivelyensures that all nested parameters are properly handled, and theisQueryParameterfunction provides a clear validation step.Assigning identifiers based on the parameter's path is a good approach for uniquely identifying each parameter within the query structure.
Regarding the TODO comment:
Consider extracting the parameter identification logic into a separate function with a descriptive name and clear documentation. This will improve the readability and maintainability of the code.
For example:
function assignParameterIdentifiers(query: QueryObject) { // Documentation: Recursively iterates over the query object and assigns identifiers to query parameters based on their path. iterateRecursively(query, (value, path) => { if (isQueryParameter(value)) { value.id = path.join('.'); } }); }Then, you can call this function within the
buildmethod:const query = { // ... }; assignParameterIdentifiers(query); return query;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- packages/backend/src/QueryEngine.test.ts (1 hunks)
- packages/backend/src/QueryEngine.ts (10 hunks)
- packages/backend/src/SynthqlError.ts (5 hunks)
- packages/backend/src/execution/executors/PgExecutor/PgExecutor.test.ts (5 hunks)
- packages/docs/static/reference/assets/navigation.js (1 hunks)
- packages/docs/static/reference/assets/search.js (1 hunks)
- packages/queries/src/index.ts (1 hunks)
- packages/queries/src/param.ts (1 hunks)
- packages/queries/src/query.test.ts (2 hunks)
- packages/queries/src/query.ts (3 hunks)
- packages/queries/src/types/QueryParameter.ts (1 hunks)
- packages/queries/src/types/QueryRequest.ts (1 hunks)
- packages/queries/src/util/iterateRecursively.test.ts (1 hunks)
- packages/queries/src/util/iterateRecursively.ts (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- packages/backend/src/execution/executors/PgExecutor/PgExecutor.test.ts
- packages/docs/static/reference/assets/navigation.js
- packages/docs/static/reference/assets/search.js
- packages/queries/src/index.ts
- packages/queries/src/query.test.ts
Additional comments not posted (23)
packages/queries/src/types/QueryParameter.ts (1)
3-3: Verify the impact of restrictingidto string type.The change to restrict the
idproperty to only accept string values is a good move for type safety and consistency.However, please ensure that this change does not break any existing code that may be using numeric
idvalues. Run the following script to verify:Verification successful
Approve the change:
idrestriction to string type is safeThe change to restrict the
idproperty to only accept string values inpackages/queries/src/types/QueryParameter.tsappears to be safe and localized. Our verification process didn't find any direct usage of numericidvalues that would be affected by this change. The numeric IDs found in the codebase are mostly entity-specific (e.g., actor_id, film_id) and not related to the genericidproperty we're modifying.This change will improve type safety and consistency in the QueryParameter interface without breaking existing code.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if numeric `id` values are used in the codebase. # Test: Search for numeric `id` usage. Expect: No occurrences of numeric `id`. rg --type typescript $'id:(\s+\d+|\s+\d+\.\d+)'Length of output: 83
Script:
#!/bin/bash # Description: Verify if numeric `id` values are used in the codebase. # Find TypeScript files and search for numeric `id` usage fd -e ts -e tsx | xargs rg 'id:\s*(\d+|\d+\.\d+)' -n # Also search for potential type definitions that allow number type for id fd -e ts -e tsx | xargs rg 'id:\s*(number|string\s*\|\s*number|number\s*\|\s*string)' -nLength of output: 13188
packages/queries/src/param.ts (1)
3-5: Verify the impact of the function signature change on the codebase.The function signature has been updated to make the
valueparameter optional and remove theidparameter. This change might have affected other parts of the codebase where the function is called with an explicitidargument.Run the following script to verify the function usage:
Verification successful
Function signature change appears compatible with existing usage
Based on the search results, the updated
param()function signature seems to be compatible with its current usage across the codebase. Key observations:
- No instances of
param()being called with two arguments were found, which would have been incompatible with the new signature.- Both
param()andparam(value)usages are present, which are supported by the new optional parameter.- Most occurrences are in test files, reducing the risk of breaking changes in production code.
However, to ensure no edge cases were missed:
- Manually review the changes in non-test files, particularly in
packages/queries/src/types/Where.ts.- Verify that removing the
idparameter doesn't cause any unexpected behavior in existing queries.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `param` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type typescript -A 5 $'param\('Length of output: 72
Script:
#!/bin/bash # Description: Verify all function calls to `param` match the new signature using multiple search methods. # Method 1: Use ripgrep without file type specification echo "Method 1: Ripgrep search" rg -n 'param\(' --type-add 'ts:*.ts' --type-add 'tsx:*.tsx' -g '*.{ts,tsx}' # Method 2: Use find and grep echo "Method 2: Find and grep search" find . -name '*.ts' -o -name '*.tsx' | xargs grep -n 'param(' # Method 3: Use fd and grep echo "Method 3: fd and grep search" fd -e ts -e tsx | xargs grep -n 'param('Length of output: 5504
packages/queries/src/types/QueryRequest.ts (3)
3-6: LGTM!The
RegularQueryRequestinterface is well-defined and follows TypeScript best practices. Thetypeproperty helps identify the type of query request, and thequeryproperty allows for flexibility in the query structure.
8-12: LGTM!The
RegisteredQueryRequestinterface is well-defined and follows TypeScript best practices. Thetypeproperty helps identify the type of query request, thequeryIdproperty allows for unique identification of the query, and theparamsproperty allows for flexibility in the parameter structure. The generic type parameterTValuewith a default value ofunknownprovides type safety and flexibility for the parameter values.
14-14: LGTM!The
QueryRequesttype is a well-defined union of theRegularQueryRequestandRegisteredQueryRequestinterfaces. This allows for the application to handle both types of query requests interchangeably and follows TypeScript best practices.packages/queries/src/util/iterateRecursively.ts (2)
6-19: LGTM!The
PrimitiveandTraversabletype definitions are clear and comprehensive. TheTraversabletype correctly allows for recursive nesting of objects and arrays.
23-55: LGTM!The implementation of the
iterateRecursivelyfunction is well-structured and handles the traversal of complex data structures effectively. The recursive approach enables deep traversal of nested structures, and the visitor function allows for custom processing of each element.The function correctly handles both arrays and objects, and it uses appropriate type checks to ensure the traversable is an object or an array. It also handles the case when the traversable is
nullor an instance ofDate.Overall, the implementation is solid and should work as intended.
packages/backend/src/QueryEngine.test.ts (2)
10-16: LGTM!The query parameters are properly defined in the
paramsobject, following a consistent naming convention. The hardcoded values are acceptable for the purpose of the test case.
18-41: LGTM!The
findFilmActorquery function is well-structured and correctly uses the@synthql/querieslibrary to construct a complex query with nested relationships. The use ofparam,col, andmaybefunctions ensures that the query references the correct parameters, columns, and optional relationships.packages/backend/src/SynthqlError.ts (5)
26-39: LGTM!The
createCardinalityErrormethod is well-structured and provides a clear error message with a helpful hint. The error type and status code are appropriate for the described scenarios.
64-85: Looks good!The updated
createJsonParsingErrormethod now provides more context by including the erroneous JSON string in the error message. This will help with troubleshooting JSON parsing issues. The additional guidance in the error message is also helpful.
87-92: Looks good to me!The
createMissingHashErrormethod is straightforward and provides a clear error message for the case when a query is missing its hash during registration.
132-135: Nice update!The additional guidance in the error message is valuable for troubleshooting response streaming errors. It covers important points like checking read access and ensuring all queries are registered.
141-152: Great refactoring!Renaming the method to
createSqlExecutionErrorbetter reflects its purpose. The use of thecomposeMessagefunction to generate a detailed error message is a good improvement. Including the error details, SQL query, parameters, and the original SynthQL query will greatly aid in debugging SQL execution errors.packages/backend/src/QueryEngine.ts (7)
2-9: LGTM!The imported types are used in the file and the imports look good.
22-26: LGTM!The JSDoc comment provides a clear description of the
urlproperty.
37-40: LGTM!The new property
dangerouslyAllowUnregisteredQueriesis well-documented and the default value offalseensures that only registered queries are executed by default. The property name and JSDoc comment clearly indicate the potential risks of using this feature.
Line range hint
49-72: LGTM!The JSDoc comment provides a clear description of the
providersproperty and the example demonstrates how to use it.
73-77: LGTM!The JSDoc comment provides a clear description of the
poolproperty and indicates that it can be used as an alternative to theurlproperty.
103-107: LGTM!The
schema,dangerouslyAllowUnregisteredQueries, andprependSqlproperties are set to reasonable default values if the corresponding config properties are not provided.
122-132: LGTM!The updated
compilemethod returns an object withsqlandparamsproperties, which is a clearer and more explicit return type. ThecomposeQueryfunction is used to generate the SQL query, which is a reasonable approach.packages/queries/src/query.ts (2)
12-13: LGTM!The new imports are relevant and properly defined.
Line range hint
1-11: Reviewed the rest of the file.No further comments.
Also applies to: 14-65, 101-587
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
jimezesinachi
changed the title
WhereClause type to include QueryParameter
There was a problem hiding this comment.
Actionable comments posted: 7
Outside diff range and nitpick comments (3)
packages/handler-next/src/createNextSynthqlHandler.ts (1)
Line range hint
176-185: Address the TODO regarding error handling in streaming responsesThere is a TODO comment indicating that the error handling logic within the
writeResponseBodyfunction for streaming responses is not effective, as errors thrown inside theNextResponsestream are propagated differently. Consider revising the streaming logic to appropriately handle errors during streaming, ensuring that clients receive meaningful error information.Do you need assistance in refactoring the streaming error handling? If you'd like, I can help propose a solution or open a GitHub issue to track this task.
packages/react/src/createBody.ts (2)
13-24: Simplify error message construction using template literalsFor improved readability, consider using template literals instead of joining array elements with
'\n'.Apply this refactor to streamline the code:
- throw new Error( - [ - 'Missing value error!', - '', - 'No value passed for the parameter:', - '', - JSON.stringify(x.id, null, 2), - '', - ].join('\n'), - ); + throw new Error(`Missing value error! No value passed for the parameter: ${JSON.stringify(x.id, null, 2)} `);
32-43: Use template literals for cleaner error messagesConsistent with the previous suggestion, using template literals here enhances code clarity.
Here's how you can refactor the code:
- throw new Error( - [ - 'Missing hash error!', - '', - 'The query:', - '', - JSON.stringify(query, null, 2), - '', - 'is missing its `hash` property', - '', - ].join('\n'), - ); + throw new Error(`Missing hash error! The query: ${JSON.stringify(query, null, 2)} is missing its \`hash\` property `);
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- packages/docs/static/reference/assets/navigation.js (1 hunks)
- packages/docs/static/reference/assets/search.js (1 hunks)
- packages/handler-express/src/createExpressSynthqlHandler.ts (5 hunks)
- packages/handler-next/src/createNextSynthqlHandler.test.ts (1 hunks)
- packages/handler-next/src/createNextSynthqlHandler.ts (5 hunks)
- packages/queries/src/index.ts (1 hunks)
- packages/queries/src/types/QueryParameter.ts (1 hunks)
- packages/queries/src/types/QueryRequest.ts (1 hunks)
- packages/queries/src/validators/isQueryParameter.ts (1 hunks)
- packages/queries/src/validators/isRegisteredQuery.ts (1 hunks)
- packages/queries/src/validators/isRegularQuery.ts (1 hunks)
- packages/react/src/createBody.ts (1 hunks)
- packages/react/src/useSynthql.ts (2 hunks)
- packages/react/src/useSynthqlExamples.test.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- packages/handler-next/src/createNextSynthqlHandler.test.ts
Files skipped from review as they are similar to previous changes (5)
- packages/docs/static/reference/assets/navigation.js
- packages/docs/static/reference/assets/search.js
- packages/queries/src/index.ts
- packages/queries/src/types/QueryParameter.ts
- packages/queries/src/types/QueryRequest.ts
Additional comments not posted (12)
packages/queries/src/validators/isQueryParameter.ts (1)
3-5: LGTM!The
isQueryParameterfunction is a well-implemented type guard that enhances type safety and helps prevent runtime errors. The logic is correct, and the use of optional chaining is a good practice to safely access thetypeproperty.The function signature correctly uses the
x is QueryParameterreturn type to enable type narrowing when the function returns true, which can improve the overall reliability of the application by ensuring that only valid query parameters are processed.packages/queries/src/validators/isRegularQuery.ts (1)
3-5: LGTM!The
isRegularQueryRequestfunction is a well-implemented type guard that enhances type safety and provides a reusable way to validate if an input is aRegularQueryRequest. The function correctly combines multiple conditions using the&&operator and safely accesses thetypeproperty ofxusing the optional chaining operator. The function signature correctly uses thex is RegularQueryRequestreturn type to indicate that it is a type guard.packages/queries/src/validators/isRegisteredQuery.ts (1)
3-5: Great use of a type guard function to enhance type safety!The
isRegisteredQueryRequestfunction is a well-implemented type guard that validates if the inputxconforms to theRegisteredQueryRequesttype. It correctly checks fornullandundefinedvalues before accessing thetypeproperty ofxusing the optional chaining operator?., which is a good practice to avoid potential runtime errors.By comparing the
typeproperty ofxwith theRegisteredQueryconstant, the function ensures that only valid query requests are processed further in the application. This is crucial for maintaining type safety and preventing errors associated with invalid data structures.The use of a type guard function allows TypeScript to infer the type of
xasRegisteredQueryRequestwhen the function returnstrue, providing better type information and enabling more accurate type checking in the code that uses this function.Overall, this is a great addition to the codebase that enhances type safety and improves the reliability of the application.
packages/react/src/useSynthql.ts (1)
6-6: Excellent encapsulation of body creation logic!The introduction of the
createBodyfunction and its usage to construct the request body is a great improvement. It enhances maintainability and readability by centralizing the logic for body creation. This change also allows for more flexibility in case complex body structures or additional processing is required in the future.Also applies to: 38-38
packages/react/src/useSynthqlExamples.test.tsx (1)
14-14: Verify the intentional change in the request object structure.The change in accessing the
whereclause fromreq.where?.id.intoreq.query.where?.id.insuggests a shift in the expected structure of the request object.Please confirm if this change is intentional and aligns with the expected behavior of the
createEchoServerfunction. Also, consider the potential impact on the filtering logic applied to the incoming data.To verify the usage of the
req.query.whereproperty across the codebase, run the following script:Verification successful
Verified: Change in request object structure is intentional and localized
The modification from
req.where?.id.intoreq.query.where?.id.inappears to be an intentional and isolated change. This alteration is consistent throughout the codebase, with no other occurrences of the oldreq.wherestructure found.Key observations:
- The change is localized to the test file
packages/react/src/useSynthqlExamples.test.tsx.- No other instances of
req.wherewere found in the codebase.Given that this change is in a test file, it likely reflects an update in the API or request structure being tested. The localized nature of the change suggests it doesn't impact the broader codebase.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `req.query.where` property. # Test: Search for the usage of `req.query.where`. # Expect: Consistent usage across the codebase. rg --type typescript $'req\.query\.where'Length of output: 77
Script:
#!/bin/bash # Description: Verify the usage of `req.query.where` property in TypeScript and TSX files. # Find all .ts and .tsx files and search for 'req.query.where' fd -e ts -e tsx | xargs rg -n -C 2 'req\.query\.where' # Additionally, search for 'req.where' to compare usage echo "Searching for 'req.where':" fd -e ts -e tsx | xargs rg -n -C 2 'req\.where'Length of output: 661
packages/handler-express/src/createExpressSynthqlHandler.ts (1)
118-127: Implementation oftryExecuteRegisteredQuerylooks goodThe new
tryExecuteRegisteredQueryfunction correctly callsqueryEngine.executeRegisteredQuerywith the appropriate parameters. This addition enhances the handling of registered queries.packages/handler-next/src/createNextSynthqlHandler.ts (5)
3-6: Import statements are correct and necessaryThe new imports of
isRegisteredQueryRequestandisRegularQueryRequestare necessary for handling the differentiation between registered and regular query requests.
70-70: Refactored request parsing for clarityThe usage of
tryParseRequestto destructure{ body, headers }improves code readability and isolates request parsing logic.
76-92: Appropriate differentiation between registered and regular queriesThe updated logic correctly distinguishes between registered queries and regular queries using
isRegisteredQueryRequest(body)andisRegularQueryRequest(body). This enhances the flexibility of request handling.
97-101: Function call updated to usewriteResponseBodyThe function
writeBodyhas been renamed towriteResponseBody, and the call has been updated accordingly. This improves clarity regarding the function's purpose.
132-141: New functiontryExecuteRegisteredQueryadded correctlyThe
tryExecuteRegisteredQueryfunction is appropriately defined to handle registered queries, delegating execution toqueryEngine.executeRegisteredQuery.packages/react/src/createBody.ts (1)
11-28: Function implementation is robust and well-structuredThe
createBodyfunction effectively constructs aQueryRequestbased on the providedAnyQuery. It correctly handles parameter extraction and necessary error checks.Also applies to: 30-56
…rty tests Signed-off-by: Jim Ezesinachi <ezesinachijim@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range and nitpick comments (11)
packages/backend/src/tests/propertyBased/arbitraries/arbitraryWhere.ts (2)
12-12: LGTM! Consider updating documentation.The addition of the
parameterizeparameter aligns well with the PR objectives of adding support for parameterized queries. This boolean parameter provides a clear way to toggle query parameterization.Consider updating the function's JSDoc comments (if they exist) to include information about this new parameter and its purpose.
Also applies to: 18-18
checkIfDateTimeColumnfunction still exists in the codebaseThe
checkIfDateTimeColumnfunction was not fully removed and is still defined inpackages/backend/src/tests/propertyBased/checkIfDateTimeColumn.ts. Please ensure that the removal of date-time column filtering logic is complete and that the function is no longer necessary.Analysis chain
Line range hint
20-38: Verify removal of date-time column filtering logic.The AI-generated summary mentions the removal of date-time column filtering logic using the
checkIfDateTimeColumnfunction. However, this change is not visible in the provided code snippet. We need to verify if this change was actually made and if it's intentional.Let's check for any remaining usage of
checkIfDateTimeColumnin this file:If the function is indeed removed, we should also verify that this removal doesn't negatively impact the handling of date-time columns in where clauses.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining usage of checkIfDateTimeColumn in the file rg --type typescript 'checkIfDateTimeColumn' packages/backend/src/tests/propertyBased/arbitraries/arbitraryWhere.tsLength of output: 150
Script:
#!/bin/bash # Description: Check for any remaining usage of checkIfDateTimeColumn in the specified file rg 'checkIfDateTimeColumn' packages/backend/src/tests/propertyBased/arbitraries/arbitraryWhere.tsLength of output: 97
Script:
#!/bin/bash # Description: Check for any remaining usage of checkIfDateTimeColumn across the entire codebase rg 'checkIfDateTimeColumn'Length of output: 136
packages/backend/src/tests/propertyBased/properties/cardinalityMaybe.test.ts (3)
18-23: Good addition of parameterized query test.The introduction of
validAndParameterizedWhereArbitraryQueryis a valuable addition to the test suite. It allows for testing of valid parameterized queries, which increases the coverage and robustness of the tests.Consider extracting the common configuration options into a separate object to reduce duplication:
const baseQueryConfig = { schema, allTablesRowsMap: await getTableRowsByTableName<DB>(pool, schema), cardinality: 'maybe', validWhere: true, }; const validWhereArbitraryQuery = arbitraryQuery<DB>({ ...baseQueryConfig, parameterize: false, }); const validAndParameterizedWhereArbitraryQuery = arbitraryQuery<DB>({ ...baseQueryConfig, parameterize: true, });This approach would make it easier to maintain and update the configurations in the future.
34-40: Good addition of invalid parameterized query test.The introduction of
invalidAndParameterizedWhereArbitraryQueryis a valuable addition to the test suite. It allows for testing of invalid parameterized queries, which increases the coverage and robustness of the tests.Similar to the suggestion for the valid queries, consider extracting the common configuration options into a separate object to reduce duplication:
const baseInvalidQueryConfig = { schema, allTablesRowsMap: await getTableRowsByTableName<DB>(pool, schema), cardinality: 'maybe', validWhere: false, }; const invalidWhereArbitraryQuery = arbitraryQuery<DB>({ ...baseInvalidQueryConfig, parameterize: false, }); const invalidAndParameterizedWhereArbitraryQuery = arbitraryQuery<DB>({ ...baseInvalidQueryConfig, parameterize: true, });This approach would make it easier to maintain and update the configurations in the future.
Clarification Needed on Skipped Test in
cardinalityMaybe.test.tsThe skipped test
it.skip.propincardinalityMaybe.test.tsdoes not have a TODO comment explaining the reason for skipping. It's important to document why this test is skipped to maintain clarity and ensure it is addressed in the future.
- Action Required: Please add a TODO comment or reference an existing issue that explains why the test is skipped and what needs to be done to re-enable it.
Analysis chain
Line range hint
42-84: Good update to include parameterized queries in tests, but clarification needed on skipped test.The changes to incorporate both parameterized and non-parameterized queries in the test cases are well-implemented. This update provides more comprehensive coverage of the
cardinalityMaybefunctionality.The structure and assertions of the tests remain consistent, which is good for maintaining the integrity of the test suite.
However, the invalid query test has been skipped using
it.skip.prop. Could you please clarify why this test is being skipped? Is it temporary, or are there known issues with invalid parameterized queries that need to be addressed?To investigate this further, let's check for any related TODO comments or issues:
If this is a temporary measure, consider adding a TODO comment explaining why the test is skipped and when it should be re-enabled.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for TODO comments related to skipped tests or invalid queries rg --type typescript "TODO.*skip.*test|TODO.*invalid.*quer" packages/backend/src/testsLength of output: 123
Script:
#!/bin/bash # Search for TODO comments related to skipped tests or invalid queries in TypeScript files rg --glob '*.ts' 'TODO.*skip.*test|TODO.*invalid.*quer' packages/backend/src/testsLength of output: 193
Script:
#!/bin/bash # Search for TODO comments near the skipped test in cardinalityMaybe.test.ts rg 'it\.skip\.prop' packages/backend/src/tests/propertyBased/properties/cardinalityMaybe.test.ts -A 2Length of output: 171
packages/backend/src/tests/propertyBased/properties/cardinalityMany.test.ts (2)
18-23: LGTM: Good addition of parameterized query generator.The introduction of
validAndParameterizedWhereArbitraryQueryis a valuable addition to the test suite. It allows testing of parameterized queries with valid where clauses, which is crucial for ensuring the system handles both parameterized and non-parameterized queries correctly.Consider extracting the common configuration options into a separate object to reduce duplication and improve maintainability. For example:
const baseQueryConfig = { schema, allTablesRowsMap: await getTableRowsByTableName<DB>(pool, schema), cardinality: 'many', validWhere: true, }; const validWhereArbitraryQuery = arbitraryQuery<DB>({ ...baseQueryConfig, parameterize: false, }); const validAndParameterizedWhereArbitraryQuery = arbitraryQuery<DB>({ ...baseQueryConfig, parameterize: true, });This approach would make it easier to maintain consistent configurations across different query generators.
34-39: LGTM: Comprehensive coverage with parameterized invalid query generator.The introduction of
invalidAndParameterizedWhereArbitraryQuerycompletes the set of query generators, providing full coverage for both valid and invalid queries in parameterized and non-parameterized forms. This is excellent for ensuring robust testing of the query engine.As suggested earlier, consider extracting common configuration options to reduce duplication. This applies to both valid and invalid query generators:
const baseQueryConfig = { schema, allTablesRowsMap: await getTableRowsByTableName<DB>(pool, schema), cardinality: 'many', }; const invalidWhereArbitraryQuery = arbitraryQuery<DB>({ ...baseQueryConfig, validWhere: false, parameterize: false, }); const invalidAndParameterizedWhereArbitraryQuery = arbitraryQuery<DB>({ ...baseQueryConfig, validWhere: false, parameterize: true, });This approach would make the configurations more maintainable and reduce the risk of inconsistencies.
packages/backend/src/tests/propertyBased/properties/cardinalityOne.test.ts (1)
43-46: Efficient test case update for both query types.Great job on updating the
it.proptest case to include bothvalidWhereArbitraryQueryandvalidAndParameterizedWhereArbitraryQuery. This efficiently tests both parameterized and non-parameterized valid queries in a single test case, reducing code duplication.For improved readability, consider using an array variable to store the query types:
const validQueries = [validWhereArbitraryQuery, validAndParameterizedWhereArbitraryQuery]; it.prop(validQueries, { verbose: 2 })( // ... rest of the test case );This makes the test structure clearer and easier to maintain if more query types are added in the future.
packages/backend/src/tests/propertyBased/arbitraries/arbitraryWhereValue.ts (3)
14-21: Consider adding documentation for the 'parameterize' parameterThe addition of the
parameterizeboolean parameter affects the behavior of thearbitraryWhereValuefunction by wrapping generated values withparam()whentrue. Adding documentation or comments to explain this parameter can improve code readability and maintainability.
Line range hint
51-54: Address incomplete matching arms and correct typographical errorThe comments indicate that the matching arms are presently incomplete, and more information is needed in the schema to enhance them. Additionally, there is a typographical error: "infomration" should be "information".
Would you like assistance in updating the schema to include the necessary information for completing the matching arms? This could improve the functionality and robustness of the code.
Line range hint
13-118: Add unit tests for the new 'parameterize' functionalityThe new
parameterizeparameter modifies the behavior of thearbitraryWhereValuefunction. It's important to add unit tests to cover scenarios whereparameterizeis bothtrueandfalse, ensuring that values are correctly wrapped withparam()when required.Do you need assistance in writing these unit tests, or would you like me to open a new GitHub issue to track this task?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- packages/backend/src/QueryEngine.ts (8 hunks)
- packages/backend/src/QueryStore.ts (1 hunks)
- packages/backend/src/tests/propertyBased/arbitraries/arbitraryQuery.ts (2 hunks)
- packages/backend/src/tests/propertyBased/arbitraries/arbitraryWhere.ts (2 hunks)
- packages/backend/src/tests/propertyBased/arbitraries/arbitraryWhereValue.ts (4 hunks)
- packages/backend/src/tests/propertyBased/properties/cardinalityMany.test.ts (1 hunks)
- packages/backend/src/tests/propertyBased/properties/cardinalityMaybe.test.ts (2 hunks)
- packages/backend/src/tests/propertyBased/properties/cardinalityOne.test.ts (2 hunks)
- packages/docs/static/reference/assets/search.js (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- packages/backend/src/QueryEngine.ts
- packages/docs/static/reference/assets/search.js
Additional comments not posted (19)
packages/backend/src/tests/propertyBased/arbitraries/arbitraryWhere.ts (1)
31-31: LGTM! Parameterization properly propagated.The
parameterizeargument is correctly passed to thearbitraryWhereValuefunction, ensuring that the parameterization option is propagated through the arbitrary generation process.packages/backend/src/tests/propertyBased/arbitraries/arbitraryQuery.ts (3)
16-16: LGTM: New property aligns with PR objectives.The addition of the
parameterize: booleanproperty to theArbitraryQueryinterface is a good enhancement. It aligns well with the PR's objective of introducing support for parameterized queries. This boolean flag will allow for flexible control over query parameterization during arbitrary query generation.
24-24: LGTM: Function signature updated consistently.The addition of the
parameterizeparameter to thearbitraryQueryfunction's destructured parameter list is correct and consistent with the interface update. This change ensures that the function can now accept and utilize the new parameterization option.
16-16: Summary: Consistent implementation of parameterize optionThe changes in this file consistently implement the new
parameterizeoption across theArbitraryQueryinterface,arbitraryQueryfunction signature, and thearbitraryWherefunction call. These modifications align well with the PR objectives of supporting parameterized queries and enhance the flexibility of the arbitrary query generation process.The implementation appears to be well-structured and consistent. However, to ensure completeness, it would be beneficial to review the implementation of the
arbitraryWherefunction to verify how it utilizes the newparameterizeoption.Also applies to: 24-24, 41-41
packages/backend/src/QueryStore.ts (5)
1-7: LGTM: Imports are appropriate and concise.The imports are well-organized and relevant to the functionality of the
QueryStoreclass. All imported items are used within the file, and there are no unnecessary imports.
8-8: LGTM: QueryFunction type is well-defined.The
QueryFunctiontype is concisely defined and appropriate for its use in theQueryStoreclass. The use ofunknownfor parameters allows for flexibility in query function definitions while maintaining type safety.
13-15: LGTM: Constructor is concise and correct.The constructor properly initializes the private
queriesMap, which is essential for the class's functionality.
63-65: LGTM: has method is simple and effective.The
hasmethod provides a clear and concise way to check for the existence of a query in the store. It correctly utilizes the Map's built-inhasmethod, making it efficient and easy to understand.
1-89: Overall, the QueryStore implementation is well-structured and functional.The
QueryStoreclass provides a solid foundation for managing parameterized queries. It effectively handles query storage, retrieval, and parameterization. The code is well-organized and follows good practices.However, there are opportunities for improvement:
- Enhance error handling with more specific error types and detailed messages.
- Implement stricter type checking and validation, especially in the
getandsetmethods.- Consider adding more comprehensive documentation, including examples of usage.
These enhancements will further improve the robustness and maintainability of the
QueryStoreclass.packages/backend/src/tests/propertyBased/properties/cardinalityMaybe.test.ts (2)
15-16: LGTM: Explicit parameterization setting for existing query.The addition of
parameterize: falseto thevalidWhereArbitraryQueryconfiguration is a good practice. It explicitly states the intended behavior for this test case, making the code more self-documenting and preparing for the introduction of parameterized queries.
31-31: LGTM: Consistent parameterization setting for invalid query.The addition of
parameterize: falseto theinvalidWhereArbitraryQueryconfiguration is consistent with the changes made to the valid query configuration. This explicit setting enhances code clarity and maintains consistency across the test cases.packages/backend/src/tests/propertyBased/properties/cardinalityMany.test.ts (3)
15-16: LGTM: Explicit parameterization setting for clarity.The addition of
parameterize: falseto thevalidWhereArbitraryQueryconfiguration is a good practice. It explicitly states that this query is not parameterized, which enhances code clarity and maintains consistency with the newly introduced parameterized query.
31-32: LGTM: Consistent parameterization setting for invalid queries.The addition of
parameterize: falseto theinvalidWhereArbitraryQueryconfiguration is consistent with the earlier change tovalidWhereArbitraryQuery. This explicit setting enhances code clarity and maintains consistency across the test suite.
42-85: LGTM for valid query test, but concern about skipped invalid query test.The updates to the test cases to include both parameterized and non-parameterized queries are excellent. This provides comprehensive coverage and ensures that both query types behave as expected.
The changes to the valid query test (lines 42-67) look good. The test now checks both
validWhereArbitraryQueryandvalidAndParameterizedWhereArbitraryQuery, which is crucial for ensuring consistent behavior across query types.However, the invalid query test (lines 69-85) is currently skipped. This could potentially leave issues with invalid parameterized queries undetected. Can you provide more context on why this test is skipped? If it's due to a known issue or ongoing work, consider adding a TODO comment explaining the reason and tracking the issue.
To investigate further, let's check if there are any related issues or comments:
This will help us understand if there's an ongoing discussion or known issues related to the skipped test.
packages/backend/src/tests/propertyBased/properties/cardinalityOne.test.ts (5)
16-17: LGTM: Explicit non-parameterized query configuration.The addition of
parameterize: falseclearly specifies that this query should not be parameterized. This explicit configuration enhances code readability and prevents potential misunderstandings.
19-24: Excellent addition: Parameterized query for comprehensive testing.The introduction of
validAndParameterizedWhereArbitraryQuerywithparameterize: trueis a valuable addition. This allows for testing both parameterized and non-parameterized queries, significantly improving the test coverage and ensuring the system handles both types correctly.
32-33: LGTM: Consistent non-parameterized configuration for invalid queries.The addition of
parameterize: falsetoinvalidWhereArbitraryQuerymaintains consistency with the valid query configuration. This explicit setting ensures clarity in test scenarios and prevents potential confusion.
35-40: Great addition: Complete test coverage with parameterized invalid queries.The introduction of
invalidAndParameterizedWhereArbitraryQuerywithparameterize: truecompletes the set of test scenarios. This ensures comprehensive coverage of all combinations: valid/invalid and parameterized/non-parameterized queries. This thorough approach significantly enhances the robustness of the test suite.
73-79: Comprehensive test case update for invalid queries, but why skipped?Excellent update to the
it.skip.proptest case to include bothinvalidWhereArbitraryQueryandinvalidAndParameterizedWhereArbitraryQuery. This efficiently tests both parameterized and non-parameterized invalid queries in a single test case, reducing code duplication.For improved readability, consider using an array variable to store the query types:
const invalidQueries = [invalidWhereArbitraryQuery, invalidAndParameterizedWhereArbitraryQuery]; it.skip.prop(invalidQueries, { verbose: 2 })( // ... rest of the test case );This makes the test structure clearer and easier to maintain if more query types are added in the future.
The test is currently skipped (
it.skip.prop). Is this intentional? If so, it might be helpful to add a comment explaining why it's skipped and when it should be unskipped. If not, consider removing the.skipto ensure these tests are being run.
packages/backend/src/tests/propertyBased/arbitraries/arbitraryWhereValue.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
packages/backend/src/SynthqlError.ts (1)
209-220: LGTM: Improved SQL execution error handlingThe changes to the
createSqlExecutionErrormethod are well-implemented. The use ofSqlExecutionErrorPropsand the separatecomposeMessagefunction improves flexibility and maintainability.Consider adding a brief comment explaining the purpose of the
SqlExecutionErrorPropsinterface for better documentation.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- packages/backend/src/QueryEngine.ts (8 hunks)
- packages/backend/src/QueryStore.ts (1 hunks)
- packages/backend/src/SynthqlError.ts (6 hunks)
- packages/backend/src/tests/propertyBased/arbitraries/arbitraryWhereValue.ts (5 hunks)
- packages/docs/static/reference/assets/search.js (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- packages/backend/src/QueryEngine.ts
- packages/backend/src/QueryStore.ts
- packages/backend/src/tests/propertyBased/arbitraries/arbitraryWhereValue.ts
- packages/docs/static/reference/assets/search.js
Additional comments not posted (6)
packages/backend/src/SynthqlError.ts (6)
22-35: LGTM: Well-structured cardinality error handlingThe
createCardinalityErrormethod is a valuable addition. It provides clear error messages and a helpful hint for developers, which can significantly improve debugging experience.
47-50: LGTM: Improved error message clarityThe changes to the
createDatabaseConnectionErrormethod enhance the clarity of the error message. The new wording provides more specific guidance to users encountering database connection issues.
104-117: LGTM: Helpful error handling for duplicate query registrationThe
createQueryAlreadyRegisteredErrormethod is a valuable addition. It provides a clear error message when attempting to register a query with an ID that already exists in the query store.
119-136: LGTM: Comprehensive error handling for queries missing hashThe
createQueryMissingHashErrormethod is well-implemented. It provides a detailed error message that includes the query object and explains the importance of thehashproperty for query registration.
138-154: LGTM: Clear error handling for unregistered queriesThe
createQueryNotRegisteredErrormethod is a valuable addition. It provides a clear error message when a query is not found in the query store and offers guidance on checking the correct queryId.
156-182: LGTM: Detailed error handling for missing query parametersThe
createQueryParameterMissingValueErrormethod is well-implemented. It provides a comprehensive error message that includes both the missing parameter IDs and the provided params object, which is extremely helpful for debugging parameter-related issues.
packages/backend/src/tests/propertyBased/arbitraries/arbitraryWhereValue.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
packages/queries/src/validators/isRegularQueryRequest.ts (1)
3-5: LGTM: Well-implemented type guard function with a minor suggestion.The
isRegularQueryRequestfunction is a well-implemented type guard. It correctly checks for null and undefined, uses optional chaining, and compares thetypeproperty with theRegularQueryconstant.Consider adding an additional check to ensure
xis an object before accessing its properties. This would make the function more robust:export function isRegularQueryRequest(x: any): x is RegularQueryRequest { return x !== null && x !== undefined && typeof x === 'object' && x.type === RegularQuery; }This change ensures that
xis an object before attempting to access itstypeproperty, providing an extra layer of type safety.packages/queries/src/validators/isRegisteredQueryRequest.ts (1)
3-5: LGTM: Well-implemented type guard with room for potential enhancement.The
isRegisteredQueryRequestfunction is correctly implemented as a type guard. It properly checks for null and undefined values, and uses optional chaining for safe property access. The check against theRegisteredQuerytype is correct.Consider enhancing the type guard to check for other required properties of
RegisteredQueryRequest, if any exist. This would make the type guard more robust. For example:export function isRegisteredQueryRequest(x: any): x is RegisteredQueryRequest { return ( x !== null && x !== undefined && x?.type === RegisteredQuery && typeof x.queryId === 'string' && // Add other necessary property checks here ); }This suggestion assumes that
queryIdis a required property ofRegisteredQueryRequest. Adjust the properties checked based on the actual definition ofRegisteredQueryRequest.packages/backend/src/execution/executors/PgExecutor/queryBuilder/exp.ts (3)
Line range hint
104-121: LGTM: EnhancedcompileExpfunction with new operators and string handling.The additions improve the function's flexibility and query capabilities, aligning with the PR objectives.
Consider adding a comment explaining the purpose of the new '= any' and 'not = any' operators for better code documentation.
264-266: TODO noted for alias validation inaddAsmethod.The TODO comment for alias validation indicates awareness of potential issues.
Would you like me to create a GitHub issue to track the implementation of alias validation in the
addAsmethod?
460-495: LGTM: Added handling for query parameters inexpressionFromWheremethod.The addition of query parameter handling directly addresses the PR objectives. The implementation includes proper type checking and error handling, and the code is well-structured, following existing patterns in the class.
For consistency, consider extracting the repeated code for creating the
expobject into a separate function, as it's used in both theisQueryParameter(op)andisQueryParameter(value)blocks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
- packages/backend/src/QueryEngine.test.ts (1 hunks)
- packages/backend/src/QueryEngine.ts (9 hunks)
- packages/backend/src/QueryStore.ts (1 hunks)
- packages/backend/src/execution/executors/PgExecutor/queryBuilder/exp.ts (14 hunks)
- packages/docs/docs/200-security.md (2 hunks)
- packages/docs/static/reference/assets/navigation.js (1 hunks)
- packages/docs/static/reference/assets/search.js (1 hunks)
- packages/handler-express/src/createExpressSynthqlHandler.ts (5 hunks)
- packages/handler-next/src/createNextSynthqlHandler.ts (5 hunks)
- packages/queries/src/index.ts (1 hunks)
- packages/queries/src/types/QueryRequest.ts (1 hunks)
- packages/queries/src/util/iterateRecursively.ts (1 hunks)
- packages/queries/src/validators/isRegisteredQueryRequest.ts (1 hunks)
- packages/queries/src/validators/isRegularQueryRequest.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- packages/backend/src/QueryEngine.test.ts
- packages/backend/src/QueryEngine.ts
- packages/backend/src/QueryStore.ts
- packages/docs/docs/200-security.md
- packages/docs/static/reference/assets/navigation.js
- packages/docs/static/reference/assets/search.js
- packages/handler-express/src/createExpressSynthqlHandler.ts
- packages/handler-next/src/createNextSynthqlHandler.ts
- packages/queries/src/index.ts
- packages/queries/src/types/QueryRequest.ts
- packages/queries/src/util/iterateRecursively.ts
🔇 Additional comments (8)
packages/queries/src/validators/isRegularQueryRequest.ts (2)
1-1: LGTM: Import statement is correct and follows best practices.The import statement correctly imports the necessary types from a separate file, which is good for code organization and maintainability.
1-5: Overall, the implementation is solid and follows TypeScript best practices.This new file introduces a well-implemented type guard function for
RegularQueryRequest. It's concise, focused, and achieves its purpose effectively. The suggested minor improvement to check ifxis an object would further enhance its robustness, but the current implementation is already good.packages/queries/src/validators/isRegisteredQueryRequest.ts (1)
1-1: LGTM: Import statement is correct and follows good practices.The import statement correctly imports the necessary types
RegisteredQueryandRegisteredQueryRequestfrom a separate file. This follows good practices for code organization and type management.packages/backend/src/execution/executors/PgExecutor/queryBuilder/exp.ts (5)
1-1: LGTM: Import ofisQueryParameteradded.The addition of
isQueryParameterimport aligns with the PR objectives to support parameterized queries.
Line range hint
183-192: LGTM: EnhancedaddConstmethod with array handling.The addition of array handling in the
addConstmethod improves its capability to handle complex constant values, aligning with the PR objectives for more dynamic query construction.
226-236: LGTM: RefactoredaddInterleavedmethod for better readability.The use of
flatMapin theaddInterleavedmethod improves code readability and potentially performance. While not directly related to the PR objectives, this change enhances the overall quality of the codebase.
242-246: LGTM: Added error handling for invalid operators inaddOperatormethod.The addition of error handling for invalid operators improves the robustness of the
addOperatormethod, aligning with the PR objectives to enhance error handling.
311-323: LGTM: Refactoredspacemethod for better handling of different scenarios.The refactoring of the
spacemethod improves its ability to handle different cases. While not directly related to the PR objectives, this change enhances the overall quality and robustness of the codebase.
| async function tryExecuteRegisteredQuery<DB>( | ||
| queryEngine: QueryEngine<DB>, | ||
| { queryId, params }: { queryId: string; params: Record<string, unknown> }, | ||
| queryOrBody: any, |
| }, | ||
| ); | ||
| } else if (isRegularQueryRequest(queryOrBody)) { | ||
| return queryEngine.execute(queryOrBody.query as Query<DB>, { |
There was a problem hiding this comment.
this cast shouldn't be needed: you should move the cast into isRegularQueryRequest
| params: Record<string, unknown>; | ||
| } | ||
|
|
||
| export interface RegisteredQueryRequest { |
There was a problem hiding this comment.
why do you need RegisteredQueryRequest and RegisteredQueryRequestBody?
There was a problem hiding this comment.
RegisteredQueryRequestBody is used to type executeRegisteredQuery()
| returnLastOnly, | ||
| }, | ||
| ); | ||
| } else if (isRegularQueryRequest(queryOrBody)) { |
There was a problem hiding this comment.
These last two branches are doing the same... something seems weird here
There was a problem hiding this comment.
Not exactly... the else if branch is matching RegisteredQueryRequests with the shape:
{
type: typeof RegisteredQuery;
queryId: string;
params: Record<string, unknown>;
}, while the else branch is just matching AnyQuery
2 participants
SynthQL pull request template
Why
We're adding a new data structure that allows us to parameterize and detect parameterized queries
What changed
Updated the
WhereClausepipe include the newQueryParametertypeImportant
Summary by CodeRabbit
Summary by CodeRabbit
New Features
QueryEnginewith a method for registering multiple queries and a flag for allowing unregistered queries.QueryStoreclass for managing parameterized queries, including methods to add, retrieve, and check queries.SynthqlErrorfor improved error handling across various scenarios.Bug Fixes
QueryParametertype to accept only strings for theidproperty, improving data consistency.Documentation