Openapi-generator: [BUG] [Ruby] client escapes all path parameters

[dkliban]

Bug Report Checklist

• [x] Have you provided a full/minimal spec to reproduce the issue?
• [ ] Have you validated the input using an OpenAPI validator (example)?
• [x] What's the version of OpenAPI Generator used?
• [x] Have you search for related issues/PRs?
• [ ] What's the actual output vs expected output?
• [ ] [Optional] Bounty to sponsor the fix (example)

Description

Our application's OpenAPI schema contains path parameters that contains '/'. The latest generated Ruby client stopped working for all the operations that contain a path parameter with '/' in it. This is a regression for our use case of openapi-generator.

openapi-generator version

I am using the latest openapi-generator-cli docker image. This is a regression since v4.0.0 tag.

OpenAPI declaration file content or url

https://docs.pulpproject.org/en/3.0/nightly/api.json

Command line used for generation

docker run -u $(id -u) --rm -v ${PWD}:/local openapitools/openapi-generator-cli generate \
    -i /local/api.json \
    -g ruby \
    -o /local/$1-client \
    -DgemName=$1_client \
    -DgemLicense="GPLv2" \
    -DgemVersion=0.1.0rc2 \
    --skip-validate-spec \
    --strict-spec=false

Steps to reproduce

Related issues/PRs

https://github.com/OpenAPITools/openapi-generator/pull/3039

Suggest a fix

@ccouzens, could you make the path parameter escaping optional?

[auto-labeler[bot]]

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

[ccouzens]

Hi @dkliban

I'm sorry to hear I broke your use case.

Just to confirm, in your use case slashes in path item values shouldn't be escaped?

There are suggestions that this should become an officially supported use case in the future.

@ccouzens, could you make the path parameter escaping optional?

I'll see what I can do.

[ccouzens]

I suspect monkey patching build_request_url would undo the escaping of slashes:

module Petstore
  class ApiClient
    def build_request_url(path)
      # Add leading and trailing slashes to path
      path = "/#{path}".gsub(/\/+/, '/')
      # restore slashes
      path = path.gsub('%2F', '/')
      @config.base_url + path
    end
  end
end

[ccouzens]

Hi @dkliban,

Can you let me know if the monkey patch is an appropriate work around for your use case?

module Petstore will need to replaced with the name of your generated client.
Add the code after the require statement that loads in your generated client.

It works by adding in this line:

      # restore slashes
      path = path.gsub('%2F', '/')

Kind regards,

Chris

[dkliban]

@ccouzens, Thanks for the suggestion. This solution would work, but it would always be in danger of breaking with each update of the Ruby client templates. What about a new config boolean for the Ruby client generator that would enable/disable path parameter encoding? "encodePathParameters" could default to False and users would set it to True when they want to enable it. The default behavior would be consistent with other language client generators. What do you think?

[ccouzens]

Hi @dkliban

"encodePathParameters" could default to False and users would set it to True when they want to enable it. The default behavior would be consistent with other language client generators. What do you think?

I can't speak for every language client generators, but OpenAPI clients are meant to encode slashes in path parameters. See this discussion for example: https://github.com/OAI/OpenAPI-Specification/issues/892

As such, I think the default should be to percent encode special characters special characters in path parameters.

If there was such a flag, ideally it would sit within the OpenAPI declaration file. I'd be surprised if this happened in the next few months. (see the linked discussion)

What's the next best alternative?

Requiring the flag at each method call

get_pet_by_id(5, encode_path_parameters: false)

Advantages: flexibility
Disadvantages: I suspect you and any other users who need this field need it for all their endpoints. Flexibility isn't very helpful if you need to specify it everywhere.

A setting on the Configuration object

config = Petstore::Configuration.new do |config|
  config.encode_path_parameters = false
end
api = Petstore::PetApi.new(Petstore::ApiClient.new(config))

Generation time setting

openapi-generator -i petstore.yml -g ruby --additional-properties encodePathParameters=false

I think this has the benefit of the person generating and distributing the client getting to decide the behaviour.

Again, I'll see what I can do,

[dkliban]

The Generation Time Setting seems like the most ideal solution to me, @ccouzens.

[ccouzens]

Hi @dkliban ,

Before I raise a pull request, can you confirm this would fix your problem?

https://github.com/OpenAPITools/openapi-generator/compare/master...ccouzens:ruby_compatibility_with_old_path_parameter_behavior?expand=1

Usage:

--additional-properties compatibilitySlashesInPathParameters

[dkliban]

@ccouzens , yes, that string substitution would fix the problem we are experiencing. However, I didn't compile and run your changes.

[ccouzens]

@dkliban - I've opened a pull request to enable compatibility with the old behaviour. #3130

[autopp]

@dkliban Thank you for report this issue and sorry for the regression.

Just to confirm, in your use case slashes in path item values shouldn't be escaped?

There are suggestions that this should become an officially supported use case in the future.

As @ccouzens said, I would like to hear more about your use case.
At the moment I think as follows.

• Slashes in path parameters should be URL encoded.
• If the option is really needed, the generator should do URL encoding by default.

@OpenAPITools/generator-core-team
When I checked about other client generations (python,go, javascript), they seemed that they did not encode the path parameter.
I think it is better if the behavior regarding path parameters and URL encode is consistent among clients. I would like to hear the opinion of the core team.

[dkliban]

@autopp My application uses the href property of a resource as the main identifier. So when using the generated client the developer writes code that looks like this:

artifacts_api.artifacts_read('/pulp/api/v3/artifacts/22991d3e-3c77-4936-88eb-84ac58fae46d/')

The href being passed in is the whole path for that resource. So the OpenAPI schema[0] for that operation has a path that consists of a single path parameter called artifact_href.

[0] https://docs.pulpproject.org/en/3.0/nightly/api.json

[jimschubert]

@ccouzens The use case in the linked OAI Specification and that of @dkliban are different, I believe. It's not uncommon for SDKs to provide a glob parameter as the last path param to "simplify" documentation or to support user defined "paths". For example, suppose your system defines different resources for dynamically created accounts... example:

/account/1/fruit/7/farmer/address
/account/1/drink/10/batch

Where this could be defined in OpenAPI as /account/{id}/{resource}. Here, resource can be user defined. APIs driven by user metadata aren't too uncommon. It seems the pulp client works similarly.

Reading over that OAI Specification thread, the focus seems to be on explicit documentation when the problem is really that it limits support for dynamic calls.

Also, sorry for the accidental close of this issue.

[ccouzens]

Thanks @jimschubert .

If the specification is used to generate a server, then I can see that the subtleties of which path parameters can be expanded is very important. But it's currently hypothetical because as I understand it there currently is no way to specify in the spec not to encode / ? or #.

I have a similar question to @autopp's question.

Am I correct to be correcting client generators to encode path parameters? (My ambitions don't currently stretch beyond the Ruby generator)
Am I correct to be offering a way to enable the previous behaviour to people who rely on path parameters not being encoded?
Is --additional-properties compatibilitySlashesInPathParameters a good way to enable the previous behaviour for people who need it?
Are there any other generators which have made this transition? How did they handle it?

[ackintosh]

How about implementing the support for a custom property of Parameter Object to allow slashes in particular path parameters? (e.g. x-allow-dynamic-resource below)

paths:
  /account/{id}/{resource}:
    get:
      operationId: 
      parameters:
        - name: id
          in: path
          schema:
            type: integer
        - name: resource
          in: path
+         # allows slashes to be used in the path parameter
+         x-allow-dynamic-resource: true 
          schema:
            type: string

• OpenAPI Specification assumes that path parameters must not contain slashes
  
  
  
  • the parameters should be encoded
  
  
• to support a dynamic call, I think it is better that users can specify if allow slashes per path parameters
  
  
  
  • FYI: Proposal: Multisegment Path Parameters · Issue #1459 · OAI/OpenAPI-Specification
  
  

[autopp]

@dkliban Thank you, I understand your use case.

• to support a dynamic call, I think it is better that users can specify if allow slashes per path parameters

I thought @ackintosh 's proposal would be better. In addition, according to OAI Specification, I think that it should be disabled by default.

[ccouzens]

I think the approach suggested is a good one.

I don't yet know my way around this codebase. If I'm implementing x-allow-dynamic-resource will someone be prepared to give me advice and answer questions?

This is where I got to:

• document the new property in docs/templating.md 👍
• search around for other examples.
• x-example was good because it applies to Parameter Objects. But it applies to all generators. Is it appropriate to copy this into src/main/java/org/openapitools/codegen/CodegenParameter.java if I'm only planning to use it in the Ruby-client generator? And set it's value in src/main/java/org/openapitools/codegen/DefaultCodegen.java public CodegenParameter fromParameter(Parameter parameter, Set<String> imports) like setParameterExampleValue is doing? And then am I automatically able to refer to it within the mustache templates?

The only test I saw for x-example is for the Ruby client generator modules/openapi-generator/src/test/java/org/openapitools/codegen/ruby/RubyClientCodegenTest.java.
What sort of tests would be appropriate for x-allow-dynamic-resource? I'm thinking of not doing a junit test. Instead I would use it on a parameter inside petstore.yaml and commit the generated ruby code that reflects the change. I think the CI on github regenerates the generated projects and thereforewould test that x-allow-dynamic-resource is working.

Does that sound like the right approach for adding this in and have I understood the Java code correctly?

[jimschubert]

I think the x-allow-dynamic-resource option is a good idea. However, this assumes that producers of a document add the property. What do we do when the owner of a spec document doesn't want to add tooling specific properties to the doc?

@ccouzens I think your approach is solid... default to what the spec or protocol defines as appropriate, but allow an option for previous behavior. In 4.0, we've added a strict spec option globally, which you may be able to reuse. As for how other generators handle it, I think it's generally expected that inputs for paths are "valid". For instance, Java clients may construct a URI from these inputs internally and anything which isn't a valid URI will result in an exception (putting the problem on the framework code rather than our generated code). Others may use frameworks which encode internally; I believe C#'s RestSharp does this.

[autopp]

However, this assumes that producers of a document add the property. What do we do when the owner of a spec document doesn't want to add tooling specific properties to the doc?

Hmm, certainly that may be a problem.

In 4.0, we've added a strict spec option globally, which you may be able to reuse.

I found it by checking #2783. It seems to be good also 👍
Instead of adding options specific to Ruby generator like #3130, I think that it is enough to change the behavior by --strict-spec.

[ccouzens]

I've started working on a branch for x-allow-dynamic-resource. Now that I've started it, I'd like to finish it (because I'm learning a fair bit by doing the work).

But after I've done that, I'm happy to do a PR for --strict-spec.

[ccouzens]

I've created a pull request for x-allow-dynamic-resource.

I've not notified any reviewers, because it sounds like there is more enthusiasm for --strict-spec.

https://github.com/OpenAPITools/openapi-generator/pull/3201

[ccouzens]

Here's the --strict-spec false pull request:

https://github.com/OpenAPITools/openapi-generator/pull/3204

[autopp]

@dkliban Since #3204 is merged, your problem seems to be solved. Could you check it?

@ccouzens (CC: @jimschubert @ackintosh) Thank you very much for your PRs. What should #3201 do?
In my opinion, I think that it may be closed at this time.

[ccouzens]

Thank you @autopp :)
I'll close #3201 and #3130. They were both alternative approaches. We went with #3204 so they're not needed.