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

Implemented fix for #6006. Mime-type support #6751

Merged

Conversation

gupbeheer
Copy link
Contributor

Technical committee: @TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx

PR checklist

  • Read the contribution guidelines.
  • [X ] 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: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming langauge.

Description of the PR

Fix for #6006 and #6454. Since the fix done in #6454 has been removed.
I tested using the Angular-v2 and v4 scripts.
This fix is needed to support rest services that use mime-types for versioning.
The configuration has been expanded with methods that determine the correct Accepts and Content-Type (for non-form posts) mime-type and can be overridden if needed when creating a configuration. (If multiple versions are supported by the rest service, the correct filtering can be done there)

There is one thing I am not completely satisfied with and that is that all generated operations have a 'consumes' block. The reason behind that is that there should be no duplicate code in the 'api.service.mustache'. But I could not find a Mustache section that is equal to '#bodyParam' and '#hasFormParams' together.

return undefined;

let type = accepts.find(x => this.isJsonMime(x));
if (type === undefined)
Copy link
Contributor

@macjohnny macjohnny Oct 19, 2017

Choose a reason for hiding this comment

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

nice, this should avoid re-introducing #6588

return undefined;

let type = contentTypes.find(x => this.isJsonMime(x));
if (type === undefined)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest using block braces { and } as usually linting rules discourage if statements without braces

* @return {boolean} True if the given MIME is JSON, false otherwise.
*/
public isJsonMime(mime: string): boolean {
let jsonMimeRegEx = new RegExp("^(application\/json|[^;/ \t]+\/[^;/ \t]+[+]json)[ \t]*(;.*)?$");
Copy link
Contributor

Choose a reason for hiding this comment

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

how about also supporting json patch mime types?

the latest version of the isJsonMime() method before it was removed can be found here:
https://github.com/swagger-api/swagger-codegen/pull/6574/files#diff-3ab1141e07abf8762d6e33d15c9d1f0eL63

    public isJsonMime(mime: string): boolean {		
        const jsonMime: RegExp = new RegExp('^(application\/json|[^;/ \t]+\/[^;/ \t]+[+]json)[ \t]*(;.*)?$', 'i');		
        return mime != null && (jsonMime.test(mime) || mime.toLowerCase() === 'application/json-patch+json');		
    }

@wing328
Copy link
Contributor

wing328 commented Oct 27, 2017

@wing328
Copy link
Contributor

wing328 commented Oct 27, 2017

I'll merge it this weekend if there's no further feedback/question.

@gupbeheer
Copy link
Contributor Author

I thought I could fix the merge conflicts by a new build. But that does not seem to be the way.@wing328 if there is anything I can/should do to get this PR merged, please let me know.

@TiFu
Copy link
Contributor

TiFu commented Oct 31, 2017

You can fix the merge conflicts by rebasing your branch onto the current master.

See https://help.github.com/articles/syncing-a-fork/ for updating your repos master branch to this repos master branch.

Then you should be able to rebase (https://git-scm.com/docs/git-rebase) typescript_angular_mimetype_support onto master.

@wing328
Copy link
Contributor

wing328 commented Nov 4, 2017

I just resolved the merge conflicts. Will merge later ...

@wing328 wing328 merged commit a558604 into swagger-api:master Nov 4, 2017
@gamb79
Copy link

gamb79 commented Nov 4, 2017

@wing328 Thank you! You're the best!

@gupbeheer gupbeheer mentioned this pull request Nov 14, 2017
4 tasks
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.

5 participants