removeNAOrNulls breaks on no swagger spec

[schloerke]

From #417

pr <- plumber$new()
pr$handle("GET", "/:path/here", function(){})

pr$run(
  port = 1234,
  swagger = function(pr_, spec, ...) {
    spec$info$title <- Sys.time()
    spec
  }
)

Visiting http://127.0.0.1:1234/openapi.json causes an error

<simpleError in if (any(toRemove)) {    x[toRemove] <- NULL}: missing value where TRUE/FALSE needed>

[meztez]

I believe it is because it evaluates an empty character string (url) in isNaOrNull. I temporarily traced the function but i'm not sure it is the best way to handle it. Any comments? I could submit a pull request.

Browse[1]> x
$url
character(0)

$description
[1] "OpenAPI"
Browse[1]> toRemove
        url description 
         NA       FALSE 
Browse[1]> isNaOrNull(x)
[1] FALSE
Browse[1]> vapply(x, isNaOrNull, logical(1))
        url description 
         NA       FALSE 
Browse[1]> logical(1)
[1] FALSE
Browse[1]> isNaOrNull
function (x) 
{
    isNa(x) || is.null(x)
}
<bytecode: 0x000001ab119c7428>
<environment: namespace:plumber>
Browse[1]> isNa(x$url)
logical(0)
Browse[1]> is.null(x$url)
[1] FALSE
Browse[1]> logical(0)
logical(0)
Browse[1]> logical(0) || FALSE
[1] NA

isNaOrNull traced

function (x) 
{
  ret <- isNa(x) || is.null(x)
  if (is.na(ret)) {
    return(FALSE)
  }
  else {
    return(ret)
  }
}

It could probably check the length of x too

[meztez]

Maybe a change in isNA?

> plumber:::isNa
function (x) 
{
    if (is.list(x) || length(x)==0) {
        return(FALSE)
    }
    is.na(x)
}
<bytecode: 0x000001ab119c6b68>
<environment: namespace:plumber>

[schloerke]

@meztez

A PR would be great!

Two things come to mind.

1. Yes, we should check for length. I believe if the length is 0, it should be removed.
2. The default url is coming in as character(0). The url is a required field. I believe this should default to http://127.0.0.1:PORT so that it isn't removed. I don't know why that isn't happening already.

Thank you for the help!

[schloerke]

Weird. The url is being supplied by the referrer. This should not be missing. Maybe a header name changed. If not, we should set a sensible default.

https://github.com/trestletech/plumber/blob/6e3417e34d8a6da260fbd8d86d61043eae0c546e/R/plumber.R#L268-L276

[meztez]

I'll look into it and submit something sensible.

[meztez]

Notes :

So this path http://127.0.0.1:8004/__swagger__/ has a HTTP_REFERER and this one http://127.0.0.1:8004/openapi.json has not (it is NULL). Might be something in httpuv, down the rabbit hole I go.

[schloerke]

Hmmmmm before we go down that route, it might be a logical flaw.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Referer

A referrer is an optional header. Maybe the url should be be fixed to be http://127.0.0.1:PORT as that is the URL location of the server. (Or set with an option. I can add the option later if you get the default url location.)

[schloerke]

A Referer header is not sent by browsers if:

• The referring resource is a local "file" or "data" URI.
• An unsecured HTTP request is used and the referring page was received with a secure protocol (HTTPS).

Hmmmm seems like it should still be sent. 😖

[meztez]

Seems to me like http://127.0.0.1:8004/openapi.json is a local file, maybe that's why.

[meztez]

I'm not sure about adding something to the news since it is a bug on a feature that has not been released yet. Still figuring out a way to test it. Probably later today.

[schloerke]

Walking through the url location with @alandipert, we believe it should set the url value in spec$servers to

url = urlHost(
  scheme = getOption("plumber.apiScheme", "http://"),
  host = getOption("plumber.apiHost", host), 
  port = port, 
  path = getOption("plumber.apiPath", "/"),
  changeHostLocation = TRUE
)

(requires an update to urlHost to pass in a scheme value and path value)

This would behave similar to the swagger console message (which also need to be updated to respect the complete undocumented plumber.apiPath option.)

[meztez]

I see, do you think the options should have priority over the values set by the run method?

i.e. in the plumber message, only the host is provided to urlHost and for the swaggerUrl, host is getOption("plumber.apiHost", host)?

https://github.com/trestletech/plumber/blob/6e3417e34d8a6da260fbd8d86d61043eae0c546e/R/plumber.R#L230-L236

https://github.com/trestletech/plumber/blob/6e3417e34d8a6da260fbd8d86d61043eae0c546e/R/plumber.R#L240

https://github.com/trestletech/plumber/blob/6e3417e34d8a6da260fbd8d86d61043eae0c546e/R/plumber.R#L310

Should there be default value for scheme and path in urlHost function?

I'm trying to get a sense of the intended uses for the options since they are not documented yet.

All the above methods work, just want to make it consistent with your views.

[schloerke]

So mental docs for this...

---

getOptions('plumber.swagger.url') is an option that can be defined to submit the final swagger url location. This is useful inside rstudio >= v1.2 when you run a plumber api as it will open the swagger location right away.

---

message("Running plumber API at ", urlHost(host, port, changeHostLocation = FALSE))

This is a message only to the console. It should state where the actual httpuv server is running. Such as 0.0.0.0:1234

urlHost(getOption("plumber.apiHost", host), port, changeHostLocation = TRUE),

This is a message only to the console to set up the swagger location. Locations like 0.0.0.0:1234 are not good to visit in the browser, so they are converted to 127.0.0.1:1234. This still does not contain a path as it is using the raw httpuv server.

---

The setup in my comment above is for the server url within the swagger spec. In the default situation with a 0.0.0.0 host (with no options set), the final result would return 0.0.0.0:1234/

Plumber can be run at 0.0.0.0 behind an nginx which is actually going to be functioning under https://somewebsite.com/sub/path/here (ex: RSConnect). For this with, options set, the final server location should be set to https://somewebsite.com:80/sub/path/here even though it is being executed at 0.0.0.0:1234.

---

Do you think the options should have priority over the values set by the method?

Yes.

Should there be a default value for scheme and path in urlHost function?

Yes .http:// and /.

---

just want to make it consistent with your views.

Thank you. But I'm also wondering if it should be simplified.

Should it be something more like:

swagger_server_url <-
  getOption(
    "plumber.openapi.server.url", # not a fixed name
    urlHost(
      scheme = getOption("plumber.apiScheme", "http://"),
      host = getOption("plumber.apiHost", host), 
      port = port, 
      path = getOption("plumber.apiPath", "/"),
      changeHostLocation = TRUE
    )
  )

It would support the legacy plumber.api* options, but it could also be set with a single option of plumber.openapi.server.url.

Thoughts?

[meztez]

I'll look into it when I'm back at work. I might need I little bit more time to get familiar with the whole source code before having a clear opinion.

At least, if it uses options, documentation should reflect it to manage expectation. I'm just not sold on the idea of having an option value that overrides what would be set in plmber$run(...). If that's the case, then maybe the plumber$run method does not need that many parameter to begin with?

[schloerke]

Plumber needs all the current parameters of host and port for httpuv. I think the goal is to have a separation between where the actual host/port is being executed with httpuv and where swagger is saying the server url is located.

Looking at it some more, we might need to use getOption("plumber.apiPort", port) to round out the behavior.

[schloerke]

I believe this has been fixed in one of the PRs above. Closing