feat(core): query builder #17958
feat(core): query builder #17958
Conversation
* WIP: query builder * restore changelog files, not sure why they were deleted... * restore changelog files without pre-commit hook * fix main changelog * refactor tests * add execute method to query builder * Implement limit + offset, refactor test to use expectsql * feat: implement remaining features * fix: prettier * fix: eslint * fix: remove js test file * fix: db2 * fix: db2 * fix: db2
WalkthroughA new functional, chainable SQL query builder API was implemented for Sequelize models. This includes a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Model
participant QueryBuilder
participant QueryGenerator
participant Database
User->>Model: Model.select()
Model->>QueryBuilder: new QueryBuilder(Model)
User->>QueryBuilder: .attributes().where().includes()...
User->>QueryBuilder: .getQuery()
QueryBuilder->>QueryGenerator: generate SQL (with options)
QueryGenerator-->>QueryBuilder: SQL string
User->>QueryBuilder: .execute()
QueryBuilder->>Database: Run SQL
Database-->>QueryBuilder: Results
QueryBuilder-->>User: Results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
🧹 Nitpick comments (7)
packages/core/src/abstract-dialect/query-generator.js (1)
1786-1786: Fix typo in property name.- : include.right && this._dialect.supports['RIGHT JOIN'] + : include.right && this.dialect.supports['RIGHT JOIN']packages/core/src/expression-builders/query-builder.ts (2)
83-83: Consider deep cloning nested objects in includes.The current implementation uses shallow copy with spread operator for includes. If include objects contain nested mutable properties, they could be shared between clones.
- newBuilder._include = this._include.map(include => ({ ...include })); + newBuilder._include = this._include.map(include => structuredClone(include));Note:
structuredClonerequires Node.js 17+. Alternatively, use a deep clone utility.
302-302: Add missing 'JOIN' to the multiline regex pattern.The regex pattern for multiline formatting is missing 'JOIN', which would not properly format JOIN clauses.
- return sql.replaceAll(/FROM|LEFT|INNER|RIGHT|WHERE|GROUP|HAVING|ORDER/g, '\n$&'); + return sql.replaceAll(/FROM|LEFT|INNER|RIGHT|JOIN|WHERE|GROUP|HAVING|ORDER/g, '\n$&');packages/core/test/integration/query-builder/query-builder.test.ts (4)
66-69: Remove redundant truncate calls after force sync.When using
sync({ force: true }), the table is already dropped and recreated, making the subsequenttruncate()calls unnecessary.- await Post.sync({ force: true }); - await Post.truncate(); - await User.sync({ force: true }); - await User.truncate(); + await Post.sync({ force: true }); + await User.sync({ force: true });
184-193: Consider tracking the position notation feature.The commented-out tests for ORDER BY position notation indicate an unimplemented feature. Consider creating an issue to track this if it's a desired functionality.
Would you like me to create an issue to track the implementation of ORDER BY position notation support?
400-492: Consider splitting this complex integration test.While this test provides valuable end-to-end validation, it tests many features at once (joins, grouping, having, ordering, aggregation). Consider splitting it into smaller, more focused tests for better maintainability and easier debugging when failures occur.
For example, you could have separate tests for:
- Complex joins with aggregation
- GROUP BY with HAVING conditions
- Multi-table queries with WHERE conditions
This would make it easier to identify which specific feature is failing if the test breaks.
1-666: Consider adding tests for additional edge cases.The test suite is comprehensive, but consider adding tests for:
- Empty WHERE objects
where({})- Multiple includes/joins on the same table with different aliases
- Circular join references (if applicable)
- Very long attribute lists or deeply nested conditions to test any potential limits
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/core/src/abstract-dialect/query-generator.js(2 hunks)packages/core/src/expression-builders/query-builder.ts(1 hunks)packages/core/src/model.d.ts(2 hunks)packages/core/src/model.js(2 hunks)packages/core/test/integration/query-builder/query-builder.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/core/src/model.d.ts (2)
packages/core/src/model.js (1)
Model(149-4648)packages/core/src/expression-builders/query-builder.ts (1)
QueryBuilder(46-336)
packages/core/src/expression-builders/query-builder.ts (3)
packages/core/src/model.js (1)
Model(149-4648)packages/core/src/model.d.ts (5)
ModelStatic(3200-3200)FindAttributeOptions(786-795)GroupOption(149-149)Order(775-775)OrderItem(767-774)packages/core/test/unit/sql/generateJoin.test.js (1)
include(18-18)
packages/core/src/model.js (1)
packages/core/src/expression-builders/query-builder.ts (1)
QueryBuilder(46-336)
🪛 Biome (2.1.2)
packages/core/src/model.js
[error] 42-43: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 43-44: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Upload install and build artifact (Node 20)
- GitHub Check: Upload install and build artifact (Node 18)
🔇 Additional comments (15)
packages/core/src/model.js (2)
43-43: LGTM: Clean import addition for QueryBuilder functionality.The import follows the established patterns and is appropriately placed among other expression-builder imports.
4631-4647: Excellent implementation of the query builder entry point.The
select()method provides a clean and intuitive API for accessing the new QueryBuilder functionality. Key strengths:
- Clean integration with the existing Model class
- Follows Sequelize's static method conventions
- Comprehensive JSDoc with practical example
- Enables fluent/chainable query building as intended
- Simple implementation that delegates to QueryBuilder appropriately
This serves as an excellent entry point for the new query builder functionality described in the PR objectives.
packages/core/src/model.d.ts (2)
29-29: LGTM! Import statement is correctly positioned and structured.The type-only import for
QueryBuilderis appropriately placed among other expression-builder imports and necessary for the newselect()method's return type.
2399-2413: LGTM! Well-structured method declaration with excellent documentation.The
select()method declaration is properly typed with generics to maintain type safety, includes comprehensive JSDoc documentation with a practical example, and aligns perfectly with the implementation inmodel.js. This provides a clean entry point to the new query builder functionality.packages/core/src/abstract-dialect/query-generator.js (1)
1593-1594: LGTM!Clean integration of custom join support by checking the
_isCustomJoinflag and delegating to the appropriate handler.packages/core/src/expression-builders/query-builder.ts (1)
210-217: Remove redundant validation check.The check for
options.onon line 215 will never be reached because line 216 throws an error for missingoncondition.includes(options: QueryBuilderIncludeOptions<M>) { if (!options.model) { throw new Error('Model is required for includes'); } - if (!options.on) { - throw new Error('Custom joins require an "on" condition to be specified'); - }Actually, I need to re-examine this - the validation looks correct as-is.
packages/core/test/integration/query-builder/query-builder.test.ts (9)
1-16: LGTM!The imports are well-organized and appropriate for integration testing the QueryBuilder functionality.
18-30: LGTM!The TypeScript interfaces are well-defined with appropriate use of Sequelize's type inference utilities.
76-149: LGTM!The basic QueryBuilder functionality tests are comprehensive and properly handle dialect-specific SQL variations.
196-237: LGTM!The GROUP BY and HAVING clause tests are well-structured and properly use SQL literals for aggregate functions.
240-286: LGTM!Excellent tests for validating the immutable/functional behavior of the QueryBuilder. This ensures predictable and safe method chaining.
288-325: LGTM!The PostgreSQL-specific tests properly validate dialect-specific operators and features.
327-340: LGTM!Good error handling tests that validate edge cases and provide clear error messages.
495-627: LGTM!Comprehensive tests for the includes/custom joins functionality with proper error handling for missing parameters.
629-665: LGTM! Consider consistent test data cleanup.The execute tests properly validate query execution and results. Note that these tests create data but rely on the beforeEach sync({ force: true }) for cleanup, which is fine but differs from other tests that might benefit from explicit cleanup.
WikiRik
left a comment
WikiRik
left a comment
There was a problem hiding this comment.
Thanks for working on this! In general it looks great! These are my first remarks, I'll need to do a more in depth review later
packages/core/test/integration/query-builder/query-builder.test.ts
Outdated
Show resolved
Hide resolved
packages/core/test/integration/query-builder/query-builder.test.ts
Outdated
Show resolved
Hide resolved
packages/core/test/integration/query-builder/query-builder.test.ts
Outdated
Show resolved
Hide resolved
packages/core/test/integration/query-builder/query-builder.test.ts
Outdated
Show resolved
Hide resolved
packages/core/test/integration/query-builder/query-builder.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
packages/core/src/expression-builders/query-builder.ts (2)
33-42: Consider renaming for clarity.As mentioned in previous reviews,
IncludeOptionshould be renamed toCustomIncludeOptionsto differentiate from Sequelize's existingIncludeOptionstype and avoid confusion.
284-290: Handle undefined values more safely.The non-null assertions (
!) on attributes, order, and group could cause runtime errors if these values are undefined.const options: SelectOptions<any> = { - attributes: this._attributes!, + attributes: this._attributes, where: this._where, include: processedIncludes, - order: this._order!, + order: this._order, limit: this._limit, offset: this._offset, - group: this._group!, + group: this._group,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/expression-builders/query-builder.ts(1 hunks)packages/core/test/integration/query-builder/query-builder.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/core/test/integration/query-builder/query-builder.test.ts
🧰 Additional context used
🪛 GitHub Actions: CI
packages/core/src/expression-builders/query-builder.ts
[warning] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues.
🔇 Additional comments (6)
packages/core/src/expression-builders/query-builder.ts (6)
47-67: LGTM!The class structure and constructor are well-designed. The use of private fields, proper inheritance from
BaseSqlExpression, and clean initialization provide a solid foundation for the query builder.
75-102: LGTM!The
clone()andselect()methods implement proper immutable patterns. The experimental warning inselect()appropriately alerts users to the feature's status, and the deep copying inclone()ensures query builders don't interfere with each other.
110-206: LGTM!All query building methods follow consistent immutable patterns and proper TypeScript typing. The
andHaving()method correctly accumulates conditions using spread syntax, and all methods maintain chainability by returning new instances.
214-243: LGTM!The
includes()method has proper validation for required parameters and sound logic for handling different join types. The default attributes extraction and custom join handling are well-implemented.
305-310: LGTM!The multiline formatting feature using regex replacement provides a useful debugging capability for generated SQL queries.
317-350: LGTM!The
execute()method, getter methods, andcreateQueryBuilder()utility function are well-implemented. The delegation to Sequelize'squeryRaw()method is appropriate for executing generated SQL.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/core/test/integration/query-builder/query-builder.test.ts (2)
30-30: Remove 'QueryBuilder' prefix from describe block.
379-382: Use dialect.supports consistently instead of environment variables.The test uses
sequelize.dialect.supports.operations.iLikecorrectly but should also check for a dialect capability instead ofSEQ_PG_MINIFY_ALIASESenvironment variable for consistency.packages/core/src/expression-builders/query-builder.ts (2)
1-1: Fix Prettier formatting violations.
284-290: Remove unsafe non-null assertions.The non-null assertions can cause runtime errors if these properties are undefined.
🧹 Nitpick comments (3)
packages/core/src/expression-builders/query-builder.ts (3)
85-85: Consider deep cloning for nested include properties.The current implementation only creates a shallow copy of include objects. If these objects contain nested references (like
whereconditions with complex objects), modifications might affect the original builder.- newBuilder._include = this._include.map(include => ({ ...include })); + newBuilder._include = this._include.map(include => structuredClone(include));Note:
structuredClonerequires Node.js 17+ or a polyfill for older versions.
97-97: Consider showing the experimental warning only once.The warning is logged every time
select()is called, which could flood logs in applications that build many queries.+ private static _hasShownWarning = false; + select(): QueryBuilder<M> { - logger.warn('Query Builder is an experimental feature. Results may be incomplete or inaccurate. Please verify all outputs before use.'); + if (!QueryBuilder._hasShownWarning) { + logger.warn('Query Builder is an experimental feature. Results may be incomplete or inaccurate. Please verify all outputs before use.'); + QueryBuilder._hasShownWarning = true; + }
306-306: Consider browser/Node.js compatibility for replaceAll.The
replaceAllmethod requires ES2021 support. For broader compatibility, consider usingreplacewith a global regex.- return sql.replaceAll(/FROM|LEFT|INNER|RIGHT|WHERE|GROUP|HAVING|ORDER/g, '\n$&'); + return sql.replace(/(FROM|LEFT|INNER|RIGHT|WHERE|GROUP|HAVING|ORDER)/g, '\n$1');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/core/src/abstract-dialect/dialect.ts(2 hunks)packages/core/src/expression-builders/query-builder.ts(1 hunks)packages/core/test/integration/query-builder/query-builder.test.ts(1 hunks)packages/postgres/src/dialect.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- packages/postgres/src/dialect.ts
- packages/core/src/abstract-dialect/dialect.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Upload install and build artifact (Node 20)
- GitHub Check: Upload install and build artifact (Node 18)
🔇 Additional comments (1)
packages/core/test/integration/query-builder/query-builder.test.ts (1)
72-228: Well-structured basic functionality tests!The tests comprehensively cover all basic QueryBuilder features with proper dialect-specific SQL assertions.
packages/core/test/integration/query-builder/query-builder.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/core/src/model.js (1)
4638-4654: Implementation looks solid, but consider previous feedback about experimental status.The method implementation is clean and well-documented. However, as mentioned in the previous review, this being a new experimental feature should be communicated to users.
Consider one of these approaches:
- Rename to
UNSTABLE_select()to clearly indicate experimental status- Add a console warning when the method is used
- Add explicit documentation noting this is an experimental feature
The current implementation provides a clean API, but users should be aware of its experimental nature.
/** * Creates a new QueryBuilder instance for this model. * This enables functional/chainable query building. + * + * @experimental This feature is experimental and may change in future versions. * * @returns {QueryBuilder} A new QueryBuilder instance for this model * ... */ - static select() { + static UNSTABLE_select() { return new QueryBuilder(this).select(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/core/src/expression-builders/query-builder.ts(1 hunks)packages/core/src/model.js(2 hunks)packages/core/test/integration/query-builder/query-builder.test.ts(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
packages/core/test/integration/query-builder/query-builder.test.ts
[error] 234-234: Test failure in QueryBuilder: AssertionError due to mismatch in expected and actual SQL query for 'should handle specific operators'.
[error] 247-247: Test failure in QueryBuilder: AssertionError due to mismatch in expected and actual SQL query for 'should handle IN queries'.
🪛 Biome (2.1.2)
packages/core/src/model.js
[error] 42-43: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
[error] 43-44: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
🔇 Additional comments (28)
packages/core/src/model.js (1)
43-43: LGTM! Import addition looks correct.The import for
QueryBuilderis properly formatted and necessary for the newselect()method implementation. The static analysis errors about import declarations are false positives in this context.packages/core/test/integration/query-builder/query-builder.test.ts (11)
1-6: LGTM: Well-structured imports and dependenciesThe imports correctly reference the QueryBuilder class and necessary test utilities. The import structure follows Sequelize patterns and includes all required dependencies for comprehensive testing.
7-21: LGTM: Clear and appropriate model interfacesThe TypeScript interfaces for
TUserandTPostare well-defined with proper optional properties and extend the correct Sequelize model types. This provides good type safety for the tests.
22-58: LGTM: Proper use of beforeEach2 as recommendedThe test setup correctly uses
beforeEach2as suggested in previous review comments. The model definitions are comprehensive with appropriate data types and table configurations. The sync operations ensure clean test state.
65-136: LGTM: Comprehensive basic functionality testsThese tests effectively cover the core QueryBuilder functionality including basic SELECT, attribute selection, aliased attributes, literal attributes, and WHERE clauses with proper dialect-specific assertions.
138-222: LGTM: Thorough coverage of SQL clausesThe tests comprehensively cover LIMIT, OFFSET, ORDER BY, GROUP BY, and HAVING clauses with appropriate dialect-specific expectations. The commented TODO section appropriately flags unimplemented functionality.
225-271: LGTM: Excellent immutability testingThese tests properly verify the functional/immutable behavior of the QueryBuilder, ensuring that method calls return new instances and don't mutate the original builder. This is crucial for the fluent API design.
310-314: LGTM: Appropriate error handling for empty attributesThe test properly validates that empty attributes arrays throw meaningful errors, which is important for preventing invalid SQL generation.
374-377: LGTM: Proper dialect capability checkingThe test correctly uses
vars.sequelize.dialect.supports.operations.iLiketo check for dialect capabilities, which aligns with previous review feedback about using dialect capabilities instead of environment variables.
472-602: LGTM: Comprehensive custom joins testingThe includes/joins tests thoroughly cover custom join functionality including LEFT/INNER joins, custom ON conditions, WHERE clauses on joined tables, custom attributes, and proper error handling for missing required parameters.
604-642: LGTM: Practical execution testingThe execute tests verify that the QueryBuilder can actually run queries against the database and return expected results, including complex scenarios with joins returning multiple rows.
288-300: No issues with IN operator implementationVerified that the
Op.inoperator is correctly implemented inwhere-sql-builder.tsand properly typed inmodel.d.ts. The integration test inpackages/core/test/integration/query-builder/query-builder.test.tsmatches the expected SQL output, so no changes are required.packages/core/src/expression-builders/query-builder.ts (16)
1-18: LGTM: Well-structured imports and dependenciesThe imports are comprehensive and correctly reference all necessary Sequelize types and utilities. The import organization follows TypeScript best practices.
19-27: LGTM: Well-defined generic include options typeThe
QueryBuilderIncludeOptionstype properly defines the structure for custom joins with appropriate optional properties and generic constraints.
33-42: LGTM: Appropriate internal type for processed includesThe
CustomIncludeOptionstype correctly extends the include options with internal properties needed for Sequelize integration, including the_isCustomJoinmarker.
47-69: LGTM: Solid class foundation with proper encapsulationThe class properly extends
BaseSqlExpression, uses appropriate visibility modifiers, and maintains necessary state for query building. The constructor correctly initializes the model and Sequelize instance.
77-91: LGTM: Efficient immutable cloning implementationThe clone method properly creates new instances and deep-copies mutable properties while avoiding unnecessary cloning of immutable objects like the static model. The comment explaining the rationale for the includes array handling is helpful.
99-111: LGTM: Appropriate experimental feature warningThe select method properly implements the one-time warning system to alert users about the experimental nature of the feature. Creating a new instance rather than modifying the current one maintains immutability.
119-124: LGTM: Clean fluent API implementationThe attributes method follows the immutable builder pattern correctly by cloning before modification and returning the new instance for chaining.
132-137: LGTM: Consistent immutable patternThe where method maintains the same clean pattern as other builder methods, ensuring immutability and chainability.
145-176: LGTM: Comprehensive GROUP BY and HAVING supportThe groupBy, having, and andHaving methods properly handle SQL aggregate query constructs. The andHaving method correctly appends to existing conditions using spread syntax.
184-215: LGTM: Standard query modifier implementationsThe orderBy, limit, and offset methods follow the established immutable pattern and properly handle their respective SQL clauses.
223-253: LGTM: Robust custom joins implementationThe includes method properly validates required parameters and constructs appropriate include options with sensible defaults. The error handling for missing model and ON conditions is appropriate.
262-289: LGTM: Proper query generation validation and setupThe getQuery method correctly validates that select() was called and properly processes custom includes with the necessary metadata for Sequelize's query generation system.
291-328: LGTM: Comprehensive query options constructionThe method properly constructs SelectOptions with all necessary properties, handles conditional assignments correctly, and provides useful multiline formatting. The HAVING clause construction with Op.and is appropriate.
335-339: LGTM: Simple and effective query executionThe execute method appropriately uses Sequelize's raw query interface to run the generated SQL and return results.
346-357: LGTM: Useful utility gettersThe tableName and model getters provide convenient access to important properties needed for query construction and debugging.
366-368: LGTM: Clean factory functionThe createQueryBuilder function provides a simple way to instantiate the QueryBuilder with proper generic type preservation.
| it('should handle specific operators', () => { | ||
| expectsql( | ||
| vars.User.select() | ||
| .where({ | ||
| name: { [Op.iLike]: '%john%' }, | ||
| age: { [Op.between]: [18, 65] }, | ||
| }) | ||
| .getQuery(), | ||
| { | ||
| default: | ||
| "SELECT [User].* FROM [users] AS [User] WHERE [User].[name] ILIKE '%john%' AND ([User].[age] BETWEEN 18 AND 65);", | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Investigate pipeline failure for specific operators test
The test appears correct but pipeline failures indicate SQL query mismatches. The iLike operator usage looks proper, but there might be dialect-specific issues.
Based on the pipeline failure, let me verify the iLike operator support:
🏁 Script executed:
#!/bin/bash
# Check iLike operator support across dialects
rg -A 10 "iLike.*:" --type tsLength of output: 9751
🏁 Script executed:
#!/bin/bash
# Display lines 250-320 of the integration test around the failing test
sed -n '250,320p' packages/core/test/integration/query-builder/query-builder.test.tsLength of output: 2481
🏁 Script executed:
#!/bin/bash
# Search for QueryBuilder class definition
rg -n "class QueryBuilder" --type ts packages/core/srcLength of output: 193
🏁 Script executed:
#!/bin/bash
# Locate expectsql definition
rg -n "expectsql" -A5 -B5 --type ts packages/core/testLength of output: 284039
🏁 Script executed:
#!/bin/bash
# Show the beginning of where-sql-builder to inspect operator handling logic
sed -n '1,200p' packages/core/src/abstract-dialect/where-sql-builder.tsLength of output: 6676
🏁 Script executed:
#!/bin/bash
# Show the top of the integration test to inspect support and default dialect setup
sed -n '1,80p' packages/core/test/integration/query-builder/query-builder.test.tsLength of output: 2558
🏁 Script executed:
#!/bin/bash
# Find support.ts or index.ts in integration support folder
fd support -t f packages/core/test/integrationLength of output: 90
🏁 Script executed:
#!/bin/bash
# Show the integration test support.ts to determine default dialect
sed -n '1,200p' packages/core/test/integration/support.tsLength of output: 6655
Add parentheses around BETWEEN clauses in multi-condition queries
The SQL builder correctly maps [Op.iLike] to ILIKE, so the test failure is due to missing grouping for the BETWEEN clause. The integration test expects:
… WHERE [User].[name] ILIKE '%john%' AND ([User].[age] BETWEEN 18 AND 65);
but the builder currently emits:
… WHERE [User].[name] ILIKE '%john%' AND [User].[age] BETWEEN 18 AND 65;
Please update the WhereSqlBuilder to wrap range operators in parentheses when combined with other predicates:
• File: packages/core/src/abstract-dialect/where-sql-builder.ts
- In the logic that formats
[Op.between]and[Op.notBetween], wrap the generated clause in(...)before passing it tojoinWithLogicalOperator.
This will align the output with the integration test’s expectation and prevent pipeline mismatches.
🤖 Prompt for AI Agents
In packages/core/src/abstract-dialect/where-sql-builder.ts around the code
handling [Op.between] and [Op.notBetween], the generated SQL clause for these
range operators is missing parentheses when combined with other conditions. To
fix this, update the logic that formats these operators to wrap the entire
BETWEEN clause in parentheses before it is joined with other predicates using
logical operators. This change will ensure the output matches the expected SQL
in the integration test at
packages/core/test/integration/query-builder/query-builder.test.ts lines
273-286.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/core/test/integration/query-builder/query-builder.test.ts (1)
22-22: Remove Mocha utils format from describePer the coding conventions, the Mocha utils format is no longer needed.
-describe('QueryBuilder', () => { +describe('QueryBuilder', () => {
🧹 Nitpick comments (2)
packages/core/test/integration/query-builder/query-builder.test.ts (2)
52-55: Remove redundant truncate calls
sync({ force: true })already drops and recreates the table, making the subsequenttruncate()calls unnecessary.await Post.sync({ force: true }); - await Post.truncate(); await User.sync({ force: true }); - await User.truncate();
172-181: TODO: ORDER BY position notation supportThe commented test indicates a desire to support positional ORDER BY notation (e.g.,
ORDER BY 2). This is a valid SQL feature that could enhance the QueryBuilder.Would you like me to help implement ORDER BY position notation support or create an issue to track this enhancement?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/test/integration/query-builder/query-builder.test.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/core/test/integration/query-builder/query-builder.test.ts (3)
packages/core/src/model.js (1)
Model(149-4655)packages/core/test/support.ts (1)
beforeEach2(553-564)packages/core/src/expression-builders/query-builder.ts (2)
QueryBuilder(47-358)where(132-137)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Upload install and build artifact (Node 18)
- GitHub Check: Upload install and build artifact (Node 20)
🔇 Additional comments (5)
packages/core/test/integration/query-builder/query-builder.test.ts (5)
64-223: Well-structured basic functionality testsThe test suite comprehensively covers all basic QueryBuilder features with proper dialect-specific SQL expectations.
225-271: Excellent immutability testsThese tests properly verify the functional/immutable behavior of the QueryBuilder, ensuring that each method call returns a new instance without mutating the original.
378-381: Correct usage of dialect supports checkGood use of
sequelize.dialect.supportsfor feature detection instead of checking specific dialects.
476-606: Comprehensive custom join testsExcellent coverage of join functionality including LEFT/INNER joins, custom conditions, attributes selection, and proper error handling for missing required parameters.
608-646: Thorough execution testsGood coverage of query execution including both simple queries and complex queries with joins, verifying that the QueryBuilder correctly executes and returns expected results.
sequelize-bot
bot
added
the
conflicted
2 participants
Pull Request Checklist
Description of Changes
Adds a simple query builder to sequelize static models. Basic functionality:
Model.select()-> will instantiate the query builder for chaining.attributes(['a', 'b'])-> select attributes.where({c: true, d: 1})-> specify where conditions.getQuery()-> return plain SQL.execute()method that will run the raw sql instead of returning it.limit()and.offset().orderBy([['a', 'DESC'], ['b', 'ASC']]).groupBy(['a', 'b', 1, 2]).having().andHaving().include()with custom join logicFuture improvements/additions:
.andWhereusing Sequelize.where(col, op, value), Sequelize.literal() andWhereBracketsas per proposal.orWhere, block simultaneous usage with.andWhereRelates to #394
Summary by CodeRabbit
Summary by CodeRabbit
select()method on models to initiate query building.iLikeoperation for PostgreSQL.