[arrow-adbc] branch main updated: fix(r/adbcdrivermanager): Make `adbc_xptr_is_valid()` return `FALSE` for external pointer to NULL (#1007)

[lidavidm]

This is an automated email from the ASF dual-hosted git repository.

lidavidm pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-adbc.git


The following commit(s) were added to refs/heads/main by this push:
     new 1bb507f3 fix(r/adbcdrivermanager): Make `adbc_xptr_is_valid()` return 
`FALSE` for external pointer to NULL (#1007)
1bb507f3 is described below

commit 1bb507f300ee257aac878b44c858bf8d5da8b2b8
Author: Dewey Dunnington <de...@voltrondata.com>
AuthorDate: Fri Sep 1 09:11:52 2023 -0300

    fix(r/adbcdrivermanager): Make `adbc_xptr_is_valid()` return `FALSE` for 
external pointer to NULL (#1007)
    
    Closes #1001.
    
    In theory one could differentiate the NULL from the "non-null but
    invalid" case, but I think returning FALSE is the least confusing thing
    to do.
---
 r/adbcdrivermanager/src/radbc.cc                |  6 +++---
 r/adbcdrivermanager/src/radbc.h                 |  4 ++--
 r/adbcdrivermanager/tests/testthat/test-utils.R | 20 ++++++++++++++++++++
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/r/adbcdrivermanager/src/radbc.cc b/r/adbcdrivermanager/src/radbc.cc
index 92f8e31c..224c7f81 100644
--- a/r/adbcdrivermanager/src/radbc.cc
+++ b/r/adbcdrivermanager/src/radbc.cc
@@ -150,7 +150,7 @@ extern "C" SEXP RAdbcMoveDatabase(SEXP database_xptr) {
 }
 
 extern "C" SEXP RAdbcDatabaseValid(SEXP database_xptr) {
-  AdbcDatabase* database = adbc_from_xptr<AdbcDatabase>(database_xptr);
+  AdbcDatabase* database = adbc_from_xptr<AdbcDatabase>(database_xptr, true);
   return Rf_ScalarLogical(database != nullptr && database->private_data != 
nullptr);
 }
 
@@ -219,7 +219,7 @@ extern "C" SEXP RAdbcMoveConnection(SEXP connection_xptr) {
 }
 
 extern "C" SEXP RAdbcConnectionValid(SEXP connection_xptr) {
-  AdbcConnection* connection = adbc_from_xptr<AdbcConnection>(connection_xptr);
+  AdbcConnection* connection = adbc_from_xptr<AdbcConnection>(connection_xptr, 
true);
   return Rf_ScalarLogical(connection != nullptr && connection->private_data != 
nullptr);
 }
 
@@ -396,7 +396,7 @@ extern "C" SEXP RAdbcMoveStatement(SEXP statement_xptr) {
 }
 
 extern "C" SEXP RAdbcStatementValid(SEXP statement_xptr) {
-  AdbcStatement* statement = adbc_from_xptr<AdbcStatement>(statement_xptr);
+  AdbcStatement* statement = adbc_from_xptr<AdbcStatement>(statement_xptr, 
true);
   return Rf_ScalarLogical(statement != nullptr && statement->private_data != 
nullptr);
 }
 
diff --git a/r/adbcdrivermanager/src/radbc.h b/r/adbcdrivermanager/src/radbc.h
index 73b37a3d..9baf8d6d 100644
--- a/r/adbcdrivermanager/src/radbc.h
+++ b/r/adbcdrivermanager/src/radbc.h
@@ -64,13 +64,13 @@ inline const char* adbc_xptr_class<ArrowSchema>() {
 }
 
 template <typename T>
-static inline T* adbc_from_xptr(SEXP xptr) {
+static inline T* adbc_from_xptr(SEXP xptr, bool null_ok = false) {
   if (!Rf_inherits(xptr, adbc_xptr_class<T>())) {
     Rf_error("Expected external pointer with class '%s'", 
adbc_xptr_class<T>());
   }
 
   T* ptr = reinterpret_cast<T*>(R_ExternalPtrAddr(xptr));
-  if (ptr == nullptr) {
+  if (!null_ok && ptr == nullptr) {
     Rf_error("Can't convert external pointer to NULL to T*");
   }
   return ptr;
diff --git a/r/adbcdrivermanager/tests/testthat/test-utils.R 
b/r/adbcdrivermanager/tests/testthat/test-utils.R
index a533ba99..3c46c489 100644
--- a/r/adbcdrivermanager/tests/testthat/test-utils.R
+++ b/r/adbcdrivermanager/tests/testthat/test-utils.R
@@ -80,7 +80,27 @@ test_that("pointer mover leaves behind an invalid external 
pointer", {
   expect_true(adbc_xptr_is_valid(stream))
   expect_true(adbc_xptr_is_valid(adbc_xptr_move(stream)))
   expect_false(adbc_xptr_is_valid(stream))
+})
+
+test_that("adbc_xptr_is_valid() returns FALSE for null pointer", {
+  db <- adbc_database_init(adbc_driver_void())
+  con <- adbc_connection_init(db)
+  stmt <- adbc_statement_init(con)
+  stream <- nanoarrow::basic_array_stream(list(), nanoarrow::na_na())
+
+  # A compact way to set the external pointer to NULL
+  db <- unserialize(serialize(db, NULL))
+  con <- unserialize(serialize(con, NULL))
+  stmt <- unserialize(serialize(stmt, NULL))
+  stream <- unserialize(serialize(stream, NULL))
+
+  expect_false(adbc_xptr_is_valid(db))
+  expect_false(adbc_xptr_is_valid(con))
+  expect_false(adbc_xptr_is_valid(stmt))
+  expect_false(adbc_xptr_is_valid(stream))
+})
 
+test_that("adbc_xptr_is_valid() errors for non-ADBC objects", {
   expect_error(
     adbc_xptr_is_valid(NULL),
     "must inherit from one of"