Skip to content

[PHP] Fix deserialize ApiException as a Model #7968

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[PHP] Fix deserialize ApiException as a Model #7968

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
afb528a
[PHP] Fix deserialize ApiException as a Model
Apr 4, 2018
b131bfa
[PHP] Update sample files
Apr 4, 2018
c8e8260
[PHP] Fix deserialize for default responses. Update samples
Apr 9, 2018
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
6 changes: 5 additions & 1 deletion modules/swagger-codegen/src/main/resources/php/api.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,12 @@ use {{invokerPackage}}\ObjectSerializer;
{{#responses}}
{{#dataType}}
{{^isWildcard}}case {{code}}:{{/isWildcard}}{{#isWildcard}}default:{{/isWildcard}}
$content = $e->getResponseBody();
if ('{{dataType}}' !== 'string') {
Copy link
Contributor

Choose a reason for hiding this comment

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

@ymilin what about other primitive types such as integer, boolean, etc? Do we need to check those as well?

Copy link
Author

@ymilin ymilin May 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@wing328 Thanks for the review 😄

I went ahead and took a better look at the ObjectSerializer::deserialize function. This PR seems to effectively break other primitive type support (especially with booleans as they get decoded as (int) 1 and null for true and false respectively).

Now it feels to me that the json_decode part should be handled in deserialize as the function has knowledge of both the data and its type.

The same approach is already used with "success" responses and is quite concerning for the same reasons:

https://github.com/swagger-api/swagger-codegen/blob/432b3589861d4a35e2d4a7a1f6a55b889645886f/modules/swagger-codegen/src/main/resources/php/api.mustache#L159-167

I'll spend some time and test with all the types from the OpenAPI spec and validate all this.

Copy link

@vladimir-shebastyuk vladimir-shebastyuk Aug 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@wing328 I was trying to resolve the same problem and found out that the approach that was used here is correct, because php json_dencode function properly handle raw types, e.g.:

echo "json_decode('123')\n";
var_dump(json_decode('123'));

echo "json_decode('123.45')\n";
var_dump(json_decode('123.45'));

echo "json_decode('true')\n";
var_dump(json_decode('true'));

echo "json_decode('false')\n";
var_dump(json_decode('false'));

will return:

json_decode('123')
int(123)
json_decode('123.45')
double(123.45)
json_decode('true')
bool(true)
json_decode('false')
bool(false)

So each primitive type handled by json_decode properly (I think the same idea was behind handling success response with the same check only for string primitive type). I also generate client and test it with real API and got correct answers.

I believe it's safe to apply theses changes:

$content = $e->getResponseBody();
if ('{{dataType}}' !== 'string') {
	$content = json_decode($content);
}
$data = ObjectSerializer::deserialize(
	$content,
	'{{dataType}}',
	$e->getResponseHeaders()
);

Is it possible to accept this PR or it is necessary to do some extra actions? E.g. update code or create a new PR?

Copy link
Author

Choose a reason for hiding this comment

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

Hi, this PR was ported and merged to the OpenAPITools/openapi-generator fork of this project.

Link to the PR over there : OpenAPITools/openapi-generator#757

$content = json_decode($content);
}
$data = ObjectSerializer::deserialize(
$e->getResponseBody(),
$content,
Copy link
Contributor

Choose a reason for hiding this comment

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

Each case section has own response type like below. ( \Swagger\Client\Model\Pet, \Swagger\Client\Model\ErrorModel in example)

        } catch (ApiException $e) {
            switch ($e->getCode()) {
                case 200:
                    $data = ObjectSerializer::deserialize(
                        $e->getResponseBody(),
                        '\Swagger\Client\Model\Pet', // <---
                        $e->getResponseHeaders()
                    );
                    $e->setResponseObject($data);
                    break;
                default:
                    $data = ObjectSerializer::deserialize(
                        $content,
                        '\Swagger\Client\Model\ErrorModel', // <---
                        $e->getResponseHeaders()
                    );
                    $e->setResponseObject($data);
                    break;
            }

So I think it is needed to put in each case section like below. 💡

        } catch (ApiException $e) {
            switch ($e->getCode()) {
                case 200:
+                  $content = $e->getResponseBody();
+                  if ('\Swagger\Client\Model\Pet' !== 'string') {
+                      $content = json_decode($content);
+                  }
                    $data = ObjectSerializer::deserialize(
                        $content,
                        '\Swagger\Client\Model\Pet', // <---
                        $e->getResponseHeaders()
                    );
                    $e->setResponseObject($data);
                    break;
                default:
+                  $content = $e->getResponseBody();
+                  if ('\Swagger\Client\Model\ErrorModel' !== 'string') {
+                      $content = json_decode($content);
+                  }
                    $data = ObjectSerializer::deserialize(
                        $content,
                        '\Swagger\Client\Model\ErrorModel', // <---
                        $e->getResponseHeaders()
                    );
                    $e->setResponseObject($data);
                    break;
            }

Copy link
Author

Choose a reason for hiding this comment

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

Oh, I see... Thank you very much for the review :-)

I'll be able to try and submit a new patch on Monday I think.

Also, my apologies I forgot to move the commits to a git branch before submitting, let me know if you want me to change that (would I need to submit a new PR?)

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to submit a new PR. 👍 Please add a new patch to this PR.

'{{dataType}}',
$e->getResponseHeaders()
);
Expand Down
1 change: 1 addition & 0 deletions samples/client/petstore/php/SwaggerClient-php/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ All URIs are relative to *http://petstore.swagger.io:80/v2*
Class | Method | HTTP request | Description
------------ | ------------- | ------------- | -------------
*AnotherFakeApi* | [**testSpecialTags**](docs/Api/AnotherFakeApi.md#testspecialtags) | **PATCH** /another-fake/dummy | To test special tags
*DefaultApi* | [**testBodyWithQueryParams**](docs/Api/DefaultApi.md#testbodywithqueryparams) | **PUT** /fake/body-with-query-params |
*FakeApi* | [**fakeOuterBooleanSerialize**](docs/Api/FakeApi.md#fakeouterbooleanserialize) | **POST** /fake/outer/boolean |
*FakeApi* | [**fakeOuterCompositeSerialize**](docs/Api/FakeApi.md#fakeoutercompositeserialize) | **POST** /fake/outer/composite |
*FakeApi* | [**fakeOuterNumberSerialize**](docs/Api/FakeApi.md#fakeouternumberserialize) | **POST** /fake/outer/number |
Expand Down
57 changes: 57 additions & 0 deletions samples/client/petstore/php/SwaggerClient-php/docs/Api/DefaultApi.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
# Swagger\Client\DefaultApi

All URIs are relative to *http://petstore.swagger.io:80/v2*

Method | HTTP request | Description
------------- | ------------- | -------------
[**testBodyWithQueryParams**](DefaultApi.md#testBodyWithQueryParams) | **PUT** /fake/body-with-query-params |


# **testBodyWithQueryParams**
> testBodyWithQueryParams($body, $query)



### Example
```php
<?php
require_once(__DIR__ . '/vendor/autoload.php');

$apiInstance = new Swagger\Client\Api\DefaultApi(
// If you want use custom http client, pass your client which implements `GuzzleHttp\ClientInterface`.
// This is optional, `GuzzleHttp\Client` will be used as default.
new GuzzleHttp\Client()
);
$body = new \Swagger\Client\Model\User(); // \Swagger\Client\Model\User |
$query = "query_example"; // string |

try {
$apiInstance->testBodyWithQueryParams($body, $query);
} catch (Exception $e) {
echo 'Exception when calling DefaultApi->testBodyWithQueryParams: ', $e->getMessage(), PHP_EOL;
}
?>
```

### Parameters

Name | Type | Description | Notes
------------- | ------------- | ------------- | -------------
**body** | [**\Swagger\Client\Model\User**](../Model/User.md)| |
**query** | **string**| |

### Return type

void (empty response body)

### Authorization

No authorization required

### HTTP request headers

- **Content-Type**: application/json
- **Accept**: Not defined

[[Back to top]](#) [[Back to API list]](../../README.md#documentation-for-api-endpoints) [[Back to Model list]](../../README.md#documentation-for-models) [[Back to README]](../../README.md)

6 changes: 5 additions & 1 deletion samples/client/petstore/php/SwaggerClient-php/lib/Api/AnotherFakeApi.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,12 @@ public function testSpecialTagsWithHttpInfo($body)
} catch (ApiException $e) {
switch ($e->getCode()) {
case 200:
$content = $e->getResponseBody();
if ('\Swagger\Client\Model\Client' !== 'string') {
$content = json_decode($content);
}
$data = ObjectSerializer::deserialize(
$e->getResponseBody(),
$content,
'\Swagger\Client\Model\Client',
$e->getResponseHeaders()
);
Expand Down
Loading