Merge pull request #168 from nextcloud/fix/openapitype/array-syntax-ambiguities

provokateurin · web-flow · commit d7c135e819d2 · 2024-11-06T12:50:20.000+01:00
diff --git a/generate-spec b/generate-spec
@@ -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');
diff --git a/src/OpenApiType.php b/src/OpenApiType.php
@@ -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,
@@ -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;
diff --git a/tests/lib/Controller/AdminSettingsController.php b/tests/lib/Controller/AdminSettingsController.php
@@ -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
 	 */
@@ -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
 	 */
@@ -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
 	 */
@@ -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
 	 */
diff --git a/tests/lib/Controller/ExAppSettingsController.php b/tests/lib/Controller/ExAppSettingsController.php
@@ -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
 	 */
@@ -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
 	 */
diff --git a/tests/lib/Controller/FederationController.php b/tests/lib/Controller/FederationController.php
@@ -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
 	 */
@@ -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
 	 */
diff --git a/tests/lib/Controller/RoutingController.php b/tests/lib/Controller/RoutingController.php
@@ -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
 	 */
@@ -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
 	 */
diff --git a/tests/lib/Controller/SettingsController.php b/tests/lib/Controller/SettingsController.php
diff --git a/tests/lib/ResponseDefinitions.php b/tests/lib/ResponseDefinitions.php

Original file line number	Diff line number	Diff line change
`@@ -154,7 +154,7 @@ if (file_exists($definitionsPath)) {`
`154`	`154`	`}`
`155`	`155`	`}`
`156`	`156`	`foreach (array_keys($definitions) as $name) {`
`157`		`- $schemas[Helpers::cleanSchemaName($name)] = OpenApiType::resolve('Response definitions', $definitions, $definitions[$name])->toArray();`
	`157`	`+ $schemas[Helpers::cleanSchemaName($name)] = OpenApiType::resolve('Response definitions: ' . $name, $definitions, $definitions[$name])->toArray();`
`158`	`158`	`}`
`159`	`159`	`} else {`
`160`	`160`	`Logger::debug('Response definitions', 'No response definitions were loaded');`
Original file line number	Diff line number	Diff line change
`@@ -18,7 +18,7 @@ class AdminSettingsController extends OCSController {`
`18`	`18`	`/**`
`19`	`19`	`* Route is only in the admin scope because there is no "NoAdminRequired" annotation or attribute`
`20`	`20`	`*`
`21`		`- * @return DataResponse<Http::STATUS_OK, array<empty>, array{}>`
	`21`	`+ * @return DataResponse<Http::STATUS_OK, list<empty>, array{}>`
`22`	`22`	`*`
`23`	`23`	`* 200: Personal settings updated`
`24`	`24`	`*/`
`@@ -29,7 +29,7 @@ public function adminScopeImplicitFromAdminRequired(): DataResponse {`
`29`	`29`	`/**`
`30`	`30`	`* Route is in the default scope because the method overwrites with the Attribute`
`31`	`31`	`*`
`32`		`- * @return DataResponse<Http::STATUS_OK, array<empty>, array{}>`
	`32`	`+ * @return DataResponse<Http::STATUS_OK, list<empty>, array{}>`
`33`	`33`	`*`
`34`	`34`	`* 200: Personal settings updated`
`35`	`35`	`*/`
`@@ -41,7 +41,7 @@ public function movedToDefaultScope(): DataResponse {`
`41`	`41`	`/**`
`42`	`42`	`* Route in default scope with tags`
`43`	`43`	`*`
`44`		`- * @return DataResponse<Http::STATUS_OK, array<empty>, array{}>`
	`44`	`+ * @return DataResponse<Http::STATUS_OK, list<empty>, array{}>`
`45`	`45`	`*`
`46`	`46`	`* 200: Personal settings updated`
`47`	`47`	`*/`
`@@ -53,7 +53,7 @@ public function movedToSettingsTag(): DataResponse {`
`53`	`53`	`/**`
`54`	`54`	`* Route in default scope with tags but without named parameters on the attribute`
`55`	`55`	`*`
`56`		`- * @return DataResponse<Http::STATUS_OK, array<empty>, array{}>`
	`56`	`+ * @return DataResponse<Http::STATUS_OK, list<empty>, array{}>`
`57`	`57`	`*`
`58`	`58`	`* 200: Personal settings updated`
`59`	`59`	`*/`
Original file line number	Diff line number	Diff line change
`@@ -20,7 +20,7 @@ class ExAppSettingsController extends OCSController {`
`20`	`20`	`/**`
`21`	`21`	`* Route is in ex_app scope because of the attribute`
`22`	`22`	`*`
`23`		`- * @return DataResponse<Http::STATUS_OK, array<empty>, array{}>`
	`23`	`+ * @return DataResponse<Http::STATUS_OK, list<empty>, array{}>`
`24`	`24`	`*`
`25`	`25`	`* 200: Personal settings updated`
`26`	`26`	`*/`
`@@ -32,7 +32,7 @@ public function exAppScopeAttribute(): DataResponse {`
`32`	`32`	`/**`
`33`	`33`	`* Route is in ex_app scope because of the override`
`34`	`34`	`*`
`35`		`- * @return DataResponse<Http::STATUS_OK, array<empty>, array{}>`
	`35`	`+ * @return DataResponse<Http::STATUS_OK, list<empty>, array{}>`
`36`	`36`	`*`
`37`	`37`	`* 200: Personal settings updated`
`38`	`38`	`*/`
Original file line number	Diff line number	Diff line change
`@@ -29,7 +29,7 @@ class FederationController extends OCSController {`
`29`	`29`	`*`
`30`	`30`	`* Route is in federation scope as per controller scope`
`31`	`31`	`*`
`32`		`- * @return DataResponse<Http::STATUS_OK, array<empty>, array{}>`
	`32`	`+ * @return DataResponse<Http::STATUS_OK, list<empty>, array{}>`
`33`	`33`	`*`
`34`	`34`	`* 200: OK`
`35`	`35`	`*/`
`@@ -43,7 +43,7 @@ public function federationByController(): DataResponse {`
`43`	`43`	`* Route is only in the default scope (moved from federation)`
`44`	`44`	`*`
`45`	`45`	`* @param NotificationsRequestProperty $property Property`
`46`		`- * @return DataResponse<Http::STATUS_OK, array<empty>, array{}>`
	`46`	`+ * @return DataResponse<Http::STATUS_OK, list<empty>, array{}>`
`47`	`47`	`*`
`48`	`48`	`* 200: Personal settings updated`
`49`	`49`	`*/`
Original file line number	Diff line number	Diff line change
`@@ -16,7 +16,7 @@`
`16`	`16`	`class RoutingController extends OCSController {`
`17`	`17`	`/**`
`18`	`18`	`* OCS Route with attribute`
`19`		`- * @return DataResponse<Http::STATUS_OK, array<empty>, array{}>`
	`19`	`+ * @return DataResponse<Http::STATUS_OK, list<empty>, array{}>`
`20`	`20`	`*`
`21`	`21`	`* 200: Success`
`22`	`22`	`*/`
`@@ -30,7 +30,7 @@ public function attributeOCS() {`
`30`	`30`	`* @NoCSRFRequired`
`31`	`31`	`*`
`32`	`32`	`* Index Route with attribute`
`33`		`- * @return DataResponse<Http::STATUS_OK, array<empty>, array{}>`
	`33`	`+ * @return DataResponse<Http::STATUS_OK, list<empty>, array{}>`
`34`	`34`	`*`
`35`	`35`	`* 200: Success`
`36`	`36`	`*/`