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.

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(type): Only allow specified boolean when given #61

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

nickvergessen
Copy link
Member

@nickvergessen nickvergessen commented Dec 21, 2023

Fixes

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

Before

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

After

                        "schema": {
                            "oneOf": [
                                
                                {
                                    "type": "boolean",
                                    "enum": [
                                        true
                                    ]
                                }
                            ]
                        }

@nickvergessen nickvergessen added the bug Something isn't working label Dec 21, 2023
@nickvergessen nickvergessen self-assigned this Dec 21, 2023
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test case for ...|true|false would also be good to have. Technically we can drop the enum part because boolean is already limited to those two values.

@nickvergessen
Copy link
Member Author

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

@delete-merged-branch delete-merged-branch bot deleted the branch main January 8, 2024 09:59
@nickvergessen nickvergessen force-pushed the bugfix/noid/limit-boolean-when-specified branch from 0af59d7 to defa2d1 Compare January 8, 2024 10:00
@nickvergessen nickvergessen changed the base branch from feat/noid/testing to main January 8, 2024 10:00
@nickvergessen nickvergessen force-pushed the bugfix/noid/limit-boolean-when-specified branch from defa2d1 to 2d51049 Compare January 8, 2024 10:09
@nickvergessen
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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.

@nickvergessen nickvergessen force-pushed the bugfix/noid/limit-boolean-when-specified branch from 2d51049 to b86ce4d Compare January 8, 2024 13:17
@nickvergessen nickvergessen merged commit d60f991 into main Jan 8, 2024
8 checks passed
@delete-merged-branch delete-merged-branch bot deleted the bugfix/noid/limit-boolean-when-specified branch January 8, 2024 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants