Skip to content
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

[Java][RestTemplate] Fixed invalid URL-encoding of query parameters #646

Merged
merged 1 commit into from
Jul 30, 2018

Conversation

rubms
Copy link
Contributor

@rubms rubms commented Jul 25, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell 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). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.2.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Fix for #644. Changed the way the URI is created from the given query parameters in ApiClient::invokeAPI in order to prevent a double escaping of % characters in the URL.

The #249 issue originally addresses the problem in the URL-encoding of query parameters in the Java RestTemplate client generation. A fix is attempted in #260, but without full success, in which this re-encoding of % characters is experienced.

#260 PR cc: @jmini @wing328 @simingweng
technical committee cc: @bbdouglas (2017/07) @JFCote (2017/08) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01)

…valid URL-encoding of query parameters in the auto-generation of the RestTemplate Java client.
@jmini
Copy link
Member

jmini commented Jul 26, 2018

Thank you for fixing this.... I really appreciate it.

To test it, I should generate a client that does a GET request with a query param and then send a value containing %?

Is this correct?

@rubms
Copy link
Contributor Author

rubms commented Jul 26, 2018

I'd say you must send some query parameter containing URL special characters that need to get URL-encoded.

I have tested it with date-time parameters including time zone: 2018-07-26T09:34:15+02:00. They are very good for this test because both the : and + are URL special characters and get URL encoded.

2018-07-26T09:34:15+02:00 should be URL-encoded as 2018-07-26T09%3A34%3A15%2B02%3A00. Before I fixed the problem it got URL-encoded as 2018-07-26T09%253A34%253A15%252B02%253A00 (you can notice that the % characters are doubly encoded).

@jmini
Copy link
Member

jmini commented Jul 30, 2018

Sorry for the late answer.

I have tested this with a client generated for this path definition (OAS3 extract):

  /ipsum/ping:
    parameters:
      - name: custom
        in: query
        schema:
          type: string
    get:
      operationId: pingGet
      responses:
        '200':
          description: OK

When I send the request like this (almost the generated unit test):

    @Test
    public void pingGetTest() {
        String custom = "2018-07-26T09:34:15+02:00";
        api.pingGet(custom);
    }

And I compare what I get in the server:

Before the fix I get:

[GET] /ipsum/ping
Parameters:
 - custom: 2018-07-26T09%3A34%3A15%2B02%3A00

After the fix I get:

[GET] /ipsum/ping
Parameters:
 - custom: 2018-07-26T09:34:15+02:00

@jmini jmini merged commit 68d80ab into OpenAPITools:master Jul 30, 2018
@jmini jmini added this to the 3.2.0 milestone Jul 30, 2018
@jmini
Copy link
Member

jmini commented Jul 30, 2018

I have merged it, thank you a lot for this PR.

@rubms rubms deleted the Fix_#644 branch July 30, 2018 08:06
@wing328 wing328 changed the title [Java][Client][RestTemplate] Fixed invalid URL-encoding of query parameters [Java][RestTemplate] Fixed invalid URL-encoding of query parameters Aug 6, 2018
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
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.

2 participants