Skip to content

config::get() silently returns the default profile when the name is unknown #59

Description

@andrie

The function config::get(config = "some-name") does not check that some-name is present in the file. An
unrecognised name yields the default profile, with no error and no warning, so asking for a
profile that does not exist is indistinguishable from asking for default.

Because default is conventionally the production profile, the failure mode is that a typo
quietly selects production and returns plausible results.

Reproducible example

d <- tempfile(); dir.create(d); setwd(d)

writeLines(c(
  "default:",
  "  dsn: PRODUCTION-DATABASE",
  "staging:",
  "  dsn: STAGING-DATABASE"
), "config.yml")

for (p in c("default", "staging", "stagng", "STAGING", "", "nonexistent")) {
  cat(sprintf("  config = %-13s -> %s\n", encodeString(p, quote = '"'),
              config::get("dsn", config = p)))
}
  config = "default"     -> PRODUCTION-DATABASE
  config = "staging"     -> STAGING-DATABASE
  config = "stagng"      -> PRODUCTION-DATABASE
  config = "STAGING"     -> PRODUCTION-DATABASE
  config = ""            -> PRODUCTION-DATABASE
  config = "nonexistent" -> PRODUCTION-DATABASE

Only the first two names exist in the file. The other four all return the production value.
Profile names are also case-sensitive, so "STAGING" misses.

R_CONFIG_ACTIVE takes the same path, which is the variant most likely to reach production:

  R_CONFIG_ACTIVE="staging" -> STAGING-DATABASE
  R_CONFIG_ACTIVE="stagng"  -> PRODUCTION-DATABASE
  R_CONFIG_ACTIVE=""        -> PRODUCTION-DATABASE

A mistyped deployment variable does not fail the job; it points it at default.

config 0.3.2, R 4.5.2.

Why this is worth an error

Beyond the typo case, two situations where the silence is expensive:

  • Renaming a profile. Rename staging to stage and miss a call site: that call site keeps
    working, against default, rather than failing. The refactor cannot be verified by running the
    code, because the wrong answer is a successful answer.
  • Deployment. R_CONFIG_ACTIVE is typically set once in an environment that is not part of
    the code under review, so the mistake is not visible where people are looking.

Nothing in the returned value indicates which profile produced it.

Mechanism

do_get <- function(config, inherited = c()) {
  ...
  active_config <- config_yaml[[config]]
  ...
}
active_config <- merge_lists(default_config, do_get(config))

config_yaml[[config]] is NULL for a name that is not a key in the file, and
merge_lists(x, NULL) returns x, so the result is default_config unchanged. The requested
name is never compared against names(config_yaml).

A missing file is already an error, with a good message about parent directories. A missing
profile is not, which reads more like an oversight than a decision.

Suggested fix

Error when the requested name is absent from the file, using the known valid set in the message:

Unknown configuration 'stagng'. config.yml defines: default, staging.

Two things to decide:

  • inherits: targets would want the same validation, or the silence just moves down a level.
  • Backward compatibility: code may exist that relies on an unknown name meaning "the default",
    though that seems more likely to be latent bugs than deliberate use. A warning first, an error
    in a later release, would be the gentler route.

This would also resolve the ambiguity behind #23 ("any way to know if entry is default?"), which
is the same problem from the other direction: today there is no way to tell "I asked for default"
apart from "I asked for something that does not exist".

Happy to send a PR if the approach and the compatibility call suit you.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions