Skip to content

[php] 2.3 - fix form params json encoding#5701

Merged
wing328 merged 4 commits intoswagger-api:2.3.0from
baartosz:php_fixFormParamsJsonEncoding
Jun 4, 2017
Merged

[php] 2.3 - fix form params json encoding#5701
wing328 merged 4 commits intoswagger-api:2.3.0from
baartosz:php_fixFormParamsJsonEncoding

Conversation

@baartosz
Copy link
Copy Markdown
Contributor

@baartosz baartosz commented May 24, 2017

PR checklist

  • Read the contribution guidelines.
  • Ran the shell/batch 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)
  • Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

Issue introduced when client was reworked to use guzzle instead of curl in #5190.

Apparently I missed this case earlier. Content of request is not json-encoded if endpoint consumes: application/json and params are in formData. This PR fixes this issue.
I added example of spec causing this issue to FakeApi definition so I could add test for it.

  /fake/jsonFormData:
    get:
      tags:
        - fake
      summary: test json serialization of form data
      description: ''
      operationId: testJsonFormData
      consumes:
        - application/json
      parameters:
        - name: param
          in: formData
          description: field1
          required: true
          type: string
        - name: param2
          in: formData
          description: field2
          required: true
          type: string
      responses:
        '200':
          description: successful operation

}
$httpBody = new MultipartStream($multipartContents); // for HTTP post (form)

} elseif ($headers['Content-Type'] === 'application/json') {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@baartosz do you mind adding a new method (e.g. isJsonMime) to determine if a MIME string is "JSON"?

For Java, we've https://github.com/swagger-api/swagger-codegen/blob/master/samples/client/petstore/java/okhttp-gson/src/main/java/io/swagger/client/ApiClient.java#L752-L754

and here are some tests:

public void testIsJsonMime() {
assertFalse(apiClient.isJsonMime(null));
assertFalse(apiClient.isJsonMime(""));
assertFalse(apiClient.isJsonMime("text/plain"));
assertFalse(apiClient.isJsonMime("application/xml"));
assertFalse(apiClient.isJsonMime("application/jsonp"));
assertFalse(apiClient.isJsonMime("example/json"));
assertFalse(apiClient.isJsonMime("example/foo+bar+jsonx"));
assertFalse(apiClient.isJsonMime("example/foo+bar+xjson"));
assertTrue(apiClient.isJsonMime("application/json"));
assertTrue(apiClient.isJsonMime("application/json; charset=UTF8"));
assertTrue(apiClient.isJsonMime("APPLICATION/JSON"));
assertTrue(apiClient.isJsonMime("application/problem+json"));
assertTrue(apiClient.isJsonMime("APPLICATION/PROBLEM+JSON"));
assertTrue(apiClient.isJsonMime("application/json\t"));
assertTrue(apiClient.isJsonMime("example/foo+bar+json"));
assertTrue(apiClient.isJsonMime("example/foo+json;x;y"));
assertTrue(apiClient.isJsonMime("example/foo+json\t;"));
assertTrue(apiClient.isJsonMime("Example/fOO+JSON"));
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll create a separate task to track this.

@wing328
Copy link
Copy Markdown
Contributor

wing328 commented May 25, 2017

@baartosz thanks for the PR. I left you a feedback in the code.

cc @arnested

@wing328 wing328 merged commit d7202a7 into swagger-api:2.3.0 Jun 4, 2017
@wing328
Copy link
Copy Markdown
Contributor

wing328 commented Jun 22, 2017

@baartosz when you've time, I wonder if you can contact me via the email address in my Github profile as I've some questions for you (not related to this PR). Thanks.

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