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.

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

Make body parsers opt in #586

Closed
schloerke opened this issue Jul 7, 2020 · 9 comments · Fixed by #584
Closed

Make body parsers opt in #586

schloerke opened this issue Jul 7, 2020 · 9 comments · Fixed by #584
Milestone

Comments

@schloerke
Copy link
Collaborator

schloerke commented Jul 7, 2020

Related #584

Goals

  • Users must provide parsers that they accept.
  • Each parser must be defined by a single line containing
  • Each parsing function should have a name when registered.
  • Allow for functions to be passed in directly.
  • Add docs for rds that rds parsing should only be done from a trusted source. Do not accept rds blindly.
  • Should a helper method be added for the return type of each parser? function(mime_type, parser){}

/upload_white_list is an example of how we could define 4 parsers for this route. By default, @parse json will be used.

/upload_anything should be allowed to use any parser that is registered. (Not recommended)

#* Parse Example
#*
#* @parse csv
#* @parse rds
#* @parse parser_json(simplifyVectors = FALSE) # returns a mime type and function
#* @parse plumber::parse_tsv(delim = "\t")
#* @post /upload_white_list
function(...) {
  # do stuff
  my_objects <- list(...)

  "parsed"
}

#* Opt in to parsing everything
#* @parse anything
#* @post /upload_anything
function(...) {
  # do stuff
  my_objects <- list(...)

  "parsed"
}


## Example
parse_xml <- function(...) {
  list(
    # mime type regex
    mime_type = "application/xml-*", # matches any thing that starts with `xml`
    parser = function(value, content_type = NULL, filename = NULL, ...) {
      # parse the xml value
    }
  )
}

cc @jcheng5

@schloerke schloerke added this to the v0.5.0 - Next CRAN release milestone Jul 7, 2020
@meztez
Copy link
Collaborator

meztez commented Jul 8, 2020

todo
map parsers in plumber step like serializers
plumb parser in plumb block using second part of regex to lookup in parser map, if null use as is
link parsers to req in route
get parsers from req in parse picker.

@schloerke
Copy link
Collaborator Author

map parsers in plumber step like serializers

  • Allow for many parsers to exist

plumb parser in plumb block using second part of regex to lookup in parser map, if null use as is

  • Allow for both name and function call
    Maybe if we can not find the name, we try to evaluate it? The function itself may exist.

link parsers to req in route
get parsers from req in parse picker.

Don't know if it'd make sense to have a list of valid parsers added to the endpoint.

Seems odd that we're traversing all paths. Could it be added as a pre-route for the particular endpoint? (Idk if it'll work.)


@meztez Are you looking to make a PR for this? If so, mind making it onto #584 ?

@meztez
Copy link
Collaborator

meztez commented Jul 8, 2020

@schloerke Yes

@meztez
Copy link
Collaborator

meztez commented Jul 8, 2020

Are you ok with the current behavior?

Parsers (and query filter by extension) are executed before we reject a request because we can't serve it.

Also
Im wondering if parsers should be restricted at the endpoint level or at the router level. My instinct says endpoint but that would mean displacing current filters execution model. First check which endpoint we are dealing with than parse, instead of the other way around.

Another alternative is to remove parsing from the postBodyFilter and to do it in a pre-exec step on req$postBodyRaw

@schloerke
Copy link
Collaborator Author

I agree that creating the req$postBodyRaw or parsing the the query string should always be done.

Parsing using a particular parser should be an endpoint thing. (Something that is opt-in.) Doing this as a pre exec (or some pre- step) sounds like a good idea!

@meztez
Copy link
Collaborator

meztez commented Jul 9, 2020

> #* @parse parser_json(simplifyVectors = FALSE) # returns a mime type and function
> #* @parse plumber::parse_tsv(delim = "\t")

I think you should only parse something like

> #* @parse json list(simplifyVectors = FALSE)
> #* @parse tsv list(delim = "\t")

Evaluating function from the plumb block would require that we eval everything outside blocks before evaluating the block itself to make sure something that is defined elsewhere in the plumb file is picked up.

@schloerke
Copy link
Collaborator Author

Yes. We could do it similar to @serializer and use a list to be applied.

#’ @parse json list(simplifyVectors = FALSE)

So @parse then a name. Then maybe a list of arguments to apply

@meztez
Copy link
Collaborator

meztez commented Jul 9, 2020

I have a working prototype, I'll commit there until i've dealt with tests.
https://github.com/meztez/plumber/tree/post_body_content_type

@schloerke
Copy link
Collaborator Author

Thank you for the help, @meztez !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants