Skip to content

fix(OpenApiType): Prevent making misktakes with empty JSON objects #244

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+26 −22
Draft

fix(OpenApiType): Prevent making misktakes with empty JSON objects #244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 16 additions & 12 deletions src/OpenApiType.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ enum: [0, 1],
if ($this->description !== null && $this->description !== '' && !$isParameter) {
$values['description'] = Helpers::cleanDocComment($this->description);
}
if ($this->items instanceof \OpenAPIExtractor\OpenApiType) {
if ($this->items instanceof OpenApiType) {
$values['items'] = $this->items->toArray();
}
if ($this->minLength !== null) {
Expand All @@ -139,7 +139,7 @@ enum: [0, 1],
}
if ($this->properties !== null && $this->properties !== []) {
$values['properties'] = array_combine(array_keys($this->properties),
array_map(static fn (OpenApiType $property): array|\stdClass => $property->toArray(), array_values($this->properties)),
array_map(static fn (OpenApiType $property): array|stdClass => $property->toArray(), array_values($this->properties)),
);
}
if ($this->additionalProperties !== null) {
Expand All @@ -150,13 +150,13 @@ enum: [0, 1],
}
}
if ($this->oneOf !== null) {
$values['oneOf'] = array_map(fn (OpenApiType $type): array|\stdClass => $type->toArray(), $this->oneOf);
$values['oneOf'] = array_map(fn (OpenApiType $type): array|stdClass => $type->toArray(), $this->oneOf);
}
if ($this->anyOf !== null) {
$values['anyOf'] = array_map(fn (OpenApiType $type): array|\stdClass => $type->toArray(), $this->anyOf);
$values['anyOf'] = array_map(fn (OpenApiType $type): array|stdClass => $type->toArray(), $this->anyOf);
}
if ($this->allOf !== null) {
$values['allOf'] = array_map(fn (OpenApiType $type): array|\stdClass => $type->toArray(), $this->allOf);
$values['allOf'] = array_map(fn (OpenApiType $type): array|stdClass => $type->toArray(), $this->allOf);
}

return $values !== [] ? $values : new stdClass();
Expand Down Expand Up @@ -184,9 +184,9 @@ public static function resolve(string $context, array $definitions, ParamTagValu
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 instanceof GenericTypeNode && ($node->type->name === 'array' || $node->type->name === 'non-empty-array' || $node->type->name === 'list' || $node->type->name === 'non-empty-list') && count($node->genericTypes) === 1) {
if ($node->type->name === 'array' || $node->type->name === 'non-empty-array') {
Logger::error($context, "The 'array<TYPE>' and 'non-empty-array<TYPE>' syntax for arrays is forbidden due to ambiguities. Use 'list<TYPE>' for JSON arrays or 'non-empty-array<string, TYPE>' for JSON objects instead.");
}

if ($node->genericTypes[0] instanceof IdentifierTypeNode && $node->genericTypes[0]->name === 'empty') {
Expand Down Expand Up @@ -232,7 +232,11 @@ public static function resolve(string $context, array $definitions, ParamTagValu
);
}

if ($node instanceof GenericTypeNode && $node->type->name === 'array' && count($node->genericTypes) === 2 && $node->genericTypes[0] instanceof IdentifierTypeNode) {
if ($node instanceof GenericTypeNode && in_array($node->type->name, ['array', 'non-empty-array']) && count($node->genericTypes) === 2 && $node->genericTypes[0] instanceof IdentifierTypeNode) {
if ($node->type->name !== 'non-empty-array') {
Logger::error($context, 'You must ensure JSON objects are not empty using the "non-empty-array" type. To allow return empty JSON objects your code must manually check if the array is empty in order to return "new \\stdClass()" and use "non-empty-array|\\stdClass" as the type.');
}

$allowedTypes = ['string', 'lowercase-string', 'non-empty-string', 'non-empty-lowercase-string'];
if (in_array($node->genericTypes[0]->name, $allowedTypes, true)) {
return new OpenApiType(
Expand Down Expand Up @@ -428,15 +432,15 @@ private static function mergeEnums(string $context, array $types): array {
}
}

return array_merge($nonEnums, array_map(static fn (string $type): \OpenAPIExtractor\OpenApiType => new OpenApiType(
return array_merge($nonEnums, array_map(static fn (string $type): OpenApiType => new OpenApiType(
context: $context,
type: $type, enum: $enums[$type],
), array_keys($enums)));
}

private static function resolveIdentifier(string $context, array $definitions, string $name): OpenApiType {
if ($name === 'array') {
Logger::error($context, "Instead of 'array' use:\n'new stdClass()' for empty objects\n'array<string, mixed>' for non-empty objects\n'array<emtpy>' for empty lists\n'array<YourTypeHere>' for lists");
Logger::error($context, "Instead of 'array' use:\n'new stdClass()' for empty objects\n'non-empty-array<string, mixed>' for non-empty objects\n'list<emtpy>' for empty lists\n'list<YourTypeHere>' for lists");
}
if (str_starts_with($name, '\\')) {
$name = substr($name, 1);
Expand All @@ -455,7 +459,7 @@ private static function resolveIdentifier(string $context, array $definitions, s
'numeric' => new OpenApiType(context: $context, type: 'number'),
// https://www.php.net/manual/en/language.types.float.php: Both float and double are always stored with double precision
'float', 'double' => new OpenApiType(context: $context, type: 'number', format: 'double'),
'mixed', 'empty', 'array' => new OpenApiType(context: $context, type: 'object'),
'mixed', 'empty' => new OpenApiType(context: $context, type: 'object'),
'object', 'stdClass' => new OpenApiType(context: $context, type: 'object', additionalProperties: true),
'null' => new OpenApiType(context: $context, nullable: true),
default => (function () use ($context, $definitions, $name) {
Expand Down
8 changes: 4 additions & 4 deletions tests/lib/Controller/ReturnArraysController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ class ReturnArraysController extends OCSController {
/**
* Route with array using string keys
*
* @return DataResponse<Http::STATUS_OK, array<string, mixed>, array{}>
* @return DataResponse<Http::STATUS_OK, non-empty-array<string, mixed>|\stdClass, array{}>
*
* 200: OK
*/
Expand All @@ -28,7 +28,7 @@ public function stringArray(): DataResponse {
/**
* Route with array using non-empty-string keys
*
* @return DataResponse<Http::STATUS_OK, array<non-empty-string, mixed>, array{}>
* @return DataResponse<Http::STATUS_OK, non-empty-array<non-empty-string, mixed>|\stdClass, array{}>
*
* 200: OK
*/
Expand All @@ -39,7 +39,7 @@ public function nonEmptyStringArray(): DataResponse {
/**
* Route with array using lowercase-string keys
*
* @return DataResponse<Http::STATUS_OK, array<lowercase-string, mixed>, array{}>
* @return DataResponse<Http::STATUS_OK, non-empty-array<lowercase-string, mixed>|\stdClass, array{}>
*
* 200: OK
*/
Expand All @@ -50,7 +50,7 @@ public function lowercaseStringArray(): DataResponse {
/**
* Route with array using non-empty-lowercase-string keys
*
* @return DataResponse<Http::STATUS_OK, array<non-empty-lowercase-string, mixed>, array{}>
* @return DataResponse<Http::STATUS_OK, non-empty-array<non-empty-lowercase-string, mixed>|\stdClass, array{}>
*
* 200: OK
*/
Expand Down
8 changes: 4 additions & 4 deletions tests/lib/Controller/SettingsController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ public function arrayListParameter(array $value = ['test']): DataResponse {
/**
* A route with keyed array
*
* @param array<string, string> $value Some array value
* @param non-empty-array<string, string>|\stdClass $value Some array value
* @return DataResponse<Http::STATUS_OK, list<empty>, array{}>
*
* 200: Admin settings updated
Expand Down Expand Up @@ -539,7 +539,7 @@ public function emptyArray(): DataResponse {
* Route with parameter
*
* @param int $simple Value
* @param array<string, string> $complex Values
* @param non-empty-array<string, string>|\stdClass $complex Values
* @return DataResponse<Http::STATUS_OK, array{test: list<empty>}, array{}>
*
* 200: OK
Expand All @@ -553,8 +553,8 @@ public function parameterRequestBody(int $simple, array $complex): DataResponse
/**
* Route with object defaults
*
* @param array<string, string> $empty Empty
* @param array<string, string> $values Values
* @param non-empty-array<string, string>|\stdClass $empty Empty
* @param non-empty-array<string, string>|\stdClass $values Values
* @return DataResponse<Http::STATUS_OK, array{test: list<empty>}, array{}>
*
* 200: OK
Expand Down
4 changes: 2 additions & 2 deletions tests/lib/ResponseDefinitions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@
* link: string,
* actions: list<NotificationsNotificationAction>,
* subjectRich?: string,
* subjectRichParameters?: array<string, mixed>,
* subjectRichParameters?: non-empty-array<string, mixed>|\stdClass,
* messageRich?: string,
* messageRichParameters?: array<string, mixed>,
* messageRichParameters?: non-empty-array<string, mixed>|\stdClass,
* icon?: string,
* shouldNotify?: bool,
* nonEmptyList: non-empty-list<string>,
Expand Down
Loading