Fix Grammar::substituteBindingsIntoRawSql fails during unnesting #246
Conversation
|
Warning Rate limit exceeded@zeriyoshi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/Query/UnnestTest.php (2)
24-24: Encourage inline documentation for magic number usageDeclaring
public const SPANNER_PARAMETERS_LIMIT = 950is a welcome improvement over using a raw numeric literal. It might be helpful to add a short comment explaining why 950 is used or where it comes from, to assist future maintainers.
103-139: Expand coverage for edge cases in raw SQL substitutionsThe test coverage looks good for typical scenarios. You may want to include additional edge-case checks, such as special characters or escaped quotes in the bindings, to ensure robust handling of all string values in substituted SQL statements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Query/Grammar.php(1 hunks)tests/Query/UnnestTest.php(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run-PHPUnit
🔇 Additional comments (2)
tests/Query/UnnestTest.php (1)
70-70: Ensure compatibility with partial function usage
array_map($this->generateUuid(...), range(0, self::SPANNER_PARAMETERS_LIMIT))leverages partial application, which requires PHP 8.0 or higher. Confirm the minimum PHP version constraint in your project to avoid potential compatibility issues.src/Query/Grammar.php (1)
198-210: Validate typed parameters and fallback logicThe override for
substituteBindingsIntoRawSqlcorrectly convertsNestedinstances to arrays before delegating. Confirm that the parent method handles all other binding types as expected. Also, note that usingmixedand arrow functions requires PHP 8.0 or later—verify your project's PHP version requirement.
src/Query/Grammar.php
Outdated
| /** | ||
| * @inheritDoc | ||
| */ | ||
| public function substituteBindingsIntoRawSql($sql, $bindings) |
There was a problem hiding this comment.
This is the wrong place to fix this. You should fix it in Connection::escape()
laravel-spanner/src/Connection.php
Line 450 in c21ad52
|
Test can be simplified to just testing the escape method. |
change #226 it possible for instances of
Colopl\Spanner\Query\Nestedobjects to be included in thebindingsofConnection::getQueryLog()when using UNNEST capable methods likewhereIn. However, the Grammar was not compatible with this change.This PR fixes object handling.
Summary by CodeRabbit
New Features
Tests
Improvements