Improve validation

DESCRIPTION

@@ -21,7 +21,7 @@ Imports:
     DBI,
     later (>= 1.0.0),
     R6,
-    rlang
+    rlang (>= 1.0.0)
 Suggests:
     covr,
     dbplyr,

NEWS.md

@@ -1,5 +1,10 @@
 # pool (development version)
 
+* `dbPool()`'s `validateQuery` is now actually used (#153).
+
+* Connections are now validated once on first checkout to ensure that the
+  connection and validation strategy are both ok.
+
 * Added support for SAP HANA databases (@marcosci, #103).
 
 # pool 0.1.6

R/DBI-pool.R

@@ -24,7 +24,7 @@ setMethod("onDestroy", "DBIConnection", function(object) {
 #' @export
 #' @rdname object
 setMethod("onValidate", "DBIConnection", function(object) {
-  pool <- attr(object, "pool_metadata", exact = TRUE)$pool
+  pool <- pool_metadata(object)$pool
   query <- pool$state$validateQuery
 
   if (!is.null(query)) {
@@ -39,13 +39,13 @@ setMethod("onValidate", "DBIConnection", function(object) {
     options <- c(
       "SELECT 1",
       "SELECT 1 FROM DUAL",
-      "SELECT 1 FROM INFORMATION_SCHEMA.SYSTEM_USERS",
-      "SELECT * FROM INFORMATION_SCHEMA.TABLES",
+      "SELECT 1 FROM INFORMATION_SCHEMA.SYSTEM_USERS WHERE 0=1",
+      "SELECT 1 FROM INFORMATION_SCHEMA.TABLES WHERE 0=1",
       "VALUES 1",
-      "SELECT 1 FROM SYSIBM.SYSDUMMY1",
-      "select count(*) from systables",
-      "select count(*) from SYS_TABLES",
-      "select count(*) from SYS.TABLES"
+      "SELECT 1 FROM SYSIBM.SYSDUMMY1 WHERE 0=1",
+      "SELECT 1 FROM systables WHERE 0=1",
+      "SELECT 1 FROM SYS_TABLES WHERE 0=1",
+      "SELECT 1 FROM SYS.TABLES WHERE 0=1"
     )
 
     ## Iterates through the possible validation queries:

R/pool-methods.R

@@ -100,11 +100,7 @@ setGeneric("poolReturn", function(object) {
 #' @export
 #' @rdname poolCheckout
 setMethod("poolReturn", "ANY", function(object) {
-  pool_metadata <- attr(object, "pool_metadata", exact = TRUE)
-  if (is.null(pool_metadata) || !pool_metadata$valid) {
-    stop("Invalid object.")
-  }
-  pool <- pool_metadata$pool
+  pool <- pool_metadata(object)$pool
   pool$release(object)
 })
 

R/pool.R

@@ -67,13 +67,10 @@ Pool <- R6::R6Class("Pool",
     ## (sets up task to destroy the object if the number of
     ## total objects exceeds the minimum)
     release = function(object) {
-      pool_metadata <- attr(object, "pool_metadata", exact = TRUE)
+      pool_metadata <- pool_metadata(object)
       if (pool_metadata$state == "free") {
         stop("This object was already returned to the pool.")
       }
-      if (is.null(pool_metadata) || !pool_metadata$valid) {
-        stop("Invalid object.")
-      }
       ## immediately destroy object if pool has already been closed
       if (!self$valid) {
         private$changeObjectStatus(object, NULL)
@@ -171,17 +168,13 @@ Pool <- R6::R6Class("Pool",
       pool_metadata$pool <- self
       pool_metadata$valid <- TRUE
       pool_metadata$state <- NULL
-      pool_metadata$lastValidated <- Sys.time()
-
-      # Now that metadata has been setup we can validate it
-      # This ensures that any problems bubble up immediately, not after
-      # validationInterval seconds
-      onValidate(object)
+      # force validation to happen immediately to surface any issues
+      pool_metadata$lastValidated <- Sys.time() - self$validationInterval - 1
 
       ## detect leaked connections and destroy them
       reg.finalizer(pool_metadata, function(e) {
-        if (pool_metadata$valid) {
-          warning("You have a leaked pooled object.")
+        if (e$valid) {
+          warning("You have a leaked pooled object.", call. = FALSE)
         }
       }, onexit = TRUE)
 
@@ -192,7 +185,7 @@ Pool <- R6::R6Class("Pool",
     ## tries to run onDestroy
     destroyObject = function(object) {
       tryCatch({
-        pool_metadata <- attr(object, "pool_metadata", exact = TRUE)
+        pool_metadata <- pool_metadata(object, check_valid = FALSE)
         if (!pool_metadata$valid) {
           warning("Object was destroyed twice.")
           return()
@@ -214,7 +207,7 @@ Pool <- R6::R6Class("Pool",
     ## gets taken and vice versa. Valid values for `from`
     ## and `to` are: NULL, "free", "taken"
     changeObjectStatus = function(object, to) {
-      pool_metadata <- attr(object, "pool_metadata", exact = TRUE)
+      pool_metadata <- pool_metadata(object)
       id <- pool_metadata$id
       from <- pool_metadata$state
 
@@ -253,7 +246,7 @@ Pool <- R6::R6Class("Pool",
     },
 
     cancelScheduledTask = function(object, task) {
-      pool_metadata <- attr(object, "pool_metadata", exact = TRUE)
+      pool_metadata <- pool_metadata(object, check_valid = FALSE)
       taskHandle <- pool_metadata[[task]]
       if (!is.null(taskHandle)) {
         pool_metadata[[task]] <- NULL
@@ -301,7 +294,7 @@ Pool <- R6::R6Class("Pool",
     ## secs have passed since the last validation (this allows
     ## us some performance gains)
     validate = function(object) {
-      pool_metadata <- attr(object, "pool_metadata", exact = TRUE)
+      pool_metadata <- pool_metadata(object)
       lastValidated <- pool_metadata$lastValidated
       ## if the object has never been validated, set `lastValidated`
       ## to guarantee that it will be validated now

tests/testthat/_snaps/release.md

@@ -0,0 +1,8 @@
+# poolReturn() errors if object is not valid
+
+    Code
+      poolReturn("x")
+    Condition
+      Error in `poolReturn()`:
+      ! `object` is not an pooled object.
+

tests/testthat/test-DBI-pool.R

@@ -0,0 +1,10 @@
+test_that("onValidate() caches query", {
+  pool <- local_pool()
+  # reset cache from initial creation + validation
+  pool$state$validateQuery <- NULL
+
+  con <- poolCheckout(pool)
+  withr::defer(poolReturn(con))
+  onValidate(con)
+  expect_equal(pool$state$validateQuery, "SELECT 1")
+})

tests/testthat/test-fetch.R

@@ -33,14 +33,14 @@ describe("fetch", {
   it("only validates after validationInterval", {
     obj <- poolCheckout(pool)
     t0 <- Sys.time()
-    pool_metadata <- attr(obj, "pool_metadata", exact = TRUE)
+    pool_metadata <- pool_metadata(obj)
     lastValidated_t0 <- pool_metadata$lastValidated
 
     poolReturn(obj)
 
     obj <- poolCheckout(pool)
     t1 <- Sys.time()
-    pool_metadata <- attr(obj, "pool_metadata", exact = TRUE)
+    pool_metadata <- pool_metadata(obj)
     lastValidated_t1 <- pool_metadata$lastValidated
 
     if (difftime(t1, t0, units = "secs") < pool$validationInterval) {
@@ -54,7 +54,7 @@ describe("fetch", {
 
     obj <- poolCheckout(pool)
     t2 <- Sys.time()
-    pool_metadata <- attr(obj, "pool_metadata", exact = TRUE)
+    pool_metadata <- pool_metadata(obj)
     lastValidated_t2 <- pool_metadata$lastValidated
 
     if (difftime(t2, t0, units = "secs") < pool$validationInterval) {
@@ -70,7 +70,7 @@ describe("fetch", {
 
     obj <- poolCheckout(pool)
     t3 <- Sys.time()
-    pool_metadata <- attr(obj, "pool_metadata", exact = TRUE)
+    pool_metadata <- pool_metadata(obj)
     lastValidated_t3 <- pool_metadata$lastValidated
 
     if (difftime(t3, t0, units = "secs") > pool$validationInterval) {
@@ -84,7 +84,7 @@ describe("fetch", {
 
     obj <- poolCheckout(pool)
     t4 <- Sys.time()
-    pool_metadata <- attr(obj, "pool_metadata", exact = TRUE)
+    pool_metadata <- pool_metadata(obj)
     lastValidated_t4 <- pool_metadata$lastValidated
 
     if (difftime(t4, t3, units = "secs") < pool$validationInterval) {

tests/testthat/test-release.R

@@ -36,11 +36,6 @@ describe("release", {
     checkCounts(pool, free = 1, taken = 0)
   })
 
-  it("throws if object is not valid", {
-    obj <- "a"
-    expect_error(poolReturn(obj), "Invalid object.")
-  })
-
   it("warns if onPassivate fails", {
     checkCounts(pool, free = 1, taken = 0)
     obj <- poolCheckout(pool)
@@ -66,5 +61,6 @@ describe("release", {
   })
 })
 
-
-
+test_that("poolReturn() errors if object is not valid", {
+  expect_snapshot(poolReturn("x"), error = TRUE)
+})

tests/testthat/utils.R

@@ -1,3 +1,24 @@
+pool_metadata <- function(x,
+                          check_valid = TRUE,
+                          error_call = caller_env(),
+                          error_arg = caller_arg(x)) {
+  meta <- attr(x, "pool_metadata", exact = TRUE)
+
+  if (is.null(meta)) {
+    abort(
+      paste0("`", error_arg, "` is not an pooled object."),
+      call = error_call
+    )
+  }
+  if (check_valid && !meta$valid) {
+    abort(
+      paste0("`", error_arg, "` is no longer valid."),
+      call = error_call
+    )
+  }
+
+  meta
+}
 
 #********************************************************************#
 #********** Create Mock Pooled object for testing purposes **********#