Merge lp:~cr3/storm/reference_is_in into lp:storm

[Marc Tardif]

[Marc Tardif (cr3)]

Proposing is_method to Reference class so that it would be possible to call this:

  Computer.account.is_in(account_objects)

Instead of

  Computer.account_id.is_in([a.id for a in account_objects])

[James Henstridge (jamesh)]

The implementation looks wrong for compound key references when a list of objects is passed in:

    for i, other in enumerate(others):
        where = self._relation.get_where_for_local(other)
        others[i] = where.expr2

In such a case, get_where_for_local() will return an And() object, which has no expr2 attribute.

Probably the best you can do in such a situation would be something like:

    return Or(*[self._relation.get_where_for_local(other)
                for other in others])

You can do better for PostgreSQL and MySQL with "(key1, key2) in ((val1, val2), (val1', val2'), ...)" but that doesn't seem to work with SQLite.

You'd also want to make sure it does the right thing when passed an empty list.

review: Needs Fixing

[Marc Tardif (cr3)]

Interestingly, I had originally implemented the is_in method on references using the Or expression but I later found it preferable to use the In expression. However, in order to support a wider range of backends, I'm fine with reverting to the Or expression.

Thanks for suggesting to test the corner case when passing an empty list. I made sure to write the corresponding test which then failed when just uring the Or expression. So, when the given argument to the is_in method is an empty list, it simply returns False which seems to work fine.

The branch has been updated with these changes.

[lp:~cr3/storm/reference_is_in]

306. By Marc Tardif on 2009-06-01

Fixed is_in method in references to support compound key references and added test when passed an empty list.

[James Henstridge (jamesh)]

> Interestingly, I had originally implemented the is_in method on references
> using the Or expression but I later found it preferable to use the In
> expression. However, in order to support a wider range of backends, I'm fine
> with reverting to the Or expression.
>
> Thanks for suggesting to test the corner case when passing an empty list. I
> made sure to write the corresponding test which then failed when just uring
> the Or expression. So, when the given argument to the is_in method is an empty
> list, it simply returns False which seems to work fine.
>
> The branch has been updated with these changes.

With your new changes, passing an Expr object to is_in() will now fail. With the previous version of your patch it was possible to do something like:

    Employee.company.is_in(Select(Company.id, Company.name.like("%corp%")))

The cast to list() in your new version would fail here.

[Jamu Kakar (jkakar)]

Is anything going on with this branch?

Unmerged revisions

306. By Marc Tardif on 2009-06-01

Fixed is_in method in references to support compound key references and added test when passed an empty list.

305. By Marc Tardif on 2009-04-01

Added is_in method to Reference class.

=== modified file 'storm/references.py'
--- storm/references.py	2009-02-19 16:44:33 +0000
+++ storm/references.py	2009-08-13 20:20:47 +0000
@@ -22,7 +22,7 @@
 from storm.store import Store, get_where_for_args
 from storm.variables import LazyValue
 from storm.expr import (
-    Select, Column, Exists, ComparableExpr, LeftJoin, Not, SQLRaw,
+    Select, Column, Exists, ComparableExpr, LeftJoin, Or, Not, SQLRaw,
     compare_columns, compile)
 from storm.info import get_cls_info, get_obj_info
 
@@ -203,6 +203,14 @@
     def __ne__(self, other):
         return Not(self == other)
 
+    def is_in(self, others):
+        others = list(others)
+        if not others:
+            return False
+
+        return Or(*[self._relation.get_where_for_local(other)
+            for other in others])
+
 
 class ReferenceSet(object):
 
 
=== modified file 'tests/store/base.py'
--- tests/store/base.py	2009-07-31 01:53:08 +0000
+++ tests/store/base.py	2009-08-13 20:20:47 +0000
@@ -3008,6 +3008,18 @@
             LinkWithRef, LinkWithRef.myself != (link.foo_id, link.bar_id)))
         self.assertTrue(link not in result, "%r not in %r" % (link, result))
 
+    def test_reference_is_in(self):
+        foo1 = self.store.get(Foo, 10)
+        foo2 = self.store.get(Foo, 20)
+
+        result = self.store.find(Bar, Bar.foo.is_in([foo1, foo2]))
+        self.assertEquals(result.count(), 2)
+        self.assertEquals([100, 200], sorted(bar.id for bar in result))
+
+    def test_reference_is_in_with_empty(self):
+        result = self.store.find(Bar, Bar.foo.is_in([]))
+        self.assertEquals(result.count(), 0)
+
     def test_reference_self(self):
         selfref = self.store.add(SelfRef())
         selfref.id = 400