`dbFetch()` gives confusing warning when used with `dbSendStatement()`

[mikkmart]

Calling dbFetch() on a result of dbSendStatement() gives a confusing warning, telling you to use dbSendStatement():

library(DBI)

fetch_bound_statement <- function(drv) {
  con <- dbConnect(drv, ":memory:")  
  on.exit(dbDisconnect(con), add = TRUE)
  dbExecute(con, "CREATE TABLE foo (bar TEXT)")

  res <- dbSendStatement(con, "INSERT INTO foo (bar) VALUES (?)")
  on.exit(dbClearResult(res), add = TRUE, after = FALSE)

  dbBind(res, list("baz"))
  dbFetch(res) # <- Should be dbGetRowsAffected()
}

fetch_bound_statement(RSQLite::SQLite())
#> Warning in result_fetch(res@ptr, n = n): SQL statements must be issued with
#> dbExecute() or dbSendStatement() instead of dbGetQuery() or dbSendQuery().
#> data frame with 0 columns and 0 rows

The duckdb driver gives a slightly different warning which got me on track in the end:

fetch_bound_statement(duckdb::duckdb())
#> Warning in dbFetch(res): Should not call dbFetch() on results that do not come
#> from SELECT, got INSERT
#> data frame with 0 columns and 0 rows

Ideally I’d like to hear something like “use dbGetRowsAffected() instead”.

[krlmlr]

Thanks, good catch. This is where I see the warning:

library(DBI)

con <- dbConnect(RSQLite::SQLite(), ":memory:")
dbExecute(con, "CREATE TABLE foo (bar TEXT)")
#> [1] 0

res <- dbSendStatement(con, "INSERT INTO foo (bar) VALUES (?)")

dbBind(res, list("baz"))
dbFetch(res) # <- Should be dbGetRowsAffected()
#> Warning in result_fetch(res@ptr, n = n): SQL statements must be issued with
#> dbExecute() or dbSendStatement() instead of dbGetQuery() or dbSendQuery().
#> data frame with 0 columns and 0 rows

dbClearResult(res)
dbDisconnect(con)

^{Created on 2024-10-16 with reprex v2.1.1}

Interestingly, a misuse of dbSendQuery() does not trigger the warning:

library(DBI)

con <- dbConnect(RSQLite::SQLite(), ":memory:")
dbExecute(con, "CREATE TABLE foo (bar TEXT)")
#> [1] 0

res <- dbSendQuery(con, "INSERT INTO foo (bar) VALUES (?)")

dbBind(res, list("baz"))
dbGetRowsAffected(res)
#> [1] 1

dbClearResult(res)
dbDisconnect(con)

^{Created on 2024-10-16 with reprex v2.1.1}

I think the best we can do with reasonable effort is to change the wording of the warning. Would you like to contribute?

[mikkmart]

Yeah I'm happy to put a little PR together.

[mikmart]

There's a bit of complexity here from the 3 failure modes to report in one warning. Is the below too much?

library(DBI)

con <- dbConnect(RSQLite::SQLite(), ":memory:")
dbExecute(con, "CREATE TABLE foo (bar TEXT)")
#> [1] 0

# Should warn about misuse of dbGetQuery().
dbGetQuery(con, "INSERT INTO foo (bar) VALUES ('baz')")
#> Warning in result_fetch(res@ptr, n = n): `dbGetQuery()`, `dbSendQuery()` and
#> `dbFetch()` should only be used with `SELECT` queries. Did you mean
#> `dbExecute()`, `dbSendStatement()` or `dbGetRowsAffected()`?
#> data frame with 0 columns and 0 rows

# Should warn about misuse of dbSendQuery()? Maybe already before the fetch?
res <- dbSendQuery(con, "INSERT INTO foo (bar) VALUES ('baz')")
dbFetch(res)
#> Warning in result_fetch(res@ptr, n = n): `dbGetQuery()`, `dbSendQuery()` and
#> `dbFetch()` should only be used with `SELECT` queries. Did you mean
#> `dbExecute()`, `dbSendStatement()` or `dbGetRowsAffected()`?
#> data frame with 0 columns and 0 rows
dbClearResult(res)

# Should warn about misuse of dbFetch().
res <- dbSendStatement(con, "INSERT INTO foo (bar) VALUES ('baz')")
dbFetch(res)
#> Warning in result_fetch(res@ptr, n = n): `dbGetQuery()`, `dbSendQuery()` and
#> `dbFetch()` should only be used with `SELECT` queries. Did you mean
#> `dbExecute()`, `dbSendStatement()` or `dbGetRowsAffected()`?
#> data frame with 0 columns and 0 rows
dbClearResult(res)

# Should remain to be OK.
res <- dbSendStatement(con, "INSERT INTO foo (bar) VALUES ('baz') RETURNING *")
dbFetch(res)
#>   bar
#> 1 baz
dbClearResult(res)

dbDisconnect(con)

[mikmart]

I guess ideally we'd have something like this:

dbGetQuery(con, "INSERT INTO foo (bar) VALUES ('baz')")
#> Warning: `dbGetQuery()` should only be used with `SELECT` queries. Did you mean `dbExecute()`?

res <- dbSendQuery(con, "INSERT INTO foo (bar) VALUES ('baz')")
#> Warning: `dbSendQuery()` should only be used with `SELECT` queries. Did you mean `dbSendStatement()`?
dbFetch(res)

res <- dbSendStatement(con, "INSERT INTO foo (bar) VALUES ('baz')")
dbFetch(res)
#> Warning: `dbFetch()` should only be used with `SELECT` queries. Did you mean `dbGetRowsAffected()`?

The problem is that the only way to tell if a statement is a SELECT statement in the SQLite 3 API seems to be with an authorizer which is set on the connection level. Would it be overkill to have dbSendQuery() set a temporary authorizer to warn about use with statements other than SELECT?

And actually when it comes to dbFetch() it doesn't know whether dbSendQuery() or dbSendStatement() was used to create the result set, so it would always just warn with a non-SELECTy result like it does currently.