[JAVA][#5172] Allow vendor json media types #5189

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

Jump to bottom

Merged

wing328 merged 9 commits into swagger-api:master from davidharrigan:FIX-5172

Mar 30, 2017

Merged

[JAVA][#5172] Allow vendor json media types #5189

wing328 merged 9 commits into swagger-api:master from davidharrigan:FIX-5172

Mar 30, 2017

Conversation

@davidharrigan

Copy link

Contributor

@davidharrigan davidharrigan commented Mar 24, 2017 •

edited

Loading

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

Adds support for vendor media type JSON (ends with +json) as specified in RFC-3839.

PR made against master since the change is non-breaking and backwards compatible (still has the same support for application/json).

More details in the issue here: #5172

David Harrigan added 3 commits

March 24, 2017 10:23


 [ #5172 ] Allow vendor json media types

8ca9240


 Revert unnecessary diffs

02639bd


 Update petstore sample

f336763

@davidharrigan davidharrigan changed the title ~~(削除) [Fixes #5172] Allow vendor json media types (削除ここまで)~~ (追記) [JAVA][#5172] Allow vendor json media types (追記ここまで)

Mar 24, 2017

David Harrigan added 2 commits

March 27, 2017 20:03


 Didn't run mvn after some edits

dfbd661


 Merge remote-tracking branch 'upstream/master' into FIX-5172

f10952a

@ePaul

Copy link

Contributor

ePaul commented Mar 29, 2017

Could you rebase your branch onto current master, and possibly regenerate the samples afterwards?

I think some of the changes in the generated samples are already incorporated there, and this would make it easier to see what actually changed.


 Merge remote-tracking branch 'upstream/master' into FIX-5172

0e62b83

ePaul

ePaul reviewed

Mar 29, 2017

View reviewed changes

modules/swagger-codegen/src/main/resources/Java/ApiClient.mustache

public boolean isJsonMime(String mime) {

return mime != null && mime.matches("(?i)application\\/json(;.*)?");

String jsonMime = "(?i)^(application/json|[^;/ \t]+/[^;/ \t]+[+]json)[ \t]*(;.*)?$";

Copy link

Contributor

@ePaul ePaul Mar 29, 2017

Choose a reason for hiding this comment

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

Are tab characters really allowed in media type names?

Copy link

Contributor Author

@davidharrigan davidharrigan Mar 29, 2017

Choose a reason for hiding this comment

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

Yes, according to the grammar described here in RFC-7230. You can see that HTAB corresponds to horizontal tab here in RFC-5234.

Copy link

Contributor

@ePaul ePaul Mar 29, 2017

Choose a reason for hiding this comment

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

Okay, thanks.

samples/client/petstore/java/jersey1/src/test/java/io/swagger/client/ApiClientTest.java

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

Contributor

@ePaul ePaul Mar 29, 2017

Choose a reason for hiding this comment

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

If you want a real-live example, application/problem+json (from RFC 7807) is often used in APIs.

Copy link

Contributor Author

@davidharrigan davidharrigan Mar 29, 2017

Choose a reason for hiding this comment

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

Sure thing, I'll update these tests with some real-live example.


 Rerun ' ./bin/java-petstore-all.sh' and './bin/security/java-petstore...

49e73a2

...-okhttp-gson.sh'

@davidharrigan

Copy link

Contributor Author

davidharrigan commented Mar 29, 2017

Looks like the failed test is fixed here: #5243

Failed tests: testApiClient(io.swagger.client.api.PetApiTest): expected:<.../petstore.swagger.io[]/v2> but was:<.../petstore.swagger.io[:80]/v2>

@ePaul

Copy link

Contributor

ePaul commented Mar 29, 2017

Yes, sorry for the update proposal, I didn't mean to cause more issues by that, but simply forgot that current master is "broken" (for sample generation) until #5243 is merged.

@davidharrigan

Copy link

Contributor Author

davidharrigan commented Mar 29, 2017

no worries :)

David Harrigan added 2 commits

March 29, 2017 14:15


 Added more realistic test cases for isJsonMime

3e81211


 Merge remote-tracking branch 'upstream/master' into FIX-5172

c31d597

@wing328 wing328 added the Client: Java label

Mar 30, 2017

@wing328 wing328 added this to the v2.2.3 milestone

Mar 30, 2017

@wing328 wing328 added the Enhancement: General label

Mar 30, 2017

wing328

wing328 reviewed

Mar 30, 2017

View reviewed changes

modules/swagger-codegen/src/main/resources/Java/libraries/okhttp-gson/ApiClient.mustache

public boolean isJsonMime(String mime) {

return mime != null && mime.matches("(?i)application\\/json(;.*)?");

String jsonMime = "(?i)^(application/json|[^;/ \t]+/[^;/ \t]+[+]json)[ \t]*(;.*)?$";

return mime != null && mime.matches(jsonMime);

Copy link

Contributor

@wing328 wing328 Mar 30, 2017 •

edited

Loading

Choose a reason for hiding this comment

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

Later I will file a PR to put these 2 lines into one without declaring a temporary variable.

wing328

wing328 reviewed

Mar 30, 2017

View reviewed changes

samples/client/petstore/java/okhttp-gson/src/test/java/io/swagger/client/ApiClientTest.java

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

Contributor

@wing328 wing328 Mar 30, 2017

Choose a reason for hiding this comment

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

We'll leverage these test cases in other API clients' unit tests

@wing328 wing328 merged commit fc01457 into swagger-api:master

Mar 30, 2017

@wing328

Copy link

Contributor

wing328 commented Mar 30, 2017

@ePaul thanks for reviewing the change.

@davidharrigan thanks for the contribution, which has been merged into master.

@wing328 wing328 mentioned this pull request

Mar 30, 2017

Update test cases for JSON MIME type detection #5261

Open

@ePaul ePaul mentioned this pull request

Apr 1, 2017

Updating (almost) all samples. #5281

Merged

3 tasks

@wing328 wing328 mentioned this pull request

Apr 27, 2017

[JAVA] isJsonMime should support vendor specific JSON MIME type #5172

Closed

@davidharrigan davidharrigan deleted the FIX-5172 branch

May 6, 2017 15:44

davidgri pushed a commit to davidgri/swagger-codegen that referenced this pull request

May 11, 2017

@davidharrigan @wing328


 [JAVA][ swagger-api#5172 ] Allow vendor json media types ( swagger-api#5189

91ec28e

)
* [swagger-api#5172] Allow vendor json media types
* Revert unnecessary diffs
* Update petstore sample
* Didn't run mvn after some edits
* Rerun ' ./bin/java-petstore-all.sh' and './bin/security/java-petstore-okhttp-gson.sh'
* Added more realistic test cases for isJsonMime

@brendandburns brendandburns mentioned this pull request

Jun 12, 2017

Add JSON-patch as a JSON mime-type #5764

Merged

Labels

Client: Java Enhancement: General

3 participants

@davidharrigan @ePaul @wing328

[JAVA][#5172] Allow vendor json media types #5189

[JAVA][#5172] Allow vendor json media types #5189

Uh oh!

Conversation

@davidharrigan davidharrigan commented Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Description of the PR

Uh oh!

ePaul commented Mar 29, 2017

Uh oh!

@ePaul ePaul Mar 29, 2017

Choose a reason for hiding this comment

Uh oh!

@davidharrigan davidharrigan Mar 29, 2017

Choose a reason for hiding this comment

Uh oh!

@ePaul ePaul Mar 29, 2017

Choose a reason for hiding this comment

Uh oh!

@ePaul ePaul Mar 29, 2017

Choose a reason for hiding this comment

Uh oh!

@davidharrigan davidharrigan Mar 29, 2017

Choose a reason for hiding this comment

Uh oh!

davidharrigan commented Mar 29, 2017

Uh oh!

ePaul commented Mar 29, 2017

Uh oh!

davidharrigan commented Mar 29, 2017

Uh oh!

@wing328 wing328 Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@wing328 wing328 Mar 30, 2017

Choose a reason for hiding this comment

Uh oh!

wing328 commented Mar 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

@davidharrigan davidharrigan commented Mar 24, 2017 •

edited

Loading

@wing328 wing328 Mar 30, 2017 •

edited

Loading