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.

Sign up for GitHub

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

[cpp-ue4] fix for generating formParams in json requests #10177

Merged
merged 3 commits into from
Aug 19, 2021
Merged

[cpp-ue4] fix for generating formParams in json requests #10177

merged 3 commits into from
Aug 19, 2021

Conversation

leith-bartrich
Copy link
Contributor

Issue 10175 describes a potential bug and potential remedy. This pull request is the basic fix I referenced in the issue.

#10175

Simply put: if there is no body parameter defined, it tries to generate a json body from formParams. If a formParam isn't required, it uses IsSet from TOptional<> before writing the parameter.

See the referenced issue for the example schema that seems to require this template fix.

To be clear, this isn't really tested against a wide array of OpenAPI schemas. And there are further issues with the generator. But as a first pass, this at least gets a standard Django api-token-auth to be accepted by a Django server, and doesn't ruin other end points in my sample schema.

tagging @Kahncode as requested.

I'd want some more eyes on this at the very least. And my conceptual question from the issue still stands. I presume this to be a template problem, rather than a generator problem. But that might not be correct.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples: 
    ./mvnw clean package 
./bin/generate-samples.sh
./bin/utils/export_docs_generators.sh
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@Kahncode
Copy link
Contributor

Kahncode commented Aug 18, 2021 via email

@wing328 wing328 changed the title [cpp-ue4] fix for generating formParams in json requests as described in issue:10175 [cpp-ue4] fix for generating formParams in json requests Aug 19, 2021
@wing328 wing328 merged commit 81269b8 into OpenAPITools:master Aug 19, 2021
@wing328
Copy link
Member

wing328 commented Aug 19, 2021

Submitted #10194 to update the samples

@wing328 wing328 added this to the 5.3.0 milestone Aug 19, 2021
@leith-bartrich leith-bartrich deleted the bug-json-formParams-01 branch August 19, 2021 12:59
@Kahncode
Copy link
Contributor

Hello, we actually have issues created from this pull request. I'd love to discuss it further.

As far as I understood, a request cannot have body and form params at the same time. But then again, form params should never be generated as json body.

The error happens when a request has a file param, as this cannot be translated into a json body.

What is the expected behavior according to you @leith-bartrich ?

@leith-bartrich
Copy link
Contributor Author

Firstly, I'd refer to my earlier remarks in the original issue: #10175

So, this was an issue stemming from empirical testing. Django being an unavoidable behemoth of a server/framework implementation. Regardless of what standards docs say, if Django does it a certain way, a functional client needs to be able to do it Django's way; probably. Too many REST, and JSON-RPC APIs exist that are built with it to ignore it.

This absolutely opens up the door to needing to understand the behavior of many different server/framework implementations to understand reality. And I don't have details of the exact issue you're referencing. So I can't look more closely at it.

In the earlier bug report, I say: "I have interpreted this as a template problem rather than a generator problem. The generator sends formParams to the template and not bodyParams. And Django, in turn, is happy receiving these as part of a Json object sent as the body.

To elaborate a bit more: the actual schema in question defines the endpoint:

  /orgbuilder/api-token-auth/:
    post:
      operationId: createAuthToken
      description: ''
      parameters: []
      requestBody:
        content:
          application/x-www-form-urlencoded:
            schema:
              $ref: '#/components/schemas/AuthToken'
          multipart/form-data:
            schema:
              $ref: '#/components/schemas/AuthToken'
          application/json:
            schema:
              $ref: '#/components/schemas/AuthToken'
      responses:
        '201':
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/AuthToken'
          description: ''
      tags:
      - orgbuilder

Again, this gets generated from boilerplate Django auth API from their tutorial.

It's worth considering there are three call types (options) here. And the code at issue is specifically the JSON-RPC option. If the client chooses to use the application/json (JSON-RPC) call option, it's now bound by JSON-RPC (protocol) rules. That means it's got to post an actual json object. And the referenced AuthToken is indeed defined as an object, not as form params. Also, when this was written, the generated client code automatically preferred the JSON-RPC implementation when really an application should be able to choose based on application logic.

I'll reference an interesting discussion I found here: https://groups.google.com/g/json-rpc/c/puvZOQVf2Jo

This afore linked discussion ponders the idea of encoding the binary file data into the json object; points out it would pedantically work; but that it's not a feasible or desirable solution for real world reasons.

I'd be curious if Django, and other server/framework solutions would accept a binary encoded file as a body param. But that's just curiosity. I agree with the conclusions in the afore linked thread that a file is better handled as a post of its own. And technically, that post of that file is not a JSON-RPC call. The application is of course free to use JSON-RPC and any other RPC or C type by its own application logic, at any time.

So, as the folks back in 2009 pointed out, I'll point out that really, JSON-RPC by itself probably doesn't have a solution for a file parameter in a JSON-RPC request. And probably you'd want to somehow incorporate the file form field upload as a separate request that isn't a JSON-RPC request.

My question would be; do you have a schema that says the following: This endpoint only takes a JSON-RPC request, and that request contains a file parameter. If so, I wonder if that's even a legal schema endpoint?

To your specific ponderance: "As far as I understood, a request cannot have body and form params at the same time. But then again, form params should never be generated as json body."

Let's be more specific. A JSON-RPC request cannot have form params at all as far as I read it. https://www.jsonrpc.org/specification All params are body params, once you've decided you're making a JSON-RPC request.

As far as formParams are concerend I originally wrote: "I have interpreted this as a template problem rather than a generator problem. The generator sends formParams to the template and not bodyParams. And Django, in turn, is happy receiving these as part of a Json object sent as the body."

It's been a while since I looked at this stuff. But looking at the code file, I wonder if there's a way to loop through the actual request types individually in their own contexts, and if the generator better handles conversion to/from formParams and bodyParams, based on the specific request type? If so, that might be a higher-level change that's needed in the template.

Also, if the generator provides that functionality, what would it do with a file parameter inside the context of a JSON-RPC request?

If no such better context exists in the generator, to make a JSON-RPC request; and if the generator insists on providing formParams; I don't see how one could ever make a valid JSON-RPC request from such an endpoint without assuming a formParam means a bodyParam. But it's been a while. Maybe there is a better way.

@Kahncode
Copy link
Contributor

Kahncode commented Mar 28, 2023 •
edited
Loading

Hello @leith-bartrich. Thanks for the thorough answer.

There are very valid points here, in particular, no matter what the standard says, we have to support as many real world cases as we reasonably can.

I fixed the issue by simply omitting the file parameters when performing the fallback to form parameters generating a json body.

I do think that, this is in fact a generator issue, as it should not give both form and body parameters in the same request, as you may either send a json payload or a binary form, and not both at the same time. But regardless, the template must handle it gracefully.

Also, when this was written, the generated client code automatically preferred the JSON-RPC implementation when really an application should be able to choose based on application logic.

Absolutely great point. This could be made as an option in the generator, although nobody has requested it. There are also many formats such as xml that are not supported in this template.

My question would be; do you have a schema that says the following: This endpoint only takes a JSON-RPC request, and that request contains a file parameter. If so, I wonder if that's even a legal schema endpoint?

Any RPC with a file parameter as form parameter and no body parameters would attempt to compile illegal code.

#15068

I improved the template to only attempt to fill the body with the form parameters if no body parameters are defined. Alongside ignoring files (and printing an error) altogether, I believe this should solve most real world cases.

If you have the chance, please review those changes and perhaps confirm that they work with your use case, as I would not want to break anything.

#15068

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Client: C++ Issue: Bug
Projects
None yet
Milestone
5.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@leith-bartrich @Kahncode @wing328