Repository navigation
feat(specs): let array params declare their item type - #14266
Conversation
|
Review complete. No blocking issues — approved ✅; 1 nitpick below. 🧹 Nitpicks (1) — 🟢 1 low
This PR adds Appwrite\SDK\Specification\Validator\Items, a delegating validator that pairs an item validator with an isArray flag, so array parameters can describe their element type rather than just being ArrayList-wrapped.
One low-severity latent gap: the WhiteList enum-promotion path that ArrayList triggers is not applied to Items, so an Items-wrapped WhiteList currently emits a plain string-array schema without the enum. No production call site hits that path yet. Reviewed commit: b5908b3 |
🟡 Tier B · Needs changes before merging
The change updates GraphQL and OpenAPI mappings for WAF conditions and
Fix with agent prompt### Issue 1
src/Appwrite/SDK/Specification/Format/OpenAPI3.php:827-828
**Array item types are hard-coded to string**
`Multiple` runs its rules against the whole value, so `TYPE_ARRAY` does not establish that each element is a string; the new test even uses an empty rule set. No item-validator wrapper or equivalent declaration is added, so array parameters with integer or object items cannot declare their type and both generators advertise them as string lists, leaving the described item-type feature unavailable beyond these special cases.
### Issue 2
src/Appwrite/SDK/Specification/Format/OpenAPI3.php:825
**Avoid emitting `mixed` as an OpenAPI type**
`Multiple` defaults to `Validator::TYPE_MIXED` (`packages/validators/src/Validator/Multiple.php`), so a default `new Multiple([...])` now emits `schema.type: mixed`. `mixed` is not a valid OpenAPI schema type, making the generated spec invalid for this supported validator construction.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.📂 Walkthrough · 4
Reviewed |
Security rulesNo new WARNING or ERROR findings from security rules. 32 existing findings tracked in
|
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
Both check a whole array, so #14265 emitted them as arrays with untyped items and the SDKs typed WAF conditions and OAuth2 resource indicators as untyped values. Conditions always holds condition strings; an array-typed Multiple is only used for the resource indicator list, so both get string items in OpenAPI and GraphQL.
b5908b3 to
326b7a1
Compare
✨ Benchmark resultsComparing
Per-scenario breakdown & investigation detailsMetrics below reflect the current branch (after). Δ P95 compares against the base.
Top API waits (after)
|
![hansi-codes[bot]](https://avatars.githubusercontent.com/in/5046327?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Tier B · 1 blocking finding to address. Summary
| $node['schema']['items'] = [ | ||
| 'type' => 'string', |
There was a problem hiding this comment.
Array item types are hard-coded to string
Multiple runs its rules against the whole value, so TYPE_ARRAY does not establish that each element is a string; the new test even uses an empty rule set. No item-validator wrapper or equivalent declaration is added, so array parameters with integer or object items cannot declare their type and both generators advertise them as string lists, leaving the described item-type feature unavailable beyond these special cases.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Appwrite/SDK/Specification/Format/OpenAPI3.php
Line: 827-828
Comment:
**Array item types are hard-coded to string**
`Multiple` runs its rules against the whole value, so `TYPE_ARRAY` does not establish that each element is a string; the new test even uses an empty rule set. No item-validator wrapper or equivalent declaration is added, so array parameters with integer or object items cannot declare their type and both generators advertise them as string lists, leaving the described item-type feature unavailable beyond these special cases.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟠 Major · bug · Reply if this doesn't apply.
| case \Utopia\Validator\Multiple::class: | ||
| // Its rules check the whole value, so an array-typed Multiple declares no | ||
| // item type. The only one in use is the OAuth2 resource indicator list. | ||
| $node['schema']['type'] = $validator->getType(); |
There was a problem hiding this comment.
Avoid emitting mixed as an OpenAPI type
Multiple defaults to Validator::TYPE_MIXED (packages/validators/src/Validator/Multiple.php), so a default new Multiple([...]) now emits schema.type: mixed. mixed is not a valid OpenAPI schema type, making the generated spec invalid for this supported validator construction.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/Appwrite/SDK/Specification/Format/OpenAPI3.php
Line: 825
Comment:
**Avoid emitting `mixed` as an OpenAPI type**
`Multiple` defaults to `Validator::TYPE_MIXED` (`packages/validators/src/Validator/Multiple.php`), so a default `new Multiple([...])` now emits `schema.type: mixed`. `mixed` is not a valid OpenAPI schema type, making the generated spec invalid for this supported validator construction.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.🟡 Minor · bug · Reply if this doesn't apply.
1 participant
Follow-up to #14265. After it, a param whose validator checks the whole array gets an array schema, but
itemsis{}, because these validators don't say what an item is. Two params land there, and both are lists of strings:conditions(Utopia\WAF\Validator\Conditions): an array of condition strings.resource(Multiple([], TYPE_ARRAY)): RFC 8707 resource indicator URIs, validated in the action.Their SDK types came out as
interface{}andDict[str, Any](flagged on appwrite/sdk-for-go#83 and appwrite/sdk-for-console-python#15). This adds both validators to OpenAPI3 and the GraphQL mapper:ConditionsjoinsQueries,PermissionsandRolesas a list of strings.Multipleemits its declared type, adding string items when that type isarray. The webhook URL and health domain params use string-typedMultiples and come out unchanged.testMultipleEmitsItsDeclaredTypeandtestMultipleMapsToItsDeclaredTypefail without the change.Related: appwrite-labs/cloud#6413