Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(OpenApiType): Forbid using ambigous array syntaxes #168

Merged
merged 2 commits into from
Nov 6, 2024
Merged

fix(OpenApiType): Forbid using ambigous array syntaxes #168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bed64c9
fix: Provide more context when parsing response definitions
provokateurin Sep 24, 2024
dd13b3f
fix(OpenApiType): Forbid using ambigous array syntaxes
provokateurin Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion generate-spec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ if (file_exists($definitionsPath)) {
}
}
foreach (array_keys($definitions) as $name) {
$schemas[Helpers::cleanSchemaName($name)] = OpenApiType::resolve('Response definitions', $definitions, $definitions[$name])->toArray();
$schemas[Helpers::cleanSchemaName($name)] = OpenApiType::resolve('Response definitions: ' . $name, $definitions, $definitions[$name])->toArray();
}
} else {
Logger::debug('Response definitions', 'No response definitions were loaded');
Expand Down
8 changes: 7 additions & 1 deletion src/OpenApiType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,13 +174,19 @@ public static function resolve(string $context, array $definitions, ParamTagValu
}

if ($node instanceof ArrayTypeNode) {
Logger::error($context, "The 'TYPE[]' syntax for arrays is forbidden due to ambiguities. Use 'list<TYPE>' for JSON arrays or 'array<string, TYPE>' for JSON objects instead.");

return new OpenApiType(
context: $context,
type: 'array',
items: self::resolve($context . ': items', $definitions, $node->type),
);
}
if ($node instanceof GenericTypeNode && ($node->type->name === 'array' || $node->type->name === 'list' || $node->type->name === 'non-empty-list') && count($node->genericTypes) === 1) {
if ($node->type->name === 'array') {
Logger::error($context, "The 'array<TYPE>' syntax for arrays is forbidden due to ambiguities. Use 'list<TYPE>' for JSON arrays or 'array<string, TYPE>' for JSON objects instead.");
}

if ($node->genericTypes[0] instanceof IdentifierTypeNode && $node->genericTypes[0]->name === 'empty') {
return new OpenApiType(
context: $context,
Expand All @@ -203,8 +209,8 @@ public static function resolve(string $context, array $definitions, ParamTagValu
$properties = [];
$required = [];
foreach ($node->items as $item) {
$type = self::resolve($context, $definitions, $item->valueType);
$name = $item->keyName instanceof ConstExprStringNode ? $item->keyName->value : $item->keyName->name;
$type = self::resolve($context . ': ' . $name, $definitions, $item->valueType);
$properties[$name] = $type;
if (!$item->optional) {
$required[] = $name;
Expand Down
8 changes: 4 additions & 4 deletions tests/lib/Controller/AdminSettingsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class AdminSettingsController extends OCSController {
/**
* Route is only in the admin scope because there is no "NoAdminRequired" annotation or attribute
*
* @return DataResponse<Http::STATUS_OK, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK, list<empty>, array{}>
*
* 200: Personal settings updated
*/
Expand All @@ -29,7 +29,7 @@ public function adminScopeImplicitFromAdminRequired(): DataResponse {
/**
* Route is in the default scope because the method overwrites with the Attribute
*
* @return DataResponse<Http::STATUS_OK, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK, list<empty>, array{}>
*
* 200: Personal settings updated
*/
Expand All @@ -41,7 +41,7 @@ public function movedToDefaultScope(): DataResponse {
/**
* Route in default scope with tags
*
* @return DataResponse<Http::STATUS_OK, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK, list<empty>, array{}>
*
* 200: Personal settings updated
*/
Expand All @@ -53,7 +53,7 @@ public function movedToSettingsTag(): DataResponse {
/**
* Route in default scope with tags but without named parameters on the attribute
*
* @return DataResponse<Http::STATUS_OK, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK, list<empty>, array{}>
*
* 200: Personal settings updated
*/
Expand Down
4 changes: 2 additions & 2 deletions tests/lib/Controller/ExAppSettingsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ class ExAppSettingsController extends OCSController {
/**
* Route is in ex_app scope because of the attribute
*
* @return DataResponse<Http::STATUS_OK, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK, list<empty>, array{}>
*
* 200: Personal settings updated
*/
Expand All @@ -32,7 +32,7 @@ public function exAppScopeAttribute(): DataResponse {
/**
* Route is in ex_app scope because of the override
*
* @return DataResponse<Http::STATUS_OK, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK, list<empty>, array{}>
*
* 200: Personal settings updated
*/
Expand Down
4 changes: 2 additions & 2 deletions tests/lib/Controller/FederationController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class FederationController extends OCSController {
*
* Route is in federation scope as per controller scope
*
* @return DataResponse<Http::STATUS_OK, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK, list<empty>, array{}>
*
* 200: OK
*/
Expand All @@ -43,7 +43,7 @@ public function federationByController(): DataResponse {
* Route is only in the default scope (moved from federation)
*
* @param NotificationsRequestProperty $property Property
* @return DataResponse<Http::STATUS_OK, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK, list<empty>, array{}>
*
* 200: Personal settings updated
*/
Expand Down
4 changes: 2 additions & 2 deletions tests/lib/Controller/RoutingController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
class RoutingController extends OCSController {
/**
* OCS Route with attribute
* @return DataResponse<Http::STATUS_OK, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK, list<empty>, array{}>
*
* 200: Success
*/
Expand All @@ -30,7 +30,7 @@ public function attributeOCS() {
* @NoCSRFRequired
*
* Index Route with attribute
* @return DataResponse<Http::STATUS_OK, array<empty>, array{}>
* @return DataResponse<Http::STATUS_OK, list<empty>, array{}>
*
* 200: Success
*/
Expand Down
Loading
Loading