Skip to content

SQL execute does not autocomplete transaction if an IntegrityError occurs #178

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

Closed
jdabtieu opened this issue Dec 5, 2023 · 4 comments
Closed
Assignees
Labels

Comments

@jdabtieu
Copy link

jdabtieu commented Dec 5, 2023

In SQL.execute, if autocommit is on (default behavior) and the SQL query fails due to an IntegrityError, the open transaction is never closed via COMMIT or ROLLBACK. Relevant snippet:

329                # Execute statement
330                if self._autocommit:
331                    connection.execute(sqlalchemy.text("BEGIN"))
332                result = connection.execute(sqlalchemy.text(statement))
333                if self._autocommit:
334                    connection.execute(sqlalchemy.text("COMMIT"))

In this case, on line 331, a transaction is started, on 332 the statement fails, and thus line 334 is never executed. In the except clause, the transaction never gets completed either.

Then, when the next query is run, the BEGIN on line 331 causes RuntimeError: cannot start a transaction within a transaction because the previous transaction was never closed.

Being able to catch an IntegrityError is important for preventing race conditions (if the query fails, find out why as opposed to check if the record already exists, and then insert)

Minimal reproducible example (tested with cs50-9.3.0 on Ubuntu 20.04 (Python 3.8) and Windows 10 (Python 3.11):

from cs50 import SQL

open("test.db", "w").close()

db = SQL("sqlite:///test.db")
db.execute("CREATE TABLE test(id integer UNIQUE)")
db.execute("INSERT INTO test VALUES(1)")
try:
  # this should fail because of the UNIQUE constraint
  db.execute("INSERT INTO test VALUES(1)")
except ValueError as e:
  print(e)
print(db.execute("SELECT * FROM test"))

Expected output:

UNIQUE constraint failed: test.id
[{'id': 1}]

Actual output:

UNIQUE constraint failed: test.id
Traceback (most recent call last):
  File "\tmp\venv\test.py", line 13, in <module>
    print(db.execute("SELECT * FROM test"))
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "\tmp\venv\lib\site-packages\cs50\sql.py", line 29, in decorator
    return f(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^
  File "\tmp\venv\lib\site-packages\cs50\sql.py", line 413, in execute
    raise e
RuntimeError: cannot start a transaction within a transaction
@dmalan
Copy link
Member

dmalan commented Dec 5, 2023

Thanks for the detailed report!

jdabtieu added a commit to jdabtieu/CTFOJ that referenced this issue Dec 6, 2023

Verified

This commit was signed with the committer’s verified signature.
jdabtieu Jonathan Wu
@jdabtieu
Copy link
Author

jdabtieu commented Dec 6, 2023

Upon further testing, it seems like the upgrade to SQLAlchemy causes this bug: in 1.4.49, the transaction gets aborted when the IntegrityError is raised, and in 2.0.23, the transaction is not aborted. The sample code I provided initially does produce the right output with SQLAlchemy 1.4.49, but not with 2.0.23.

@dmalan dmalan closed this as completed in e18486c Dec 17, 2023
@jdabtieu
Copy link
Author

Thanks for the fix! I think you should update setup.py to require SQLAlchemy==2 since your fix breaks the sample code when using SQLAlchemy v1

@dmalan
Copy link
Member

dmalan commented Dec 18, 2023

Ah, thanks!

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

No branches or pull requests

3 participants