launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/get_one into lp:maas.

Requested reviews:
  MAAS Maintainers (maas-maintainers)

For more details, see:
https://code.launchpad.net/~jtv/maas/get_one/+merge/121126

Discussed with Julian.  The get() in Django's ORM is great for looking up an object that you expect to be there, but not if you're expecting zero-or-one objects because it raises an exception in the zero case and expects you to catch it, typically immediately.  In this branch I provide something that's more like Storm's one(): “I firmly believe that this result set contains at most one item.  Give me that item, or None.”  To maintain flexibility in its use, it also supports other sequences and iterables: you don't exactly need to know whether you're passing it a raw result set or the result of a wrapper that happens to listify the raw result.

I believe this is healthier for situations where finding no object is part of regular, successful program flow.  Exceptions are not part of routine program flow.  Bertrand Meyer says an exception occurs when a function's preconditions are met but it cannot meet its postcondition.  A try/except construct covering a single statement differs from an if/else in that it conveys to readers that the “except” code path should be read in the light of failure recovery, and to the language implementation that the path's performance can be sacrificed in favour of the non-exceptional path.

This is not to be confused with Grace Hopper's “begging forgiveness is easier than asking for permission” paradigm, which applies to interaction with external resources.  There, the choice of code path is best discovered by attempting a single interaction.  That principle is useful to push the control-flow choice down in the call stack so that the program can avoid double interaction with the outside world, and with it, high costs or race conditions.

Jeroen
-- 
https://code.launchpad.net/~jtv/maas/get_one/+merge/121126
Your team MAAS Maintainers is requested to review the proposed merge of lp:~jtv/maas/get_one into lp:maas.

=== added file 'src/maasserver/utils/orm.py'
--- src/maasserver/utils/orm.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/utils/orm.py	2012-08-24 07:18:51 +0000
@@ -0,0 +1,54 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""ORM-related utilities."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'get_one',
+    ]
+
+from itertools import islice
+
+from django.core.exceptions import MultipleObjectsReturned
+
+
+def get_exception_class(items):
+    """Return exception class to raise.
+
+    If `items` looks like a Django ORM result set, returns the
+    `MultipleObjectsReturned` class as defined in that model.  Otherwise,
+    returns the generic class.
+    """
+    model = getattr(items, 'model', None)
+    return getattr(model, 'MultipleObjectsReturned', MultipleObjectsReturned)
+
+
+def get_one(items):
+    """Assume there's at most one item in `items`, and return it (or None).
+
+    If `items` contains more than one item, raise an error.  If `items` looks
+    like a Django ORM result set, the error will be of the same model-specific
+    Django `MultipleObjectsReturned` type that `items.get()` would raise.
+    Otherwise, a plain Django :class:`MultipleObjectsReturned` error.
+
+    :param items: Any sequence.
+    :return: The one item in that sequence, or None if it was empty.
+    """
+    # The only numbers we care about are zero, one, and "many."  Fetch
+    # just enough items to distinguish between these.  Use islice so as
+    # to support both sequences and iterators.
+    retrieved_items = tuple(islice(items, 0, 2))
+    length = len(retrieved_items)
+    if length == 0:
+        return None
+    elif length == 1:
+        return retrieved_items[0]
+    else:
+        raise get_exception_class(items)("Got more than one item.")

=== added file 'src/maasserver/utils/tests/test_orm.py'
--- src/maasserver/utils/tests/test_orm.py	1970-01-01 00:00:00 +0000
+++ src/maasserver/utils/tests/test_orm.py	2012-08-24 07:18:51 +0000
@@ -0,0 +1,97 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test ORM utilities."""
+
+from __future__ import (
+    absolute_import,
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from django.core.exceptions import MultipleObjectsReturned
+from maasserver.utils.orm import get_one
+from maastesting.factory import factory
+from maastesting.testcase import TestCase
+from mock import Mock
+
+
+class FakeModel:
+
+    class MultipleObjectsReturned(MultipleObjectsReturned):
+        pass
+
+    def __init__(self, name):
+        self.name == name
+
+    def __repr__(self):
+        return self.name
+
+
+class FakeQueryResult:
+    """Something that looks, to `get_one`, close enough to a Django model."""
+
+    def __init__(self, model, items):
+        self.model = model
+        self.items = items
+
+    def __iter__(self):
+        return self.items.__iter__()
+
+    def __repr__(self):
+        return "<FakeQueryResult: %r>" % self.items
+
+
+class TestGetOne(TestCase):
+
+    def test_get_one_returns_None_for_empty_list(self):
+        self.assertIsNone(get_one([]))
+
+    def test_get_one_returns_single_list_item(self):
+        item = factory.getRandomString()
+        self.assertEqual(item, get_one([item]))
+
+    def test_get_one_returns_None_from_any_empty_sequence(self):
+        self.assertIsNone(get_one("no item" for counter in range(0)))
+
+    def test_get_one_returns_item_from_any_sequence_of_length_one(self):
+        item = factory.getRandomString()
+        self.assertEqual(item, get_one(item for counter in range(1)))
+
+    def test_get_one_does_not_trigger_database_counting(self):
+        # Avoid typical performance pitfall of querying objects *and*
+        # the number of objects.
+        item = factory.getRandomString()
+        sequence = FakeQueryResult(type(item), [item])
+        sequence.__len__ = Mock(side_effect=Exception("len() was called"))
+        self.assertEqual(item, get_one(sequence))
+
+    def test_get_one_does_not_iterate_long_sequence_indefinitely(self):
+        # Avoid typical performance pitfall of retrieving all objects.
+        # In rare failure cases, there may be large numbers.  Fail fast.
+
+        class InfinityException(Exception):
+            """Iteration went on indefinitely."""
+
+        def infinite_sequence():
+            """Generator: count to infinity (more or less), then fail."""
+            for counter in range(3):
+                yield counter
+            raise InfinityException()
+
+        # Raises MultipleObjectsReturned as spec'ed.  It does not
+        # iterate to infinity first!
+        self.assertRaises(
+            MultipleObjectsReturned, get_one, infinite_sequence())
+
+    def test_get_one_raises_model_error_if_query_result_is_too_big(self):
+        self.assertRaises(
+            FakeModel.MultipleObjectsReturned,
+            get_one,
+            FakeQueryResult(FakeModel, range(2)))
+
+    def test_get_one_raises_generic_error_if_other_sequence_is_too_big(self):
+        self.assertRaises(MultipleObjectsReturned, get_one, range(2))