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

[php] Fix JSON object deserialization #8481

Merged
merged 4 commits into from
Mar 28, 2022

Conversation

ybelenko
Copy link
Contributor

@ybelenko ybelenko commented Jan 20, 2021

When I call API via generated PHP client I got encoded json string instead of decoded object. Response looks like:

["{\"foobar\":\"foobaz\"}"]

UPD: I've checked response HTTP headers and Content-Type: application/json has been set correctly.

Closes #8394

Circle-CI fail log:

09:30:50.626 [qtp2074407503-11] INFO  i.s.sample.resource.PetStoreResource - placeOrder ID 0 STATUS placed
09:30:50.628 [qtp2074407503-14] INFO  i.s.sample.resource.UserResource - createUser ID 1000 STATUS gopher
09:30:50.630 [qtp2074407503-14] INFO  i.s.sample.resource.UserResource - createUsersWithArrayInput
09:30:50.636 [qtp2074407503-14] INFO  i.s.sample.resource.UserResource - updateUser ID 1000 STATUS gopher
09:31:13.284 [qtp2074407503-22] INFO  i.s.sample.resource.PetResource - addPet ID 1611135073268 STATUS available

Too long with no output (exceeded 10m0s): context deadline exceeded

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master, 5.1.x, 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@jebentier, @dkarlovi, @mandrean, @jfastnacht, @ackintosh, @renepardon

@thomasphansen
Copy link
Contributor

Hello @ybelenko,
We maintain a fork of openapi-generator for our internal use, and crossed with this problem before (I should have presented a push request, my bad - too much work in my side... :) )
This is the solution we found: instead of fixing it in the ObjectSerializer::deserialize() method, we adjust it in the api.mustache:

around line 222, responses section:

            {{#dataType}}
                {{^isWildcard}}case {{code}}:{{/isWildcard}}{{#isWildcard}}default:{{/isWildcard}}
                    if ('{{{dataType}}}' === '\SplFileObject') {
                        $content = $responseBody; //stream goes to serializer
                    } else {
                        $content = (string) $responseBody;
                        if ('{{{dataType}}}' !== 'string') {     // HERE
                            $content = json_decode($content);    // HERE
                        }                                        // HERE
                    }

and then we do the same thing around line 241, and something similar in the Async method as well.

This works because json_decode() will convert '1.2' to (float)1.2, 'true' to (bool)true and, of course, a json string to an object/array. This way, the serializer will never receive encoded data, and everything works as it should! :)

I'd recommend this change over the suggested one: the deserialize() method is recursive, and this error can only happen when it is called from the API. Adding the fix to it will make it do unnecessary checks for every recursion, while we only need to guarantee that the first call receives valid data...

Cheers! :)

@ybelenko ybelenko force-pushed the 8394_json_decode_missing branch from a3a79cc to 5ee86eb Compare April 22, 2021 11:09
Copy link
Contributor

@thomasphansen thomasphansen left a comment

Choose a reason for hiding this comment

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

Thanks for the new push! :)
But you need to replicate this change also on lines 244 and 259 :)
Cheers! :)

@ybelenko
Copy link
Contributor Author

Thanks for the new push! :)
But you need to replicate this change also on lines 244 and 259 :)
Cheers! :)

Sorry, seems like I'm not following anymore(line 259). Please, push commit yourself or give me exact diff.

@thomasphansen
Copy link
Contributor

My bad, @ybelenko, I gave you the wrong line ref! :)
Here is the diff:

diff --git a/modules/openapi-generator/src/main/resources/php/api.mustache b/modules/openapi-generator/src/main/resources/php/api.mustache
index ffa136a44a..aa1db956fc 100644
--- a/modules/openapi-generator/src/main/resources/php/api.mustache
+++ b/modules/openapi-generator/src/main/resources/php/api.mustache
@@ -240,6 +240,9 @@ use {{invokerPackage}}\ObjectSerializer;
                 $content = $response->getBody(); //stream goes to serializer
             } else {
                 $content = (string) $response->getBody();
+                if ($returnType !== 'string') {
+                    $content = json_decode($content);
+                }
             }
 
             return [
@@ -358,6 +361,10 @@ use {{invokerPackage}}\ObjectSerializer;
                         $content = $response->getBody(); //stream goes to serializer
                     } else {
                         $content = (string) $response->getBody();
+                        if ($returnType !== 'string') {
+                            $content = json_decode($content);
+                        }
+
                     }
 
                     return [

Cheers!

@ybelenko
Copy link
Contributor Author

@thomasphansen thanks for approve, but I guess we need to wait @wing328 since he showed interest in this bugfix.

@ybelenko
Copy link
Contributor Author

ybelenko commented Oct 4, 2021

@dkarlovi didn't you face that issue with PHP client? Maybe you can take a brief look if you have time.
PR is tiny, but we can't approve it for so long.

@ybelenko ybelenko mentioned this pull request Oct 15, 2021
@wing328 wing328 added this to the 6.0.0 milestone Mar 28, 2022
@wing328 wing328 merged commit 956ad2a into OpenAPITools:master Mar 28, 2022
@ybelenko ybelenko deleted the 8394_json_decode_missing branch March 29, 2022 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG][php] json_decode missing when response is object
3 participants