add conditional header split

[DavZim]

This PR adds a first (discussion) version to #187. It implements an unordered set in cpp which keeps track of all headers which can be split (I didnt find a comprehensive list nor extensive documentation for each header which lists if it can have multiple elements, instead I looked through multiple examples (see comment in code) and took the ones which have examples with multiple elements).

As mentioned in the issue, I found no easy way to test this properly, as app$process_request() does not call parse_headers(). Instead I used the manual approach:

library(RestRserve)
app = Application$new()
app$add_get("/foo", FUN = function(.req, .res) {
  print("/foo")
  print(.req$headers)
  .res$set_body(42)
  .res$set_status_code(200)
})

backend = BackendRserve$new()
backend$start(app, http_port = 8080)

and then using CURL I test the following:

curl http://localhost:8080/foo -H "If-Modified-Since: Tue, 15 Mar 2022 10:27:07 GMT" -H "Accept: abc, def, ghi"

which results in the following output in the logs of the Rsession:

[1] "/foo"
$accept
[1] "abc" "def" "ghi"

$`if-modified-since`
[1] "Tue, 15 Mar 2022 10:27:07 GMT"

$`user-agent`
[1] "curl/7.79.1"

$host
[1] "localhost:8080"

As we can see, the Accept header is split, but the If-Modified-Since header is not split.

Let me know what you think about this!

[codecov[bot]]

Codecov Report

Merging #189 (88a932f) into dev (852785a) will increase coverage by 0.10%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev     #189      +/-   ##
==========================================
+ Coverage   94.86%   94.97%   +0.10%     
==========================================
  Files          28       28              
  Lines        1364     1392      +28     
==========================================
+ Hits         1294     1322      +28     
  Misses         70       70              

Impacted Files	Coverage Δ
R/Application.R	96.71% <ø> (ø)
R/BackendRserve.R	94.84% <100.00%> (+0.10%)	:arrow_up:
R/ETagMiddleware.R	92.20% <100.00%> (+2.37%)	:arrow_up:
src/parse_headers.cpp	100.00% <100.00%> (ø)

---

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 852785a...88a932f. Read the comment docs.

[artemklevtsov]

Code is ok. Need tests. I'm not sure if it's worth letting the user control this.

[DavZim]

Agree on the tests, do you know how we can test this?

[artemklevtsov]

Please look at here: https://github.com/rexyai/RestRserve/blob/master/inst/tinytest/test-parse-headers.R

[dselivanov]

I think it worth to make list of splittable headers public/ not hardcoded. So it is possible to modify it before backend starts.

[DavZim]

With the latest commits, the user can specify the headers to split, the entry point for that is the BackendRserve class, which gains the headers_to_split argument, also the user can access the http_headers_to_split_default() function which is used to find which headers to split by default.

A quick example would look like this:

library(RestRserve)
app = Application$new()
app$add_get("/foo", FUN = function(.req, .res) {
  print("/foo")
  print(.req$headers)
  .res$set_body(42)
  .res$set_status_code(200)
})

backend = BackendRserve$new(headers_to_split = c("test-header", "accept"))
backend$start(app, http_port = 8080)

Now we can test it from curl with

curl http://localhost:8080/foo -H "test-header: abc, def, ghi" -H "Accept: abc, def, ghi" -H "te: now,te,is,not,split"

[DavZim]

Any updates on this? Are there any further tasks that need to be done before we can merge this? Thanks

[dselivanov]

@DavZim I've pushed some updates which move split header to standard R options. Also I'm thinking where to put documentation for such things. Suggestions?

[DavZim]

Do you mean long form of documentation such as a vignette or shorter: a "chapter" in the readme?

The functionality itself is documented in Backend, right?

[dselivanov]

Short paragraph. New "split headers" option in theory should hold same logic for all backends (if for example we add httpuv).