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

add content Type and Disposition #455

Closed
wants to merge 14 commits into from
Closed

add content Type and Disposition #455

wants to merge 14 commits into from

Conversation

ycphs
Copy link
Contributor

@ycphs ycphs commented Jul 3, 2019

reprex::reprex({
  library(plumber)
library(openxlsx)
#* @serializer contentTypeDisposition list(type="application/vnd.openxmlformats-officedocument.spreadsheetml.sheet",disposition='attachment; filename="tmp.xlsx"')

#* @get /xlsx
function(){
   tmp <- tempfile(fileext = ".xlsx")
  data.frame(a=1:10,b=11:20)
write.xlsx(erg,tmp,withFilter=T)
  
  readBin(tmp, "raw", n=file.info(tmp)$size)
}
})

The serializer contentType returned named files without extension.

 library(plumber)
library(openxlsx)
#* @serializer contentType list(type="application/vnd.openxmlformats-officedocument.spreadsheetml.sheet")

#* @get /xlsx
function(){
   tmp <- tempfile(fileext = ".xlsx")
  data.frame(a=1:10,b=11:20)
write.xlsx(erg,tmp,withFilter=T)
  
  readBin(tmp, "raw", n=file.info(tmp)$size)
}

@CLAassistant
Copy link

CLAassistant commented Jul 3, 2019

CLA assistant check
All committers have signed the CLA.

@ycphs
Copy link
Contributor Author

ycphs commented Jul 3, 2019

corrected documentation

@codecov-io
Copy link

codecov-io commented Jul 3, 2019

Codecov Report

Merging #455 into master will increase coverage by 0.13%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
+ Coverage   89.84%   89.98%   +0.13%     
==========================================
  Files          29       29              
  Lines        1527     1537      +10     
==========================================
+ Hits         1372     1383      +11     
+ Misses        155      154       -1
Impacted Files Coverage Δ
R/serializer-content-type.R 95.23% <100%> (+4.32%) ⬆️
R/response.R 100% <0%> (ø) ⬆️
R/cookie-parser.R 100% <0%> (+6.25%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a159aa6...ce5933c. Read the comment docs.

ycphs added 2 commits Jul 3, 2019
Add test for content type disposition
tests/testthat/test-serializer.R Outdated Show resolved Hide resolved
tests/testthat/test-serializer.R Outdated Show resolved Hide resolved
tests/testthat/test-serializer.R Outdated Show resolved Hide resolved
@schloerke
Copy link
Collaborator

schloerke commented Jul 5, 2019

@ycphs Thank you for the PR and tests!

With content disposition being tied to content type, how do you feel about making content_type serializer better vs making a new method?

# untested
serializer_content_type <- function(type, disposition = NULL, filename = NULL) {
  if (missing(type)) {
    stop("You must provide the custom content type to the serializer_content_type_disposition")
  }

  # https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition
  if (!is.null(disposition)) {
    disposition <- match.arg(disposition, c("inline", "attachment"))
    if (disposition == "attachment" && !is.null(filename)) {
      filename <- as.character(filename)
      if (grepl("\"", filename, fixed = TRUE)) {
        stop("quotes can not be used in the value of `filename`")
      }
      disposition <- paste0(disposition, "; filename=\"", filename, "\"")
    }
  }

  function(val, req, res, errorHandler) {
    tryCatch({
      res$setHeader("Content-Type", type)
      if (!is.null(disposition)) {
        res$setHeader("Content-Disposition", disposition)
      }
      res$body <- val

      return(res$toResponse())
    }, error=function(e) {
      errorHandler(req, res, e)
    })
  }
}

@ycphs
Copy link
Contributor Author

ycphs commented Jul 10, 2019

@schloerke: Yes for me that's also good.

Do you change it yourself or should I change it?

@ycphs
Copy link
Contributor Author

ycphs commented Jul 15, 2019

@schloerke I updated the method and tests.

@ycphs ycphs closed this Jul 30, 2019
@ycphs ycphs reopened this Jul 30, 2019
@ycphs
Copy link
Contributor Author

ycphs commented Jul 30, 2019

change according of appveyor.yml to: Provide more advice re: env vars #147

@schloerke schloerke requested a review from cpsievert Jun 18, 2020
@cpsievert
Copy link
Contributor

cpsievert commented Jun 18, 2020

@schloerke

  • consider generalizing this so that other exported serializers can take advantage

  • consider adding a withHeaders serializer

@schloerke schloerke added the Fixed in PR A PR has implemented a fix for this Issue label Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed in PR A PR has implemented a fix for this Issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants