#183: Error handling for the connection object being made before start_db_capturing()

[KoderKow]

This is for issue #183

I created a local postgres database to test this out more. I added an rlang::abort call to the send query bit. This seems to be a common function that a lot of the DBI functions use such as dbListTables, dbListFields, and dbExistsTable. They also face a similar error. The screen shot is the PR minus the null checks on the 3 db functions:

You can see they print the dbSendQuery abort message along with the error "Error in if (nzchar(file)) { : argument is of length zero". I added in a similar check to those 3 functions so they return NULL, which will make the dbSendQuery error print

Few things:

1. Do you like the error message? If not, what would you like it to show? Would you prefer sentences over the bullets? I am open to your preference :)
2. I am not sure what needs to be added to clear out those warning messages
   • If you know how to silence these warnings let me know!
3. The dplyr::tbl(con, dbplyr::in_schema("dbo", "iris")) bit produces two error messages, I am not sure how to confront that or if its fine as is
4. Any other guidelines you have that I missed let me know!

Reprex for when you are developing within dittodb:

devtools::load_all()

# Values redacted for security
con <- DBI::dbConnect(
  drv = RPostgres::Postgres(),
  dbname = dbname,
  host = host,
  port = port,
  user = user,
  password = password
)

dbExecute(con, "CREATE SCHEMA dbo;")

DBI::dbWriteTable(con, '"dbo"."iris"', iris)

DBI::dbAppendTable(con, DBI::SQL('"dbo"."iris"'), iris)

start_db_capturing()

dbGetQuery(con, "SELECT * FROM dbo.iris")
dbListTables(con)
dbListFields(con, '"dbo"."iris"')
dbExistsTable(con, '"dbo"."iris"')
dplyr::tbl(con, dbplyr::in_schema("dbo", "iris"))

stop_db_capturing()

dbDisconnect(con)
rm(con)

[jonkeane]

First, thank you so much for this PR. It's a really great approach, and I like the detailed and helpful message.

1. Do you like the error message? If not, what would you like it to show? Would you prefer sentences over the bullets? I am open to your preference :)

I like the details there and that the message is actionable. So far I haven't used rlang::abort() in dittodb, but that is probably more old fashioned than it needs to be these days! I'm ok keeping rlang::abort here, I've also added #187 to track possibly using it elsewhere to be consistent.

2. I am not sure what needs to be added to clear out those warning messages
   • If you know how to silence these warnings let me know!

Hmm, that is funny — have you looked in the DBI source to see if that's where the message is coming from? That might help identify how to silence it. Though to be honest they might be fine there — it's an error condition, and hopefully folks would read the error and not the warning.

3. The dplyr::tbl(con, dbplyr::in_schema("dbo", "iris")) bit produces two error messages,
   • I am not sure how to confront that or if its fine as is

This is coming from https://github.com/tidyverse/dbplyr/blob/0739c751e63b1aca17812e3f2db2640857e46043/R/db-sql.R#L1166-L1178 in dbplyr. I don't think there's much we can do about that, but I suspect that's fine since our message is right there and clear about what's going on.

4. Any other guidelines you have that I missed let me know!

Have you thought about adding a test for this? You might add it to https://github.com/ropensci/dittodb/blob/main/tests/testthat/test-a-recording.R which has examples of recording a sqlite connection successfully, so we could do the wrong way around and confirm there's an error. It could also go in https://github.com/ropensci/dittodb/blob/main/tests/testthat/test-capture-requests.R which has other tests for capture-requests.R.

[KoderKow]

leaving a comment as a to-do to add unit tests

seems like the fixes i added broke some unit tests. i will look into those as soon as i can 👍

[KoderKow]

can i get some assistance on passing testthat 2e tests?

[jonkeane]

can i get some assistance on passing testthat 2e tests?

Aaaah, yes! I'm pretty sure what's going on there is that 2e swallows all but the last (or first? I can't remember) error / warning there. In order to deal with this there's a small helper testthat_transition() in tests/testthat/helper.R.

Here is an example of that (where I assert the content of a message, but error should be the same)

dittodb/tests/testthat/test-use-dittodb.R

Lines 71 to 74 in 4ada9c9

	testthat_transition(
	expect_message(add_dittodb_to_helper(f), "Creating"),
	expect_message(expect_message(add_dittodb_to_helper(f), "Creating"), "Adding library")
	)

Try that out and see if that resolves the 2e tests.

Also, would it be possible to add in parts of the expected error messages there? Like I do with the message, just a part of the error is totally fine, but good to ensure we're getting the error(s) we expect there. For the 2e version, I usually have to run the test, see which one is bubbling up and using that.

[KoderKow]

@jonkeane yippeee! thanks for the help, let me know if you had any other items you would like updated/implemented :)

[jonkeane]

Thank you so much for pushing on this more!

I have ~one substantive suggestion which is basically: let's simplify check_db_path() to always produce the detailed error even if it means more lines of error when people run into this. And then some small test changes to adjust to that.

Other than that it's minor style things that the linter is complaining about. I believe you can run styler::style_pkg() and it will fix everything, but I know some times there are small and subtle things that the styler doesn't get just right. If this is frustrating, let me know and I'm happy to fix things up and push to the branch.

And again, thank you for your effort on this 💯

[KoderKow]

I have resolved all of the comments you left :) I seem to be facing a different set of errors now. Apologizing for not knowing how to address these solo, can I get some assistance, so I know how to address these?

[jonkeane]

Sorry for my delay here!

The failures you're seeing are actually coming from a dbplyr update. I just fixed those in main, would you mind rebasing or meging main and see if that resolves it?

I'll take a look at the changes themselves, but wanted to get this to you ASAP

[jonkeane]

One small request. LMK if you're having trouble with getting the main update in and working.

Thanks again for this

[KoderKow]

@jonkeane i think were set for a final pass, let me know if you want a version bump or NEWS update :)

[jonkeane]

That looks great — a news item would be lovely (sorry, should have asked for that in my last comment) and then I'll merge this (+start the process to release to CRAN to avoid too much difference with that dbplyr change)

[KoderKow]

NEWS.md has been update, let me know if there is anything else! Appreciate your work and spending time on this :D

[jonkeane]

Thank you!