Thread: [SQLObject] More bugs with inheritance and transactions. Patch attached
SQLObject is a Python ORM.
Brought to you by:
ianbicking,
phd
From: Evandro V. M. <ev...@as...> - 2005-08-09 18:00:55
|
Trying to run the script bellow I got many errors: 1- SQLObject is not setting properly the child_name column for inheritable tables in database. Take a look first in the records saved before apply my patch. 2- SQLObject can not list properly all the objects of a inheritable tables. Try doing a table.select().count() and after that list(table.select()). Probably this happens because the first problem. 3- SQLObject is not setting the parent columns like '_SO_val_', as defined by instanceName function. So we can't change column values for inherited objects. 4- Because the first problem we also couldn't have more than 2 levels of inheritance. Some comments About my patch: * The code removed in iteration.py seems to be completely wrong for me. Print the select statement before and after this code and you will see that the change doesn't make so much sense. * In main.py I had to remove the delIndex reference because after test my applications I saw some kind of objects defined which doesn't have this attribute and, in this case, an exception is raised. I'm not sure if removing this line is the best and even the rigth approach. import datetime from sqlobject import * from sqlobject.inheritance import InheritableSQLObject from config import conn __connection__ = connectionForURI(conn) __connection__.autoCommit = False class SystemData(SQLObject): model_created = DateTimeCol(default=datetime.datetime.now()) class InheritableModel(InheritableSQLObject): sys_data = ForeignKey('SystemData', default=None) def _create(self, id, **kw): if 'sys_data' in kw: raise TypeError("Invalid argument 'sys_data'") kw['sys_data'] = SystemData(connection=self._connection) InheritableSQLObject._create(self, id, **kw) class AbstractSellable(InheritableModel): description = StringCol() class Product(AbstractSellable): supplier_name = StringCol() class Product2(Product): second_name = StringCol() class Product3(Product2): third_name = StringCol() t = __connection__.transaction() tables = [SystemData, InheritableModel, AbstractSellable, Product, Product2, Product3] for table in tables: table.dropTable(ifExists=True, cascade=True, connection=t) table.createTable(connection=t) table.sqlmeta.cacheValues = False t.commit() t2 = __connection__.transaction() InheritableModel(connection=t2) AbstractSellable(description='Abstract', connection=t2) Product(description='Bed', supplier_name='A company', connection=t2) Product2(second_name='second name', supplier_name='second supplier', description='second desc', connection=t2) Product3(third_name='third name', supplier_name='third supplier', description='third description', connection=t2, second_name='second name') t2.commit() # # Testing transactions with inheritance # def get_class_names(table): class_names = [] for item in table.select(): class_names.append(item.__class__.__name__) return class_names tables.remove(tables[0]) for table in tables: print '%s, count: %s, list: %s' % (table.__name__, table.select(connection=t2).count(), get_class_names(table)) print '-'*70 # Testing queries query = Product.q.description == 'Bed' print list(Product.select(query, connection=t2)) query = AbstractSellable.q.description == 'Abstract' print list(AbstractSellable.select(query, connection=t2)) query = Product2.q.description == 'second desc' print list(Product2.select(query, connection=t2)) query = Product3.q.description == 'third description' print list(Product3.select(query, connection=t2)) # Testing fetching and changing data query = Product2.q.description == 'second desc' p = Product2.select(query, connection=t2)[0] p.second_name = 'ev' t2.commit() print 'done' product = Product.select(connection=t)[0] product.supplier_name = 'evandro' t.commit() print 'data changed' Using trunk(revision 897), Postgres 7.4.7 and psycopg 1.1.18. -- Evandro Vale Miquelito : ev...@as... Async Open Source - Brazil | http://www.async.com.br |
From: Philippe N. <ph...@ba...> - 2005-08-10 06:11:05
|
Le mardi 09 ao=FBt 2005 =E0 13:13 -0300, Evandro Vale Miquelito a =E9crit= : > 3- SQLObject is not setting the parent columns like '_SO_val_', as=20 > defined by instanceName function. So we can't change column values for=20 > inherited objects. >=20 Hrm hrm :) This reminds some mails few days ago about failing regression tests ..... Philippe |
From: Oleg B. <ph...@ph...> - 2005-08-10 11:53:48
|
On Tue, Aug 09, 2005 at 01:13:03PM -0300, Evandro Vale Miquelito wrote: > Trying to run the script bellow I got many errors: The program is too big, there is too much output.:( > 1- SQLObject is not setting properly the child_name column for Where?! > inheritable tables in database. Take a look first in the records saved > before apply my patch. At what record? There are too many. > 3- SQLObject is not setting the parent columns like '_SO_val_', as > defined by instanceName function. So we can't change column values for > inherited objects. That's a known deficiency. But is it a deficiency at all? SQLObject is not hiding SQL layer, it just maps SQL unto Python. If you want to change a column - change it in the table the column belongs to. > 4- Because the first problem we also couldn't have more than 2 levels of > inheritance. That's simply wrong. The inheritance code has been using in a really big program that has the following hierarchy: class DataTable(InheritableSQLObject): """Common parent for all data definitions class ClassifiedDatatable(DataTable): """A table with classCode column class DatatableWithII(ClassifiedDatatable): """A table with instance identifiers""" class Entity(DatatableWithII): """A physical thing, group of physical things or an organization class Person(Entity): """A human being. class Organization(Entity): """A formalized group of entities with a common purpose And this is only a small part! > Some comments About my patch: > > * The code removed in iteration.py seems to be completely wrong for me. Why?! It did an optimization for the case where one has a lot of rows of every child class (we really have a lot of Entities, Persons and Organizations). > Print the select statement before and after this code and you will see > that the change doesn't make so much sense. And what is the result? > * In main.py I had to remove the delIndex reference because after test > my applications I saw some kind of objects defined which doesn't have > this attribute and, in this case, an exception is raised. I'm not sure > if removing this line is the best and even the rigth approach. I very much doubt sqlmeta attributes have anything with inheritance. BTW, in modern days you don't need to manipulate connection objects, just pass in parameters in the URI: __connection__ = connectionForURI("sqlite:/:memory:?debug=1&autoCommit=") Pass an empty string for autoCommit... Oleg. -- Oleg Broytmann http://phd.pp.ru/ ph...@ph... Programmers don't die, they just GOSUB without RETURN. |
From: Evandro V. M. <ev...@as...> - 2005-08-10 14:18:24
|
Oleg Broytmann wrote: > The program is too big, there is too much output.:( Because there is more than one problem and I tried to explain all of them throught that script. Bad idea I see... >>1- SQLObject is not setting properly the child_name column for > Where?! You should take a look at your database after running the script. There is a new smaller script in the end of this mail. After running that using SQLObject trunk(revision 897) I got this: evandro=# select * from data_table; id | child_name ----+----------------- 1 | 2 | DatatableWithII 3 | Entity 4 | Organization (4 rows) evandro=# select * from classified_datatable; id | child_name ----+------------ 2 | 3 | 4 | (3 rows) evandro=# select * from datatable_with_i_i; id | child_name ----+------------ 2 | 3 | 4 | (3 rows) evandro=# select * from entity; id | child_name ----+------------ 3 | 4 | (2 rows) evandro=# select * from person; id | child_name ----+------------ (0 rows) evandro=# select * from organization; id | child_name ----+------------ 4 | (1 row) How can we expect inheritance works in this case ? Traceback after running the script follows: File "./small_test.py", line 50, in ? print '%s, count: %s, list: %s' % (table.__name__, File "./small_test.py", line 45, in get_class_names for item in table.select(): File "/home/evandro/devel/sqlobject/inheritance/iteration.py", line 57, in next childResults=childResults, connection=self.dbconn) File "/home/evandro/devel/sqlobject/inheritance/__init__.py", line 81, in get return val._childClasses[childName].get(id, connection=connection, selectResults=childResults) KeyError: 'DatatableWithII' > At what record? There are too many. I think this question has now an answer. > That's a known deficiency. But is it a deficiency at all? SQLObject is > not hiding SQL layer, it just maps SQL unto Python. If you want to change > a column - change it in the table the column belongs to. What you mean here ? Probably I didn't explain exactly the problem. I'm having problems trying to access only foreign key attributes of inherited tables. Lets supose I have the following table: class SellableItem(InheritableSQLObject): sale = ForeignKey('Sale') if I have an object sellableitem of this table type, I can't do print sellableitem.sale, which will give me a Sale object, without having an exception. I can always in this case access sellableitem.saleID. I can only set this attribute using obj.set(**args), never through obj.sale = saleobj > That's simply wrong. The inheritance code has been using in a really big > program that has the following hierarchy: Fine. Just try running my script which uses your example: from sqlobject import * from sqlobject.inheritance import InheritableSQLObject __connection__ = connectionForURI('postgres://evandro@anthem/evandro') __connection__.autoCommit = False class DataTable(InheritableSQLObject): """Common parent for all data definitions""" class ClassifiedDatatable(DataTable): """A table with classCode column""" class DatatableWithII(ClassifiedDatatable): """A table with instance identifiers""" class Entity(DatatableWithII): """A physical thing, group of physical things or an organization""" class Person(Entity): """A human being.""" class Organization(Entity): """A formalized group of entities with a common purpose""" t = __connection__.transaction() tables = [DataTable, ClassifiedDatatable, DatatableWithII, Entity, Person, Organization] for table in tables: table.dropTable(ifExists=True, cascade=True, connection=t) table.createTable(connection=t) table.sqlmeta.cacheValues = False t.commit() DataTable(connection=t) DatatableWithII(connection=t) Entity(connection=t) Organization(connection=t) t.commit() def get_class_names(table): class_names = [] for item in table.select(): class_names.append(item.__class__.__name__) return class_names for table in tables: print '%s, count: %s, list: %s' % (table.__name__, table.select(connection=t).count(), get_class_names(table)) print '-'*70 print 'done' -- Evandro Vale Miquelito : ev...@as... Async Open Source - Brazil | http://www.async.com.br |
From: Oleg B. <ph...@ph...> - 2005-08-10 15:08:42
|
On Wed, Aug 10, 2005 at 11:18:08AM -0300, Evandro Vale Miquelito wrote: > You should take a look at your database after running the script. > There is a new smaller script in the end of this mail. After running > that using SQLObject trunk(revision 897) I got this: > > evandro=# select * from data_table; > id | child_name > ----+----------------- > 1 | > 2 | DatatableWithII > 3 | Entity > 4 | Organization > (4 rows) > > evandro=# select * from classified_datatable; > id | child_name > ----+------------ > 2 | > 3 | > 4 | > (3 rows) > > evandro=# select * from datatable_with_i_i; > id | child_name > ----+------------ > 2 | > 3 | > 4 | > (3 rows) > > evandro=# select * from entity; > id | child_name > ----+------------ > 3 | > 4 | > (2 rows) > > evandro=# select * from person; > id | child_name > ----+------------ > (0 rows) > > evandro=# select * from organization; > id | child_name > ----+------------ > 4 | > (1 row) > > How can we expect inheritance works in this case ? That's absolutely normal. childName column is None (child_name IS NULL) for children rows; childName is only set in the topmost parent row. > Traceback after running the script follows: > > File "./small_test.py", line 50, in ? > print '%s, count: %s, list: %s' % (table.__name__, > File "./small_test.py", line 45, in get_class_names > for item in table.select(): > File "/home/evandro/devel/sqlobject/inheritance/iteration.py", line > 57, in next > childResults=childResults, connection=self.dbconn) > File "/home/evandro/devel/sqlobject/inheritance/__init__.py", line > 81, in get > return val._childClasses[childName].get(id, connection=connection, > selectResults=childResults) > KeyError: 'DatatableWithII' Ouch! :( I see. Something strange goes here. Oleg. -- Oleg Broytmann http://phd.pp.ru/ ph...@ph... Programmers don't die, they just GOSUB without RETURN. |
From: Evandro V. M. <ev...@as...> - 2005-08-11 20:57:38
Attachments:
patchev.diff
|
Oleg Broytmann wrote: >>How can we expect inheritance works in this case ? > > > That's absolutely normal. childName column is None (child_name IS NULL) > for children rows; childName is only set in the topmost parent row. I think you made a mistake here in this comment or in the revision 900 because I saw that now we are setting the childName for each parent too. I think this is the right behavior and that's how things were working until revision 854. Sorry but testing revision 900 I got more problems. Did you test some of my scripts ? :-) Here is another script with comments where we have problems. I had to apply the patch attached to make it run almost perfectly. Almost because we still have problems with the last line after that. import inspect from sqlobject import * from sqlobject.inheritance import InheritableSQLObject __connection__ = connectionForURI('postgres://evandro@anthem/evandro') __connection__.autoCommit = False class Sale(SQLObject): description = StringCol() class DataTable(InheritableSQLObject): """Common parent for all data definitions""" class ClassifiedDatatable(DataTable): """A table with classCode column""" class DatatableWithII(ClassifiedDatatable): """A table with instance identifiers""" sale = ForeignKey('Sale') class Entity(DatatableWithII): """A physical thing, group of physical things or an organization""" class Person(Entity): """A human being.""" class Organization(Person): """A formalized group of entities with a common purpose""" t = __connection__.transaction() tables = [DataTable, ClassifiedDatatable, Sale, DatatableWithII, Entity, Person, Organization] for table in tables: table.dropTable(ifExists=True, cascade=True, connection=t) table.createTable(connection=t) table.sqlmeta.cacheValues = False t.commit() DataTable(connection=t) ClassifiedDatatable(connection=t) sale = Sale(description='A sale', connection=t) DatatableWithII(sale=sale, connection=t) Entity(connection=t, sale=sale) Person(connection=t, sale=sale) Organization(connection=t, sale=sale) t.commit() def get_class_names(table): class_names = [] for item in table.select(): class_names.append(item.__class__.__name__) return class_names # XXX Bug here. See listing data for tables with more than two # leves of inheritance for table in tables: print '%s, count: %s, list: %s' % (table.__name__, table.select(connection=t).count(), get_class_names(table)) print '-'*70 print 'done' # XXX Bug: we can't list all the members for a SQLObject without # raise an exception. We also have lots of DeprecationWarning after # that. Is it correct? assert inspect.getmembers(Sale) assert inspect.getmembers(DataTable) assert inspect.getmembers(Entity) sale = Sale(description='second sale', connection=t) dtable = Entity(sale=sale, connection=t) assert dtable.saleID # XXX Bug: we can not get the object of a foreignkey attribute for an # InheritableSQLObject instance. assert dtable.sale -- Evandro Vale Miquelito : ev...@as... Async Open Source - Brazil | http://www.async.com.br |
From: Andrew B. <an...@ca...> - 2005-08-12 00:42:53
|
On Thu, Aug 11, 2005 at 05:57:01PM -0300, Evandro Vale Miquelito wrote: > Oleg Broytmann wrote: > > >>How can we expect inheritance works in this case ? > > > > > > That's absolutely normal. childName column is None (child_name IS NULL) > >for children rows; childName is only set in the topmost parent row. > > I think you made a mistake here in this comment or in the revision 900 > because I saw that now we are setting the childName for each parent too. > I think this is the right behavior and that's how things were working > until revision 854. > > Sorry but testing revision 900 I got more problems. Did you test some of > my scripts ? :-) If you can turn your scripts into test cases that can be added to the test suite, then testing that they work will be easier :) -Andrew. |
From: Evandro V. M. <ev...@as...> - 2005-08-12 17:18:20
|
Andrew Bennetts wrote: > If you can turn your scripts into test cases that can be added to the test > suite, then testing that they work will be easier :) Actually I was planning to do that next time. Anyway, thanks for your comment. -- Evandro Vale Miquelito : ev...@as... Async Open Source - Brazil | http://www.async.com.br |
From: Oleg B. <ph...@ph...> - 2005-08-12 07:39:23
|
On Thu, Aug 11, 2005 at 05:57:01PM -0300, Evandro Vale Miquelito wrote: > Sorry but testing revision 900 I got more problems. Did you test some of > my scripts ? :-) Some of them. Except for the script with transactions. Now I'll look at it. > Index: inheritance/iteration.py > =================================================================== > --- inheritance/iteration.py (revision 900) > +++ inheritance/iteration.py (working copy) > @@ -12,15 +12,6 @@ class InheritableIteration(Iteration): > # will be attached by sourceClass.get) > lazyColumns = select.ops.get('lazyColumns', False) > sourceClass = select.sourceClass > - if sourceClass._parentClass and not lazyColumns: > - addClauses = [] > - while sourceClass._parentClass: > - addClauses.append(sourceClass._parentClass.q.childName > - == sourceClass.__name__) > - sourceClass = sourceClass._parentClass > - select = select.__class__(sourceClass, > - sqlbuilder.AND(select.clause, *addClauses), > - select.clauseTables, **select.ops) Oh no, not again! I hope you wouldn't suggest to cut my head off when I say my head aches ;) Instead of removing why not just fix it? The optimization helps to avoid .get()ing every child (i.e., avoid one SELECT for an every child) which is a big win when you have hundreds of rows. Without that part InheritableIteration has no sense and can be removed altogether. Oleg. -- Oleg Broytmann http://phd.pp.ru/ ph...@ph... Programmers don't die, they just GOSUB without RETURN. |
From: Oleg B. <ph...@ph...> - 2005-08-12 08:02:43
|
On Thu, Aug 11, 2005 at 05:57:01PM -0300, Evandro Vale Miquelito wrote: > Index: inheritance/iteration.py > =================================================================== > --- inheritance/iteration.py (revision 900) > +++ inheritance/iteration.py (working copy) > @@ -12,15 +12,6 @@ class InheritableIteration(Iteration): > # will be attached by sourceClass.get) > lazyColumns = select.ops.get('lazyColumns', False) > sourceClass = select.sourceClass > - if sourceClass._parentClass and not lazyColumns: > - addClauses = [] > - while sourceClass._parentClass: > - addClauses.append(sourceClass._parentClass.q.childName > - == sourceClass.__name__) > - sourceClass = sourceClass._parentClass > - select = select.__class__(sourceClass, > - sqlbuilder.AND(select.clause, *addClauses), > - select.clauseTables, **select.ops) > super(InheritableIteration, self).__init__(dbconn, rawconn, select, keepConnection) > self.lazyColumns = lazyColumns > self.cursor.arraysize = self.defaultArraySize Ok, I have commited this part and the test (rev 902)... > Index: main.py > =================================================================== > --- main.py (revision 900) > +++ main.py (working copy) > @@ -808,7 +808,6 @@ class SQLObject(object): > addJoin = _sqlmeta_attr('addJoin', 2) > delJoin = _sqlmeta_attr('delJoin', 2) > addIndex = _sqlmeta_attr('addIndex', 2) > - delIndex = _sqlmeta_attr('delIndex', 2) > > # @classmethod > def _SO_setupSqlmeta(cls, new_attrs, is_base): ...but not this part. I still fail to see what delIndex has with inheritance. Oleg. -- Oleg Broytmann http://phd.pp.ru/ ph...@ph... Programmers don't die, they just GOSUB without RETURN. |
From: Evandro V. M. <ev...@as...> - 2005-08-10 14:40:03
|
Sorry, I forgot to create instances for all the tables. Here is and updated script: from sqlobject import * from sqlobject.inheritance import InheritableSQLObject __connection__ = connectionForURI('postgres://evandro@anthem/evandro') __connection__.autoCommit = False class DataTable(InheritableSQLObject): """Common parent for all data definitions""" class ClassifiedDatatable(DataTable): """A table with classCode column""" class DatatableWithII(ClassifiedDatatable): """A table with instance identifiers""" class Entity(DatatableWithII): """A physical thing, group of physical things or an organization""" class Person(Entity): """A human being.""" class Organization(Person): """A formalized group of entities with a common purpose""" t = __connection__.transaction() tables = [DataTable, ClassifiedDatatable, DatatableWithII, Entity, Person, Organization] for table in tables: table.dropTable(ifExists=True, cascade=True, connection=t) table.createTable(connection=t) table.sqlmeta.cacheValues = False t.commit() DataTable(connection=t) ClassifiedDatatable(connection=t) DatatableWithII(connection=t) Entity(connection=t) Person(connection=t) Organization(connection=t) t.commit() def get_class_names(table): class_names = [] for item in table.select(): class_names.append(item.__class__.__name__) return class_names for table in tables: print '%s, count: %s, list: %s' % (table.__name__, table.select(connection=t).count(), get_class_names(table)) print '-'*70 print 'done' -- Evandro Vale Miquelito : ev...@as... Async Open Source - Brazil | http://www.async.com.br |
From: Oleg B. <ph...@ph...> - 2005-08-10 14:46:36
|
On Wed, Aug 10, 2005 at 11:38:47AM -0300, Evandro Vale Miquelito wrote: > Sorry, I forgot to create instances for all the tables. You have not. :) Oleg. -- Oleg Broytmann http://phd.pp.ru/ ph...@ph... Programmers don't die, they just GOSUB without RETURN. |
From: Oleg B. <ph...@ph...> - 2005-08-11 16:13:34
|
Hello! I have just commited a patch. Revision 900(!) I reworked the patch to be in sync with sqlmeta changes, so the patch is a bit biger than your. I also added a test for deep inheritance; the test fails with the old code and passed with the new. Please test and report which of your problems it fixes, and which are still to be fixed. Oleg. -- Oleg Broytmann http://phd.pp.ru/ ph...@ph... Programmers don't die, they just GOSUB without RETURN. |
From: Evandro V. M. <ev...@as...> - 2005-08-11 22:44:33
Attachments:
patch_test.diff
|
Hi !! > I have just commited a patch. Revision 900(!) Thanks for your job here Oleg ! > I reworked the patch to be in sync with sqlmeta changes, so the patch is > a bit biger than your. I also added a test for deep inheritance; the test > fails with the old code and passed with the new. Sorry, but you forgot to test all the tables. Here is a patch for that. Try running the test again and you will see that it fails :-( > Please test and report which of your problems it fixes, and which are > still to be fixed. It's already reported in may last e-mail. -- Um abraço. Evandro Vale Miquelito : ev...@as... Async Open Source - Brazil | http://www.async.com.br |
From: Oleg B. <ph...@ph...> - 2005-08-10 15:38:15
|
On Tue, Aug 09, 2005 at 01:13:03PM -0300, Evandro Vale Miquelito wrote: > 4- Because the first problem we also couldn't have more than 2 levels of > inheritance. Aha! Now I started to see the light. I must admit now though we have a very deep and broad hierarchy we really use only the last two levels. In my examples we use only Entity, Person and Organization. DatatableWithII and all that are never used - they only keep the common code! I have never looked at the program from this point of view. :) I am sure it's not really because of the first problem. I'm testing your patch. It seems do the job even without removing optimization from InheritableIteration. Oleg. -- Oleg Broytmann http://phd.pp.ru/ ph...@ph... Programmers don't die, they just GOSUB without RETURN. |