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

dbWithTransaction #110

Merged
merged 7 commits into from Jun 14, 2016
Merged

dbWithTransaction #110

merged 7 commits into from Jun 14, 2016

Conversation

bborgesr
Copy link
Contributor

@bborgesr bborgesr commented Jun 6, 2016

Moving the relevant code for the dbWithTransaction() function to this PR, originally in #108

Note: The Travis Build is failing because the Examples section of dbWithTransaction() makes use of dbExecute(), which does not exist on this branch. So, this should not be a problem once we merge everything into master.

error = function(e) {
success <- dbRollback(conn)
if (!success) stop("`dbRollback` was not successful")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this error message needs to include the error message from e as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't those different things though? Independently of whether or not eoccurs, dbRollback should always succeed when called... And there might be a situation in which the user wants the transaction to fail, so he's expecting to see e, but not this other error related to dbRollback. By making those two things completely separate from one another, we don't risk any ambiguity. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that currently if both the code and the rollback fail, you only see that the rollback fails. I don't think it's likely to happen in practice, but it seems like it's slightly safer to print the messages from all errors that occurred, not just the most recent error.

setMethod("dbWithTransaction", "DBIConnection", function(conn, code) {
## check if each operation is successful
if (!dbBegin(conn)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see how dbBegin() etc. are currently declared, but I'd rather like the interface to raise an error if a transaction can't be opened, committed, or rolled back, #112. (An error during rollback is fatal in most cases, I'd expect a well-behaved DBMS to reject any further write operations in this case.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is fine in the short-term, right? This works with the current interface that returns a logical value, and will also work with the future interface that throws errors. We can remove the redundant stop()s once most backends are updated to throw errors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right, it works in both cases, but will break with DBI backends that don't return a logical. Perhaps something like if (isFALSE(dbBegin())) with a suitable implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krlmlr, Are there currently any backends that don't return a logical? From what I gather, it seems to me that you'd like to check if either dbBegin is FALSE or if it returns an error. If one of those conditions apply, then we should throw an error, right? If this is the case, it seems that the best check would be something like:

call <- dbBegin()
if(!isTRUE(call) || inherits(call, "error")) stop(...)

Copy link
Member

@krlmlr krlmlr Jun 13, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's slightly better to do

if (isFALSE(call)) stop(...)

because this prepares for "new-style" DBI backends that don't return a logical anymore. If dbBegin() throws an error, that code won't be reached anyway.

We'll have to define our own isFALSE().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I added the identical check. It doesn't really seem necessary to me to wrap it into a new isFALSE function though... The check is pretty straightforward and it doesn't take up that much space, so it feels like overkill (unless there's some weird performance improvement that I'm completely unaware of). Let me know if you feel strongly about this...
Otherwise, I think this is ready to be merged... ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. The same logic should be applied to the dbCommit() and dbRollback() calls.

Could you please also merge the master branch into this branch and resolve the conflicts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy to, will you give me write access? Thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also do it the other way round: You do

git fetch --all
git merge origin/master
# resolve conflicts
git add .....
git commit

and then I can merge without conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krlmlr, done :) Let me know if you encounter any issues.

krlmlr pushed a commit that referenced this pull request Jun 8, 2016
- Remove redundant declaration of transaction methods (#110, @@bborgesr).
krlmlr pushed a commit that referenced this pull request Jun 10, 2016
- Remove redundant declaration of transaction methods (#110, @@bborgesr).
- New `dbExecute()`, forwards to `dbSendQuery()` by default (#109, @bborgesr).
Merge remote-tracking branch 'dbi/master' into dbWithTransaction

# Conflicts:
#	NAMESPACE
#	NEWS.md
#	R/DBConnection.R
#	R/transactions.R
#	man/dbDisconnect.Rd
#	man/dbExecute.Rd
#	man/dbExistsTable.Rd
#	man/dbGetException.Rd
#	man/dbGetQuery.Rd
#	man/dbListFields.Rd
#	man/dbListResults.Rd
#	man/dbListTables.Rd
#	man/dbReadTable.Rd
#	man/dbRemoveTable.Rd
#	man/dbSendQuery.Rd
@codecov-io
Copy link

codecov-io commented Jun 14, 2016

Current coverage is 43.80%

Merging #110 into master will decrease coverage by 2.43%

@@             master       #110   diff @@
==========================================
  Files            15         16     +1   
  Lines           413        436    +23   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits            191        191          
- Misses          222        245    +23   
  Partials          0          0          

Powered by Codecov. Last updated by 5213bf6...53105bc

@@ -1,9 +1,3 @@
# DBI 0.4-2 (2016-06-08)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to keep those NEWS.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, sorry! Re-added (and removed the extra @)

@krlmlr
Copy link
Member

krlmlr commented Jun 14, 2016

LGTM, apart from the NEWS removal.

@krlmlr krlmlr merged commit a1367f7 into r-dbi:master Jun 14, 2016
@krlmlr
Copy link
Member

krlmlr commented Jun 14, 2016

Thanks!

krlmlr pushed a commit that referenced this pull request Jul 31, 2016
- The default implementation of `dbQuoteString()` doesn't call `encodeString()` anymore: Neither SQLite nor Postgres understand e.g. `\n` in a string literal, and all of SQLite, Postgres, and MySQL accept an embedded newline (#121).
- New `dbWithTransaction()` that calls `dbBegin()` and `dbCommit()`, and `dbRollback()` on failure (#110, @bborgesr).
@krlmlr krlmlr mentioned this pull request Jul 31, 2016
krlmlr added a commit that referenced this pull request Aug 11, 2016
- Interface changes
    - `dbDataType()` maps `character` values to `"TEXT"` by default (#102).
    - The default implementation of `dbQuoteString()` doesn't call `encodeString()` anymore: Neither SQLite nor Postgres understand e.g. `\n` in a string literal, and all of SQLite, Postgres, and MySQL accept an embedded newline (#121).

- Interface enhancements
    - New `dbSendStatement()` generic, forwards to `dbSendQuery()` by default (#20, #132).
    - New `dbExecute()`, calls `dbSendStatement()` by default (#109, @bborgesr).
    - New `dbWithTransaction()` that calls `dbBegin()` and `dbCommit()`, and `dbRollback()` on failure (#110, @bborgesr).
    - New `dbBreak()` function which allows aborting from within `dbWithTransaction()` (#115, #133).
    - Export `dbFetch()` and `dbQuoteString()` methods.

- Documentation improvements:
    - One example per function (except functions scheduled for deprecation) (#67).
    - Consistent layout and identifier naming.
    - Better documentation of generics by adding links to the class and related generics in the "See also" section under "Other DBI... generics" (#130). S4 documentation is directed to a hidden page to unclutter documentation index (#59).
    - Fix two minor vignette typos (#124, @mdsumner).
    - Add package documentation.
    - Remove misleading parts in `dbConnect()` documentation (#118).
    - Remove misleading link in `dbDataType()` documentation.
    - Remove full stop from documentation titles.
    - New help topic "DBIspec" that contains the full DBI specification (currently work in progress) (#129).
    - HTML documentation generated by `staticdocs` is now uploaded to http://rstats-db.github.io/DBI for each build of the "production" branch (#131).
    - Further minor changes and fixes.

- Internal
    - Use `contains` argument instead of `representation()` to denote base classes (#93).
    - Remove redundant declaration of transaction methods (#110, @bborgesr).
@github-actions github-actions bot locked and limited conversation to collaborators Oct 8, 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

Successfully merging this pull request may close these issues.

None yet

5 participants