[R] fix to-list and to-json functionality #20132

mattpollock · 2024-11-19T01:12:07Z

This PR fixes an issue with converting R6 objects to JSON and R list types.

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 || exit
./bin/generate-samples.sh ./bin/configs/*.yaml || exit
./bin/utils/export_docs_generators.sh || exit
```
(For Windows users, please run the script in Git BASH)
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*.
IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

mattpollock · 2024-11-19T01:16:17Z

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

    toJSON = function() {
+      .Deprecated(new = "toList", msg = "Use the '$toList()' method instead since that is more learly named. Use '$toJSONstring()' to get a JSON string")
+      return(self$toList())


I deprecated toJSON() because it doesn't actually convert the R6 object to a JSON structure, just an R list type. The functionality is still there in the renamed toList() function.

mattpollock · 2024-11-19T01:18:02Z

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

-      jsoncontent <- paste(jsoncontent, collapse = ",")
-      json_string <- as.character(jsonlite::minify(paste("{", jsoncontent, "}", sep = "")))
+    toJSONString = function(minify = TRUE, ...) {
+      json_obj <- self$toList()


In local testing I was getting some errors from $toJSONString(). It also seemed pretty redundant with the content in toJSON()/toList(). I refactored this method to use $toList() internally and then convert the list-type to JSON using the jsonlite library.

wing328 · 2024-11-20T06:43:42Z

thanks for the PR

I ran the integration tests locally but got errors:

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:
   1. ├─testthat::expect_equal(nested_oneof$toJSONString(), "{\"size\":15,\"nested_pig\":{\"className\":\"BasquePig\",\"color\":\"red\"}}") at test_petstore.R:281:3
   2. │ └─testthat::quasi_label(enquo(object), label, arg = "object")
   3. │   └─rlang::eval_bare(expr, quo_get_env(quo))
   4. └─nested_oneof$toJSONString()
   5.   └─self$toList()
  ── Failure ('test_petstore.R:322:3'): Tests anyOf ──────────────────────────────
  danish_pig$toJSONString() not equal to original_danish_pig$toJSONString().
  Classes differ: 'character' is not 'json'
  ── Failure ('test_petstore.R:328:3'): Tests anyOf ──────────────────────────────
  basque_pig$toJSONString() not equal to original_basque_pig$toJSONString().
  Classes differ: 'character' is not 'json'

  [ FAIL 11 | WARN 5 | SKIP 107 | PASS 62 ]
  Error: Test failures
  Execution halted

* DONE
Status: 1 ERROR, 1 WARNING
See
  ‘/mnt/c/Users/wing3/Code/openapi-generator/samples/client/petstore/R/petstore.Rcheck/00check.log’
for details.

can you pleasea take a look?

to run the test, please run mvn integration-test -f samples/client/petstore/R/pom.xml

you will need to run the petstore server locally: https://github.com/OpenAPITools/openapi-generator/wiki/Integration-Tests

mattpollock · 2024-11-20T18:28:24Z

Thanks @wing328. I had some trouble with the integration tests (my hosts file is locked down) but I think I fixed the issue. jsonlite::toJSON creates an object of type "json" instead of a character string, I converted back to a string to match the behavior in master.

mattpollock · 2024-11-20T20:36:19Z

I poked at this some more and while the patch I pushed fixes the JSON string error, I think #20131 is needed to clean up the unit tests (specifically, this).

If you agree I'll rebase this PR on top of master after #20131 is merged.

mattpollock · 2024-11-21T17:52:44Z

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

    #' @description
    #' Serialize {{{classname}}} to JSON string.
-    #'
+    #' 
+    #' @param ... Parameters passed to `jsonlite::toJSON`
    #' @return JSON string representation of the {{{classname}}}.
-    toJSONString = function() {
+    toJSONString = function(...) {
+      simple <- self$toSimpleType()
      if (!is.null(self$actual_instance)) {
-        as.character(jsonlite::minify(self$actual_instance$toJSONString()))
+        json <- jsonlite::toJSON(simple, auto_unbox = TRUE, ...)
+        return(as.character(jsonlite::minify(json)))
      } else {
-        NULL
+        return(NULL)
      }
    },

    #' @description
-    #' Serialize {{{classname}}} to JSON.
-    #'
-    #' @return JSON representation of the {{{classname}}}.
+    #' Convert to an R object. This method is deprecated. Use `toSimpleType()` instead.
    toJSON = function() {
+      .Deprecated(new = "toSimpleType", msg = "Use the '$toSimpleType()' method instead since that is more learly named. Use '$toJSONString()' to get a JSON string")
+      return(self$toSimpleType())
+    },
+
+    #' @description
+    #' Convert {{{classname}}} to a base R type
+    #'
+    #' @return A base R type, e.g. a list or numeric/character array.
+    toSimpleType = function() {
      if (!is.null(self$actual_instance)) {
-        self$actual_instance$toJSON()
+        return(self$actual_instance$toSimpleType())
      } else {
-        NULL
+        return(NULL)
      }


OK, I refactored this PR a bit since a realized that not all model types necessarily get transformed to R list types enroute to JSON.

The gist is that

I depreciated toJSON in favor of toSimpleType and made the behavior consistent (i.e., it returns an R list, or character vector, etc.). Previously toJSON might return a list or a JSON string, depending on the model type.

I added toSimpleType to all models

I modified toJSONString to use toSimpleType() to get the R object and then invoke the jsonlite transformations on that object (toJSON and minifiy).

mattpollock · 2024-11-21T17:54:22Z

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

+    #' @description
+    #' Convert to a List
+    #'
+    #' Convert the R6 object to a list to work more easily with other tooling.
+    #'
+    #' @return {{{classname}}} as a base R list.
+    #' @examples
+    #' # convert array of {{{classname}}} (x) to a data frame
+    #' \dontrun{
+    #' df <- x |> purrr::map_dfr(\(y)y$toList())
+    #' df
+    #' }
+    toList = function() {
+      return(self$toSimpleType())
+    },


for the generic model only, I also added a toList method which just wraps toSimpleType but

is more clear with respect to return type

is documented to support converting an array of R6 objects to a data.frame, which is how most R users prefer to operate

mattpollock · 2024-11-25T15:41:13Z

@wing328 thanks for merging #20131. I rebased this PR on those changes (which fixed some of the R petstore unit tests) and all tests are passing for me locally now.

wing328 · 2024-11-26T09:15:13Z

thank you. let me test and see if i've any questions.

wing328 · 2024-11-27T08:35:45Z

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

    toJSON = function() {
+      .Deprecated(new = "toSimpleType", msg = "Use the '$toSimpleType()' method instead since that is more learly named. Use '$toJSONString()' to get a JSON string")


@mattpollock should it be "clearly named" instead of "learly named"?

Thanks - pushed a fix.

wing328 · 2024-11-27T09:26:38Z

FYI. Filed follow-up PR #20196 as it fails to run under WSL (Windows). (was running fine previously on Mac)

mattpollock · 2024-11-27T10:13:21Z

I'm running/testing on a Mac too - thanks for handling the windows issue.

mattpollock commented Nov 19, 2024

View reviewed changes

mattpollock changed the title ~~[r client] fix to-list and to-json functionality~~ [R] fix to-list and to-json functionality Nov 19, 2024

mattpollock commented Nov 21, 2024

View reviewed changes

mattpollock added 5 commits November 25, 2024 10:22

[r client] fix to-list and to-json functionality

9c52be0

fix type of json string

637e8e9

wip

0fba5cb

refactor pr

839ed04

regenerate samples

6b9cedd

mattpollock force-pushed the r-cli-list branch from 0e78426 to 6b9cedd Compare November 25, 2024 15:39

update to-dataframe example to use non-superceded tidyverse functions

90145b6

wing328 added Client: R Issue: Bug Enhancement: Code Cleanup General refactoring, removal of deprecated things, commenting, etc. labels Nov 26, 2024

wing328 added this to the 7.11.0 milestone Nov 26, 2024

wing328 reviewed Nov 27, 2024

View reviewed changes

fix typo

0ddf0bb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[R] fix to-list and to-json functionality #20132

[R] fix to-list and to-json functionality #20132

mattpollock commented Nov 19, 2024 •

edited

Loading

mattpollock Nov 19, 2024 •

edited

Loading

mattpollock Nov 19, 2024 •

edited

Loading

wing328 commented Nov 20, 2024

mattpollock commented Nov 20, 2024

mattpollock commented Nov 20, 2024

mattpollock Nov 21, 2024

mattpollock Nov 21, 2024

mattpollock commented Nov 25, 2024

wing328 commented Nov 26, 2024

wing328 Nov 27, 2024

mattpollock Nov 27, 2024

wing328 commented Nov 27, 2024

mattpollock commented Nov 27, 2024

		toJSON = function() {
		.Deprecated(new = "toSimpleType", msg = "Use the '$toSimpleType()' method instead since that is more learly named. Use '$toJSONString()' to get a JSON string")

[R] fix to-list and to-json functionality #20132

Are you sure you want to change the base?

[R] fix to-list and to-json functionality #20132

Conversation

mattpollock commented Nov 19, 2024 • edited Loading

PR checklist

mattpollock Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

mattpollock Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

wing328 commented Nov 20, 2024

mattpollock commented Nov 20, 2024

mattpollock commented Nov 20, 2024

mattpollock Nov 21, 2024

Choose a reason for hiding this comment

mattpollock Nov 21, 2024

Choose a reason for hiding this comment

mattpollock commented Nov 25, 2024

wing328 commented Nov 26, 2024

wing328 Nov 27, 2024

Choose a reason for hiding this comment

mattpollock Nov 27, 2024

Choose a reason for hiding this comment

wing328 commented Nov 27, 2024

mattpollock commented Nov 27, 2024

mattpollock commented Nov 19, 2024 •

edited

Loading

mattpollock Nov 19, 2024 •

edited

Loading

mattpollock Nov 19, 2024 •

edited

Loading