fix(ktor-client): do not add jsonBlock if using kotlinx-serialization by bcmedeiros · Pull Request #15793 · OpenAPITools/openapi-generator

bcmedeiros · 2023-06-08T15:36:21Z

Fix for #15794

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.
In case you are adding a new generator, run the following additional script :
```
./bin/utils/ensure-up-to-date
```
Commit all changed files.
File the PR against the correct branch: master (6.3.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

bcmedeiros · 2023-06-08T16:07:37Z

FYI
@jimschubert @dr4ke616 @karismann @Zomzog @andrewemery @4brunu @yutaka0m @stefankoppier

bcmedeiros · 2023-06-08T16:32:15Z

Error seems totally unrelated and I'm unable to reproduce locally. Is this a flaky testing failing just in CI?

stefankoppier · 2023-06-08T20:12:05Z

Error seems totally unrelated and I'm unable to reproduce locally. Is this a flaky testing failing just in CI?

Hi, thanks for the PR. It seems that a sample is changed but not pushed. Did you merge master and generate all samples? If not, please try that. Edit: I'm on my phone, so I can't review property right now. But did you add the new sample to the list in https://github.com/OpenAPITools/openapi-generator/blob/master/.github/workflows/samples-kotlin-client.yaml?

…t.yaml pipeline actually, kotlin-jvm-ktor-gson was duplicated, so I'm replacing the second one by kotlin-jvm-ktor-kotlinx_serialization

bcmedeiros · 2023-06-09T00:01:22Z

@stefankoppier tahnks for pointing that out, I was indeed missing the pipeline file. I'm not sure how that was causing the test error, but it seems to be gone now.

wing328 · 2023-06-09T09:46:45Z

the CI tests failed: https://github.com/OpenAPITools/openapi-generator/actions/runs/5216700141/jobs/9422350503?pr=15793

can you please take a look when you've time?

…ktor + kotlinx_serialization

bcmedeiros · 2023-06-10T14:45:59Z

@stefankoppier @wing328 sorry for the back and forth, both errors should be fixed now, I'm getting the gist of the way the project is organized.
Just as a heads-up, I found another issue with ktor + kotlinx-serialization, in regards to not setting the content type, I'll submit another issue/PR as soon as this one gets sorted.

wing328 · 2023-06-11T09:36:17Z

+generatorName: kotlin
+outputDir: samples/client/petstore/kotlin-jvm-ktor-kotlinx_serialization
+library: jvm-ktor
+inputSpec: modules/openapi-generator/src/test/resources/2_0/petstore.yaml


please use 3.0 spec instead

modules/openapi-generator/src/test/resources/3_0/petstore.yaml

please use 3.0 spec instead

I was using 2.0 because all the other kotlin/ktor were using it, but sure, I can update this one.

Just as a heads-up, I found another issue with ktor + kotlinx-serialization, in regards to not setting the content type, I'll submit another issue/PR as soon as this one gets sorted.

Also, since we are doing changes that are not related strictly to jsonBlock anymore, should I add the other fixes I have to this same PR, or do you still prefer that I raise another issue/ticket?

I was using 2.0 because all the other kotlin/ktor were using it, but sure, I can update this one.

yes, i know what you're talking about. we know yet have time to update those to use 3.0 spec.

for new one (test), we definitely want to start with 3.0 spec

Also, since we are doing changes that are not related strictly to jsonBlock anymore, should I add the other fixes I have to this same PR, or do you still prefer that I raise another issue/ticket?

We prefer small PRs for easier review.

Ok, I'll raise another PR right after.

Also worth sharing with you that we are gradually moving away from petstore and will switch to echo server instead for client testings. https://github.com/OpenAPITools/openapi-generator/wiki/Integration-Tests#echo-server has more info.

(no need to do anything in this PR. more of an FYI)

bcmedeiros · 2023-06-11T11:19:28Z

@wing328 changes pushed. Also made a comment above, let me know how you want me to proceed.

bcmedeiros · 2023-06-11T11:57:50Z

@wing328 FYI all tests passed.

wing328 · 2023-06-11T12:45:32Z

Merged. Thanks for the contribution.

…OpenAPITools#15793) * fix(ktor-client): do not add jsonBlock if using kotlinx-serialization * update existing templates * add new kotlin-jvm-ktor-kotlinx_serialization * add new kotlin-jvm-ktor-kotlinx_serialization to samples-kotlin-client.yaml pipeline actually, kotlin-jvm-ktor-gson was duplicated, so I'm replacing the second one by kotlin-jvm-ktor-kotlinx_serialization * extra FILES entry in the template * enumUnknownDefaultCase=false for now since it's currently broken for ktor + kotlinx_serialization * use openapi 3 petstore

bcmedeiros added 3 commits June 8, 2023 23:33

fix(ktor-client): do not add jsonBlock if using kotlinx-serialization

0e1abfa

update existing templates

6bf4bf9

add new kotlin-jvm-ktor-kotlinx_serialization

71ee332

bcmedeiros requested a review from jimschubert as a code owner June 8, 2023 23:23

add new kotlin-jvm-ktor-kotlinx_serialization to samples-kotlin-clien…

51dfcb2

…t.yaml pipeline actually, kotlin-jvm-ktor-gson was duplicated, so I'm replacing the second one by kotlin-jvm-ktor-kotlinx_serialization

bcmedeiros force-pushed the fix-ktor-kotlinx-serialization branch from 4301831 to 51dfcb2 Compare June 8, 2023 23:36

wing328 added Client: Kotlin Issue: Bug labels Jun 9, 2023

bcmedeiros added 2 commits June 10, 2023 22:03

extra FILES entry in the template

ddb8916

enumUnknownDefaultCase=false for now since it's currently broken for …

8c8bc18

…ktor + kotlinx_serialization

wing328 added this to the 7.0.0 milestone Jun 11, 2023

wing328 reviewed Jun 11, 2023

View reviewed changes

use openapi 3 petstore

8ba67ba

wing328 merged commit ca80fcd into OpenAPITools:master Jun 11, 2023

bcmedeiros mentioned this pull request Jun 11, 2023

fix(kotlin): set content type on ktor-jvm library #15812

Closed

6 tasks

Uh oh!

Conversation

bcmedeiros commented Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

bcmedeiros commented Jun 8, 2023

Uh oh!

bcmedeiros commented Jun 8, 2023

Uh oh!

stefankoppier commented Jun 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcmedeiros commented Jun 9, 2023

Uh oh!

wing328 commented Jun 9, 2023

Uh oh!

bcmedeiros commented Jun 10, 2023

Uh oh!

wing328 Jun 11, 2023

Choose a reason for hiding this comment

Uh oh!

bcmedeiros Jun 11, 2023

Choose a reason for hiding this comment

Uh oh!

wing328 Jun 11, 2023

Choose a reason for hiding this comment

Uh oh!

wing328 Jun 11, 2023

Choose a reason for hiding this comment

Uh oh!

bcmedeiros Jun 11, 2023

Choose a reason for hiding this comment

Uh oh!

wing328 Jun 11, 2023

Choose a reason for hiding this comment

Uh oh!

bcmedeiros commented Jun 11, 2023

Uh oh!

bcmedeiros commented Jun 11, 2023

Uh oh!

wing328 commented Jun 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bcmedeiros commented Jun 8, 2023 •

edited

Loading

stefankoppier commented Jun 8, 2023 •

edited

Loading