Allows port to be specified as an environment variable #963
Conversation
|
|
R/find-port.R
Outdated
| @@ -44,5 +44,5 @@ findPort <- function(port){ | |||
| stop("Unable to start a Plumber server. Either the port specified was unavailable or we were unable to find a free port.") | |||
| } | |||
|
|
|||
| port | |||
| as.integer(port) | |||
There was a problem hiding this comment.
The problem is that this doesn't fully handle validation of user input because all of the code above it assumes that port wasn't provided.
For example, given obviously bad user input both doesn't work AND doesn't error.
options(plumber.port = "blue"); plumber::pr_run(plumber::pr())
#> Running plumber API at http://127.0.0.1:NA
#> Running swagger Docs at http://127.0.0.1:NA/__docs__/There was a problem hiding this comment.
I refactored findPort() so that, in the missing port case we immediately hand over control to findRandomPort() which handles finding a random port.
This leaves the rest of findPort() to handle coercing user input to an integer and checking that it's a valid value.
There was a problem hiding this comment.
Right, Thanks. It seems I only addressed the narrower issue that (correct) ports specified via env var were failing. Didn't expect NA to be accepted as a port, should this not fail somewhere else? What happens if an out of port range integer is specified?
There was a problem hiding this comment.
Thanks @shikokuchuo, and yeah somehow NA would flow through because findPort() didn't currently do any validating of the port number. You're right, we should also check the valid port range, that wouldn't fail later either. I'll add that to this PR.
There was a problem hiding this comment.
Updated with further validation in cf5086c
* In 1024-49151 (following IANA service port numbers) * Not an unsafe port (using httpuv's unsafe port list)
* main: (28 commits) feat: add `excel` serializer/parser (#975) ci: Error on spelling (#981) docs: Improve forward docs a bit (#978) Fix file path tests on windows (#979) bug: Update default debug behaviour to `FALSE` (#976) perf: Avoid disk I/O during parsing (#972) docs: Add note on annotating required parameters (#971) Allows port to be specified as an environment variable (#963) tests: Replace `with_mock()` with `with_mocked_bindings()` (#970) feat(serializer): Add support for ragg and svglite (#964) chore: Fix pkgdown warnings. Use lifecycle inline R badges (#965) v1.2.2 release candidate (#948) bug: Use `{rlang}` instead of `{ellipsis}` (#958) docs: Update URLs (#954) docs: fix table spacing (#951) docs: change link to httr2 in routing and input vignette (#944) Specify interactive mode in doc (#932) Fall back to sys env var if R option doesn't exist (#934) Add support for quoted boundary for multipart request parsing. (#924) chore: Remove parse comment to match implementation (#931) ...
Fixes #956 (tagged 'help wanted').
The error returned there is an Rcpp error, as the httpuv function that's ultimately called is expecting an integer input and can't handle the implicit character -> integer conversion. Environment variables are by default retrieved as strings.
I've added the
as.integer()conversion as a catch-all, rather than add a conditional branch as:i) I find it slightly more readable
ii) slightly more defensive, in case the methods to find a random port above it are updated
iii)
getRandomPort()returns a double so there would have been an implicit conversion at the Rcpp level anywayIncludes a regression test.
PR task list:
devtools::document()