Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dbFetch() gives confusing warning when used with dbSendStatement() #523

Open
mikkmart opened this issue Oct 16, 2024 · 4 comments · May be fixed by #524
Open

dbFetch() gives confusing warning when used with dbSendStatement() #523

mikkmart opened this issue Oct 16, 2024 · 4 comments · May be fixed by #524

Comments

@mikkmart
Copy link

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
Copy link
Member

krlmlr commented Oct 16, 2024

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
Copy link
Author

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

@mikmart
Copy link

mikmart commented Oct 16, 2024

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
Copy link

mikmart commented Oct 16, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants