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

Attempting to use options package. #936

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

meztez
Copy link
Collaborator

@meztez meztez commented Nov 28, 2023

Pending a doc ordering issue in options package.

dgkf/options#7

PR task list:

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

@meztez meztez marked this pull request as draft November 29, 2023 20:01
@meztez
Copy link
Collaborator Author

meztez commented Nov 30, 2023

I'm thinking using the options package would maybe open up an attack vector, because of this line here:
https://github.com/dgkf/options/blob/a2c110229f844de49b3dbb56bb0a19542b92899a/R/envvars.R#L168

I'm going to ice this one for now.

@schloerke
Copy link
Collaborator

@meztez Currently, envvar_eval_or_raw is a default parameter to options::option_spec(). We could easily supply a safe version when defining the plumber option spec and we'd be fine.

@schloerke
Copy link
Collaborator

The option_spec() handles the extra args supplied to define_option.character().

So we could use a helper function to define the options (setting the envvar_fn value for every option):

# Copied from https://github.com/dgkf/options/blob/a2c1102/R/envvars.R#L1-L4
# Sets up pretty printing
fn_with_desc <- function(f, desc) {
  attr(f, "desc") <- desc
  f
}
# Keep sys env var as raw string only
plumber_envvar_raw <- fn_with_desc(
  function(raw, name, ...) {
    raw
  },
  "string"
)
define_plumber_option <- function(...) {
  options::define_option(
    ...,
    envvar_fn = plumber_envvar_raw
  )
}
# Ex usage
define_plumber_option(
  option = "port",
  default = NULL,
  desc = paste(
    "Port Plumber will attempt to use to start http server.",
    "If the port is already in use, server will not be able to start."
  )
)

@meztez
Copy link
Collaborator Author

meztez commented Nov 30, 2023

Noted, still have to figure out what to do with options_plumber, return values. Probably do not want to change the behavior. Options does not have an opt<- equivalent yet. Integrating httpproblems at the moment.

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.

2 participants