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

Session should be rolled back on SQLAlchemyError #449

Closed
hussaintamboli opened this issue Jul 2, 2015 · 4 comments
Closed

Session should be rolled back on SQLAlchemyError #449

hussaintamboli opened this issue Jul 2, 2015 · 4 comments
Labels

Comments

@hussaintamboli
Copy link

Yesterday, I came across a really painful issue.

I use flask-restless and SQLAlchemy for my API. I got Internal server error due to OperationalError exception in one of my APIs. It was because one of the required column was not passed to the POST request. After that, for all the subsequent requests I started getting another Internal server error due to InvalidRequestError exception.

InvalidRequestError: This Session's transaction has been rolled back due to a previous exception during flush. To begin a new transaction with this Session, first issue Session.rollback() .... 

After a long search I found that a rollback() needs to be issued when an error occurs in commit(). But in this case the commit and rollback is not in my hands but restful's.

So I started looking where and when the rollback is being issued in restless and found these lines in views.py

190 # TODO should `sqlalchemy.exc.InvalidRequestError`s also be caught?
191 except (DataError, IntegrityError, ProgrammingError) as exception:
192   session.rollback()

Note that OperationalError is not handled as a rollback case.

So as a FIX to my issue I modified the views.py and put OperationalError in it. Then I thought, to be on safer side why not issue the rollback on SQLAlchemyError (SQLAlchemy base exception class)

I have created a pull request with my fix.

Please let me know your thoughts on this

@hussaintamboli hussaintamboli changed the title rollback() has to happen on sqlalchemy.exc.SQLAlchemyError exception and not just (DataError, IntegrityError, ProgrammingError). views.py needs to be fixed rollback() has to be issued on sqlalchemy.exc.SQLAlchemyError exception and not just (DataError, IntegrityError, ProgrammingError). views.py needs to be fixed Jul 2, 2015
hussaintamboli pushed a commit to hussaintamboli/flask-restless that referenced this issue Jul 2, 2015
@hussaintamboli
Copy link
Author

Hi,

Could you please accept my merge request. Because I really want to install flask_restless using pip. And I don't wanna put flask_restless with my fix inside version control.

Without this fix, the randomness issue persists.

@jwg4
Copy link

jwg4 commented Jan 15, 2016

@hussaintamboli Why did you close the issue if your code has not been merged? You should re-open the issue.

@jfinkels jfinkels changed the title rollback() has to be issued on sqlalchemy.exc.SQLAlchemyError exception and not just (DataError, IntegrityError, ProgrammingError). views.py needs to be fixed Session should be rolled back on SQLAlchemyError Feb 4, 2016
@jfinkels jfinkels added the bug label Feb 4, 2016
@jfinkels
Copy link
Owner

jfinkels commented Feb 4, 2016

This should be fixed by 56c8261.

@jfinkels jfinkels closed this as completed Feb 4, 2016
@hussaintamboli
Copy link
Author

Great.

Actually the pull request that I had sent before (4bed33f) has the same fix.

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