Skip to content

Conversation

@sjoblomj
Copy link
Contributor

@sjoblomj sjoblomj commented Jan 4, 2022

resolve #7199

The Kotlin client codegen currently cannot handle path params of type array. The code being generated for the path is currently this: path = "/stuff/{ids}".replace("{"+"ids"+"}", "$ids"). This works fine, but not if ids is a list or an array, since the toString() method is used to create the string.

With the current code, if ids contains 123 and 456, then path would be set to:

  • /stuff/[123,456] if collectionType is list.
  • /stuff/[Ljava.lang.String;@6a98f353 if collectionType is array.

In both cases, /stuff/123,456 is the expected result.

If we change the code from
path = "/stuff/{ids}".replace("{"+"ids"+"}", "$ids")
to
path = "/stuff/{ids}".replace("{"+"ids"+"}", ids.joinToString(",")),
we get the expected result.

OpenAPI Spec demonstrating this:

openapi: 3.0.0
info:
  version: 1.0.0
  title: Demo
paths:
  '/{ids}':
    get:
      parameters:
        - name: ids
          in: path
          required: true
          schema:
            type: array
            items:
              type: string
          style: simple
      responses:
        200:
          description: Successful operation

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.3.0), 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.

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

@sjoblomj sjoblomj changed the title Make Kotlin client generate comma separated list for array path params [BUG][kotlin] Make Kotlin client generate comma separated list for array path params Jan 4, 2022
@4brunu
Copy link
Contributor

4brunu commented Jan 5, 2022

Hi, could you please run the following commands and commit the changes, please? Thanks

./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh

@sjoblomj
Copy link
Contributor Author

sjoblomj commented Jan 5, 2022

Hi, could you please run the following commands and commit the changes, please? Thanks

./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh

Done, although I see no changes except for the scripts butchering a lot of files by turning paths and file endings into Windows style, and performing some formatting.

@4brunu
Copy link
Contributor

4brunu commented Jan 5, 2022

Sorry the script did end up creating a lot of noise not related to this command.
Could you please revert the last commit and commit the changes created by this command?

./bin/generate-samples.sh bin/configs/kotlin*

@sjoblomj sjoblomj force-pushed the kotlin-client-comma-separate-arrays branch from 685af6b to d6fe994 Compare January 5, 2022 12:05
@sjoblomj
Copy link
Contributor Author

sjoblomj commented Jan 5, 2022

Sorry the script did end up creating a lot of noise not related to this command. Could you please revert the last commit and commit the changes created by this command?

./bin/generate-samples.sh bin/configs/kotlin*

Reverted and forced pushed the output from the command.

Again, there are no changes except for the script re-indenting some lines, and it looks to me like the changes are for the worse. Should I revert this change too, or do we want it? :)

@4brunu
Copy link
Contributor

4brunu commented Jan 5, 2022

Can you revert it please?
Looks like the API spec doesn't have a test case for this.
Did you tested this locally?

@sjoblomj sjoblomj force-pushed the kotlin-client-comma-separate-arrays branch from d6fe994 to c5c8f07 Compare January 5, 2022 12:14
@sjoblomj
Copy link
Contributor Author

sjoblomj commented Jan 5, 2022

Can you revert it please? Looks like the API spec doesn't have a test case for this. Did you tested this locally?

Reverted. I tested it locally.

I wanted to add a test case, but did not see where or how to test it. If you could give me some pointers to how it would be tested, I could try to add one.

@4brunu
Copy link
Contributor

4brunu commented Jan 5, 2022

The easiest way would be to create a spec file and a new sample project

@4brunu
Copy link
Contributor

4brunu commented Jan 5, 2022

Or you could create a new spec file based on another one that already exists, and update one of the kotlin sample projects to use it, this would be much easier

@sjoblomj
Copy link
Contributor Author

sjoblomj commented Jan 6, 2022

Or you could create a new spec file based on another one that already exists, and update one of the kotlin sample projects to use it, this would be much easier

Just to be clear, when you say one of the kotlin sample projects, which file(s) do you refer to?

@4brunu
Copy link
Contributor

4brunu commented Jan 7, 2022

The sample projects have a configuration file, for example this one.
https://github.com/OpenAPITools/openapi-generator/blob/master/bin/configs/kotlin-gson.yaml
In that directory you will find other yaml files for the different kotlin sample projects.
In the configuration file, you will find the inputSpec, the outputDir, the additionalProperties, etc.
You can create a new configuration file and a new input spec to create a new sample project.

@wing328
Copy link
Member

wing328 commented Jan 12, 2022

Tested with the provided spec and the result is good:

Starting a Gradle Daemon (subsequent builds will be faster)

> Task :compileKotlin
w: /private/tmp/kotlin-client/src/main/kotlin/org/openapitools/client/infrastructure/ApiClient.kt: (121, 6): This class can only be used with the compiler argument '-Xopt-in=kotlin.RequiresOptIn'

BUILD SUCCESSFUL in 42s
3 actionable tasks: 3 executed

The output looks correct:

        return RequestConfig(
            method = RequestMethod.GET,
            path = "/{ids}".replace("{"+"ids"+"}", ids.joinToString(",")),
            query = localVariableQuery,
            headers = localVariableHeaders,
            body = localVariableBody
        )

@wing328
Copy link
Member

wing328 commented Jan 12, 2022

I'll add a CI test after merging this PR

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][kotlin] Path params with type: array doesn't work

3 participants