-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[Kotlin] fix #20231, OkHttp client can handle a field with a list of files #20232
Conversation
… are optional with multiple files.
parameterToString(obj).toRequestBody(null) | ||
) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: what about adding docstrings to these new functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I'll add them in a bit :-)
pet_id: | ||
type: string | ||
description: Pet ID | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this new test spec used in the PR?
did you intend to create a config file under ./bin/config to test with this spec?
since the change only affects ApiClient.kt so I don't think a new sample is needed to cover this case
we can use the echo server to test it to confirm the request looks good
please ping me via Slack if you want to go with this approach
https://join.slack.com/t/openapi-generator/shared_invite/zt-2uoef5v0g-XGwo8~2oJ3EoziDSO1CmdQ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure! I will ping you. As I mentioned, I didn't really get a clear understanding on how to add the tests to check this fix (I tested it against my own project), so some guidance on how to do it would be extremely appreciated!
I added that test spec just by looking at the project, but I didn't know what to do with it.
I'll poke you in Slack some time during the day!
issue-20231: Kotlin OkHttp client does not process correctly when a field in the endpoint is defined as an array of lists like this:
If the autogenerated client is used, the server will receive always 1 item in the list but it is not a file or anything and it is trying to serialize like a normal object.
I noticed this issue while developing my own project, so I modified this code (by looking at the Java counterpart that can be seen here) and tested it by building the project in my local machine and adding the plugin to my project manually.
I tried to add some testing (I added the faulty specification) but I was not really sure how to do it. Also by reading the documentation, it didn't find it very clearly what I should do. Any guidance regarding how to add test for that would be very much appreciated.
PR checklist
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*
.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master
(upcoming7.x.0
minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)Committee: @dr4ke616 @karismann @Zomzog @andrewemery @4brunu @yutaka0m @stefankoppier @e5l