Incorrect behavior of obj.flush(): assertion failed after exception #138

Closed
socketpair opened this Issue Jul 27, 2015 · 7 comments

Projects

None yet

3 participants

@socketpair
from pony.orm import *
db = Database('sqlite', ':memory:', create_db=True)
class Person(db.Entity):
    name = Required(str, unique=True)
db.generate_mapping(create_tables=True)
with db_session:
    Person(name='a1')
with db_session:
    Person(name='a0')
    try:
        p = Person(name='a1')
        p.flush()
    except:
        p.delete()
Traceback (most recent call last):
  File "./zzz.py", line 35, in <module>
    p.delete()
  File "<auto generated wrapper of delete() function>", line 2, in delete
  File "/home/mmarkk/.local/lib/python2.7/site-packages/pony/utils.py", line 88, in cut_traceback
    return func(*args, **kwargs)
  File "/home/mmarkk/.local/lib/python2.7/site-packages/pony/orm/core.py", line 3866, in delete
    obj._delete_()
  File "/home/mmarkk/.local/lib/python2.7/site-packages/pony/orm/core.py", line 3838, in _delete_
    assert save_pos is not None
AssertionError
@socketpair

Also, how to write code that tries to create entity, and if it fail, do nothing, but without rollback operations that was issued prior to current? nested db_session? does not work!

My experiments shows sometimes CommitException, sometimes CacheIndexError, sometimes TransactionIntegrityError. How to write SAFE code?

It maybe SAVEPOINT, but AFAIK Pony does not support that.

@kozlovsky kozlovsky added the bug label Jul 27, 2015
@kozlovsky kozlovsky self-assigned this Jul 27, 2015
@kozlovsky
Contributor

Thanks for the reporting! I'll try to fix it.

How to write SAFE code?

Creation of object can fail by different reasons. Some of them are valid situations, others are bugs in the application logic. I think it is not reasonable to treat all possible creation failures in the same way, because it can mask some application bugs.

Typically the normal reason of failure during object creation is the violation of some unique index. I think that the most correct way is to check such indexes beforehand:

    username = request.args['username']
    email = request.args['email']
    error = None
    p = Person.get(username=username)
    if p is not None:
        error = 'Username is already in use'
    else:
        p = Person.get(email=email)
        if p is not None:
            error = 'Email address is already in use'
        else:
            p = Person(username=username, email=email)
    if error is not None:
        ...

This way may be a bit verbose, but it have two benefits: (1) you can specify the most detailed error message, and (2) if creation of an object still leads to error, you will know that this error was caused by a bug in the application code.

My experiments shows sometimes CommitException, sometimes CacheIndexError, sometimes TransactionIntegrityError.

CacheIndexError arises when Pony sees that the index constraint is violated before sending the query to the database (that is, another object was already loaded into identity map). TransactionIntegfrityError arises when the database fails to execute SQL query. CommitException arises when any of the above exception happened upon exiting of db_session? and the initial exception caused the error can be retrieved as e.exceptions[0].

It maybe SAVEPOINT, but AFAIK Pony does not support that.

I hope we can add SAVEPOINT support in the future, but I don't think it is a good way to solve object creation problem, because it can mask the exact reason of an error.

In principle, we can change the code for creation of new object in such a way that Pony will automatically check the uniqueness of each index before execution of INSERT command, but in general it is inefficient, because it multiplies the number of roundtrips to the database.

@socketpair

Yes, but be race-condition may occur between checking of record existence and actual inserting.

@kozlovsky
Contributor

We can add get_or_create method which will not have race conditions. But restriction for such atomic method will be that the table should have exactly one one unique index. This restriction is imposed by database implementation of atomic upsert command.

@socketpair

Do not see any requirements to have only one unique column.

@JakeAustwick
  • 1 for upsert command. Have been using a custom hacky method to do upserts for several months now.
@kozlovsky kozlovsky closed this in b152976 Feb 3, 2016
@kozlovsky kozlovsky added this to the 0.6.3 milestone Feb 4, 2016
@kozlovsky kozlovsky changed the title from Assertion failed after exception to Incorrect behavior of obj.flush(): assertion failed after exception Feb 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment