feat: add support for specifying explicit range bounds #380
Conversation
WalkthroughThe changes refactor several range-related Doctrine function classes ( Changes
Sequence Diagram(s)sequenceDiagram
participant DQL as Doctrine DQL Parser
participant RangeFunc as Range Function (e.g., Daterange, Int4range)
participant BaseVariadic as BaseVariadicFunction
DQL->>RangeFunc: Parse function call with arguments
RangeFunc->>BaseVariadic: getFunctionName()
RangeFunc->>BaseVariadic: getNodeMappingPattern()
RangeFunc->>BaseVariadic: getMinArgumentCount(), getMaxArgumentCount()
BaseVariadic-->>RangeFunc: Validate arguments and map nodes
RangeFunc-->>DQL: Return SQL translation of function call
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (20)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php (2)
25-28: Allow numeric expressions, not only string literalsFor numeric/decimal ranges, forcing every bound to be parsed as
StringPrimarymay compel users to quote numbers, e.g.'1.5', instead of passing1.5or:value. This can introduce avoidable casts on the PostgreSQL side.
Consider switching (or extending) the node pattern toArithmeticPrimary/InputParameteras suggested forInt8range.
15-38: Reduce code repetitionSame note as in
Int8range: extract the common boilerplate to an abstract helper to keep individual classes minimal.src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4range.php (2)
25-28: Broaden accepted AST node types
int4rangealso deals with numeric (32-bit) values. Please revisit theStringPrimaryrestriction here for the same reasons outlined in theInt8rangecomment.
15-38: Refactor suggestionSee the duplication note in
Int8range; the same applies here.
🧹 Nitpick comments (2)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8range.php (2)
25-28: Consider broadening the accepted AST node type
getNodeMappingPattern()restricts every argument toStringPrimary.
For an 8-byte integer range, callers will typically pass numeric literals, path expressions, or parameters (e.g.:from,:to) rather than quoted strings. Depending on the internal implementation ofBaseVariadicFunction, this restriction might force users to wrap numeric values in quotes, leading to unnecessary type coercions in PostgreSQL.If
ArithmeticPrimary(orInputParameter) is sufficient for your parser you could widen the pattern to accept all common numeric inputs:- return ['StringPrimary']; + return ['ArithmeticPrimary'];or allow both:
- return ['StringPrimary']; + return ['StringPrimary', 'ArithmeticPrimary'];Please verify that the doctrine AST walker still builds the correct SQL and that existing tests continue to pass.
15-38: High duplication across range-function classes
Int4range,Int8range,Numrange,Daterange,Tsrange, andTstzrangenow share identical method bodies except forgetFunctionName(). Consider introducing a small intermediary abstract class (e.g.AbstractRangeFunction) that implementsgetNodeMappingPattern(),getMinArgumentCount(), andgetMaxArgumentCount(). Each concrete range function would then only need to provide its specific SQL name, reducing maintenance overhead and lowering the risk of future inconsistencies.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Daterange.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4range.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8range.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tsrange.php(1 hunks)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tstzrange.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DaterangeTest.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php(1 hunks)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (1)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php (1)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4range.php (1)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
🧬 Code Graph Analysis (7)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php (5)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DaterangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (1)
getDqlStatements(27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (5)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DaterangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (1)
getDqlStatements(27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DaterangeTest.php (5)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (1)
getDqlStatements(27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (5)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DaterangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (1)
getDqlStatements(27-33)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (5)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DaterangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (1)
getDqlStatements(27-33)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tstzrange.php (6)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/BaseVariadicFunction.php (1)
BaseVariadicFunction(20-135)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Daterange.php (4)
getFunctionName(17-20)getNodeMappingPattern(25-28)getMinArgumentCount(30-33)getMaxArgumentCount(35-38)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4range.php (4)
getFunctionName(17-20)getNodeMappingPattern(25-28)getMinArgumentCount(30-33)getMaxArgumentCount(35-38)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8range.php (4)
getFunctionName(17-20)getNodeMappingPattern(25-28)getMinArgumentCount(30-33)getMaxArgumentCount(35-38)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php (4)
getFunctionName(17-20)getNodeMappingPattern(25-28)getMinArgumentCount(30-33)getMaxArgumentCount(35-38)src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tsrange.php (4)
getFunctionName(17-20)getNodeMappingPattern(25-28)getMinArgumentCount(30-33)getMaxArgumentCount(35-38)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (5)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DaterangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php (1)
getDqlStatements(27-33)tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (1)
getDqlStatements(27-33)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.2 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: sloth
🔇 Additional comments (24)
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php (2)
22-23: Well-structured test case organization.Adding descriptive keys to the expected SQL statements improves test readability and clearly distinguishes between the basic range with default bounds and the range with explicit bounds. This aligns with the PR's objective to add support for specifying explicit range bounds.
30-31: Good test coverage for explicit bounds functionality.The test cases are well-organized with descriptive keys that match the expected SQL statements. This ensures proper coverage of both the default behavior and the new explicit bounds functionality, which aligns with similar changes in other range function tests.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (2)
22-23: Well-structured test case organization.Adding descriptive keys to the expected SQL statements improves test readability and clearly distinguishes between the basic range with default bounds and the range with explicit bounds. This aligns with the PR's objective to add support for specifying explicit range bounds.
30-31: Good test coverage for explicit bounds functionality.The test cases are well-organized with descriptive keys that match the expected SQL statements. This ensures proper coverage of both the default behavior and the new explicit bounds functionality, which aligns with similar changes in other range function tests.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DaterangeTest.php (2)
22-23: Well-structured test case organization.Adding descriptive keys to the expected SQL statements improves test readability and clearly distinguishes between the basic range with default bounds and the range with explicit bounds. This aligns with the PR's objective to add support for specifying explicit range bounds.
30-31: Good test coverage for explicit bounds functionality.The test cases are well-organized with descriptive keys that match the expected SQL statements. This ensures proper coverage of both the default behavior and the new explicit bounds functionality, which aligns with similar changes in other range function tests.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (2)
22-23: Well-structured test case organization.Adding descriptive keys to the expected SQL statements improves test readability and clearly distinguishes between the basic range with default bounds and the range with explicit bounds. This aligns with the PR's objective to add support for specifying explicit range bounds.
30-31: Good test coverage for explicit bounds functionality.The test cases are well-organized with descriptive keys that match the expected SQL statements. This ensures proper coverage of both the default behavior and the new explicit bounds functionality, which aligns with similar changes in other range function tests.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (2)
21-23: Great enhancement to test coverage!The addition of descriptive keys to the test cases improves readability and clearly distinguishes between different use cases. Adding a specific test for explicit bounds with the '[)' notation ensures the new variadic functionality is properly tested.
29-31: Well-structured test cases with clear intent.The DQL statements appropriately match the expected SQL statements, with clear labels that document the purpose of each test case. This approach is consistent with the pattern used across other range function tests.
tests/Unit/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (2)
21-23: Improved test clarity and coverage.Good job organizing the expected SQL statements with descriptive keys that clarify the test intent. The addition of a test case for explicit bounds aligns with the pattern seen in other range function tests and properly validates the new functionality.
29-31: Consistent implementation of test cases.The DQL statements follow the same pattern as other range function tests, using descriptive keys and including tests for both default and explicit bounds. This consistency helps maintain a clear and uniform testing approach across the codebase.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tstzrange.php (3)
15-20: Good refactoring to support variadic arguments.Extending
BaseVariadicFunctionand implementinggetFunctionName()enables the function to support the optional third argument for specifying explicit range bounds. This aligns with PostgreSQL'ststzrangefunction, which can accept a bounds parameter.
22-28: Well-documented node mapping pattern.The PHPDoc comment for the return type is clear, and using
StringPrimaryas the mapping pattern is appropriate for this function. This simplifies the node mapping compared to the previous implementation.
30-38: Appropriate argument constraints.Setting
getMinArgumentCount()to 2 andgetMaxArgumentCount()to 3 correctly implements the PostgreSQL function signature fortstzrange, which requires at least two arguments (start and end of range) and optionally accepts a third argument for bounds specification.src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tsrange.php (3)
15-20: Good refactoring to support variadic arguments.Extending
BaseVariadicFunctionand implementinggetFunctionName()enables the function to support the optional third argument for specifying explicit range bounds. This aligns with PostgreSQL'stsrangefunction, which can accept a bounds parameter.
22-28: Well-documented node mapping pattern.The PHPDoc comment for the return type is clear, and using
StringPrimaryas the mapping pattern is appropriate for this function. This simplifies the node mapping compared to the previous implementation.
30-38: Appropriate argument constraints.Setting
getMinArgumentCount()to 2 andgetMaxArgumentCount()to 3 correctly implements the PostgreSQL function signature fortsrange, which requires at least two arguments (start and end of range) and optionally accepts a third argument for bounds specification.src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8range.php (1)
17-20: Implementation looks correctThe function name is exposed properly and the change to
BaseVariadicFunctionaligns with the refactor performed across the codebase.src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php (1)
17-20: Implementation looks correctThe migration to
BaseVariadicFunctionand the exposure of thenumrangeSQL name are both sound.src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4range.php (1)
17-20: Implementation looks correctThe refactor adheres to the new variadic-function pattern.
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Daterange.php (3)
15-20: Good upgrade toBaseVariadicFunction.Moving the implementation to
BaseVariadicFunctionremoves boiler-plate and centralises validation logic, which simplifies maintenance and keeps the class consistent with newer range helpers.
30-38: Argument count guards look correct.PostgreSQL’s
daterangeconstructor requires 2 arguments (lower, upper) and allows a 3rd bounds specifier, somin=2 / max=3aligns with the database contract.
25-28:❓ Verification inconclusive
StringPrimarymay be too restrictive for date expressions.
StringPrimaryin Doctrine’s grammar accepts only string-based tokens (literals, concat operations, etc.).
Typical DQL calls such asdaterange(e.startDate, e.endDate)passPathExpressions (mappedDateTimefields), which are parsed underSimpleArithmeticExpression/ArithmeticPrimary, notStringPrimary.
UsingStringPrimarywill therefore raise aQueryExceptionfor the most common use-case.Consider widening the mapping pattern:
- return ['StringPrimary']; + // Allows literals, parameters, path expressions and functions + return ['SimpleArithmeticExpression'];This change mirrors the pattern used in
Int4range,Numrange, etc., and keeps all range helpers consistent.
I’d like to confirm how other range functions define their node mapping patterns before updating Daterange. Let’s pull all
getNodeMappingPattern()implementations in the AST functions directory:This will let us verify what patterns
Int4range,Numrange, etc. use (e.g.SimpleArithmeticExpression) and ensure Daterange stays consistent.
🏁 Script executed:
#!/bin/bash rg -n "function getNodeMappingPattern" -n src/MartinGeorgiev/Doctrine/ORM/Query/AST/FunctionsLength of output: 4380
I’d like to confirm the patterns used by other range functions. Let’s inspect their
getNodeMappingPattern()implementations:
🏁 Script executed:
#!/bin/bash rg -n "protected function getNodeMappingPattern" -C3 src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/*range.phpLength of output: 4209
Review Daterange’s node mapping for PathExpressions
By returning only
StringPrimary, calls likedaterange(e.startDate, e.endDate)(wheree.startDateande.endDatearePathExpressions) will fail to parse—StringPrimarymatches only string literals, parameters, concatenations, etc. To support typical usage, widen the pattern toSimpleArithmeticExpression(which covers literals, parameters, path expressions and functions):-protected function getNodeMappingPattern(): array -{ - return ['StringPrimary']; -} +protected function getNodeMappingPattern(): array +{ + // Accepts literals, parameters, path expressions and functions + return ['SimpleArithmeticExpression']; +}Check consistency across other range helpers, which currently use
StringPrimary:
- src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4range.php
- src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8range.php
- src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php
- src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tsrange.php
- src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tstzrange.php
Please verify and adjust these mappings as needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (1)
1-28: 🛠️ Refactor suggestionLGTM, but enhance test coverage by adding an explicit bounds test case.
The integration test for the
Tstzrangefunction is well structured and follows the repository's testing conventions. However, the PR's main objective is to "add support for specifying explicit range bounds", but the test only covers the basic case with default bounds.From the relevant code snippets, I can see that
Tstzrangenow extendsBaseVariadicFunctionand supports 2-3 arguments (where the third argument would be for explicit bounds). To properly test this new feature, consider adding a test case that includes the third argument for explicit bounds.Add a test method for the explicit bounds functionality:
public function test_tstzrange_with_explicit_bounds(): void { $dql = 'SELECT TSTZRANGE(\'2024-01-01 00:00:00+00\', \'2024-12-31 23:59:59+00\', \'[)\') as result FROM Fixtures\MartinGeorgiev\Doctrine\Entity\ContainsJsons t WHERE t.id = 1'; $result = $this->executeDqlQuery($dql); $this->assertIsArray($result); $this->assertNotEmpty($result[0]['result']); $this->assertIsString($result[0]['result']); $this->assertSame('["2024-01-01 00:00:00+00","2024-12-31 23:59:59+00")', $result[0]['result']); // Test with different bounds $dql = 'SELECT TSTZRANGE(\'2024-01-01 00:00:00+00\', \'2024-12-31 23:59:59+00\', \'(]\') as result FROM Fixtures\MartinGeorgiev\Doctrine\Entity\ContainsJsons t WHERE t.id = 1'; $result = $this->executeDqlQuery($dql); $this->assertSame('("2024-01-01 00:00:00+00","2024-12-31 23:59:59+00"]', $result[0]['result']); }This will provide complete coverage for the new explicit bounds feature and serve as documentation for how users can utilize it.
🧹 Nitpick comments (4)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (1)
1-37: Consider adding tests for edge cases.While the current tests cover the basic functionality well, consider adding tests for edge cases such as:
- Null or invalid timestamp values
- Different bound formats (e.g.,
"()","[]")- Invalid bound format strings
- Empty or equal timestamps
These additional tests would further improve test coverage and ensure robust handling of various inputs.
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (2)
9-9: Class inheritance might not be appropriate.The test class extends
JsonTestCasewhich appears to be designed for testing JSON functions, not range functions. This could lead to unnecessary test setup for JSON fixtures that aren't relevant to testing integer ranges.Consider extending a more appropriate base class that's specific to range function tests or a more general base class if range-specific fixtures aren't needed.
1-44: Add test cases for edge cases in range boundaries.The current tests cover basic range functionality but don't test edge cases like null boundaries, empty ranges, or single-point ranges.
Consider adding test cases for:
- A null lower bound:
INT4RANGE(NULL, 20)- A null upper bound:
INT4RANGE(10, NULL)- A single-point range:
INT4RANGE(10, 10, '[]')- An empty range:
INT4RANGE(10, 10)This would provide more comprehensive test coverage for the range function's behavior with various inputs.
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1)
18-29: LGTM! Consider adding more test cases for comprehensive coverage.The basic range test looks good and correctly verifies the function output. To improve coverage, consider adding tests for:
- Different bound types (e.g.,
(],[],())- Edge cases (e.g., negative values, very large values close to the 64-bit integer limits)
- NULL values
- Invalid inputs and error handling
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DaterangeTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (2)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase, and therefore don't need an explicit import.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), not PHPUnit's TestCase, and therefore don't need an explicit import statement.
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (3)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase, and therefore don't need an explicit import.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), not PHPUnit's TestCase, and therefore don't need an explicit import statement.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (2)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase, and therefore don't need an explicit import.
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DaterangeTest.php (2)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase, and therefore don't need an explicit import.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), not PHPUnit's TestCase, and therefore don't need an explicit import statement.
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (3)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase, and therefore don't need an explicit import.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), not PHPUnit's TestCase, and therefore don't need an explicit import statement.
🧬 Code Graph Analysis (4)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (3)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tstzrange.php (1)
Tstzrange(15-39)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/JsonTestCase.php (1)
JsonTestCase(9-56)tests/Integration/MartinGeorgiev/TestCase.php (1)
executeDqlQuery(236-250)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (3)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Tsrange.php (1)
Tsrange(15-39)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTestCase.php (1)
DateTestCase(9-49)tests/Integration/MartinGeorgiev/TestCase.php (1)
executeDqlQuery(236-250)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DaterangeTest.php (7)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Daterange.php (1)
Daterange(15-39)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DateTestCase.php (1)
DateTestCase(9-49)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1)
getStringFunctions(11-16)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (1)
getStringFunctions(11-16)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (1)
getStringFunctions(11-16)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (1)
getStringFunctions(11-16)tests/Integration/MartinGeorgiev/TestCase.php (1)
executeDqlQuery(236-250)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (3)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4range.php (1)
Int4range(15-39)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/JsonTestCase.php (1)
JsonTestCase(9-56)tests/Integration/MartinGeorgiev/TestCase.php (1)
executeDqlQuery(236-250)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: sloth
- GitHub Check: PostgreSQL 17 + PHP 8.4
- GitHub Check: PostgreSQL 16 + PHP 8.4
- GitHub Check: PostgreSQL 17 + PHP 8.3
- GitHub Check: PostgreSQL 16 + PHP 8.3
- GitHub Check: PostgreSQL 17 + PHP 8.1
- GitHub Check: PostgreSQL 17 + PHP 8.2
- GitHub Check: PostgreSQL 16 + PHP 8.2
- GitHub Check: PostgreSQL 16 + PHP 8.1
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.1 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
🔇 Additional comments (8)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DaterangeTest.php (1)
1-37: Integration test looks good and effectively verifies the new functionality!The test class properly implements integration tests for the
Daterangefunction, covering both default behavior and the new explicit bounds functionality. The test methods contain thorough assertions to verify the expected output format and content.The implementation follows the same pattern as other range function tests in the codebase, making it consistent and maintainable.
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (4)
1-10: Implementation aligns well with project conventions.The class declaration, namespace, and imports follow the project's established patterns. Extending
DateTestCaseis appropriate as it provides the necessary fixture setup with datetime fields required for testing timestamp range functionality.
11-16: LGTM! Proper function registration.The
getStringFunctions()method correctly registers theTSRANGEfunction to map to theTsrangeclass, consistent with how other range functions are registered in their respective test classes.
18-26: Test case properly verifies default range behavior.This test effectively verifies the basic functionality of the
TSRANGEfunction with two arguments (start and end timestamps). The assertions correctly validate that the function returns the expected timestamp range format with default bounds.
28-36: Test case correctly verifies explicit bounds functionality.This test properly validates the new feature of specifying explicit bounds (third parameter). The assertions correctly check that the function returns a timestamp range with the specified
"[)"bounds format.tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (1)
18-29: The int4range test with default bounds looks good.The test correctly verifies that the
INT4RANGEfunction works with two parameters (start and end) and produces the expected range notation '[10,20)'.tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (2)
31-42: LGTM! The explicit bounds test properly validates the new functionality.This test case correctly verifies the functionality added in this PR - specifying explicit range bounds. The assertions properly validate that the resulting range string has the expected format.
20-20:❌ Incorrect review comment
Consider using a more appropriate entity fixture for Int8range tests.
The test is currently using
Fixtures\MartinGeorgiev\Doctrine\Entity\ContainsJsonsentity to test an integer range function. Based on the repository's conventions, it would be more appropriate to use a fixture specifically designed for integer types (likeContainsIntegersor a dedicatedContainsInt8fixture if available).
🏁 Script executed:
#!/bin/bash # Check for integer-related fixtures that might be more appropriate echo "Looking for integer-related fixtures..." fd "Contains.*\.php" --type f "src/Fixtures" || fd "Contains.*\.php" --type f "tests/Fixtures"Length of output: 382
I couldn’t find any existing
ContainsIntegersorContainsDecimalsfixtures in the repository paths tried—let’s search the entire repo:
🏁 Script executed:
#!/bin/bash echo "Looking for relevant fixture classes across the repo..." rg -l "class ContainsIntegers" rg -l "class ContainsDecimals" rg -l "class ContainsJsons"Length of output: 396
Let’s inspect the test’s setup to confirm which test case and fixtures it uses:
🏁 Script executed:
#!/bin/bash sed -n '1,200p' tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.phpLength of output: 1503
Here’s the
JsonTestCaseclass—let’s check how it loads fixtures:
🏁 Script executed:
#!/bin/bash rg -n "class JsonTestCase" -A 30Length of output: 3517
Let’s compare with the existing Int4rangeTest and NumrangeTest to see their base classes and fixtures:
🏁 Script executed:
#!/bin/bash echo "=== Int4rangeTest ===" int4=$(rg -l "class Int4rangeTest" -g "tests/Integration/**/*.php") echo "$int4" sed -n '1,200p' "$int4" echo -e "\n=== NumrangeTest ===" num=$(rg -l "class NumrangeTest" -g "tests/Integration/**/*.php") echo "$num" sed -n '1,200p' "$num"Length of output: 2073
Let’s inspect the fixture entity definitions to understand their schema and usage:
🏁 Script executed:
#!/bin/bash echo "=== ContainsIntegers Fixture ===" sed -n '1,200p' fixtures/MartinGeorgiev/Doctrine/Entity/ContainsIntegers.php echo -e "\n=== ContainsDecimals Fixture ===" sed -n '1,200p' fixtures/MartinGeorgiev/Doctrine/Entity/ContainsDecimals.php echo -e "\n=== ContainsJsons Fixture ===" sed -n '1,200p' fixtures/MartinGeorgiev/Doctrine/Entity/ContainsJsons.phpLength of output: 1614
Ignore fixture change suggestion for Int8rangeTest
The existing
Int8rangeTestmirrors theInt4rangeTestby extendingJsonTestCaseand targeting theContainsJsonsfixture. There is currently no integer‐specific test setup (e.g. aNumericTestCaseusingContainsIntegers), so swapping inContainsIntegerswould break consistency with the rest of the suite.Likely an incorrect or invalid review comment.
| public function test_int4range_with_bounds(): void | ||
| { | ||
| $dql = 'SELECT INT4RANGE(:start, :end, "[)") as result FROM Fixtures\MartinGeorgiev\Doctrine\Entity\ContainsJsons t WHERE t.id = 1'; | ||
| $result = $this->executeDqlQuery($dql, [ | ||
| 'start' => 10, | ||
| 'end' => 20, | ||
| ]); | ||
| $this->assertIsArray($result); | ||
| $this->assertNotEmpty($result[0]['result']); | ||
| $this->assertIsString($result[0]['result']); | ||
| $this->assertSame('[10,20)', $result[0]['result']); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test for explicit bounds doesn't effectively verify the bounds parameter.
Both test methods use the same expected result ('[10,20)'), even though the second test explicitly specifies "[)" bounds. Since "[)" is the default bounds format anyway (as shown in the first test), this doesn't effectively verify that specifying explicit bounds works as expected.
Add a test case that uses different explicit bounds (e.g., '[]' for inclusive-inclusive) and verify that the result changes accordingly:
public function test_int4range_with_bounds(): void
{
- $dql = 'SELECT INT4RANGE(:start, :end, "[)") as result FROM Fixtures\MartinGeorgiev\Doctrine\Entity\ContainsJsons t WHERE t.id = 1';
+ $dql = 'SELECT INT4RANGE(:start, :end, "[]") as result FROM Fixtures\MartinGeorgiev\Doctrine\Entity\ContainsJsons t WHERE t.id = 1';
$result = $this->executeDqlQuery($dql, [
'start' => 10,
'end' => 20,
]);
$this->assertIsArray($result);
$this->assertNotEmpty($result[0]['result']);
$this->assertIsString($result[0]['result']);
- $this->assertSame('[10,20)', $result[0]['result']);
+ $this->assertSame('[10,20]', $result[0]['result']);
}🤖 Prompt for AI Agents
In
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php
around lines 31 to 42, the test for explicit bounds uses the default "[)" bounds
and expects the same result as the implicit default, which does not verify the
bounds parameter effectively. Add a new test method that uses different explicit
bounds such as "[]" (inclusive-inclusive) and assert that the result string
reflects these bounds correctly, for example '[10,20]'. This will ensure the
bounds parameter is properly tested.
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (1)
28-36: Test for explicit bounds doesn't effectively verify the bounds parameter.The
test_int4range_with_bounds()method doesn't effectively verify that the bounds parameter works as expected. It uses the default[)bounds (inclusive-exclusive) and expects the same result as the default test, which doesn't validate that specifying explicit bounds actually changes the output.Consider modifying the test to use different bounds (e.g.,
[]for inclusive-inclusive) and verifying that the result changes accordingly:public function test_int4range_with_bounds(): void { - $dql = "SELECT INT4RANGE(t.integer1, t.integer2, '[)') as result FROM Fixtures\\MartinGeorgiev\\Doctrine\\Entity\\ContainsNumerics t WHERE t.id = 1"; + $dql = "SELECT INT4RANGE(t.integer1, t.integer2, '[]') as result FROM Fixtures\\MartinGeorgiev\\Doctrine\\Entity\\ContainsNumerics t WHERE t.id = 1"; $result = $this->executeDqlQuery($dql); $this->assertIsArray($result); $this->assertNotEmpty($result[0]['result']); $this->assertIsString($result[0]['result']); - $this->assertSame('[10,20)', $result[0]['result']); + $this->assertSame('[10,20]', $result[0]['result']); }tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1)
28-36: Test for explicit bounds doesn't effectively verify the bounds parameter.The
test_int8range_with_bounds()method doesn't effectively verify that the bounds parameter works as expected. It uses the default[)bounds (inclusive-exclusive) and expects the same result as the default test, which doesn't validate that specifying explicit bounds actually changes the output.Consider modifying the test to use different bounds (e.g.,
[]for inclusive-inclusive) and verifying that the result changes accordingly:public function test_int8range_with_bounds(): void { - $dql = "SELECT INT8RANGE(t.bigint1, t.bigint2, '[)') as result FROM Fixtures\\MartinGeorgiev\\Doctrine\\Entity\\ContainsNumerics t WHERE t.id = 1"; + $dql = "SELECT INT8RANGE(t.bigint1, t.bigint2, '[]') as result FROM Fixtures\\MartinGeorgiev\\Doctrine\\Entity\\ContainsNumerics t WHERE t.id = 1"; $result = $this->executeDqlQuery($dql); $this->assertIsArray($result); $this->assertNotEmpty($result[0]['result']); $this->assertIsString($result[0]['result']); - $this->assertSame('[1000,2000)', $result[0]['result']); + $this->assertSame('[1000,2000]', $result[0]['result']); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DaterangeTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumericTestCase.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php(1 hunks)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DaterangeTest.php
- tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php
- tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php (1)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (1)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumericTestCase.php (3)
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), rather than PHPUnit's TestCase, and therefore don't need an explicit import.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#318
File: tests/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/XmlAggTest.php:1-9
Timestamp: 2025-03-29T03:31:17.114Z
Learning: Tests in the `Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions` namespace extend a custom `TestCase` class from the same namespace (`Tests\MartinGeorgiev\Doctrine\ORM\Query\AST\Functions\TestCase`), not PHPUnit's TestCase, and therefore don't need an explicit import statement.
Learnt from: martin-georgiev
PR: martin-georgiev/postgresql-for-doctrine#263
File: src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Numrange.php:19-21
Timestamp: 2025-03-11T12:32:10.726Z
Learning: In the postgresql-for-doctrine repository, PostgreSQL range functions have distinct implementations for different data types. The `Numrange` function works with numeric/decimal values and is tested using the `ContainsDecimals` fixture with properties typed as `float`. In contrast, the `Int4range` function works with 32-bit integers and is tested using the `ContainsIntegers` fixture with properties typed as `int`. While the PHP implementations share a similar structure (extending `BaseFunction`), they are semantically different as they handle different PostgreSQL data types.
🧬 Code Graph Analysis (3)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int4rangeTest.php (7)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumericTestCase.php (1)
NumericTestCase(9-49)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1)
getStringFunctions(11-16)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TstzrangeTest.php (1)
getStringFunctions(11-16)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/DaterangeTest.php (1)
getStringFunctions(11-16)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php (1)
getStringFunctions(11-16)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/TsrangeTest.php (1)
getStringFunctions(11-16)tests/Integration/MartinGeorgiev/TestCase.php (1)
executeDqlQuery(236-250)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (3)
src/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8range.php (1)
Int8range(15-39)tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumericTestCase.php (1)
NumericTestCase(9-49)tests/Integration/MartinGeorgiev/TestCase.php (1)
executeDqlQuery(236-250)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumericTestCase.php (1)
tests/Integration/MartinGeorgiev/TestCase.php (2)
createTestSchema(149-154)dropTestTableIfItExists(204-211)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.4 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.2 + Doctrine ORM 2.18 + Doctrine Lexer latest
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.3 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.4 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.2 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer latest
- GitHub Check: PHP 8.2 + Doctrine ORM 3.0 + Doctrine Lexer latest
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.3 + Doctrine ORM 2.18 + Doctrine Lexer 3.0
- GitHub Check: PHP 8.3 + Doctrine ORM latest + Doctrine Lexer 3.0
- GitHub Check: PHP 8.4 + Doctrine ORM 2.14 + Doctrine Lexer 2.1
- GitHub Check: PHP 8.1 + Doctrine ORM latest + Doctrine Lexer latest
- GitHub Check: PHP 8.1 + Doctrine ORM 3.0 + Doctrine Lexer 3.0
🔇 Additional comments (2)
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/Int8rangeTest.php (1)
9-10: Good job updating the base class for Int8rangeTest.The test class now extends
NumericTestCasewhich provides a more appropriate testing infrastructure for numeric range functions than the previously usedJsonTestCase. This change improves test clarity and reduces unnecessary coupling.tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumericTestCase.php (1)
9-49: Well-structured test fixture for numeric tests.The
NumericTestCaseabstract class provides a solid foundation for testing numeric functions. It creates a table with appropriate column types (INTEGER, BIGINT, DECIMAL) and populates it with test data, making it reusable across multiple range function tests.
| public function test_numrange_with_bounds(): void | ||
| { | ||
| $dql = "SELECT NUMRANGE(t.decimal1, t.decimal2, '[)') as result FROM Fixtures\\MartinGeorgiev\\Doctrine\\Entity\\ContainsNumerics t WHERE t.id = 1"; | ||
| $result = $this->executeDqlQuery($dql); | ||
| $this->assertIsArray($result); | ||
| $this->assertNotEmpty($result[0]['result']); | ||
| $this->assertIsString($result[0]['result']); | ||
| $this->assertSame('[10.5,20.5)', $result[0]['result']); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Test for explicit bounds doesn't effectively verify the bounds parameter.
The test_numrange_with_bounds() method doesn't effectively verify that the bounds parameter works as expected. It uses the default [) bounds (inclusive-exclusive) and expects the same result as the default test, which doesn't validate that specifying explicit bounds actually changes the output.
Consider modifying the test to use different bounds (e.g., [] for inclusive-inclusive) and verifying that the result changes accordingly:
public function test_numrange_with_bounds(): void
{
- $dql = "SELECT NUMRANGE(t.decimal1, t.decimal2, '[)') as result FROM Fixtures\\MartinGeorgiev\\Doctrine\\Entity\\ContainsNumerics t WHERE t.id = 1";
+ $dql = "SELECT NUMRANGE(t.decimal1, t.decimal2, '[]') as result FROM Fixtures\\MartinGeorgiev\\Doctrine\\Entity\\ContainsNumerics t WHERE t.id = 1";
$result = $this->executeDqlQuery($dql);
$this->assertIsArray($result);
$this->assertNotEmpty($result[0]['result']);
$this->assertIsString($result[0]['result']);
- $this->assertSame('[10.5,20.5)', $result[0]['result']);
+ $this->assertSame('[10.5,20.5]', $result[0]['result']);
}🤖 Prompt for AI Agents
In
tests/Integration/MartinGeorgiev/Doctrine/ORM/Query/AST/Functions/NumrangeTest.php
around lines 28 to 36, the test_numrange_with_bounds method uses the default
bounds '[)' and expects the same result as the default test, which does not
verify that specifying explicit bounds changes the output. Modify the test to
use different bounds such as '[]' (inclusive-inclusive) in the NUMRANGE function
call and update the expected result accordingly to confirm that the bounds
parameter affects the output as intended.
Summary by CodeRabbit
Refactor
Tests