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

Driver object should optionally contain connection information #280

Closed
krlmlr opened this issue Jun 19, 2019 · 10 comments
Closed

Driver object should optionally contain connection information #280

krlmlr opened this issue Jun 19, 2019 · 10 comments
Milestone

Comments

@krlmlr
Copy link
Member

krlmlr commented Jun 19, 2019

Hannes suggests to store the information how to connect, i.e. the arguments to dbConnect(), in the driver object.

Solves a bunch of problems:

  • reconnecting
  • context for DBItest
  • configuration of test databases

CC @hannesmuehleisen.

@jimhester
Copy link
Contributor

Is there any concern this would increase the chances of exposing db passwords inadvertently? Obviously this is not really a concern in the testing cases.

@krlmlr
Copy link
Member Author

krlmlr commented Jun 19, 2019

We could do something like

drv <- RPostgres(..., password = function() get_password_from_keychain(...))

and have dbConnect() call the password getter upon connecting. Or partial specification:

drv <- RPostgres(...)
dbConnect(drv, password = ...)

@krlmlr krlmlr added this to the 1.1.0 milestone Aug 23, 2019
@krlmlr
Copy link
Member Author

krlmlr commented Oct 13, 2019

  • private .conn_args slot
  • getter
  • setter
  • ensure arguments are picked up by dbConnect()

Maybe a new "DBIConnector" class, inheriting from "DBIDriver", could take care of forwarding to bare dbConnect(), to minimize downstream implementation effort?

@krlmlr
Copy link
Member Author

krlmlr commented Oct 13, 2019

Prefer aggregation over inheritance, they say.

@krlmlr
Copy link
Member Author

krlmlr commented Oct 13, 2019

Restrictions for arguments:

  • must be named
  • must not contain drv
  • if function, must be callable without arguments

@krlmlr
Copy link
Member Author

krlmlr commented Oct 13, 2019

Pros of a new connector class:

  • works immediately across all backends

Cons:

  • it's possible to create a connector object with invalid argument names -- checking arguments still is the responsibility of dbConnect()
  • if we want to support RSQLite::SQLite() returning a connector object instead of a driver, we need to tweak DBItest and perhaps implement dbDriver("DBIConnector")

krlmlr added a commit that referenced this issue Oct 13, 2019
@krlmlr krlmlr closed this as completed Oct 31, 2019
@krlmlr
Copy link
Member Author

krlmlr commented Oct 31, 2019

Needs documentation tweaks.

@krlmlr krlmlr reopened this Oct 31, 2019
@hannes
Copy link
Contributor

hannes commented Oct 31, 2019

Would be nice to have a close() method or something of the like

@krlmlr krlmlr closed this as completed in 31c2d6d Dec 14, 2019
@krlmlr
Copy link
Member Author

krlmlr commented Dec 14, 2019

I forgot -- what do we need a close() method for?

@github-actions
Copy link

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue and link to this old issue if necessary.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants