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

Commit transaction after post processors #511

Closed
wants to merge 3 commits into from
Closed

Commit transaction after post processors #511

wants to merge 3 commits into from

Conversation

rezun
Copy link
Contributor

@rezun rezun commented Apr 6, 2016

Quick solution for Issue #508

@jfinkels
Copy link
Owner

jfinkels commented Apr 6, 2016

The build error is my fault, you can ignore it.

I'll take a look at this. Thanks!

@jfinkels
Copy link
Owner

Is there any way to add a unit test for at least one of these methods? For example, can I use a postprocessor to check that the session has been flushed but not committed, and then check that a session has been committed after the test client receives the response from the server?

Also, thanks for finding my TODO comments in the code and providing a fix for them. It might be nice to add a comment in the code explaining why the flush occurs there and why the commit occurs later, as a reminder to future readers of the code (me, for example).

@rezun
Copy link
Contributor Author

rezun commented Apr 13, 2016

Alright. I wrote a test and applied it to different processors. I hope that this meets your expectations.

@@ -42,6 +42,7 @@
from flask.ext.restless import DeserializationException
from flask.ext.restless import SerializationException
from flask.ext.restless import simple_serialize
from flask.ext.restless import ProcessingException
Copy link
Owner

Choose a reason for hiding this comment

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

Please alphabetize the imports.

@jfinkels
Copy link
Owner

How does this interact with the catch_integrity_errors decorator https://github.com/jfinkels/flask-restless/blob/master/flask_restless/views/base.py#L430, and the _handle_validation_exception method https://github.com/jfinkels/flask-restless/blob/master/flask_restless/views/base.py#L1228, which also roll back a session?

The last thing to do is to update the changelog and update the documentation to state exactly when the session is flushed, committed, and rolled back, but I can do that myself. I'll merge this soon, thanks for your work!

@rezun
Copy link
Contributor Author

rezun commented Apr 13, 2016

You're right. I've corrected those issues.

I didn't think about those two functions.
The decorator catch_integrity_errors should work as expected. The flushing and the deferred commiting should not influence the ability to roll back on error.
The _handle_validation_exception should also behave the same. The flush() should throw all the exceptions commit() did, as it does exactly the same, except flagging the database transaction as commited.
But this raises another question. What about validation exceptions coming from postprocessors? they didn't get handled so far. But if the transaction is still open and the postprocessors are intented to do additional database operations, maybe they should.

@rezun
Copy link
Contributor Author

rezun commented May 3, 2016

I've squashed and rebased my branch to simplify the history and to solve merge conflicts.

@jfinkels
Copy link
Owner

jfinkels commented Jun 9, 2016

I merged these commits manually, in 6662e64, fb8cabd, and 027b482. Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants