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

[BUG][dart-dio-next] Generator uses url encoded contentType even if the server content type requires application/json #9334

Closed
5 of 6 tasks
themisir opened this issue Apr 24, 2021 · 9 comments · Fixed by #9517

Comments

@themisir
Copy link

themisir commented Apr 24, 2021

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • Have you tested with the latest master to confirm the issue still exists?
  • Have you searched for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description

I'm using Asp.Net Core Swashbuckle to generate openapi scheme for my backend. The backend supports multiple content-type variants of application/json. But generated code uses url encoded content type instead of application/json.

Here's generated code output which picks first match which is "application/json-patch+json".

contentType: [
  'application/json-patch+json',
  'application/json',
  'text/json',
  'application/_*+json',
].first

But then Did checks if content-type is application/json then uses json encoding but if it isn't, it uses url encoding and my server returns HTTP 415 Unsupported Media Type status.

openapi-generator version

5.1.0

OpenAPI declaration file content or url

The OpenAPI declaration that caused this issue was used for private api. So I couldn't share it here.

Generation Details
Steps to reproduce
Related issues/PRs
Suggest a fix
  1. Overriding content-type header using command line options provided to generator CLI
  2. Updating contentType matching part to choose most suitable match rather than first one
@kuhnroyal
Copy link
Contributor

Thanks for the report.
You can try changing the order of the content-types in the spec around, not sure if that works.

But yea this needs some fixing.

@themisir
Copy link
Author

Thanks for the report.
You can try changing the order of the content-types in the spec around, not sure if that works.

I've already fixed the issue by manually modifying generated dart code (since scheme is generated on runtime on our backend, modifying dart generated code is way easier for me).

But I think this needs to be fixed. Actually I can work on this issue but I might need a bit of direction for contributing (for example which file(s) implements dart-dio-next generation).

@kuhnroyal
Copy link
Contributor

@themisir Do you need some help or pointers to get started?

@themisir
Copy link
Author

@themisir Do you need some help or pointers to get started?

Actually the issue is I'm not sure how can I implement it. Currently thinking about continuing the loop below until one of the known types is found.

for (Map<String, String> consume : op.consumes) {
if (consume.containsKey("mediaType")) {
String type = consume.get("mediaType");
isJson = type.equalsIgnoreCase("application/json");
isForm = type.equalsIgnoreCase("application/x-www-form-urlencoded");
isMultipart = type.equalsIgnoreCase("multipart/form-data");
break;
}
}

I can implement the logic like this.

for (Map<String, String> consume : op.consumes) {
    if (consume.containsKey("mediaType")) {
        String type = consume.get("mediaType");
        isJson = type.equalsIgnoreCase("application/json");
        isForm = type.equalsIgnoreCase("application/x-www-form-urlencoded");
        isMultipart = type.equalsIgnoreCase("multipart/form-data");
        if (isJson || isForm || isMultipart) {
            break;
        }
    }
}

So the loop will continue until it matches any known request content type.

What do you think about that?

@kuhnroyal
Copy link
Contributor

I am actually not sure if we still use the output of this snippet in the templates and if we have a case like yours in the example.

@themisir
Copy link
Author

themisir commented May 11, 2021

I am actually not sure if we still use the output of this snippet in the templates and if we have a case like yours in the example.

Yes you're right. openapi-generator doesn't uses the snipped / or at least the snippet is not related to my issue. I realized that a few minutes after creating the PR. Can I somehow change order of op.consumes?

@kuhnroyal
Copy link
Contributor

No, I don't think so.
Basically for dio and Dart, we only care about application/json, application/x-www-form-urlencoded and multipart/form-data.
Everything else we need to directly filter out and if none of these match we should probably add a warning and default to json.

@themisir
Copy link
Author

No, I don't think so.
Basically for dio and Dart, we only care about application/json, application/x-www-form-urlencoded and multipart/form-data.
Everything else we need to directly filter out and if none of these match we should probably add a warning and default to json.

You're right, thanks for the hint. I'll work on it.

kuhnroyal added a commit to kuhnroyal/openapi-generator that referenced this issue May 21, 2021
* fixes OpenAPITools#9334
* superseeds OpenAPITools#9454
* use `prioritizedContentTypes` in the same way `JavaClientCodegen` does
* move `application/json` to the front if it exists
* don't do anything if it is multi-part or url-encoded as for this the first content-type already needs to match
* log warning if an unsupported content-type is first after prioritizing
* remove some unused code blocks from dio generators
wing328 pushed a commit that referenced this issue May 24, 2021
* [dart] Improve content-type handling

* fixes #9334
* superseeds #9454
* use `prioritizedContentTypes` in the same way `JavaClientCodegen` does
* move `application/json` to the front if it exists
* don't do anything if it is multi-part or url-encoded as for this the first content-type already needs to match
* log warning if an unsupported content-type is first after prioritizing
* remove some unused code blocks from dio generators

* Only use first prioritized content-type in dio generators

* don't default to any content-type in dio-next, dio defaults itself to JSON
premiumitsolution pushed a commit to premiumitsolution/openapi-generator that referenced this issue May 26, 2021
* [dart] Improve content-type handling

* fixes OpenAPITools#9334
* superseeds OpenAPITools#9454
* use `prioritizedContentTypes` in the same way `JavaClientCodegen` does
* move `application/json` to the front if it exists
* don't do anything if it is multi-part or url-encoded as for this the first content-type already needs to match
* log warning if an unsupported content-type is first after prioritizing
* remove some unused code blocks from dio generators

* Only use first prioritized content-type in dio generators

* don't default to any content-type in dio-next, dio defaults itself to JSON
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants