File stored even though SQLAlchemy transaction rollbacked

[davidparsson]

I have observed this behavior:

1. Create an SQLAlchemy model with a Depot file field, stored using the LocalFileStorage
2. Add the model to a session
3. Rollback the session

Here I would not expect the file from step 1 to have been stored to disk, but it seems to me that it is. Is this intentional?

Performing session.flush() before the rollback will however result in the file being removed during the rollback.

[vlcinsky]

@davidparsson I can confirm your observation.

My test case (written using pytest) shows such behaviour:

def test_rollback(db, db_session, depot, checker):
    file_ids = []
    sess = db_session
    tox_name = u"to commit tox.ini file"
    rq_name = u"to rollback requirements.txt file"

    with open("tox.ini") as f:
        doc = Document(name=tox_name, content=f)
    sess.add(doc)
    file_id = doc.content.file_id
    file_ids.append(file_id)
    assert checker.check_exists(file_id)
    with open("requirements.txt") as f:
        doc = Document(name=rq_name, content=f)
    file_id = doc.content.file_id
    file_ids.append(file_id)
    sess.add(doc)
    assert checker.check_exists(file_id)
    # sess.flush() before sess.rollback() fixes the problem
    sess.flush()  # comment this out to fail the test
    sess.rollback()
    # sess.flush() after sess.rollback() does not help
    sess.flush()
    query = sess.query(Document)
    res = query.all()
    assert len(res) == 0
    for file_id in file_ids:
        checker.check_is_missing(file_id)

Personally I think, that sqlalchemy does not emit some event for items, which are not persisted yet
as there is no reason to clean what is not existing in the database.

Unfortunatelly depot is missing such event and has no chance to clean the file.

Workaround

If you want to be sure, files created as part of the record are cleaned in case of rollback, do
session.flush() after you add any record containing attached depot file.

[vlcinsky]

The test case mentioned above is available in my bitbucket repository https://bitbucket.org/vlcinsky/playwith_filedepot

Commenting line 95 in tests/test_sql.py shall fail the test.

To run only the rollback problem:

(py27)$ pytest tests/test_sql.py::test_rollback

[amol-]

c144c1b should provide a solution for previously specified case and includes two tests for it.

[vlcinsky]

@amol- I tested the modification and in some cases it still fails. The tests from my bitbucket did not pass, so I tried to dive into your test suite (looks nice and thorough) and play a bit with session.flush().

See my PR #38

Dreaming of possible solutions, I see two options:

• solution catching the rollback event not requiring session.flush() will be found and coded - ideal solution.
• it will be found impossible or difficult to fix it and it will be declared a requirement to flush the record with file attached to be synchronized at rollback.

Both cases are good for my use cases.

[amol-]

I see, the problem is now related to the fact that object is modified before being attached to a session, so depot cannot register the changes into the session. It can probably solved by checking the files once the object is attached to session

[vlcinsky]

I can try to modify the test to see, that if I attach the files after the doc instance is already added to a session helps.
Personally I do not expect any improvement.

[amol-]

I took inspiration from your patch and made all SQLAlchemy tests run both with and without flush in 0963497 there are 2 tests failing due to that condition, I'll try to fix it across this week through before_attach event.

[vlcinsky]

Really elegant parametrization of test case (I use pytest in most cases - but as can be seen, standard test case class is working very well too if in hands of skilled programmer)

Good luck with before_attach (or whatever would fix the issue).

[amol-]

After fdccacb all tests pass with and without flushing

[vlcinsky]

I rerun my original pytest based test with your new update from git and now it passes.

Well done.

If @davidparsson has no objections, I would recommend closing this issue as resolved.

[davidparsson]

Sure, go ahead.

…

On Thu, 16 Mar 2017 at 01:25, Jan Vlčinský ***@***.***> wrote: I rerun my original pytest based test with your new update from git and now it passes. Well done. If @davidparsson <https://github.com/davidparsson> has no objections, I would recommend closing this issue as resolved. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/amol-/depot/issues/36#issuecomment-286921389>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAT2zfNgUC_NlfqVCY_5plAuq4eAq-B5ks5rmIF2gaJpZM4KAN5W> .