Work in progress, combined PR #536

meztez · 2020-05-17T05:57:24Z

I combined all the work done into one PR, it was getting hard to track all work across different branches.

This is still a work in progress.

What this branch contains:

Still to be done:

http error generalization ??
fromOpenAPI
modular plumb methods for plumbing files / custom tags
parser text/csv / serializer
Proof read docs once development stabilizes

Breaking:

swagger/swaggerCallback in run method of PlumberRouter :

Now there is a ui and callback param
ui can be TRUE, FALSE or character vector
It supports "Swagger" and "Redoc" (meztez/redoc, CRAN pending)
callback has the same function as before (RStudio)
PlumberRouter gains customSpec field for Custom OpenAPI (expect named list)
Removed deprecated code for major version release 5.0.0
Removed requestBody from req env

Why I broke stuff : I felt it was needed for 5.0.0 to make a clear split between the UIs (Swagger, Redoc) and OpenAPI specifications. You don't need to run a router to get the specs with your custom definitions in this PR. I also think the run method had too much clutter.
Custom definitions are applied using utils::modifyList and overwrite default plumber spec. This is a bit different than prior approach.

Open questions to maintainers:

Body parsing

Which dep? mimes, webutils, or write multipart parser?
When picking a body parser, what should happen when picker finds multiple matches? Currently uses the first indexed one
Normally nothing should read rook.input before postBodyFilter, rewind to be safe if change are made elsewhere?
Is it memory wise to store the entire body in req$requestBody when it contains whole files

Decide what to do with files sent to API

Should plumber let the user delete them?
Handling maxfilesize limit, with option ok?
Should it even write file sent to disk?
What should be the default max request body size (0 Unlimited?)

Misc

Get rid of Legacy methods in Plumber R6 object?
in R6 Class PlumberStep, should getPathParams, getTypedParams and getFunctParams return all in the format? Maybe a name change in methods here could help distinguish between getTypedParams and getPathParams?
Bump to R 4.0.0 already in droplet?

…#421

…ge_serializer

…oad. More swagger swag.

Merge remote-tracking branch 'upstream/master' into to_from_openapi # Conflicts: # R/plumber-response.R # man/plumber.Rd

Merge branch 'rm_pr_link_news' into to_from_openapi # Conflicts: # NEWS.md

schloerke · 2020-06-18T19:13:16Z

@meztez Do you mind remerging the master branch? (Sometime before this Monday)
Thank you!

meztez · 2020-06-18T19:14:43Z

I was thinking about splitting this one in smaller chunk since you merge #532 if it is ok with you. yes before monday.

schloerke · 2020-06-18T19:19:33Z

Even better! 🙌

Merge remote-tracking branch 'upstream/master' into to_from_openapi # Conflicts: # DESCRIPTION # NAMESPACE # NEWS.md # R/content-types.R # R/globals.R # R/plumb-block.R # R/plumber.R # R/post-body.R # R/query-string.R # R/swagger.R # inst/examples/15-swagger-spec/entrypoint.R # man/addParser.Rd # man/parsers.Rd # tests/testthat/test-postbody.R # tests/testthat/test-swagger.R

schloerke · 2020-06-22T19:04:01Z

R/openapi-types.R

+dataTypesInfo <- list()
+dataTypesMap <- list()


May we use a more specific name? Maybe apiTypesInfo? (sub("data", "api", .))

It's a lot of name changes, but it helps when reading to supply context

I was in the process a decoupling the mountui part and de renaming, reorganizing part.

Also wondering what would be a good name for dataType.

It refers to OpenAPI data types. If you think apiTypes would work, I'm down for it.

Don't worry about replacing stuff. Naming things is the hardest part.

Also wondering what would be a good name for dataType.

apiType helps a lot. It lets me know that it's prolly a string value describing the OpenAPI Spec type.

dataType feels like an output of typeof() or something unrelated to OpenAPI

I was in the process a decoupling the mountui part and de renaming, reorganizing part.

Great! Feel free to make those changes there. 😄

I am hoping to refer to all things OpenAPI by using api in the variable name.

(Things with plumber are pr or router.)

schloerke · 2020-06-22T19:05:34Z

R/plumber.R

+      ui = interactive(),
      debug = interactive(),
-      swaggerCallback = getOption('plumber.swagger.url', NULL)
+      callback = getOption('plumber.ui.callback', getOption('plumber.swagger.url', NULL)),
+      ...


Can we change the order? And set ui to TRUE.

..., ui = TRUE, debug = interactive(), callback = ....

schloerke · 2020-06-22T19:10:44Z

R/plumber.R

+      if (is.list(self$customSpec)) {
+        ret <- utils::modifyList(ret, self$customSpec)
+      }
      ret <- removeNaOrNulls(ret)
-
      ret
    },
-    #' @description Retrieve openAPI file
-    openAPIFile = function() {
-      self$swaggerFile()
-    },
+    #' @field customSpec A list of custom spec to overlay over openAPI spec
+    #' generated from a plumber router.
+    customSpec = NULL,


Rather than using a static object, I'd like to supply a function directly.

#### in the private defs apiHandler = force,

#' @description Add a function to customize what is returned in `$apiSpec()`. #' @param api_fun This function should accept an OpenAPI Specification autogenerated by `plumber`. It should return a list object in valid OpenAPI Specification format. This will not be validated. setApiHandler = function(api_fun = force) { # allow null values if (!is.null(api_fun)) { stopifnot(is.function(api_fun)) } private$apiHandler <- api_fun }

schloerke · 2020-06-22T19:12:08Z

R/plumber.R

-      # Lay those over the default globals so we ensure that the required fields
-      # (like API version) are satisfied.
+    #' @description Retrieve OpenAPI specification
+    openAPISpec = function() {


I think it's a little shorter and less confusing with the caps

Suggested change

openAPISpec = function() {

apiSpec = function() {

In the documentation, we can always refer to it as the OpenAPI Specification function

schloerke · 2020-06-22T19:13:10Z

R/plumber.R

-
-      # Lay those over the default globals so we ensure that the required fields
-      # (like API version) are satisfied.
+    #' @description Retrieve OpenAPI specification


They seem to like Specification to be capitalized

The OpenAPI Specification (OAS) defines ....

Suggested change

#' @description Retrieve OpenAPI specification

#' @description Retrieve OpenAPI Specification

schloerke · 2020-06-22T19:13:51Z

R/plumber.R


    ### Legacy/Deprecated
+    #' @description Retrieve OpenAPI specification (deprecated and will be removed in a coming release)
+    openAPIFile = function() {
+      self$openAPISpec()


Suggested change

self$openAPISpec()

warning("`$openAPIFile()` has been deprecated in v1.0.0 and will be removed in a coming release. Please use `$apiSpec()`.")

self$openAPISpec()

schloerke · 2020-06-22T19:14:13Z

R/plumber.R

+      self$openAPISpec()
+    },
+    #' @description Retrieve OpenAPI specification (deprecated and will be removed in a coming release)
+    swaggerFile = function() {


Suggested change

swaggerFile = function() {

warning("`$swaggerFile()` has been deprecated in v1.0.0 and will be removed in a coming release. Please use `$apiSpec()`.")

swaggerFile = function() {

schloerke · 2020-06-22T19:14:59Z

R/plumber.R

@@ -1071,7 +985,7 @@ plumber <- R6Class(
      private$ends[[preempt]] <- c(private$ends[[preempt]], ep)
    },

-    swaggerFileWalkMountsInternal = function(router, parentPath = "") {
+    routerSpecificationInternal = function(router, parentPath = "") {


Thank you for renaming these variables! 😃

meztez · 2020-06-23T13:37:53Z

By adding #562, this one can die in peace. Goodbye shemera PR.

Bruno Tremblay and others added 30 commits March 29, 2020 18:40

adding an image serializer to respect serializer behavior

5214cc9

update news

81cff55

Fix bug in setting openapi.json url when HTTP_REFERER is null rstudio…

f9d713d

…#421

come up with sensible solution for openapi server url

6fee9b9

update tests

82d92b3

update r version

300f370

Update NEWS.md

6fa4026

Update NEWS.md

e79b5b7

Update NEWS.md

d0a70e8

update doc R6, fix docs check

1111740

add libsodium-dev

6569d8c

switch to remotes

cf01475

missed a file there

66bee80

Merge branch 'master' into image_serializer

711a4f3

fix test

9fc8b30

fix test

e30b9bb

Merge branch 'image_serializer' of github.com:meztez/plumber into ima…

1527bce

…ge_serializer

fix test

948a119

fix test

dc760a5

fix test

640909c

work on support for [] array syntax and object type

499bc1d

Added tests, finish adding features

556695e

its the news

22e0766

change minItems to match required

52d27bc

Added plumberExpression args metadata detection, support for file upl…

0047692

…oad. More swagger swag.

fix for linux

5fa61bc

generalize body parsing, big chunk

877819f

few tweaks, early morning, late night polish

c1644e7

rds parser and filename extension detection

574221e

perf improvement, recursive parseRaw for multipart

37a70db

Tremblay added 5 commits June 12, 2020 11:46

coma, coma, coma, coma, coma chameleon

0c629fd

fix doc

2a26e32

Remove PR/Issue number from News

eb463f4

merge master

92787c5

Merge remote-tracking branch 'upstream/master' into to_from_openapi # Conflicts: # R/plumber-response.R # man/plumber.Rd

merge news

61984e7

Merge branch 'rm_pr_link_news' into to_from_openapi # Conflicts: # NEWS.md

Bruno Tremblay added 11 commits June 21, 2020 22:12

remove magrittr stumb

d80cb7b

remove magriitr

07ca0fe

remove plumber-options

44d8da5

remove todo

2a93ac6

remove yaml support

2a362b0

remove from to openapi

9a78482

mount one ui only

500978e

update doc

f35e53e

remove duplicate test

94515e4

expects_warning

cbe9376

schloerke reviewed Jun 22, 2020

View reviewed changes

meztez closed this Jun 23, 2020

meztez deleted the to_from_openapi branch June 24, 2020 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Work in progress, combined PR #536

Work in progress, combined PR #536

meztez commented May 17, 2020 •

edited

Loading

schloerke commented Jun 18, 2020

meztez commented Jun 18, 2020

schloerke commented Jun 18, 2020

schloerke Jun 22, 2020

meztez Jun 22, 2020

schloerke Jun 22, 2020

schloerke Jun 22, 2020

schloerke Jun 22, 2020

schloerke Jun 22, 2020

schloerke Jun 22, 2020

schloerke Jun 22, 2020

schloerke Jun 22, 2020 •

edited

Loading

schloerke Jun 22, 2020

schloerke Jun 22, 2020

meztez commented Jun 23, 2020

	#' @description Retrieve OpenAPI specification
	#' @description Retrieve OpenAPI Specification

	self$openAPISpec()
	warning("`$openAPIFile()` has been deprecated in v1.0.0 and will be removed in a coming release. Please use `$apiSpec()`.")
	self$openAPISpec()

	swaggerFile = function() {
	warning("`$swaggerFile()` has been deprecated in v1.0.0 and will be removed in a coming release. Please use `$apiSpec()`.")
	swaggerFile = function() {

		dataTypesInfo <- list()
		dataTypesMap <- list()

Work in progress, combined PR #536

Work in progress, combined PR #536

Conversation

meztez commented May 17, 2020 • edited Loading

What this branch contains:

Still to be done:

Breaking:

Open questions to maintainers:

schloerke commented Jun 18, 2020

meztez commented Jun 18, 2020

schloerke commented Jun 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schloerke Jun 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meztez commented Jun 23, 2020

meztez commented May 17, 2020 •

edited

Loading

schloerke Jun 22, 2020 •

edited

Loading