feat: add Query\Builder::whereInEmbedded #221
Conversation
taka-oyama
requested review from
dkkoma,
kt81,
zeriyoshi,
halnique,
Uyan712 and
t-matsuno-777
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughVersion 8.2.0 introduces a new method, Changes
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- CHANGELOG.md (1 hunks)
- README.md (1 hunks)
- src/Query/Builder.php (1 hunks)
- tests/Query/BuilderTest.php (2 hunks)
Additional context used
PHPStan
src/Query/Builder.php
null-null: Ignored error pattern #^Strict comparison using === between Illuminate\Database\Query\IndexHint and null will always evaluate to false.$# in path /cache/d6c69f7b-f4e3-418b-a964-7366d864d0f7/home/jailuser/git/src/Query/Builder.php was not matched in reported errors. (ignore.unmatched)
27-27: Class Colopl\Spanner\Query\Builder extends unknown class Illuminate\Database\Query\Builder. (class.notFound)
Learn more at https://phpstan.org/user-guide/discovering-symbols
43-43: Method Colopl\Spanner\Query\Builder::insert() has no return type specified. (missingType.return)
45-45: Colopl\Spanner\Query\Builder::insert() calls parent::insert() but Colopl\Spanner\Query\Builder does not extend any class. (class.noParent)
51-51: Method Colopl\Spanner\Query\Builder::update() has no return type specified. (missingType.return)
59-59: Colopl\Spanner\Query\Builder::update() calls parent::update() but Colopl\Spanner\Query\Builder does not extend any class. (class.noParent)
65-65: Method Colopl\Spanner\Query\Builder::updateOrInsert() has no return type specified. (missingType.return)
67-67: Call to an undefined method Colopl\Spanner\Query\Builder::where(). (method.notFound)
81-81: Call to an undefined method Colopl\Spanner\Query\Builder::limit(). (method.notFound)
81-81: Call to static method except() on an unknown class Illuminate\Support\Arr. (class.notFound)
Learn more at https://phpstan.org/user-guide/discovering-symbols
87-87: Method Colopl\Spanner\Query\Builder::upsert() has no return type specified. (missingType.return)
87-87: Method Colopl\Spanner\Query\Builder::upsert() has parameter $uniqueBy with no type specified. (missingType.parameter)
87-87: Method Colopl\Spanner\Query\Builder::upsert() has parameter $update with no type specified. (missingType.parameter)
103-103: Call to an undefined method Colopl\Spanner\Query\Builder::applyBeforeQueryCallbacks(). (method.notFound)
106-106: Access to an undefined property Colopl\Spanner\Query\Builder::$grammar. (property.notFound)
Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property
107-107: Call to an undefined method Colopl\Spanner\Query\Builder::cleanBindings(). (method.notFound)
107-107: Call to static method flatten() on an unknown class Illuminate\Support\Arr. (class.notFound)
Learn more at https://phpstan.org/user-guide/discovering-symbols
114-114: Method Colopl\Spanner\Query\Builder::truncate() has no return type specified. (missingType.return)
116-116: Call to an undefined method Colopl\Spanner\Query\Builder::applyBeforeQueryCallbacks(). (method.notFound)
118-118: Access to an undefined property Colopl\Spanner\Query\Builder::$grammar. (property.notFound)
Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property
141-141: Access to an undefined property Colopl\Spanner\Query\Builder::$wheres. (property.notFound)
Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property
143-143: Call to an undefined method Colopl\Spanner\Query\Builder::addBinding(). (method.notFound)
154-154: Parameter $values of method Colopl\Spanner\Query\Builder::whereInUnnest() has invalid type Illuminate\Contracts\Support\Arrayable. (class.notFound)
161-161: Access to an undefined property Colopl\Spanner\Query\Builder::$wheres. (property.notFound)
Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property
163-163: Call to an undefined method Colopl\Spanner\Query\Builder::addBinding(). (method.notFound)
175-175: Call to an undefined method Colopl\Spanner\Query\Builder::getGrammar(). (method.notFound)
177-177: Call to an undefined method Colopl\Spanner\Query\Builder::whereRaw(). (method.notFound)
191-191: Call to static method isAssoc() on an unknown class Illuminate\Support\Arr. (class.notFound)
Learn more at https://phpstan.org/user-guide/discovering-symbols
201-201: Method Colopl\Spanner\Query\Builder::runSelect() has no return type specified. (missingType.return)
203-203: Call to an undefined method Colopl\Spanner\Query\Builder::toSql(). (method.notFound)
204-204: Call to an undefined method Colopl\Spanner\Query\Builder::getBindings(). (method.notFound)
229-229: Access to an undefined property Colopl\Spanner\Query\Builder::$indexHint. (property.notFound)
Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property
229-229: Class Colopl\Spanner\Query\IndexHint does not have a constructor and must be instantiated without any parameters. (new.noConstructor)
239-239: Access to an undefined property Colopl\Spanner\Query\Builder::$indexHint. (property.notFound)
Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property
LanguageTool
CHANGELOG.md
[duplication] ~82-~82: Possible typo: you repeated a word (ENGLISH_WORD_REPEAT_RULE)
Context: ...an error (#169) # v6.1.2 (2024-01-16) Fixed - Fixed an error when rolling back a transactio...
[grammar] ~92-~92: You’ve repeated a verb. Did you mean to only write one of them? (REPEATED_VERBS)
Context: ...ondition (#152) # v6.1.0 (2023-11-29) Added - Add support for [NUMERIC](https://cloud.goo...
[grammar] ~100-~100: You’ve repeated a verb. Did you mean to only write one of them? (REPEATED_VERBS)
Context: ...10.34.0. (#150) # v6.0.0 (2023-11-22) Added - Add [Data Boost](https://cloud.google.com/s...
[uncategorized] ~104-~104: When ‘spanner-specific’ is used as a modifier, it is usually spelled with a hyphen. (SPECIFIC_HYPHEN)
Context: ...tionscursorWithOptions` which allows spanner specific options to be set for each query. (#122...
[style] ~116-~116: Style-wise, it’s not ideal to insert an adverb (‘correctly’) in the middle of an infinitive construction (‘to run’). Try moving the adverb to avoid split infinitives. (SPLIT_INFINITIVE)
Context: ...- Explicitly stage/clear transaction on commit to correctly run afterCommit jobs in Laravel >= [v10.32.0](https://github...
[duplication] ~119-~119: Possible typo: you repeated a word (ENGLISH_WORD_REPEAT_RULE)
Context: ...l/48859) (#144) # v5.2.2 (2023-08-22) Fixed - Fixed a case where queries were not being ret...
[duplication] ~129-~129: Possible typo: you repeated a word (ENGLISH_WORD_REPEAT_RULE)
Context: ...ry/Builder::toRawSql(#127) # v5.2.0 Added - Added deprecation warnings toConnection::ru...
[duplication] ~148-~148: Possible typo: you repeated a word (ENGLISH_WORD_REPEAT_RULE)
Context: ... ROW DELETION POLICY(#125) # v5.1.0 Added - AddedConnection::runDdlBatch` which runs DD...
[duplication] ~153-~153: Possible typo: you repeated a word (ENGLISH_WORD_REPEAT_RULE)
Context: ...oublefor better compatibility. (#97) Fixed - Fixed bug where runningConnection::statemen...
[uncategorized] ~160-~160: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ...n pool and authentication per connection so transaction works properly. (#89) - Ses...
[uncategorized] ~181-~181: Possible missing preposition found. (AI_HYDRA_LEO_MISSING_IT)
Context: ...rowBadMethodCallExceptionif called. Was ignored in previous versions. (#76) - [...
[uncategorized] ~194-~194: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...e - Removedramsey/uuidfrom composer.json since laravel already includes it. Fix...
[uncategorized] ~251-~251: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...urns'Y-m-d\TH:i:s.uP'instead of the default which does work correctly in Cloud Span...README.md
[uncategorized] ~138-~138: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...anner creates transactions for all data operations even if you do not explicitly create tr...
[style] ~184-~184: Unless you want to emphasize “not”, use “cannot” which is more common. (CAN_NOT_PREMIUM)
Context: ...saction withsingleUseoption. So you can not run as read-write transaction. ### Dat...
[uncategorized] ~332-~332: When ‘Spanner-specific’ is used as a modifier, it is usually spelled with a hyphen. (SPECIFIC_HYPHEN)
Context: ...Secondary Index Options You can define Spanner specific index options like [null filtering](htt...
[grammar] ~358-~358: You should probably use “call”. (AGREEMENT_SENT_START)
Context: ...work the same way as DML. All mutations calls within a transaction are queued and sen...
[style] ~363-~363: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM)
Context: ...ction. ### SessionPool and AuthCache In order to improve the performance of the first co...
[uncategorized] ~372-~372: Possible missing comma found. (AI_HYDRA_LEO_MISSING_COMMA)
Context: ...ions remain active for 60 minutes after use so deleting the sessions during the shu...
[uncategorized] ~377-~377: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short). (COMMA_COMPOUND_SENTENCE_2)
Context: ...sed, the connection will be disconnected so the session can be released into the se...
[style] ~395-~395: Consider a shorter alternative to avoid wordiness. (IN_ORDER_TO_PREMIUM)
Context: ...some environment variables must be set. In order to set the variables, rename [.env.sample]...
[grammar] ~403-~403: Did you mean the adverb/preposition “within”? (WITH_IN)
Context: ...NER_DATABASE_ID| Name of the database with in the Google Cloud Spanner instance | |...
Markdownlint
CHANGELOG.md
6-6: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
11-11: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
21-21: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
36-36: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
41-41: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
51-51: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
81-81: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
86-86: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
91-91: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
99-99: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
113-113: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
118-118: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
123-123: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
128-128: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
147-147: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
163-163: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
188-188: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
199-199: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
205-205: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
213-213: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
227-227: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
245-245: null (MD025, single-title, single-h1)
Multiple top-level headings in the same document
4-4: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
9-9: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
14-14: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
19-19: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
24-24: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
39-39: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
44-44: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
49-49: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
54-54: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
67-67: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
78-78: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
84-84: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
89-89: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
94-94: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
97-97: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
102-102: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
109-109: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
116-116: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
121-121: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
126-126: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
131-131: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
138-138: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
144-144: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
150-150: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
155-155: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
159-159: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
168-168: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
173-173: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
186-186: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
191-191: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
194-194: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
197-197: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
202-202: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
208-208: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
211-211: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
216-216: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
222-222: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
225-225: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
230-230: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
240-240: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
243-243: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
248-248: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
251-251: null (MD032, blanks-around-lists)
Lists should be surrounded by blank linesREADME.md
10-10: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
18-18: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
52-52: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
80-80: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
89-89: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
95-95: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
97-97: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
100-100: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
105-105: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
109-109: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
111-111: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
117-117: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
167-167: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
186-186: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
209-209: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
222-222: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
239-239: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
255-255: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
276-276: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
301-301: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
330-330: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
346-346: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
362-362: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
375-375: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
381-381: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
392-392: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
394-394: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
410-410: Expected: setext; Actual: atx (MD003, heading-style)
Heading style
86-86: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
106-106: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
113-113: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
372-372: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
377-377: Expected: 0 or 2; Actual: 1 (MD009, no-trailing-spaces)
Trailing spaces
238-238: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
254-254: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
329-329: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
361-361: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
380-380: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
390-390: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
391-391: Expected: 1; Actual: 3 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
413-413: Expected: 1; Actual: 2 (MD012, no-multiple-blanks)
Multiple consecutive blank lines
18-18: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
52-52: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
97-97: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
100-100: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
105-105: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
117-117: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
239-239: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
255-255: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
381-381: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
394-394: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
410-410: Expected: 1; Actual: 0; Below (MD022, blanks-around-headings)
Headings should be surrounded by blank lines
101-101: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
233-233: null (MD032, blanks-around-lists)
Lists should be surrounded by blank lines
21-21: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
350-350: null (MD040, fenced-code-language)
Fenced code blocks should have a language specified
Additional comments not posted (1)
src/Query/Builder.php (1)
176-178: Review the performance implications of usingwhereInEmbeddedmethod with large datasets.Tools
PHPStan
177-177: Call to an undefined method Colopl\Spanner\Query\Builder::whereRaw(). (method.notFound)
| public function test_whereInEmbedded_can_do_more_than_950_parameters(): void | ||
| { | ||
| $conn = $this->getDefaultConnection(); | ||
| $tableName = self::TABLE_NAME_USER; | ||
| $qb = $conn->table($tableName); | ||
| $id1 = $this->generateUuid(); | ||
| $id2 = $this->generateUuid(); | ||
| $dummyIds = array_map($this->generateUuid(...), range(0, 950)); | ||
|
|
||
| $qb->insert([['userId' => $id1, 'name' => 't1'], ['userId' => $id2, 'name' => 't2']]); | ||
| $ids = $qb->whereInEmbedded('userId', [$id1, $id2, ...$dummyIds])->pluck('userId'); | ||
| $this->assertEquals([$id1, $id2], $ids->all()); | ||
| } |
There was a problem hiding this comment.
Consider adding more robust assertions to ensure the correctness of whereInEmbedded.
The test verifies that the IDs match, but it could be enhanced by checking that no additional IDs are returned and that the exact expected IDs are present in the correct order. This would make the test more robust against potential modifications that could alter the behavior of whereInEmbedded.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
This will allow users to use whereIn without the fear of reaching the 950 query parameter limit.
Summary by CodeRabbit
New Features
Documentation
Tests