Skip to content

Conversation

@tomekc
Copy link
Contributor

@tomekc tomekc commented Jan 4, 2017

PR checklist

  • Read the contribution guildelines.
  • Ran the shell/batch 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)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

Made POST requests allow use query parameters, besides of body. Currently generated code ignores all query parameters when body exists.

Reported in #2557 and also in #2483

Note
This is a workaround of flawed Alamofire API which is actual limitation. Next time I will post a Alamofire-less template :)

@tomekc
Copy link
Contributor Author

tomekc commented Jan 4, 2017

Interesting, check failed for seemingly unrelated reason:

[INFO]                                                                         
[INFO] ------------------------------------------------------------------------
[INFO] Building TS Fetch Default Petstore Client 1.0-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[INFO] 
[INFO] --- maven-dependency-plugin:2.8:copy-dependencies (default) @ TypeScriptAngularBuildPestoreClientTests ---
[INFO] 
[INFO] --- exec-maven-plugin:1.2.1:exec (npm-install) @ TypeScriptAngularBuildPestoreClientTests ---
npm WARN package.json typescript-fetch-api@0.0.0 No repository field.
npm ERR! Linux 4.4.0-51-generic
npm ERR! argv "/home/travis/.nvm/versions/node/v4.1.2/bin/node" "/home/travis/.nvm/versions/node/v4.1.2/bin/npm" "install"
npm ERR! node v4.1.2
npm ERR! npm  v2.14.4
npm ERR! code ECONNRESET
npm ERR! errno ECONNRESET
npm ERR! syscall read
npm ERR! network read ECONNRESET
npm ERR! network This is most likely not a problem with npm itself
npm ERR! network and is related to network connectivity.
npm ERR! network In most cases you are behind a proxy or have bad network settings.
npm ERR! network 
npm ERR! network If you are behind a proxy, please make sure that the
npm ERR! network 'proxy' config is set properly.  See: 'npm help config'

@wing328 wing328 added this to the v2.2.2 milestone Jan 6, 2017
@wing328
Copy link
Contributor

wing328 commented Jan 6, 2017

@tomekc thanks for the PR. The test result looks good:

** TEST SUCCEEDED **

Test Suite 'All tests' started at 2017-01-06 21:42:28.060
Test Suite 'SwaggerClientTests.xctest' started at 2017-01-06 21:42:28.061
Test Suite 'PetAPITests' started at 2017-01-06 21:42:28.062
Test Case '-[SwaggerClientTests.PetAPITests test1CreatePet]' started.
Test Case '-[SwaggerClientTests.PetAPITests test1CreatePet]' passed (0.882 seconds).
Test Case '-[SwaggerClientTests.PetAPITests test2GetPet]' started.
Test Case '-[SwaggerClientTests.PetAPITests test2GetPet]' passed (0.497 seconds).
Test Case '-[SwaggerClientTests.PetAPITests test3DeletePet]' started.
Test Case '-[SwaggerClientTests.PetAPITests test3DeletePet]' passed (0.459 seconds).
Test Suite 'PetAPITests' passed at 2017-01-06 21:42:29.902.
	 Executed 3 tests, with 0 failures (0 unexpected) in 1.837 (1.841) seconds
Test Suite 'StoreAPITests' started at 2017-01-06 21:42:29.903
Test Case '-[SwaggerClientTests.StoreAPITests test1PlaceOrder]' started.
Test Case '-[SwaggerClientTests.StoreAPITests test1PlaceOrder]' passed (0.471 seconds).
Test Case '-[SwaggerClientTests.StoreAPITests test2GetOrder]' started.
Test Case '-[SwaggerClientTests.StoreAPITests test2GetOrder]' passed (0.465 seconds).
Test Case '-[SwaggerClientTests.StoreAPITests test3DeleteOrder]' started.
Test Case '-[SwaggerClientTests.StoreAPITests test3DeleteOrder]' passed (0.465 seconds).
Test Case '-[SwaggerClientTests.StoreAPITests testDownloadProgress]' started.
Test Case '-[SwaggerClientTests.StoreAPITests testDownloadProgress]' passed (0.469 seconds).
Test Suite 'StoreAPITests' passed at 2017-01-06 21:42:31.778.
	 Executed 4 tests, with 0 failures (0 unexpected) in 1.870 (1.875) seconds
Test Suite 'UserAPITests' started at 2017-01-06 21:42:31.780
Test Case '-[SwaggerClientTests.UserAPITests test1CreateUser]' started.
Test Case '-[SwaggerClientTests.UserAPITests test1CreateUser]' passed (0.476 seconds).
Test Case '-[SwaggerClientTests.UserAPITests test2GetUser]' started.
Test Case '-[SwaggerClientTests.UserAPITests test2GetUser]' passed (0.465 seconds).
Test Case '-[SwaggerClientTests.UserAPITests test3DeleteUser]' started.
Test Case '-[SwaggerClientTests.UserAPITests test3DeleteUser]' passed (0.506 seconds).
Test Case '-[SwaggerClientTests.UserAPITests testLogin]' started.
Test Case '-[SwaggerClientTests.UserAPITests testLogin]' passed (0.489 seconds).
Test Case '-[SwaggerClientTests.UserAPITests testLogout]' started.
Test Case '-[SwaggerClientTests.UserAPITests testLogout]' passed (0.457 seconds).
Test Suite 'UserAPITests' passed at 2017-01-06 21:42:34.179.
	 Executed 5 tests, with 0 failures (0 unexpected) in 2.393 (2.399) seconds
Test Suite 'SwaggerClientTests.xctest' passed at 2017-01-06 21:42:34.180.
	 Executed 12 tests, with 0 failures (0 unexpected) in 6.100 (6.119) seconds
Test Suite 'All tests' passed at 2017-01-06 21:42:34.182.
	 Executed 12 tests, with 0 failures (0 unexpected) in 6.100 (6.122) seconds
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:32 min
[INFO] Finished at: 2017-01-06T21:42:34+08:00
[INFO] Final Memory: 11M/245M
[INFO] ------------------------------------------------------------------------

cc @jaz-ah @Edubits @hexelon

@jaz-ah
Copy link
Contributor

jaz-ah commented Jan 6, 2017

+1 @wing328

@hexelon
Copy link
Contributor

hexelon commented Jan 7, 2017

+1 @wing328

One thing I'd add is some scenario where a call "mapValuesToQueryItems" is generated. There's no such scenario in the current petstore.

@wing328
Copy link
Contributor

wing328 commented Jan 7, 2017

@hexelon good idea. We might want to leverage the fake Petstore for the additional test case

@wing328 wing328 merged commit 1e8c718 into swagger-api:master Jan 7, 2017
@tomekc
Copy link
Contributor Author

tomekc commented Jan 7, 2017

@wing328 @hexelon Speaking about tests, I have more improvements underway and found http://httpbin.org useful for testing all possible request types.

@wing328 wing328 changed the title [swift3] allow POST with both body and query parameters [Swift3] allow POST with both body and query parameters Feb 20, 2017
davidgri pushed a commit to davidgri/swagger-codegen that referenced this pull request May 11, 2017
…4490)

* [swift3] allow POST with both body and query parameters

* Correctly support non-string and optional query parameters.
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.

4 participants