Fix error when giving an array as parameter to an endpoint body request #1037

thomasphansen · 2018-09-14T09:19:55Z

…equest

PR checklist

Read the contribution guidelines.
Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
Filed the PR against the correct branch: master, 3.3.x, 4.0.x. Default: master.
Copied the technical committee to review the pull request if your PR is targeting a particular programming language.
@jebentier
@dkarlovi
@mandrean
@jfastnacht
@ackintosh
@ybelenko

Description of the PR

This PR targets Issue #1036 : as of today, it is not possible to provide an array of model objects as parameter for an operation, since it will not be properly json-encoded. This PR fixes this, by adding a recursive method that is able to properly json-encode arrays of model objects.

wing328 · 2018-09-14T13:24:27Z

modules/openapi-generator/src/main/resources/php/api.mustache

+            if($headers['Content-Type'] !== 'application/json') {
+                $httpBody = $_tempBody;
+            } else {
+                $httpBody = $this->formatBody($_tempBody);


@thomasphansen thanks for the PR. What about using ObjectSerializer::sanitizeForSerialization instead?

Yep, makes sense! :D

thomasphansen · 2018-09-17T06:35:24Z

(BTW, I just found this very nice article on how to contribute to openapi-generator: https://medium.com/ringcentral-developers/openapi-generator-for-go-contribution-quickstart-8cc72bf37b53
Maybe it could be a nice thing to have link for it in CONTRIBUTING.md? :) )

thomasphansen · 2018-09-17T11:01:01Z

So: Travis is failing because the ObjectSerializer::sanitizeForSerialization() method deals poorly with \stdClass objects. I can see some possible solutions here:

Adjust ObjectSerializer::sanitizeForSerialization() to deal with \stdClass. This could be done by either:
a. ignoring \stdClass objects (and just return them untouched); or
b. (probably the right thing to do):

   (...)
   } elseif (is_object($data)) {
       if ($data instanceOf ModelInterface) {
           // Current code for dealing with objects
       } else {
           //  for each property inside the object, run sanitizeForSerialization() on its value (just like it's currently done for all objects, but without using the OpenAPI-specific methods)
       }
   }

test for \stdClass objects before calling sanitizeForSerialization()

What do you think @wing328 ? :)

thomasphansen · 2018-09-17T16:31:15Z

Pushed a commit implementing 1.b option... :)

…equest

…objects properly

thomasphansen · 2018-09-18T06:50:13Z

Well, I'm not being able to get travis-ci finishing the tests: I'm getting the message "
The job exceeded the maximum time limit for jobs, and has been terminated.". I don't think there is anything I can do to fix it. I've tested this solution, and manually run all the phpunit tests under samples/client/petstore/php/OpenAPIClient-php/tests. What else can I do? :)

wing328 · 2018-09-18T13:02:24Z

@thomasphansen I've restarted the job and all tests pass.

cc @jebentier @dkarlovi @mandrean @jfastnacht @ackintosh @ybelenko

ackintosh

Thanks for this PR!

ackintosh · 2018-09-18T15:42:43Z

modules/openapi-generator/src/main/resources/php/ObjectSerializer.mustache

-                    && !in_array($value, $openAPIType::getAllowableEnumValues(), true)) {
-                    $imploded = implode("', '", $openAPIType::getAllowableEnumValues());
-                    throw new \InvalidArgumentException("Invalid value for enum '$openAPIType', must be one of: '$imploded'");
+            if($data instanceof ModelInterface) {


Nit: please put a space after if.

ackintosh · 2018-09-18T15:43:26Z

modules/openapi-generator/src/main/resources/php/api.mustache

-            // \stdClass has no __toString(), so we should encode it manually
-            if ($httpBody instanceof \stdClass && $headers['Content-Type'] === 'application/json') {
-                $httpBody = \GuzzleHttp\json_encode($httpBody);
+            if($headers['Content-Type'] !== 'application/json') {


Nit: please put a space after if.

How about invert the if condition like below:

if ($headers['Content-Type'] !== 'application/json') { $httpBody = $_tempBody; } else { $httpBody = \GuzzleHttp\json_encode(ObjectSerializer::sanitizeForSerialization($_tempBody)); }

↓↓↓

if ($headers['Content-Type'] === 'application/json') { $httpBody = \GuzzleHttp\json_encode(ObjectSerializer::sanitizeForSerialization($_tempBody)); } else { $httpBody = $_tempBody; }

The === in if statement may be Intuitive and readable. 💡

ackintosh · 2018-09-18T23:21:58Z

I've restarted the travis-ci build as it was resulted in the error.

[ERROR] Failed to execute goalorg.fortasoft:gradle-maven-plugin:1.0.8:invoke(default) on projectopenapi-generator-gradle-plugin-mvn-wrapper:org.gradle.tooling.GradleConnectionException: Could not execute build using Gradle distribution 'https://services.gradle.org/distributions/gradle-4.10-rc-1-bin.zip'. ->[Help 1]

ackintosh

Thanks for the update. Looks good to me. ✨

@ackintosh

OpenAPITools#1037) * PR: Fix error when giving an array as parameter to an endpoint body request * PR OpenAPITools#1037 - Fix issue with array as parameter to operation * update samples * PHP - ObjectSerializer::sanitizeForSerialization(): manage \stdClass objects properly * update samples * bug fix: missing "use" clause * update samples * Changes after @ackintosh review * update samples

wing328 · 2018-10-02T02:15:26Z

@thomasphansen thanks again for the PR, which is included in the v3.3.0 minor release: https://twitter.com/oas_generator/status/1046941449609068544

@ackintosh

OpenAPITools#1037) * PR: Fix error when giving an array as parameter to an endpoint body request * PR OpenAPITools#1037 - Fix issue with array as parameter to operation * update samples * PHP - ObjectSerializer::sanitizeForSerialization(): manage \stdClass objects properly * update samples * bug fix: missing "use" clause * update samples * Changes after @ackintosh review * update samples

thomasphansen mentioned this pull request Sep 14, 2018

[PHP] array of objects as parameter not supported #1036

Closed

wing328 added Client: PHP Issue: Bug labels Sep 14, 2018

wing328 added this to the 3.3.0 milestone Sep 14, 2018

wing328 reviewed Sep 14, 2018

View reviewed changes

thomasphansen added 7 commits September 18, 2018 07:48

PR: Fix error when giving an array as parameter to an endpoint body r…

aec1abb

…equest

PR OpenAPITools#1037 - Fix issue with array as parameter to operation

69443ef

update samples

25c7986

PHP - ObjectSerializer::sanitizeForSerialization(): manage \stdClass …

a6eb56d

…objects properly

update samples

6308185

bug fix: missing "use" clause

1a0ecab

update samples

87ffe6f

thomasphansen force-pushed the th_fix_php_array_parameter branch from 18ad57d to 87ffe6f Compare September 18, 2018 05:53

ackintosh reviewed Sep 18, 2018

View reviewed changes

thomasphansen added 2 commits September 18, 2018 23:34

Changes after @ackintosh review

75178e2

update samples

e9db0c0

ackintosh approved these changes Sep 19, 2018

View reviewed changes

ackintosh merged commit ca9a4a2 into OpenAPITools:master Sep 19, 2018

thomasphansen deleted the th_fix_php_array_parameter branch September 19, 2018 12:27

wing328 changed the title ~~PR: Fix error when giving an array as parameter to an endpoint body r…~~ Fix error when giving an array as parameter to an endpoint body request Oct 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error when giving an array as parameter to an endpoint body request #1037

Fix error when giving an array as parameter to an endpoint body request #1037

thomasphansen commented Sep 14, 2018

wing328 Sep 14, 2018

thomasphansen Sep 17, 2018

thomasphansen commented Sep 17, 2018

thomasphansen commented Sep 17, 2018 •

edited

Loading

thomasphansen commented Sep 17, 2018

thomasphansen commented Sep 18, 2018

wing328 commented Sep 18, 2018

ackintosh left a comment

ackintosh Sep 18, 2018

ackintosh Sep 18, 2018

ackintosh Sep 18, 2018

ackintosh commented Sep 18, 2018

ackintosh left a comment

wing328 commented Oct 2, 2018

Fix error when giving an array as parameter to an endpoint body request #1037

Fix error when giving an array as parameter to an endpoint body request #1037

Conversation

thomasphansen commented Sep 14, 2018

PR checklist

Description of the PR

wing328 Sep 14, 2018

Choose a reason for hiding this comment

thomasphansen Sep 17, 2018

Choose a reason for hiding this comment

thomasphansen commented Sep 17, 2018

thomasphansen commented Sep 17, 2018 • edited Loading

thomasphansen commented Sep 17, 2018

thomasphansen commented Sep 18, 2018

wing328 commented Sep 18, 2018

ackintosh left a comment

Choose a reason for hiding this comment

ackintosh Sep 18, 2018

Choose a reason for hiding this comment

ackintosh Sep 18, 2018

Choose a reason for hiding this comment

ackintosh Sep 18, 2018

Choose a reason for hiding this comment

ackintosh commented Sep 18, 2018

ackintosh left a comment

Choose a reason for hiding this comment

wing328 commented Oct 2, 2018

thomasphansen commented Sep 17, 2018 •

edited

Loading