nextcloud / openapi-extractor

A tool for extracting OpenAPI specifications from Nextcloud source code
https://docs.nextcloud.com/server/latest/developer_manual/client_apis/OCS/ocs-openapi.html
GNU Affero General Public License v3.0
7 stars 2 forks source link

fix(type): Only allow specified boolean when given #61

Closed nickvergessen closed 10 months ago

nickvergessen commented 10 months ago

Fixes

    /**
     * @param 0|1|'yes'|'no'|true $weird Weird list
     */

Before

                        "schema": {
                            "oneOf": [
                                {
                                    "type": "boolean"
                                },
                                …
                            ]
                        }

After

                        "schema": {
                            "oneOf": [
                                …
                                {
                                    "type": "boolean",
                                    "enum": [
                                        true
                                    ]
                                }
                            ]
                        }
nickvergessen commented 10 months ago

Can do that after #59 is in, also to avoid issues with the test it brings from #45 which could be conflicting

nickvergessen commented 10 months ago

Technically we can drop the enum part because boolean is already limited to those two values.

What is the correct replacement for new OpenApiType(type: "boolean", enum: [false]) then? I couldn't figure it out...

provokateurin commented 10 months ago

What is the correct replacement for new OpenApiType(type: "boolean", enum: [false]) then? I couldn't figure it out...

It is correct, I was just proposing to merge two boolean enums for a type like ...|true|false. Same should be done for cases of ...|bool|true or ...|bool|false since the bool type already captures all the possible values.

nickvergessen commented 10 months ago

true|false

That happens already.

bool|true

Those can be achieved by:

diff --git a/src/OpenApiType.php b/src/OpenApiType.php
index dd34b01..3b01d77 100644
--- a/src/OpenApiType.php
+++ b/src/OpenApiType.php
@@ -280,7 +280,7 @@ class OpenApiType {
                foreach ($types as $type) {
                        if ($type->enum !== null) {
                                if (array_key_exists($type->type, $enums)) {
-                                       $enums[$type->type] = array_merge($enums[$type->type], $type->enum);
+                                       $enums[$type->type] = array_unique(array_merge($enums[$type->type], $type->enum));
                                } else {
                                        $enums[$type->type] = $type->enum;
                                }
@@ -307,7 +307,7 @@ class OpenApiType {
                        "positive-int" => new OpenApiType(type: "integer", format: "int64", minimum: 1),
                        "negative-int" => new OpenApiType(type: "integer", format: "int64", maximum: -1),
                        "non-positive-int" => new OpenApiType(type: "integer", format: "int64", maximum: 0),
-                       "bool", "boolean" => new OpenApiType(type: "boolean"),
+                       "bool", "boolean" => new OpenApiType(type: "boolean", enum: [true, false]),
                        "true" => new OpenApiType(type: "boolean", enum: [true]),
                        "false" => new OpenApiType(type: "boolean", enum: [false]),
                        "double" => new OpenApiType(type: "number", format: "double"),

but it makes the enum listed on all items:

diff --git a/tests/openapi.json b/tests/openapi.json
index f8a06f6..e028c52 100644
--- a/tests/openapi.json
+++ b/tests/openapi.json
@@ -45,7 +45,11 @@
                         "type": "string"
                     },
                     "primary": {
-                        "type": "boolean"
+                        "type": "boolean",
+                        "enum": [
+                            true,
+                            false
+                        ]
                     }
                 }
             },
@@ -120,7 +124,11 @@
                         "type": "string"
                     },
                     "shouldNotify": {
-                        "type": "boolean"
+                        "type": "boolean",
+                        "enum": [
+                            true,
+                            false
+                        ]
                     }
                 }
             },
@@ -143,7 +151,11 @@
                         "type": "string"
                     },
                     "primary": {
-                        "type": "boolean"
+                        "type": "boolean",
+                        "enum": [
+                            true,
+                            false
+                        ]
                     }
                 }
             },

If we are okay with that I can commit that. Otherwise it needs to wait a longer time until we find a better way to handle the enum situation.

provokateurin commented 10 months ago

If we are okay with that I can commit that

No, I think it's a bad idea.

Let's merge it as is and open a follow up issue to improve it later.