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

Use bytes charset for multipart input #504

Closed
wants to merge 3 commits into from
Closed

Conversation

krlmlr
Copy link

@krlmlr krlmlr commented Jan 6, 2020

Closes #75.

My fork of @sellorm's demo at https://github.com/krlmlr/plumber-uploader/tree/f-multipart contains an example of reliably processing multipart data using mime::parse_multipart(). This PR adds special treatment for multipart input.

Pull Request

Before you submit a pull request, please do the following:

  • Add an entry to NEWS concisely describing what you changed.

  • Add unit tests in the tests/testthat directory.

  • Run Build->Check Package in the RStudio IDE, or devtools::check(), to make sure your change did not add any messages, warnings, or errors.

Doing these things will make it easier for the plumber development team to evaluate your pull request. Even so, we may still decide to modify your code or even not merge it at all. Factors that may prevent us from merging the pull request include:

  • breaking backward compatibility
  • adding a feature that we do not consider relevant for plumber
  • is hard to understand
  • is hard to maintain in the future
  • is computationally expensive
  • is not intuitive for people to use

We will try to be responsive and provide feedback in case we decide not to merge your pull request.

Minimal reproducible example

Finally, please include a minimal reprex. The goal of a reprex is to make it as easy as possible for me to recreate your problem so that I can fix it. If you've never heard of a reprex before, start by reading https://github.com/jennybc/reprex#what-is-a-reprex, and follow the advice further down the page. Do NOT include session info unless it's explicitly asked for, or you've used reprex::reprex(..., si = TRUE) to hide it away.

reprex::reprex({
  library(plumber)
  # insert reprex here

})

Delete these instructions once you have read them.


Brief description of the original problem and the approach behind your solution.

# insert reprex here

PR task list:

  • Update NEWS
  • Add tests
  • Update documentation with devtools::document()

@CLAassistant
Copy link

CLA assistant check
All committers have signed the CLA.

@meztez
Copy link
Collaborator

meztez commented May 5, 2020

See #532 as it opens up a whole lot more possibility and parse multipart correctly. I don't think we should let the end user deals with the bytes, it should just work ™.

A generalized approach to input parser would be more friendly to developer.

@schloerke
Copy link
Collaborator

@krlmlr Thank you for getting the conversation started! I believe @meztez has covered this in #532.

I ran your example with the current master branch (which has #532 merged) and I believe it worked as expected. I ran it with the current CRAN version and had unexpected behavior involving queryString.

Closing this PR.

@schloerke schloerke closed this Jun 18, 2020
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 this pull request may close these issues.

Provide a clean way to upload files via an endpoint
4 participants