AssertionError when changing part of a composite key #122

Closed
mkalinski opened this Issue Mar 31, 2015 · 1 comment

Projects

None yet

2 participants

@mkalinski

When changing the value of a foreign key attribute, if the attribute is a part of a composite key, then pony throws AssertionError.

This is a semi-minimal setup where the error occurs:

#!/usr/bin/python

import pony.orm as po

db = po.Database()

class A(db.Entity):
    id = po.PrimaryKey(int, auto=True)
    adata = po.Required(str)

    cset = po.Set("C")

class B(db.Entity):
    id = po.PrimaryKey(int, auto=True)
    bdata = po.Required(str)

    cset = po.Set("C")

class C(db.Entity):
    id = po.PrimaryKey(int, auto=True)
    cdata = po.Required(str)

    akey = po.Required("A")
    bkey = po.Required("B")

    po.composite_key(akey, bkey)

@po.db_session
def init_pony():
    a = A(adata="foo")
    b = B(bdata="bar")
    C(cdata="baz", akey=a, bkey=b)

@po.db_session
def break_pony():
    newa = A(adata="oof")
    c = C.get(cdata="baz")
    c.akey = newa

if __name__ == "__main__":
    db.bind("mysql", user="xxx", passwd="xxx", db="xxx")
    db.generate_mapping(check_tables=True, create_tables=True)
    init_pony()
    break_pony()

This is the traceback:

  File "./minitest.py", line 38, in break_pony
    c.akey = newa
  File "<auto generated wrapper of __set__() function>", line 2, in __set__
  File "/usr/local/lib/python2.7/dist-packages/pony/utils.py", line 88, in cut_traceback
    return func(*args, **kwargs)
  File "/usr/local/lib/python2.7/dist-packages/pony/orm/core.py", line 1454, in __set__
    elif not is_reverse_call: attr.update_reverse(obj, old_val, new_val, undo_funcs)
  File "/usr/local/lib/python2.7/dist-packages/pony/orm/core.py", line 1522, in update_reverse
    if old_val not in (None, NOT_LOADED): reverse.reverse_remove((old_val,), obj, undo_funcs)
  File "/usr/local/lib/python2.7/dist-packages/pony/orm/core.py", line 2130, in reverse_remove
    assert setdata is not None
AssertionError

Judging by variable values in debugger, there appears to be some sort of mismatch:

> /usr/local/lib/python2.7/dist-packages/pony/orm/core.py(2130)reverse_remove()
-> assert setdata is not None
(Pdb) l
2125            undo = []
2126            cache = item._session_cache_
2127            objects_with_modified_collections = cache.modified_collections[attr]
2128            for obj in objects:
2129                setdata = obj._vals_.get(attr)
2130 ->             assert setdata is not None
2131                assert item in setdata
2132                if setdata.removed is None: setdata.removed = set()
2133                else: assert item not in setdata.removed
2134                in_added = setdata.added and item in setdata.added
2135                was_modified_earlier = obj in objects_with_modified_collections
(Pdb) p obj
A[1]
(Pdb) p attr
B.cset

Is this a bug, or am I doing something wrong?

@kozlovsky kozlovsky closed this in c579cba Apr 1, 2015
@kozlovsky kozlovsky added the bug label Apr 1, 2015
@kozlovsky kozlovsky self-assigned this Apr 1, 2015
@kozlovsky kozlovsky added this to the 0.6.2 milestone Apr 1, 2015
@kozlovsky
Contributor

Thanks for the reporting! This nasty bug remained unnoticed for a long time since release 0.5.2.
Should be fixed now.

In case your are interested, the bug appeared when we ported the code to Python 3. The previous (simplified) code looks like:

attr = ...
for attrs, i in attr.composite_keys:
    vals = map(obj._vals_.get, attrs)

Our Python 3 porting strategy is to make the same source work both in Python 2 and in Python 3. In Python 3 the map function was changed - it now returns a generator instead of a list. Because of this, it was necessary to change the last line of the code in order to have a list value assigned to vals variable both in Python 2 and Python 3.

So, we changed the above code in the following way:

attr = ...
for attrs, i in attr.composite_keys:
    vals = [ obj._vals_[attr] for attr in attrs ]

Looks clear and more understandable then the previous version. But this code have a nasty bug: the attr variable of the list comprehension in Python 2 leaks into the outer scope. So, after the loop the value of the attr variable become changed.

It is interesting that the Python 3 documentation in such situations recommends to change map function to list comprehension and does not caveat about this potential issue:

map() and filter() return iterators. If you really need a list and the input sequences are all of equal length, a quick fix is to wrap map() in list(), e.g. list(map(...)), but a better fix is often to use a list comprehension

@kozlovsky kozlovsky added a commit that referenced this issue Jan 11, 2016
@kozlovsky kozlovsky Pony ORM Release 0.6.2 (2015-01-11)
The documentation was moved from this repo to a separate one at https://github.com/ponyorm/pony-doc
The compiled version can be found at https://docs.ponyorm.com

# New features

* Python 3.5 support
* #132, #145: raw_sql() function was added
* #126: Ability to use @db_session with generator functions
* #116: Add support to select by UUID
* Ability to get string SQL statement using the Query.get_sql() method
* New function delete(gen) and Query.delete(bulk=False)
* Now it is possible to override Entity.__init__() and declare custom entity methods

# Backward incompatible changes

* Normalizing table names for symmetric relationships
* Autostrip - automatically remove leading and trailing characters

# Bugfixes

* #87: Pony fails with pymysql installed as MySQLdb
* #118: Pony should reconnect if previous connection was created before process was forked
* #121: Unable to update value of unique attribute
* #122: AssertionError when changing part of a composite key
* #127: a workaround for incorrect pysqlite locking behavior
* #136: Cascade delete does not work correctly for one-to-one relationships
* #141, #143: remove restriction on adding new methods to entities
* #142: Entity.select_random() AssertionError
* #147: Add 'atom_expr' symbol handling for Python 3.5 grammar
ab09f64
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment