[R] Bug - Invalid code generated for POST with no request object by robertpyke · Pull Request #8067 · OpenAPITools/openapi-generator

robertpyke · 2020-12-02T07:57:32Z

PR is just a suggestion of how I think you can fix it. I'm not setup to actually run the build system at the moment. i.e. I'm not claiming this is ready to merge as is, it's just what I expect will address the issue based on my observation. I.e. You can think of this more as a bug report with attached code pointer if you like.

Description

Our model has a POST that has no request modeled. i.e. no body. The code generated normally for a PUT with a request generates something akin to:

       if (!missing(`athena.create.table.request`)) {
         body <- `athena.create.table.request`$toJSONString()
       } else {
         body <- NULL
       }

...

      resp <- self$apiClient$CallApi(url = paste0(self$apiClient$basePath, urlPath),
                                 method = "PUT",
                                 queryParams = queryParams,
                                 headerParams = headerParams,
                                 body = body,
                                 ...)

In the case where no request object is modeled, this initial code block (where body is set) isn't generated, so you just end up with something like:

      resp <- self$apiClient$CallApi(url = paste0(self$apiClient$basePath, urlPath),
                                 method = "POST",
                                 queryParams = queryParams,
                                 headerParams = headerParams,
                                 body = body,
                                 ...)

This in turn means self$apiClient$CallApi is called with body set as the vale of the built in function base::body (as body isn't a local variable).

base::body
==>
function (fun = sys.function(sys.parent())) 
{
    if (is.character(fun)) 
        fun <- get(fun, mode = "function", envir = parent.frame())
    .Internal(body(fun))
}

This results in a strange error indicating that body was unexpected:

api.instance <- DefaultApi$new()
result <- api.instance$TokenExchangeStsPost()

where TokenExchangeStsPost is a POST with no request body modeled.

Error: Unknown type of `body`: must be NULL, FALSE, character, raw or list
Traceback:

1. api.instance$TokenExchangeStsPost()
2. self$TokenExchangeStsPostWithHttpInfo(...)
3. self$apiClient$CallApi(url = paste0(self$apiClient$basePath, 
 .     urlPath), method = "POST", queryParams = queryParams, headerParams = headerParams, 
 .     body = body, ...)
4. httr::POST(url, query = queryParams, headers, body = body, httr::content_type("application/json"), 
 .     httpTimeout, httr::user_agent(self$userAgent), ...)
5. request_build("POST", hu$url, body_config(body, match.arg(encode)), 
 .     as.request(config), ...)
6. body_config(body, match.arg(encode))
7. stop("Unknown type of `body`: must be NULL, FALSE, character, raw or list", 
 .     call. = FALSE)

Swagger-codegen version

Build package: org.openapitools.codegen.languages.RClientCodegen

  <dependency>
    <groupId>org.openapitools</groupId>
    <artifactId>openapi-generator-cli</artifactId>
    <version>4.2.3</version>
  </dependency>

Swagger declaration file content or url

Note: I removed some of our model from what does the repro (to not expose certain info). The important bit is that the token_exchange_sts is a post, and has no request defined.

openapi: "3.0.1"
info:
    title: "${title}"
    version: "1.0"
    description: "${description}"
x-amazon-apigateway-policy: ${policy}
servers:
    -
        url: "${endpoint}"
        x-amazon-apigateway-endpoint-configuration:
            vpcEndpointIds:
                - "${vpc_ep_id}"

paths:
    /token_exchange_sts:
        post:
            responses:
                '200':
                    description: Success
                    content:
                        application/json:
                            schema:
                                $ref: '#/components/schemas/tokenExchangeSTSResponse'
            x-amazon-apigateway-integration:
                httpMethod: POST
                type: aws_proxy
                uri: arn:aws:apigateway:${region}:lambda:path/2015-03-31/functions/${service_function_arn}:${alias}/invocations
                credentials: ${service_invoker_role_arn}

  

security:
    - CustomAuthorizer: []
components:
    securitySchemes:
        CustomAuthorizer:
            in: header
            type: apiKey
            name: Authorization
            x-amazon-apigateway-authorizer:
                type: token
                authorizerResultTtlInSeconds: 300
                authorizerUri: arn:aws:apigateway:${region}:lambda:path/2015-03-31/functions/${authorizer_function_arn}:${alias}/invocations
                identityValidationExpression: Bearer .+
                authorizerCredentials: ${auth_invoker_role_arn}
            x-amazon-apigateway-authtype: custom
    schemas:
        # Response Models
        tokenExchangeSTSResponse:
            type: object
            properties:
                token:
                    $ref: '#/components/schemas/stsCredentials'

            required:
                - token


        # Object Models
        stsCredentials:
            description: Represents STS credentials for a Session (An assumed role)
            type: object
            properties:
                access_key_id:
                    type: string
                    readOnly: true
                secret_access_key:
                    format: password  # Hint that this shouldn't be exposed.
                    type: string
                    readOnly: true
                session_token:
                    type: string
                    readOnly: true
            required:
                - access_key_id
                - secret_access_key
                - session_token

Command line used for generation

        "-J-Dmodels",
        "-J-Dapis",
        "-J-DsupportingFiles",
        "-J-DmodelTests=false",
        "-J-DapiTests=false",
        "generate",
        "-i",
        "MY FILE.yaml",
        "-g",
        "r",
        "-o",
        "MY OUTPUT PATH",
        "-p",
        "packageName=rproxyserviceclient"

Steps to reproduce

Generate the client in R for a POST model with no request body (as shown in the example).
Observe that the file default_api.R has the following:

TokenExchangeStsPostWithHttpInfo = function(...){
      args <- list(...)
      queryParams <- list()
      headerParams <- c()

      urlPath <- "/token_exchange_sts"
      # API key authentication
      if ("Authorization" %in% names(self$apiClient$apiKeys) && nchar(self$apiClient$apiKeys["Authorization"]) > 0) {
        headerParams['Authorization'] <- paste(unlist(self$apiClient$apiKeys["Authorization"]), collapse='')
      }

      resp <- self$apiClient$CallApi(url = paste0(self$apiClient$basePath, urlPath),
                                 method = "POST",
                                 queryParams = queryParams,
                                 headerParams = headerParams,
                                 body = body,
                                 ...)

      if (httr::status_code(resp) >= 200 && httr::status_code(resp) <= 299) {
        deserializedRespObj <- tryCatch(
          self$apiClient$deserialize(resp, "TokenExchangeSTSResponse", loadNamespace("rproxyserviceclient")),
          error = function(e){
             stop("Failed to deserialize response")
          }
        )
        ApiResponse$new(deserializedRespObj, resp)
      } else if (httr::status_code(resp) >= 300 && httr::status_code(resp) <= 399) {
        ApiResponse$new(paste("Server returned " , httr::status_code(resp) , " response status code."), resp)
      } else if (httr::status_code(resp) >= 400 && httr::status_code(resp) <= 499) {
        ApiResponse$new("API client error", resp)
      } else if (httr::status_code(resp) >= 500 && httr::status_code(resp) <= 599) {
        ApiResponse$new("API server error", resp)
      }
    }

You can see this code is invalid:

                                 body = body,

is "broken".

You can validate it by running the client like so:

library(rproxyserviceclient)
api.instance <- DefaultApi$new()
result <- api.instance$TokenExchangeStsPost()

api.instance$TokenExchangeStsPost() will break (Unknown type of body: must be NULL, FALSE, character, raw or list").

Suggest a fix/enhancement

I think you should either set

body <- NULL

above in the case where no request is defined. I believe that will fix it.

Setting it here:

openapi-generator/modules/openapi-generator/src/main/resources/r/api.mustache

Line 180 in 634c4c0

would effectively default it to NULL, and then the rest of the code generation could override it.

Alternatively if you weren't using a variable name that maps to the base::body, then that might avoid it as well.

I made a local change to api_client.R

      # Default body to NULL when incorrectly set to a function.
      # Body is incorrectly set to base::body (a built-in R function) in
      # some cases due to a bug in the OpenAPI code generator
      # (it assumes it has generated a local variable body when it hasn't).
      # This workaround sets body to NULL if it's ever set as function by the time it gets here.
      if (is.function(body)) {
          body = NULL
      }

but that's clearly a hack.. it works because the body is assigned to the built in function in this special case, is otherwise not a function. I only made the change in this file, as it doesn't change as we update the model, unlike default_api.R, so I changed api_client.R and added it to the ignore file for the generator (so the fix sticks).

FYI : the same template works fine in Python without any changes/hacks.

auto-labeler · 2020-12-02T07:57:36Z

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

robertpyke · 2020-12-02T08:09:42Z

Me attempting to copy the R maintainers (apologies if I misunderstood the request) @Ramanth and @saigiridhar21 .
And to be clear, I barely know R, so apologies if the suggestion is terrible, but hopefully the bug report has all the context you need.

wing328 · 2020-12-02T13:59:07Z

Pushed 2ff382a to update the samples so that the CI can verify the change.

wing328 · 2020-12-02T15:07:05Z

  <dependency>
    <groupId>org.openapitools</groupId>
    <artifactId>openapi-generator-cli</artifactId>
    <version>4.2.3</version>
  </dependency>

Can you please try the latest master or 5.0.0-beta3 to see if it's still an issue?

robertpyke · 2020-12-02T18:30:40Z

Hi, I've confirmed with openapi-generator-cli-5.0.0-beta3.jar I still get the same error.

The generated code for the relevant functions are:

    TokenExchangeStsPost = function(...){
      apiResponse <- self$TokenExchangeStsPostWithHttpInfo(...)
      resp <- apiResponse$response
      if (httr::status_code(resp) >= 200 && httr::status_code(resp) <= 299) {
        apiResponse$content
      } else if (httr::status_code(resp) >= 300 && httr::status_code(resp) <= 399) {
        apiResponse
      } else if (httr::status_code(resp) >= 400 && httr::status_code(resp) <= 499) {
        apiResponse
      } else if (httr::status_code(resp) >= 500 && httr::status_code(resp) <= 599) {
        apiResponse
      }
    },

    TokenExchangeStsPostWithHttpInfo = function(...){
      args <- list(...)
      queryParams <- list()
      headerParams <- c()

      urlPath <- "/token_exchange_sts"
      # API key authentication
      if ("Authorization" %in% names(self$apiClient$apiKeys) && nchar(self$apiClient$apiKeys["Authorization"]) > 0) {
        headerParams['Authorization'] <- paste(unlist(self$apiClient$apiKeys["Authorization"]), collapse='')
      }

      resp <- self$apiClient$CallApi(url = paste0(self$apiClient$basePath, urlPath),
                                 method = "POST",
                                 queryParams = queryParams,
                                 headerParams = headerParams,
                                 body = body,
                                 ...)

      if (httr::status_code(resp) >= 200 && httr::status_code(resp) <= 299) {
        deserializedRespObj <- tryCatch(
          self$apiClient$deserialize(resp, "TokenExchangeSTSResponse", loadNamespace("rproxyserviceclient")),
          error = function(e){
             stop("Failed to deserialize response")
          }
        )
        ApiResponse$new(deserializedRespObj, resp)
      } else if (httr::status_code(resp) >= 300 && httr::status_code(resp) <= 399) {
        ApiResponse$new(paste("Server returned " , httr::status_code(resp) , " response status code."), resp)
      } else if (httr::status_code(resp) >= 400 && httr::status_code(resp) <= 499) {
        ApiResponse$new("API client error", resp)
      } else if (httr::status_code(resp) >= 500 && httr::status_code(resp) <= 599) {
        ApiResponse$new("API server error", resp)
      }
    }

As you can see it has the same issue of setting body = body, without first assigning a local variable to body (resulting in body being passed in as the built-in function body).

I get this error when running the code:

> library(rproxyserviceclient)
> api.instance <- DefaultApi$new()
> result <- api.instance$TokenExchangeStsPost()
Error: Unknown type of `body`: must be NULL, FALSE, character, raw or list

wing328 · 2020-12-03T04:06:05Z

Looks like the change breaks one of the tests:

All user-level objects in a package should have documentation entries.
See chapter ‘Writing R documentation files’ in the ‘Writing R
Extensions’ manual.
* checking examples ... NONE
* checking for unstated dependencies in ‘tests’ ... OK
* checking tests ...
  Running ‘testthat.R’
 ERROR
Running the tests in ‘tests/testthat.R’ failed.
Last 13 lines of output:
  ── Skip (test_user_api.R:77:1): LogoutUser ─────────────────────────────────────
  Reason: empty test
  
  ── Skip (test_user_api.R:87:1): UpdateUser ─────────────────────────────────────
  Reason: empty test
  
  ── Skipped tests  ──────────────────────────────────────────────────────────────
  ● empty test (47)
  
  ══ testthat results  ═══════════════════════════════════════════════════════════
  ERROR (test_petstore.R:13:1): (code run outside of `test_that()`)
  
  [ FAIL 1 | WARN 0 | SKIP 47 | PASS 0 ]
  Error: Test failures
  Execution halted
* DONE

Status: 1 ERROR, 2 WARNINGs, 1 NOTE
See
  ‘/home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/R/petstore.Rcheck/00check.log’
for details.

This line failed the tests: https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/R/tests/testthat/test_petstore.R#L13

robertpyke · 2020-12-03T04:24:30Z

Hi, thanks for the response. As I mentioned at the outset, I didn’t have the build system set up - I initially intended this to be just a bug report, but figured I might as well try provide a code pointer along with it ;)

I’m not super familiar with R, but I can try to setup the build system properly, and see if I can fix whatever is going on.

robertpyke · 2020-12-03T04:45:52Z

I think I've got the build system working such that I can run those R tests now. I'll report back when I've got something useful to add.

robertpyke · 2020-12-03T04:52:56Z

I think that error was there before my change:

I'm running:
mvn integration-test -f samples/client/petstore/R/pom.xml

when I'm at: "634c4c09e46271c5baba28439f12424b562a5e34"

% git log
commit 634c4c09e46271c5baba28439f12424b562a5e34 (HEAD -> master, origin/master, origin/HEAD)
Author: Thomas Hervé <[email protected]>
Date:   Wed Dec 2 04:04:23 2020 +0100

    Fix generation of map model examples (#8063)

    When generation examples of objects with additional properties, we use
    the map syntax inside a model instantiation, which is incorrect. This
    fixes it by checking for model at the right place.

which is the mainline I'm forked from (so before my change), and I get the error:

  ══ testthat results  ═══════════════════════════════════════════════════════════
  FAILURE (test_petstore.R:17:3): AddPet
  FAILURE (test_petstore.R:78:3): GetPetById
  FAILURE (test_petstore.R:79:3): GetPetById
  FAILURE (test_petstore.R:80:3): GetPetById
  FAILURE (test_petstore.R:84:3): GetPetById
  FAILURE (test_petstore.R:85:3): GetPetById
  FAILURE (test_petstore.R:87:3): GetPetById
  FAILURE (test_petstore.R:88:3): GetPetById

  [ FAIL 8 | WARN 0 | SKIP 47 | PASS 9 ]
  Error: Test failures
  Execution halted
* DONE

Status: 1 ERROR, 2 WARNINGs, 1 NOTE
See
  ‘/Users/robertpyke/Documents/git/openapi-generator/samples/client/petstore/R/petstore.Rcheck/00check.log’
for details.

[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 44.289 s
[INFO] Finished at: 2020-12-02T20:51:57-08:00
[INFO] Final Memory: 10M/80M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.2.1:exec (nose-test) on project RPetstoreClientTests: Command execution failed.: Process exited with an error: 1 (Exit value: 1) -> [Help 1]
[ERROR]
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR]
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MojoExecutionException

wing328 · 2020-12-03T04:58:24Z

The CircleCI runs a local test server instead of using the public one (which fails pretty much all the test).

I'll see what I can do later today to help you pinpoint the root cause of the issue.

robertpyke · 2020-12-03T05:03:55Z

I think I found the issue with my code:

This is the generated code..

    AddPetWithHttpInfo = function(body, ...){
      args <- list(...)
      queryParams <- list()
      headerParams <- c()
      body <- NULL

My naive defaulting is nuking the case where body is a provided param.

Now I've got it building locally, I'll see if I can make a valid fix.

robertpyke · 2020-12-03T05:54:27Z

Okay, I believe that's it. I updated the samples. I also tried running the integ tests again, and now I get the same result as what I did in mainline.

I updated it to only default body to NULL when the model had no "hasBodyParam".

I generated code locally with my sample file, and confirmed the change would fix my issue.

robertpyke · 2020-12-03T06:09:17Z

...
    TokenExchangeStsPostWithHttpInfo = function(...){
      args <- list(...)
      queryParams <- list()
      headerParams <- c()

      body <- NULL
      urlPath <- "/token_exchange_sts"

      resp <- self$apiClient$CallApi(url = paste0(self$apiClient$basePath, urlPath),
                                 method = "POST",
                                 queryParams = queryParams,
                                 headerParams = headerParams,
                                 body = body,
                                 ...)

...

Example generated using my latest change, showing it would address my issue.

robertpyke · 2020-12-03T06:38:45Z

I reviewed the sample generated code and noticed one other case - where hasBodyParam was false, but hasFormParams was true, would result in overriding body. Fixed that now. I don't see any other issues having reviewed the generated code (looks like body is only set to NULL when it's safe to do so now - which should fix my case, where hasBodyParam and hasFormParams are false for a POST).

Default body to NULL

82fad9f

auto-labeler Bot added the Client: R label Dec 2, 2020

robertpyke commented Dec 2, 2020

View reviewed changes

Comment thread modules/openapi-generator/src/main/resources/r/api.mustache

robertpyke mentioned this pull request Dec 2, 2020

[R] Bug - Invalid code generated for POST with no request object swagger-api/swagger-codegen#10591

Closed

update samples

2ff382a

wing328 added the Issue: Bug label Dec 2, 2020

wing328 added this to the 5.0.0 milestone Dec 2, 2020

Update to set body to NULL only when no hasBodyParam

6d11fc8

Revert any whitespace changes.

e0bc10f

Makes defaulting body conditional on hasFormParams also not being set

58cf605

wing328 merged commit 7644f3e into OpenAPITools:master Dec 3, 2020

Uh oh!

Conversation

robertpyke commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Swagger-codegen version

Swagger declaration file content or url

Command line used for generation

Steps to reproduce

Suggest a fix/enhancement

Uh oh!

auto-labeler Bot commented Dec 2, 2020

Uh oh!

Uh oh!

robertpyke commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wing328 commented Dec 2, 2020

Uh oh!

wing328 commented Dec 2, 2020

Uh oh!

robertpyke commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wing328 commented Dec 3, 2020

Uh oh!

robertpyke commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertpyke commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertpyke commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wing328 commented Dec 3, 2020

Uh oh!

robertpyke commented Dec 3, 2020

Uh oh!

robertpyke commented Dec 3, 2020

Uh oh!

robertpyke commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robertpyke commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robertpyke commented Dec 2, 2020 •

edited

Loading

robertpyke commented Dec 2, 2020 •

edited

Loading

robertpyke commented Dec 2, 2020 •

edited

Loading

robertpyke commented Dec 3, 2020 •

edited

Loading

robertpyke commented Dec 3, 2020 •

edited

Loading

robertpyke commented Dec 3, 2020 •

edited

Loading

robertpyke commented Dec 3, 2020 •

edited

Loading

robertpyke commented Dec 3, 2020 •

edited

Loading