fix: skip generated CLI query flags that collide with spec parameters - #1655
fix: skip generated CLI query flags that collide with spec parameters#1655ChiragAgg5k wants to merge 1 commit into
Conversation
Greptile SummaryThis PR fixes a Commander.js duplicate option binding crash by suppressing auto-generated CLI query flags (
Confidence Score: 4/5Safe to merge — fixes a real crash with minimal, well-scoped changes. The crash fix is correct and the $collides closure logic is sound for the stated limit/offset use case. The all-or-nothing suppression of the filtering group is slightly broader than the minimum required, which could silently drop valid CLI flags if a future spec parameter happens to share a name with just one of the filtering flag identifiers. src/SDK/Language/CLI.php — specifically the $hasFilteringQueries collision guard Important Files Changed
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
src/SDK/Language/CLI.php:244
**All-or-nothing filtering suppression on partial collision**
`$collides(['filter', 'where', 'sortAsc', 'sortDesc', 'cursorAfter', 'cursorBefore'])` returns `true` if *any single* name in the list matches a spec parameter, which then suppresses the entire filtering group. If a future endpoint declares, say, a `where` parameter but none of the others, the generated `--filter`, `--sort-asc`, `--sort-desc`, `--cursor-after`, and `--cursor-before` flags would all be silently dropped even though only `--where` would have caused a Commander collision. For the immediate `limit`/`offset` case this is irrelevant, but the filtering group check could be overly aggressive for later spec additions.
Reviews (1): Last reviewed commit: "(fix): skip generated CLI query flags th..." | Re-trigger Greptile |
| $hasSelectionOnlyQueries = $hasQueries && in_array($methodName, ['getDocument', 'getRow'], true); | ||
| $hasFilteringQueries = $hasQueries && !$hasOnlyLimitOffsetQueries && !$hasSelectionOnlyQueries; | ||
| $hasPaginationQueries = $hasQueries && !$hasSelectionOnlyQueries; | ||
| $hasFilteringQueries = $hasQueries && !$hasOnlyLimitOffsetQueries && !$hasSelectionOnlyQueries && !$collides(['filter', 'where', 'sortAsc', 'sortDesc', 'cursorAfter', 'cursorBefore']); |
There was a problem hiding this comment.
All-or-nothing filtering suppression on partial collision
$collides(['filter', 'where', 'sortAsc', 'sortDesc', 'cursorAfter', 'cursorBefore']) returns true if any single name in the list matches a spec parameter, which then suppresses the entire filtering group. If a future endpoint declares, say, a where parameter but none of the others, the generated --filter, --sort-asc, --sort-desc, --cursor-after, and --cursor-before flags would all be silently dropped even though only --where would have caused a Commander collision. For the immediate limit/offset case this is irrelevant, but the filtering group check could be overly aggressive for later spec additions.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/SDK/Language/CLI.php
Line: 244
Comment:
**All-or-nothing filtering suppression on partial collision**
`$collides(['filter', 'where', 'sortAsc', 'sortDesc', 'cursorAfter', 'cursorBefore'])` returns `true` if *any single* name in the list matches a spec parameter, which then suppresses the entire filtering group. If a future endpoint declares, say, a `where` parameter but none of the others, the generated `--filter`, `--sort-asc`, `--sort-desc`, `--cursor-after`, and `--cursor-before` flags would all be silently dropped even though only `--where` would have caused a Commander collision. For the immediate `limit`/`offset` case this is irrelevant, but the filtering group check could be overly aggressive for later spec additions.
How can I resolve this? If you propose a fix, please make it concise.|
Consolidated into #1653 so all pipelines can validate the pin bump and the flag-collision fix together. |
The CLI generator adds
--filter/--sort-*/--limit/--offset/--cursor-*/--selectoptions to any command whose method has aqueriesarray parameter. The new cloudusageendpoints (listEvents,listGauges) declare literallimitandoffsetparameters alongsidequeries, so the generated command binds--limit/--offsettwice and Commander throws at registration:This is what currently fails the
cli (console)validation job (visible on #1653 after the 1.9.x specs sync).getCliQueryConfig()now suppresses a generated flag group when any of its names collide with the method's own parameters, so the spec-declared options win.Verified by generating from the current 1.9.x console spec:
usagecommands keep their single spec-declared--limit/--offset,bun run linux-x64compiles,npm run buildpasses, andnode dist/cli.cjs usage list-events --helprenders correctly (with@appwrite.io/console@15.3.0installed — the remaining validation dependency is the pin bump in #1653).