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

[R] improve binary/text response handling #20131

Merged
merged 6 commits into from
Nov 24, 2024
Merged

[R] improve binary/text response handling #20131

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b3022dd
[R client] better support for binary/compressed responses
mattpollock Nov 15, 2024
17f9a7c
cleanup
mattpollock Nov 18, 2024
3c847fb
revert change after PR review
mattpollock Nov 20, 2024
47c9dca
update samples
mattpollock Nov 20, 2024
09a5f4e
fix R tests
mattpollock Nov 20, 2024
88fc70d
move private api methods to api-client, revert breaking method name c…
mattpollock Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions modules/openapi-generator/src/main/resources/r/ApiResponse.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,11 @@ ApiResponse <- R6::R6Class(
#'
#' @param from_encoding The encoding of the raw response.
#' @param to_encoding The target encoding of the return value.
response_as_text = function(from_encoding = "", to_encoding = "UTF-8") {
ResponseAsText = function(from_encoding = "", to_encoding = "UTF-8") {
Copy link
Member

Choose a reason for hiding this comment

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

shall we keep the snake case convention for the time being?

changing it results in breaking changes (without fallback) which is not something we want to do in the next minor release v7.11.0 (FYI we allow breaking changes with fallback in minor release)

Copy link
Member

Choose a reason for hiding this comment

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

to use PascalCase, I can think of a solution using lambda but I prefer we deal with it in another PR instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll revert to snake case. I switched to pearl based on the contributing guide, but I appreciate avoiding breaking changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if (is.null(self$response)) {
self$response <- charToRaw(jsonlite::toJSON("NULL"))
}
text_response <- iconv(readBin(self$response, character()), from = from_encoding, to = to_encoding)
if (is.na(text_response)) {
warning("The response is binary and will not be converted to text.")
}
return(text_response)
}
)
Expand Down
77 changes: 61 additions & 16 deletions modules/openapi-generator/src/main/resources/r/api.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,13 +153,13 @@

{{/vendorExtensions.x-streaming}}
if (local_var_response$status_code >= 200 && local_var_response$status_code <= 299) {
local_var_response$content
return(local_var_response$content)
} else if (local_var_response$status_code >= 300 && local_var_response$status_code <= 399) {
local_var_response
return(local_var_response)
} else if (local_var_response$status_code >= 400 && local_var_response$status_code <= 499) {
local_var_response
return(local_var_response)
} else if (local_var_response$status_code >= 500 && local_var_response$status_code <= 599) {
local_var_response
return(local_var_response)
}
},

Expand Down Expand Up @@ -543,24 +543,21 @@
if (local_var_resp$status_code >= 200 && local_var_resp$status_code <= 299) {
{{#returnType}}
{{#isPrimitiveType}}
local_var_content <- local_var_resp$response
local_var_resp, "text", encoding = "UTF-8", simplifyVector = FALSE
)
# save response in a file
if (!is.null(data_file)) {
write(local_var_content, data_file)
private$WriteFile(local_var_resp, data_file)
}

ApiResponse$new(content,resp)
{{/isPrimitiveType}}
{{^isPrimitiveType}}
# save response in a file
if (!is.null(data_file)) {
write(local_var_resp$response, data_file)
private$WriteFile(local_var_resp, data_file)
}

deserialized_resp_obj <- tryCatch(
self$api_client$deserialize(local_var_resp$response_as_text(), "{{returnType}}", loadNamespace("{{packageName}}")),
private$Deserialize(local_var_resp, "{{returnType}}"),
error = function(e) {
{{#useDefaultExceptionHandling}}
stop("Failed to deserialize response")
Expand All @@ -579,10 +576,13 @@
{{! Returning the ApiResponse object with NULL object when the endpoint doesn't return anything}}
local_var_resp$content <- NULL
{{/returnType}}
local_var_resp
} else if (local_var_resp$status_code >= 300 && local_var_resp$status_code <= 399) {
return(local_var_resp)
}

local_var_error_msg <- local_var_resp$ResponseAsText()
if (local_var_resp$status_code >= 300 && local_var_resp$status_code <= 399) {
{{#returnExceptionOnFailure}}
local_var_error_msg <- local_var_resp$response

if (local_var_error_msg == "") {
local_var_error_msg <- paste("Server returned ", local_var_resp$status_code, " response status code.")
}
Expand All @@ -600,7 +600,6 @@
{{/returnExceptionOnFailure}}
} else if (local_var_resp$status_code >= 400 && local_var_resp$status_code <= 499) {
{{#returnExceptionOnFailure}}
local_var_error_msg <- local_var_resp$response
if (local_var_error_msg == "") {
local_var_error_msg <- "Api client exception encountered."
}
Expand All @@ -618,7 +617,6 @@
{{/returnExceptionOnFailure}}
} else if (local_var_resp$status_code >= 500 && local_var_resp$status_code <= 599) {
{{#returnExceptionOnFailure}}
local_var_error_msg <- local_var_resp$response
if (local_var_error_msg == "") {
local_var_error_msg <- "Api server exception encountered."
}
Expand All @@ -635,11 +633,58 @@
if (is.null(local_var_resp$response) || local_var_resp$response == "") {
local_var_resp$response <- "API server error"
}
local_var_resp
return(local_var_resp)
{{/returnExceptionOnFailure}}
}
}{{^-last}},{{/-last}}
{{/operation}}
),
private = list(
#' @description
#' Write response to a file
#'
#' The function will write out data.
#'
#' 1. If binary data is detected it will use `writeBin`
#' 2. If the raw response is coercable to text, the text will be written to a file
#' 3. If the raw response is not coercable to text, the raw response will be written
#'
#' @param local_var_resp The API response
#' @param file The name of the data file to save the result
WriteFile = function(local_var_resp, file) {
if (private$IsBinary(local_var_resp$response)) {
writeBin(local_var_resp$response, file)
} else {
response <- private$Deserialize(local_var_resp)
base::write(response, file)
}
},

#' @description
#' Check response for binary content
#'
#' @param local_var_resp The API response
IsBinary = function(x) {
# ref: https://stackoverflow.com/a/17098690/1785752
b <- readBin(x, "int", n = 1000, size=1, signed=FALSE)
return(max(b) > 128)
},

#' @description
#' Deserialize the response
#'
#' @param local_var_resp The API response
#' @param return_type The target return type for the endpoint (e.g., `"object"`). If `NULL` text will be left as-is.
#' @return If the raw response is corecable to text, return the text. Otherwise return the raw resposne.
Deserialize = function(local_var_resp, return_type = NULL) {
text <- local_var_resp$ResponseAsText()
if (is.na(text)) {
return(local_var_resp$response)
} else if (is.null(return_type)) {
return(text)
}
return(self$api_client$deserialize(text, return_type, loadNamespace("{{packageName}}")))
}
Copy link
Member

Choose a reason for hiding this comment

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

what about adding these functions to api_client (which contains other utility functions)?

otherwise we will be repeating the same set of functions (private) in every auto-generated API files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Note that the code will be appear once in generated clients, but will need to be maintained in 2 places:

  1. modules/openapi-generator/src/main/resources/r/api_client.mustache
  2. modules/openapi-generator/src/main/resources/r/libraries/httr2/api_client.mustache

Copy link
Member

Choose a reason for hiding this comment

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

thanks. we can use partial mustache files but that can be done in a separate PR instead.

)
)
{{/operations}}
4 changes: 2 additions & 2 deletions modules/openapi-generator/src/main/resources/r/api_exception.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,15 +32,15 @@ ApiException <- R6::R6Class(
initialize = function(status = NULL, reason = NULL, http_response = NULL) {
if (!is.null(http_response)) {
self$status <- http_response$status_code
errorMsg <- http_response$response
errorMsg <- http_response$ResponseAsText()
if (is.null(errorMsg) || errorMsg == "") {
errorMsg <- "Api exception encountered. No details given."
}
self$body <- errorMsg
self$headers <- http_response$headers
self$reason <- http_response$http_status_desc
{{#errorObjectType}}
self$error_object <- {{errorObjectType}}$new()$fromJSONString(http_response$response)
self$error_object <- {{errorObjectType}}$new()$fromJSONString(http_response$ResponseAsText())
{{/errorObjectType}}
} else {
self$status <- status
Expand Down
5 changes: 1 addition & 4 deletions samples/client/echo_api/r/R/api_response.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,14 +52,11 @@ ApiResponse <- R6::R6Class(
#'
#' @param from_encoding The encoding of the raw response.
#' @param to_encoding The target encoding of the return value.
response_as_text = function(from_encoding = "", to_encoding = "UTF-8") {
ResponseAsText = function(from_encoding = "", to_encoding = "UTF-8") {
if (is.null(self$response)) {
self$response <- charToRaw(jsonlite::toJSON("NULL"))
}
text_response <- iconv(readBin(self$response, character()), from = from_encoding, to = to_encoding)
if (is.na(text_response)) {
warning("The response is binary and will not be converted to text.")
}
return(text_response)
}
)
Expand Down
89 changes: 71 additions & 18 deletions samples/client/echo_api/r/R/auth_api.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,13 @@ AuthApi <- R6::R6Class(
TestAuthHttpBasic = function(data_file = NULL, ...) {
local_var_response <- self$TestAuthHttpBasicWithHttpInfo(data_file = data_file, ...)
if (local_var_response$status_code >= 200 && local_var_response$status_code <= 299) {
local_var_response$content
return(local_var_response$content)
} else if (local_var_response$status_code >= 300 && local_var_response$status_code <= 399) {
local_var_response
return(local_var_response)
} else if (local_var_response$status_code >= 400 && local_var_response$status_code <= 499) {
local_var_response
return(local_var_response)
} else if (local_var_response$status_code >= 500 && local_var_response$status_code <= 599) {
local_var_response
return(local_var_response)
}
},

Expand Down Expand Up @@ -133,26 +133,29 @@ AuthApi <- R6::R6Class(
if (local_var_resp$status_code >= 200 && local_var_resp$status_code <= 299) {
# save response in a file
if (!is.null(data_file)) {
write(local_var_resp$response, data_file)
private$WriteFile(local_var_resp, data_file)
}

deserialized_resp_obj <- tryCatch(
self$api_client$deserialize(local_var_resp$response_as_text(), "character", loadNamespace("openapi")),
private$Deserialize(local_var_resp, "character"),
error = function(e) {
stop("Failed to deserialize response")
}
)
local_var_resp$content <- deserialized_resp_obj
local_var_resp
} else if (local_var_resp$status_code >= 300 && local_var_resp$status_code <= 399) {
return(local_var_resp)
}

local_var_error_msg <- local_var_resp$ResponseAsText()
if (local_var_resp$status_code >= 300 && local_var_resp$status_code <= 399) {
ApiResponse$new(paste("Server returned ", local_var_resp$status_code, " response status code."), local_var_resp)
} else if (local_var_resp$status_code >= 400 && local_var_resp$status_code <= 499) {
ApiResponse$new("API client error", local_var_resp)
} else if (local_var_resp$status_code >= 500 && local_var_resp$status_code <= 599) {
if (is.null(local_var_resp$response) || local_var_resp$response == "") {
local_var_resp$response <- "API server error"
}
local_var_resp
return(local_var_resp)
}
},

Expand All @@ -166,13 +169,13 @@ AuthApi <- R6::R6Class(
TestAuthHttpBearer = function(data_file = NULL, ...) {
local_var_response <- self$TestAuthHttpBearerWithHttpInfo(data_file = data_file, ...)
if (local_var_response$status_code >= 200 && local_var_response$status_code <= 299) {
local_var_response$content
return(local_var_response$content)
} else if (local_var_response$status_code >= 300 && local_var_response$status_code <= 399) {
local_var_response
return(local_var_response)
} else if (local_var_response$status_code >= 400 && local_var_response$status_code <= 499) {
local_var_response
return(local_var_response)
} else if (local_var_response$status_code >= 500 && local_var_response$status_code <= 599) {
local_var_response
return(local_var_response)
}
},

Expand Down Expand Up @@ -221,27 +224,77 @@ AuthApi <- R6::R6Class(
if (local_var_resp$status_code >= 200 && local_var_resp$status_code <= 299) {
# save response in a file
if (!is.null(data_file)) {
write(local_var_resp$response, data_file)
private$WriteFile(local_var_resp, data_file)
}

deserialized_resp_obj <- tryCatch(
self$api_client$deserialize(local_var_resp$response_as_text(), "character", loadNamespace("openapi")),
private$Deserialize(local_var_resp, "character"),
error = function(e) {
stop("Failed to deserialize response")
}
)
local_var_resp$content <- deserialized_resp_obj
local_var_resp
} else if (local_var_resp$status_code >= 300 && local_var_resp$status_code <= 399) {
return(local_var_resp)
}

local_var_error_msg <- local_var_resp$ResponseAsText()
if (local_var_resp$status_code >= 300 && local_var_resp$status_code <= 399) {
ApiResponse$new(paste("Server returned ", local_var_resp$status_code, " response status code."), local_var_resp)
} else if (local_var_resp$status_code >= 400 && local_var_resp$status_code <= 499) {
ApiResponse$new("API client error", local_var_resp)
} else if (local_var_resp$status_code >= 500 && local_var_resp$status_code <= 599) {
if (is.null(local_var_resp$response) || local_var_resp$response == "") {
local_var_resp$response <- "API server error"
}
local_var_resp
return(local_var_resp)
}
}
),
private = list(
#' @description
#' Write response to a file
#'
#' The function will write out data.
#'
#' 1. If binary data is detected it will use `writeBin`
#' 2. If the raw response is coercable to text, the text will be written to a file
#' 3. If the raw response is not coercable to text, the raw response will be written
#'
#' @param local_var_resp The API response
#' @param file The name of the data file to save the result
WriteFile = function(local_var_resp, file) {
if (private$IsBinary(local_var_resp$response)) {
writeBin(local_var_resp$response, file)
} else {
response <- private$Deserialize(local_var_resp)
base::write(response, file)
}
},

#' @description
#' Check response for binary content
#'
#' @param local_var_resp The API response
IsBinary = function(x) {
# ref: https://stackoverflow.com/a/17098690/1785752
b <- readBin(x, "int", n = 1000, size=1, signed=FALSE)
return(max(b) > 128)
},

#' @description
#' Deserialize the response
#'
#' @param local_var_resp The API response
#' @param return_type The target return type for the endpoint (e.g., `"object"`). If `NULL` text will be left as-is.
#' @return If the raw response is corecable to text, return the text. Otherwise return the raw resposne.
Deserialize = function(local_var_resp, return_type = NULL) {
text <- local_var_resp$ResponseAsText()
if (is.na(text)) {
return(local_var_resp$response)
} else if (is.null(return_type)) {
return(text)
}
return(self$api_client$deserialize(text, return_type, loadNamespace("openapi")))
}
)
)
Loading
Loading