merge upstream

Author: simonpcouch

NEWS.md

@@ -1,5 +1,8 @@
 # odbc (development version)
 
+
+* snowflake: Runtime driver configuration checks on `MacOS` (#857).
+
 * Separate column content and name encoding by adding
   a new `name_encoding` argument to `dbConnect` to complement
   the existing `encoding` parameter (#845).

R/driver-databricks.R

@@ -17,6 +17,10 @@ NULL
 #' Workbench or the Databricks CLI on desktop.
 #' All of these credentials are detected automatically if present using
 #' [standard environment variables](https://docs.databricks.com/en/dev-tools/auth.html#environment-variables-and-fields-for-client-unified-authentication).
+#' In addition, on macOS platforms, the `dbConnect()` method will check
+#' for irregularities with how the driver is configured,
+#' and attempt to fix in-situ, unless the `odbc.no_config_override`
+#' environment variable is set.
 #'
 #' @inheritParams DBI::dbConnect
 #' @param httpPath,HTTPPath To query a cluster, use the HTTP Path value found
@@ -57,7 +61,6 @@ NULL
 #' }
 #' @export
 databricks <- function() {
-  configure_spark()
   new("DatabricksOdbcDriver")
 }
 
@@ -99,6 +102,9 @@ setMethod("dbConnect", "DatabricksOdbcDriver",
       session = session,
       ...
     )
+    # Perform some sanity checks on MacOS
+    configure_simba(spark_simba_config(args$driver),
+      action = "modify")
     inject(dbConnect(odbc(), !!!args))
   }
 )
@@ -343,3 +349,25 @@ workbench_databricks_token <- function(host, cfg_file) {
   }
   token
 }
+
+# p. 44 https://downloads.datastax.com/odbc/2.6.5.1005/Simba%20Spark%20ODBC%20Install%20and%20Configuration%20Guide.pdf
+spark_simba_config <- function(driver) {
+  spark_env <- Sys.getenv("SIMBASPARKINI")
+  if (!identical(spark_env, "")) {
+    return(spark_env)
+  }
+  common_dirs <- c(
+    driver_dir(driver),
+    "/Library/simba/spark/lib",
+    "/etc",
+    getwd(),
+    Sys.getenv("HOME")
+  )
+  return(list(
+    path = list.files(
+      common_dirs,
+      pattern = "simba\\.sparkodbc\\.ini$",
+      full.names = TRUE),
+    url = "https://www.databricks.com/spark/odbc-drivers-download"
+  ))
+}

R/driver-snowflake.R

@@ -98,7 +98,10 @@ setMethod("odbcDataType", "Snowflake",
 #'
 #' In particular, the custom `dbConnect()` method for the Snowflake ODBC driver
 #' detects ambient OAuth credentials on platforms like Snowpark Container
-#' Services or Posit Workbench.
+#' Services or Posit Workbench.  In addition, on macOS platforms, the
+#' `dbConnect` method will check and warn if it detects irregularities with
+#' how the driver is configured, unless the `odbc.no_config_override`
+#' environment variable is set.
 #'
 #' @inheritParams DBI::dbConnect
 #' @param account A Snowflake [account
@@ -189,6 +192,9 @@ setMethod(
       session = session,
       ...
     )
+    # Perform some sanity checks on MacOS
+    configure_simba(snowflake_simba_config(args$driver),
+      action = "modify")
     inject(dbConnect(odbc(), !!!args))
   }
 )
@@ -264,13 +270,41 @@ snowflake_default_driver_paths <- function() {
       "/usr/lib/snowflake/odbc/lib/libSnowflake.so"
     )
   } else if (Sys.info()["sysname"] == "Darwin") {
-    paths <- "/opt/snowflake/snowflakeodbc/lib/universal/libSnowflake.dylib"
+    paths <- c(
+      "/opt/snowflake/snowflakeodbc/lib/universal/libSnowflake.dylib",
+      "/opt/snowflake-osx-x64/bin/lib/libsnowflakeodbc_sb64-universal.dylib"
+    )
   } else {
     paths <- character()
   }
   paths[file.exists(paths)]
 }
 
+snowflake_simba_config <- function(driver) {
+  snowflake_env <- Sys.getenv("SIMBASNOWFLAKEINI")
+  if (!identical(snowflake_env, "")) {
+    return(snowflake_env)
+  }
+  # Posit configuration is likely at:
+  # /opt/snowflake-osx-x64/bin/lib/rstudio.snowflakeodbc.ini
+  # OEM configuration is likely at:
+  # /opt/snowflake/snowflakeodbc/lib/universal/simba.snowflake.ini
+  common_dirs <- c(
+    driver_dir(driver),
+    "/opt/snowflake-osx-x64/bin/lib/",
+    "/opt/snowflake/snowflakeodbc/lib/universal/",
+    getwd(),
+    Sys.getenv("HOME")
+  )
+  return(list(
+    path = list.files(
+      simba_config_dirs(driver),
+      pattern = "snowflake(odbc)?\\.ini$",
+      full.names = TRUE),
+    url = "https://docs.snowflake.com/en/developer-guide/odbc/odbc-download"
+  ))
+}
+
 snowflake_server <- function(account) {
   if (nchar(account) == 0) {
     abort(

R/utils.R

@@ -288,28 +288,44 @@ check_attributes <- function(attributes, call = caller_env()) {
 }
 
 # apple + spark drive config (#651) --------------------------------------------
-configure_spark <- function(call = caller_env()) {
+# Method will attempt to:
+# 1. Locate an installation of unixodbc / error out otherwise.
+# 2. Verify the driver_config argument.  Expect this to be a list with
+#    two fields:
+#    * path Vector of viable driver paths ( only first one is used )
+#    * url A location where the user can downlaod the driver from.
+#    See spark_simba_config, for example.  Its return value is used as
+#    the value for this argument.
+# 3. Inspect the config for some settings that can impact how our package
+#    performs.
+# 4. If action == "modify" then we attempt to modify the config in-situ.
+# 5. Otherwise we throw a warning asking the user to revise.
+configure_simba <- function(driver_config,
+                            action = "modify", call = caller_env()) {
   if (!is_macos()) {
     return(invisible())
   }
+  if (!is.null(getOption("odbc.no_config_override"))) {
+    return(invisible())
+  }
 
   unixodbc_install <- locate_install_unixodbc()
   if (length(unixodbc_install) == 0) {
     error_install_unixodbc(call)
   }
 
-  spark_config <- locate_config_spark()
-  if (length(spark_config) == 0) {
-    abort(
-      c(
-        "Unable to locate the needed spark ODBC driver.",
-        i = "Please install the needed driver from https://www.databricks.com/spark/odbc-drivers-download."
-      ),
+  simba_config <- driver_config$path
+  if (length(simba_config) == 0) {
+    func <- cli::cli_warn
+    if (action == "modify") {
+      fun <- cli::cli_abort
+    }
+    func(
+      c(i = "Please install the needed driver from {driver_config$url}"),
       call = call
     )
   }
-
-  configure_unixodbc_spark(unixodbc_install[1], spark_config[1], call)
+  configure_unixodbc_simba(unixodbc_install[1], simba_config[1], action, call)
 }
 
 locate_install_unixodbc <- function() {
@@ -369,51 +385,51 @@ error_install_unixodbc <- function(call) {
   )
 }
 
-# p. 44 https://downloads.datastax.com/odbc/2.6.5.1005/Simba%20Spark%20ODBC%20Install%20and%20Configuration%20Guide.pdf
-locate_config_spark <- function() {
-  spark_env <- Sys.getenv("SIMBASPARKINI")
-  if (!identical(spark_env, "")) {
-    return(spark_env)
-  }
-
-  common_dirs <- c(
-    "/Library/simba/spark/lib",
-    "/etc",
-    getwd(),
-    Sys.getenv("HOME")
-  )
+configure_unixodbc_simba <- function(unixodbc_install, simba_config, action, call) {
 
-  list.files(
-    common_dirs,
-    pattern = "simba\\.sparkodbc\\.ini$",
-    full.names = TRUE
-  )
-}
-
-configure_unixodbc_spark <- function(unixodbc_install, spark_config, call) {
   # As shipped, the simba spark ini has an incomplete final line
   suppressWarnings(
-    spark_lines <- readLines(spark_config)
+    simba_lines <- readLines(simba_config)
   )
-
-  spark_lines_new <- replace_or_append(
-    lines = spark_lines,
-    pattern = "^ODBCInstLib=",
+  res <- replace_or_append(
+    lines = simba_lines,
+    key_pattern = "^ODBCInstLib=",
+    accepted_value = unixodbc_install,
     replacement = paste0("ODBCInstLib=", unixodbc_install)
   )
-
-  spark_lines_new <- replace_or_append(
-    lines = spark_lines_new,
-    pattern = "^DriverManagerEncoding=",
+  warnings <- character()
+  if (action != "modify" && res$modified) {
+     warnings <- c(warnings, c("*" = "Please consider revising the {.arg ODBCInstLib}
+       field in {.file {simba_config}} and setting its value to {.val {unixodbc_install}}"))
+  }
+  simba_lines_new <- res$new_lines
+  res <- replace_or_append(
+    lines = simba_lines_new,
+    key_pattern = "^DriverManagerEncoding=",
+    accepted_value = "UTF-16|utf-16",
     replacement = "DriverManagerEncoding=UTF-16"
   )
+  if (action != "modify" && res$modified) {
+     warnings <- c(warnings, c("*" = "Please consider revising the
+       {.arg DriverManagerEncoding} field in {.file {simba_config}} and setting its
+       value to {.val UTF-16}."))
+  }
+  if (length(warnings)) {
+    cli::cli_warn(c(
+      c(i = "Detected potentially unsafe driver settings:"),
+      warnings
+    ))
+  }
+  simba_lines_new <- res$new_lines
 
-  write_spark_lines(spark_lines, spark_lines_new, spark_config, call)
+  if (action == "modify") {
+    write_simba_lines(simba_lines, simba_lines_new, simba_config, call)
+  }
 
   invisible()
 }
 
-write_spark_lines <- function(spark_lines, spark_lines_new, spark_config, call) {
+write_simba_lines <- function(spark_lines, spark_lines_new, spark_config, call) {
   if (identical(spark_lines, spark_lines_new)) {
     return(invisible())
   }
@@ -436,23 +452,52 @@ write_spark_lines <- function(spark_lines, spark_lines_new, spark_config, call)
   writeLines(spark_lines_new, spark_config)
 }
 
+# Interpret the argument as an `ODBC` driver
+# and attempt to infer the directory housing it.
+# It will return an empty character vector if unable to.
+driver_dir <- function(driver) {
+  # driver argument could be an outright path, or a name
+  # of a driver specified in odbcinst.ini  Try to discern
+  driver_spec <- subset(odbcListDrivers(), name == driver)
+  if (nrow(driver_spec)) {
+    driver_path <- subset(driver_spec, attribute == "Driver")$value
+  } else {
+    driver_path <- driver
+  }
+
+  ret <- dirname(driver_path)
+  if (ret == ".") {
+    ret <- character()
+  }
+  return(ret)
+}
+
 is_writeable <- function(path) {
   tryCatch(file.access(path, mode = 2)[[1]] == 0, error = function(e) FALSE)
 }
 
-# given a vector of lines in an ini file, look for a given key pattern.
-# the `replacement` is the whole intended line, giving the "key=value" pair.
-# if the key is found, replace that line with `replacement`.
-# if the key isn't found, append a new line with `replacement`.
-replace_or_append <- function(lines, pattern, replacement) {
-  matching_lines_loc <- grepl(pattern, lines)
+# Given a vector of lines in an ini file, look for a given key pattern.
+# If found:
+# - No action if the `accepted_value` argument is found on line.
+# - Replace otherwise.
+# If not found: append.
+# The `replacement` is the whole intended line, giving the desired
+# "key=value" pair.
+# @return a list with two elements:
+# - new_lines: Potentially modified lines
+# - modified: Whether method modified lines, where modified means
+# both changed or appended.
+replace_or_append <- function(lines, key_pattern, accepted_value, replacement) {
+  matching_lines_loc <- grepl(key_pattern, lines)
   matching_lines <- lines[matching_lines_loc]
+  found_ok <- length(matching_lines) != 0 &&
+    any(grepl(accepted_value, lines[matching_lines_loc]))
   if (length(matching_lines) == 0) {
     lines <- c(lines, replacement)
-  } else {
+  } else if (!found_ok) {
     lines[matching_lines_loc] <- replacement
   }
-  lines
+  return(list(new_lines = lines, modified = !found_ok))
 }
 
 check_shiny_session <- function(x,