fukamachi / cl-dbi

Database independent interface for Common Lisp
202 stars 28 forks source link

Refactored the way how transactions and savepoints are handled. #50

Closed svetlyak40wt closed 4 years ago

svetlyak40wt commented 4 years ago

Now their state is stored in small structures and at any point of time it possible to know if the transaction already was committed or rolled back.

Also, the issue https://github.com/fukamachi/cl-dbi/issues/49 about non-local exit from dbi:commit function. Now control flow continues after the manuall call to dbi:commit. But second manual call to dbi:commit or dbi:rollback will cause error.

Other changes

svetlyak40wt commented 4 years ago

Hm, tests fails for mysql and postgresql.

I'll add a docker-compose file to run all tests in development,

svetlyak40wt commented 4 years ago

@fukamachi please, review this pull. It is ready now.

svetlyak40wt commented 4 years ago

@fukamachi Eitaro, please, review this pull :)

fukamachi commented 4 years ago

Sorry for the late reply, and thank you for your contribution. Especially, Docker configurations for testing look so nice :)

I suspect that SQL executions after a manual commit/rollback will be auto-committed though it's in a with-transaction, won't they?

For example, the following code probably adds a new (name="cat") animal in spite of it will raise an error:

(with-transaction *connection*
  (do-sql *connection* "INSERT INTO animal (name) VALUES ('dog')")
  (rollback *connection*)

  ;; Will this be auto committed?
  (do-sql *connection* "INSERT INTO animal (name) VALUES ('cat')")

  ;; Will raise an error.
  (rollback *connection*))
svetlyak40wt commented 4 years ago

Yes, this is why I've added a call to turn-off-autocommit in unittests.

Probably it is better to change MySQL's driver to turn off auto-commit for with-transaction block.

What do you think?

fukamachi commented 4 years ago

My understanding is that disabling autocommit implicitly begins a transaction when executing an SQL. Let's think about an example like the following code:

(with-transaction *connection*
  (turn-off-autocommit)

  (do-sql *connection* "INSERT INTO animal (name) VALUES ('dog')")
  (rollback *connection*)

  ;; This implicitly starts a new transaction
  (do-sql *connection* "INSERT INTO animal (name) VALUES ('cat')"))

;; And the transaction still lives here?

The implicit transaction opened by "INSERT INTO" would be leaked outside of with-transaction.

fukamachi commented 4 years ago

IMHO, all DB operations after manual commit/rollback inside with-transaction should be prohibited.

svetlyak40wt commented 4 years ago

all DB operations after manual commit/rollback inside with-transaction should be prohibited.

@fukamachi do you want me to implement this requirement?

fukamachi commented 4 years ago

If you don't mind and have some time for it.

svetlyak40wt commented 4 years ago

Ok.

svetlyak40wt commented 4 years ago

@fukamachi please, review this pull again.

I've added a necessary check to prohibit SQL queries after the manual commit or rollback. The main job was made in the f8f7401cac2ca2a23e1360210dd1624dc6bd4080 commit.

fukamachi commented 4 years ago

Merged. Thank you for your effort!

svetlyak40wt commented 4 years ago

Great! Thank you for the code review.