Merge lp:~mpontillo/maas/node-statistics-bug-1584926--part1 into lp:~maas-committers/maas/trunk

[Mike Pontillo]

Node API usability enhancements, part 1.

 * Refactor APIs to support summaries and counts more easily.
   - <nodes|machines|devices|region-controllers|rack-controllers> read:
     now supports the 'count' and 'summary' parameters.
 * Add new API to fetch machines using a "group by":
   - nodes by-type [summary=<bool> | count=<bool>]

[Mike Pontillo (mpontillo)]

I moved the changes for the devices and machines API to a separate branch. This branch is now just the refactoring needed to support summaries and counts, and the change in the nodes API to use that refactoring.

[Gavin Panella (allenap)]

This is useful functionality but the implementation is overcomplicated.

Almost all that's needed is a function to figure out what to show:

  SHOW_COUNT, SHOW_SUMMARY = "count", "summary"

  def get_show_option(request):
      summary = get_optional_param(
          request.GET, 'summary', default=False, validator=StringBool)
      count = get_optional_param(
          request.GET, 'count', default=False, validator=StringBool)
      if count:
          return SHOW_COUNT
      elif summary:
          return SHOW_SUMMARY
      else:
          return None

(I've said elsewhere that I think a single `show` parameter would be
better than two separate parameters, but that's a separate point.)

Then, in each handler method:

  def read(self, request):
      query = ...
      show = get_show_option(request)
      if show == SHOW_COUNT:
          return query.count()
      elif show == SHOW_SUMMARY:
          return list(map(self.summarize, query))
      else:
          return query.prefetch(...)

There's no need for a framework to support this.

The code added to OperationsHandlerMixin makes it hard to follow what's
going on. It's convoluted, and OperationsHandlerMixin would be the wrong
place for it anyway.

One day it might conceivably be necessary to have some more frameworky
support for this behaviour, but not now, and this wouldn't be it. As is,
this ties us closer to Piston, and it's not well structured.

The prefetch and summary methods are not too bad, but where they're
placed is too general. Prefetching can vary; it's unlikely that a whole
handler, which can process GET, POST, DELETE, PUT and custom operations,
is always going to have the same needs for prefetching. Same goes for
summaries.

result_summary, query_or_count, and prepare_results don't fit together
well. The first especially, which has two isinstance checks to see if it
should actually get out of the way, begging the question: why is it
being called at all? Both query_or_count and result_summary could be
eliminated by small changes to prepare_results:

  def prepare_results(self, query, count=False, summary=False):
      if count:
          return query.count()
      elif summary:
          query = self._prefetch_for_summary_dump(query)
          return list(map(self._summarize, query))
      else:
          return self._prefetch_for_full_dump(query)

Even now prepare_results would be in the wrong place; every handler in
MAAS would get this method, but it's not so generally useful. And it's
so simple that it's not necessary; each handler method can figure this
stuff out on its own and not have to adopt this API. If a very clear
pattern emerges then maybe it's worth extracting a new method to do it,
but today is not that day.

I've put up a branch that removes the OperationsHandlerMixin code, adds
a get_show_option function, updates the handlers, and fixes a few broken
tests here:

  lp~allenap/maas/mpontillo--node-statistics-bug-1584926--part1

I did this to prove to myself that I wasn't talking nonsense above, but
it's good enough to use, and I think you should use it.

review: Needs Fixing

[Blake Rouse (blake-rouse)]

I also think this is getting rather large and not something that we should land in MAAS 2.0. We need to get rc1 today, this will just add more bug surface.

Lets target this for MAAS 2.1. Since this is a feature and not a bug, and it was requested basically in the MAAS 2.1 cycle we should be fixing it there and not in MAAS 2.0.

[Mike Pontillo (mpontillo)]

I'm fine with landing this in 2.1.

Gavin, did you look at the part2 branch? Because that was where the pattern emerged that led me to the refactoring (which I thought could one day be generally useful). But I'm fine with tabling this until we can have that design discussion. That was the main reason I asked you to look at this branch, to be honest.

[Mike Pontillo (mpontillo)]

Gavin, thanks for doing the derived branch.

I like what you did with get_show_option(), and I think it's debatable what the exact format is. We should agree on something future-proof because we'll have to live with it in 2.1, and any future APIs that mimic this functionality.

I also like your suggestion to simplify prepare_results.

I don't agree that my changes to the Mixin were too general. I think, while the code you wrote for by-type is easier to read, it's highly redundant if repeated 4 more times in the machines API (again, see part2).

So the question in my mind is, if OperationsHandlerMixin isn't the right place for it (and I somewhat suspected it wasn't), what is? Can we create another mixin class? I was on the fence, since I do feel it could be generally useful throughout the entire API.

[Gavin Panella (allenap)]

> Gavin, did you look at the part2 branch? Because that was where the
> pattern emerged that led me to the refactoring (which I thought could
> one day be generally useful). But I'm fine with tabling this until we
> can have that design discussion. That was the main reason I asked you
> to look at this branch, to be honest.

I did look at it briefly and didn't see anything that really changed my
mind. I have since looked at it in greater depth and seen something that
makes it even clearer that it's not a good pattern...

>
> ...
>
> I like what you did with get_show_option(), and I think it's debatable
> what the exact format is. We should agree on something future-proof
> because we'll have to live with it in 2.1, and any future APIs that
> mimic this functionality.

As I've said, I prefer a single parameter like show=[full|summary|count]
for example, because it's clearer (versus 2 booleans = 4 possibilities
to select only 3 options) and more concise. But, I grant it's only a
little thing, and it'll be wrapped up behind a client library for most
folk anyway.

>
> I also like your suggestion to simplify prepare_results.
>
> I don't agree that my changes to the Mixin were too general. I think,
> while the code you wrote for by-type is easier to read, it's highly
> redundant if repeated 4 more times in the machines API (again, see
> part2).

Putting it in a mix-in at all is too general; it is relevant only to GET
and POST+op, not to DELETE, PUT, or POST.

> So the question in my mind is, if OperationsHandlerMixin isn't the
> right place for it (and I somewhat suspected it wasn't), what is? Can
> we create another mixin class? I was on the fence, since I do feel it
> could be generally useful throughout the entire API.

The problematic thing I discovered is related to performance.

Dumping a node of any shade to JSON is inefficient already. Despite
prefetching there are a still a lot of queries performed for each node.
(There's plenty of work needed already to fix these kinds of issues,
should anyone want to do it.)

The prepare_results() pattern makes this worse. An example from
DevicesHandler.by_parent:

  return {
      node.system_id if by_system_id else node.hostname:
          self.prepare_results(
              self._query(request).filter(parent=node),
              count=count, summary=summary)
      for node in Node.objects.filter(children__isnull=False)
  }

Given a single machine with two child devices, a call to by_parent (not
summary, not count) needs 37 queries.

But given _two_ machines with two child devices each, a similar call to
by_parent needs 57 queries. That's 20 extra queries for 3 nodes. A MAAS
install of 100+ nodes might need 500 queries or more to service
by_parent.

For a summary the overhead is 8 queries per extra machine with two
children, and the overhead for a count is 2 queries.

I have spent some time trying to improve these figures but Django and
Piston do get in the way. Sadly, so does prepare_results, including the
alternate version I wrote. It's not a good fit however it's implemented.

I strongly think we need to start by writing these methods out the long
way, paying attention to performance too (with tests...

[MAAS Lander (maas-lander)]

Transitioned to Git.

lp:maas has now moved from Bzr to Git.
Please propose your branches with Launchpad using Git.

git clone https://git.launchpad.net/maas

Unmerged revisions

5141. By Mike Pontillo on 2016-06-16

Revert changes to devices.py and machines.py. (they'll be moved to another branch)

5140. By Mike Pontillo on 2016-06-16

Fix lint, review comments.

5139. By Mike Pontillo on 2016-06-16

More tests.

5138. By Mike Pontillo on 2016-06-16

Start adding test cases.

5137. By Mike Pontillo on 2016-06-15

Format imports.

5136. By Mike Pontillo on 2016-06-15

Add 'devices by-parent [by_system_id=<bool>'.

5135. By Mike Pontillo on 2016-06-15

Refactor result_summary() to be a little more defensive.

5134. By Mike Pontillo on 2016-06-15

Clean up API docs.

5133. By Mike Pontillo on 2016-06-15

Missed an occurance of ModelCountMixin.

5132. By Mike Pontillo on 2016-06-15

Remove ModelCountMixin. (using read with a parameter seems to make more sense)

=== modified file 'src/maasserver/api/nodes.py'
--- src/maasserver/api/nodes.py	2016-05-24 13:50:54 +0000
+++ src/maasserver/api/nodes.py	2016-06-16 19:59:56 +0000
@@ -15,6 +15,7 @@
 import bson
 from django.http import HttpResponse
 from django.shortcuts import get_object_or_404
+from formencode.validators import StringBool
 from maasserver.api.support import (
     admin_method,
     AnonymousOperationsHandler,
@@ -47,7 +48,13 @@
     Node,
     OwnerData,
 )
-from maasserver.models.node import typecast_to_node_type
+from maasserver.models.node import (
+    Device,
+    Machine,
+    RackController,
+    RegionController,
+    typecast_to_node_type,
+)
 from maasserver.models.nodeprobeddetails import get_single_probed_details
 from piston3.utils import rc
 from provisioningserver.power.schema import UNKNOWN_POWER_TYPE
@@ -281,6 +288,21 @@
     anonymous = AnonNodesHandler
     base_model = Node
 
+    def prefetch(self, nodes):
+        nodes = nodes.prefetch_related('interface_set__node')
+        nodes = nodes.prefetch_related('interface_set__ip_addresses')
+        nodes = nodes.prefetch_related('tags')
+        nodes = nodes.prefetch_related('zone')
+        return nodes
+
+    def _query(self, request):
+        """Helper method to filter the list of machines.
+
+        Based on the request, the list will be filtered based on the currently
+        logged in user, and whatever search criteria are supplied.
+        """
+        return filtered_nodes_list_from_request(request, model=self.base_model)
+
     def read(self, request):
         """List Nodes visible to the user, optionally filtered by criteria.
 
@@ -311,27 +333,42 @@
         :param agent_name: An optional agent name.  Only nodes relating to the
             nodes with matching agent names will be returned.
         :type agent_name: unicode
+
+        :param summary: If True, prints a summary of each node instead of the
+            complete object.
+        :type summary: bool
+
+        :param count: If True, prints a count of nodes instead of any
+            node details.
+        :type count: bool
         """
+        summary = get_optional_param(
+            request.GET, 'summary', default=False, validator=StringBool)
+        count = get_optional_param(
+            request.GET, 'count', default=False, validator=StringBool)
+        return self.prepare_results(
+            self._read(
+                request, summary=summary, count=count),
+            count=count, summary=summary)
 
-        if self.base_model == Node:
+    def _read(self, request, summary=False, count=False):
+        """List nodes visible to the user."""
+        if self.base_model == Node and not summary and not count:
             # Avoid circular dependencies
             from maasserver.api.devices import DevicesHandler
             from maasserver.api.machines import MachinesHandler
             from maasserver.api.rackcontrollers import RackControllersHandler
             nodes = list(chain(
-                DevicesHandler().read(request).order_by("id"),
-                MachinesHandler().read(request).order_by("id"),
-                RackControllersHandler().read(request).order_by("id"),
+                DevicesHandler().read(request),
+                MachinesHandler().read(request),
+                RackControllersHandler().read(request),
             ))
             return nodes
         else:
             nodes = filtered_nodes_list_from_request(request, self.base_model)
             # Prefetch related objects that are needed for rendering the
             # result.
-            nodes = nodes.prefetch_related('interface_set__node')
-            nodes = nodes.prefetch_related('interface_set__ip_addresses')
-            nodes = nodes.prefetch_related('tags')
-            nodes = nodes.prefetch_related('zone')
+            nodes = self.prefetch(nodes)
             return nodes.order_by('id')
 
     @admin_method
@@ -356,6 +393,46 @@
             raise MAASAPIValidationError(form.errors)
         form.save()
 
+    def summary(self, node):
+        """Returns a summary a node."""
+        return {
+            'system_id': node.system_id,
+            'hostname': node.hostname
+        }
+
+    @operation(idempotent=True)
+    def by_type(self, request):
+        """Prints a listing of nodes grouped by their type.
+
+        Optional arguments:
+        summary: If True, prints a summary of each node instead of the complete
+        object.
+        count: If True, prints a count of each node type instead of any
+        node details.
+
+        Also accepts any filter parameter the 'read' operation accepts.
+        """
+        summary = get_optional_param(
+            request.GET, 'summary', default=False, validator=StringBool)
+        count = get_optional_param(
+            request.GET, 'count', default=False, validator=StringBool)
+        label_to_model = {
+            "machines": Machine,
+            "devices": Device,
+            "rack-controllers": RackController,
+            "region-controllers": RegionController
+        }
+        if count:
+            # We don't have a definition for the displayed fields of a plain
+            # node, so only include that if it's a summary or a count.
+            label_to_model["nodes"] = Node
+        return {
+            label: self.prepare_results(
+                filtered_nodes_list_from_request(request, model=model),
+                count=count, summary=summary)
+            for label, model in label_to_model.items()
+        }
+
     @classmethod
     def resource_uri(cls, *args, **kwargs):
         return ('nodes_handler', [])
 
=== modified file 'src/maasserver/api/support.py'
--- src/maasserver/api/support.py	2016-05-20 14:31:34 +0000
+++ src/maasserver/api/support.py	2016-06-16 19:59:56 +0000
@@ -13,6 +13,7 @@
 from functools import wraps
 
 from django.core.exceptions import PermissionDenied
+from django.db.models.query import QuerySet
 from django.http import Http404
 from maasserver.api.doc import get_api_description_hash
 from maasserver.exceptions import MAASAPIBadRequest
@@ -260,6 +261,88 @@
         else:
             return function(self, request, *args, **kwargs)
 
+    def prefetch(self, query):
+        """Pre-fetches the specified query.
+
+        This method should pre-fetch anything anticipated to be recursively
+        queried for in the non-summary version of the output.
+
+        By default, does no prefetching.
+
+        Override this in a subclass to do prefetching in case result_summary()
+        in case non-summary output is requested.
+        """
+        return query
+
+    def summary(self, obj):
+        """Return a summarized version of the specified model object.
+
+        This method is intended to return the object in a condensed format,
+        with a minimal amount of useful identifying information.
+
+        Override this in a subclass to allow result_summary() to work
+        correctly.
+        """
+        raise NotImplementedError
+
+    def result_summary(self, result, summary=False, order_by='id'):
+        """Utility method to provide a shorter, summary output for the objects
+        in the specified query, if desired.
+
+        :param result: must be either a QuerySet, or an integer.
+            If an integer is passed in, it is simply returned. (This means
+            the user requested a count instead of the query results.)
+        :param summary: if True, returns a smaller object containing only
+            essential information, rather than the complete object.
+        :param order_by: optional field to order by (defaults to 'id').
+        """
+        if isinstance(result, int):
+            # The user chose to count the objects in the query, instead of
+            # returning the results of the query. So just give them the result.
+            return result
+        elif isinstance(result, QuerySet):
+            # If this is a QuerySet, we need to ensure we both prefetch related
+            # objects, and return it in a consistent order.
+            result = self.prefetch(result).order_by(order_by)
+            if not summary:
+                # Pass this through to Piston if we're not in summary mode.
+                return result
+            else:
+                # It would be nice if we could return a generator expression
+                # here, but that doesn't seem to be supported by Piston.
+                return [
+                    self.summary(obj) for obj in result
+                ]
+        else:
+            # We're not sure what this is. So just let Piston do whatever
+            # it was going to do with it.
+            return result
+
+    def query_or_count(self, filter, count=False):
+        """Returns either the results of the filter, or a count.
+
+        :param count: if True, calls count() on the specified filter and
+            returns that. Otherwise, returns the filter unchanged.
+        """
+        if count:
+            return filter.count()
+        else:
+            return filter
+
+    def prepare_results(self, query, count=False, summary=False):
+        """Prepares the result set for the specified query.
+
+        Calls the 'summary' and 'prefetch' methods on subclasses, as
+        appropriate.
+
+        :param count: If True, return a count of the objects that would be
+            returned, instead of the objects themselves.
+        :param summary: If True, return summarized objects, instead of the
+            objects themselves.
+        """
+        return self.result_summary(
+            self.query_or_count(query, count=count), summary=summary)
+
     @classmethod
     def decorate(cls, func):
         """Decorate all exported operations with the given function.
 
=== modified file 'src/maasserver/api/tests/test_machines.py'
--- src/maasserver/api/tests/test_machines.py	2016-06-09 23:39:53 +0000
+++ src/maasserver/api/tests/test_machines.py	2016-06-16 19:59:56 +0000
@@ -208,14 +208,49 @@
         machine1 = factory.make_Node()
         machine2 = factory.make_Node(
             status=NODE_STATUS.ALLOCATED, owner=self.user)
+        # This device should be ignored.
+        factory.make_Device()
         response = self.client.get(reverse('machines_handler'))
         parsed_result = json.loads(
             response.content.decode(settings.DEFAULT_CHARSET))
-
-        self.assertEqual(http.client.OK, response.status_code)
-        self.assertItemsEqual(
-            [machine1.system_id, machine2.system_id],
-            extract_system_ids(parsed_result))
+        self.assertEqual(http.client.OK, response.status_code)
+        self.assertItemsEqual(
+            [machine1.system_id, machine2.system_id],
+            extract_system_ids(parsed_result))
+
+    def test_GET_lists_machines_with_summary(self):
+        # The api allows for fetching the list of Machines.
+        machine1 = factory.make_Node()
+        machine2 = factory.make_Node(
+            status=NODE_STATUS.ALLOCATED, owner=self.user)
+        # This device should be ignored.
+        factory.make_Device()
+        response = self.client.get(reverse('machines_handler'), {
+            'summary': True
+        })
+        parsed_result = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        self.assertEqual(http.client.OK, response.status_code)
+        self.assertItemsEqual(
+            [machine1.system_id, machine2.system_id],
+            extract_system_ids(parsed_result))
+        # Ensure each result is a summary (only contains two items)
+        self.expectThat(len(parsed_result[0]), Equals(2))
+
+    def test_GET_lists_machines_with_count(self):
+        # The api allows for fetching the list of Machines.
+        num_hosts = random.choice([3, 5, 7, 9])
+        for _ in range(num_hosts):
+            factory.make_Node()
+        # This device should be ignored.
+        factory.make_Device()
+        response = self.client.get(reverse('machines_handler'), {
+            'count': True
+        })
+        parsed_result = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        self.assertEqual(http.client.OK, response.status_code)
+        self.expectThat(parsed_result, Equals(num_hosts))
 
     def test_GET_machines_issues_constant_number_of_queries(self):
         # XXX: GavinPanella 2014-10-03 bug=1377335
 
=== modified file 'src/maasserver/api/tests/test_nodes.py'
--- src/maasserver/api/tests/test_nodes.py	2016-05-24 22:05:45 +0000
+++ src/maasserver/api/tests/test_nodes.py	2016-06-16 19:59:56 +0000
@@ -25,6 +25,7 @@
 from maasserver.utils import ignore_unused
 from maasserver.utils.orm import reload_object
 from maastesting.djangotestcase import count_queries
+from testtools.matchers import Equals
 
 
 class AnonymousIsRegisteredAPITest(APITestCase.ForAnonymous):
@@ -272,8 +273,109 @@
             extract_system_ids_from_nodes(node_list))
 
 
-class TestNodesAPI(APITestCase.ForUser):
-    """Tests for /api/2.0/nodes/."""
+class TestAdminNodesAPI(APITestCase.ForAdmin):
+    """Tests for /api/2.0/nodes/. (administrative users)"""
+
+    def test_GET_by_type_groups_by_type(self):
+        # The api allows for fetching the list of Machines.
+        machine = factory.make_Node()
+        device = factory.make_Device()
+        regionrack = factory.make_RegionRackController()
+        rack = factory.make_RackController()
+        region = factory.make_RegionController()
+        response = self.client.get(reverse('nodes_handler'), {
+            'op': "by_type",
+            'summary': False,
+        })
+        result = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        self.assertEqual(http.client.OK, response.status_code)
+        # Since we're testing as a normal user...
+        self.assertItemsEqual(
+            [
+                machine.system_id,
+            ],
+            extract_system_ids(result["machines"]))
+        self.assertItemsEqual(
+            [
+                device.system_id,
+            ],
+            extract_system_ids(result["devices"]))
+        self.assertItemsEqual(
+            [
+                regionrack.system_id,
+                region.system_id
+            ],
+            extract_system_ids(result["region-controllers"]))
+        self.assertItemsEqual(
+            [
+                regionrack.system_id,
+                rack.system_id,
+            ],
+            extract_system_ids(result["rack-controllers"]))
+
+    def test_GET_by_type_with_summary_groups_by_type(self):
+        # The api allows for fetching the list of Machines.
+        machine = factory.make_Node()
+        device = factory.make_Device()
+        regionrack = factory.make_RegionRackController()
+        rack = factory.make_RackController()
+        region = factory.make_RegionController()
+        response = self.client.get(reverse('nodes_handler'), {
+            'op': "by_type",
+            'summary': False,
+        })
+        result = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        self.assertEqual(http.client.OK, response.status_code)
+        # Since we're testing as a normal user...
+        self.assertItemsEqual(
+            [
+                machine.system_id,
+            ],
+            extract_system_ids(result["machines"]))
+        self.assertItemsEqual(
+            [
+                device.system_id,
+            ],
+            extract_system_ids(result["devices"]))
+        self.assertItemsEqual(
+            [
+                regionrack.system_id,
+                region.system_id
+            ],
+            extract_system_ids(result["region-controllers"]))
+        self.assertItemsEqual(
+            [
+                regionrack.system_id,
+                rack.system_id,
+            ],
+            extract_system_ids(result["rack-controllers"]))
+
+    def test_GET_by_type_with_count_groups_by_type(self):
+        # The api allows for fetching the list of Machines.
+        factory.make_Node()
+        factory.make_Device()
+        factory.make_RegionRackController()
+        factory.make_RackController()
+        factory.make_RegionController()
+        response = self.client.get(reverse('nodes_handler'), {
+            'op': "by_type",
+            'count': True,
+        })
+        result = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        self.assertEqual(http.client.OK, response.status_code)
+        # Since we're testing as an admin, we can see everything.
+        self.assertThat(result["nodes"], Equals(5))
+        self.assertThat(result["machines"], Equals(1))
+        self.assertThat(result["devices"], Equals(1))
+        self.assertThat(result["rack-controllers"], Equals(2))
+        self.assertThat(result["region-controllers"], Equals(2))
+
+
+class TestUserNodesAPI(APITestCase.ForUser):
+    """Tests for /api/2.0/nodes/. (normal users)"""
 
     def test_handler_path(self):
         self.assertEqual(
@@ -284,14 +386,139 @@
         node1 = factory.make_Node()
         node2 = factory.make_Node(
             status=NODE_STATUS.ALLOCATED, owner=self.user)
+        device = factory.make_Device()
         response = self.client.get(reverse('nodes_handler'))
         parsed_result = json.loads(
             response.content.decode(settings.DEFAULT_CHARSET))
-
-        self.assertEqual(http.client.OK, response.status_code)
-        self.assertItemsEqual(
-            [node1.system_id, node2.system_id],
-            extract_system_ids(parsed_result))
+        self.assertEqual(http.client.OK, response.status_code)
+        self.assertItemsEqual(
+            [node1.system_id, node2.system_id, device.system_id],
+            extract_system_ids(parsed_result))
+
+    def test_GET_lists_nodes_with_summary(self):
+        # The api allows for fetching the list of Machines.
+        machine1 = factory.make_Node()
+        machine2 = factory.make_Node(
+            status=NODE_STATUS.ALLOCATED, owner=self.user)
+        device = factory.make_Device()
+        response = self.client.get(reverse('nodes_handler'), {
+            'summary': True
+        })
+        parsed_result = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+
+        self.assertEqual(http.client.OK, response.status_code)
+        self.assertItemsEqual(
+            [machine1.system_id, machine2.system_id, device.system_id],
+            extract_system_ids(parsed_result))
+        # Ensure each result is a summary (only contains two items)
+        self.expectThat(len(parsed_result[0]), Equals(2))
+
+    def test_GET_lists_nodes_with_count(self):
+        # The api allows for fetching the list of Machines.
+        num_hosts = random.choice([3, 5, 7, 9])
+        for _ in range(num_hosts):
+            factory.make_Node()
+            factory.make_Device()
+        response = self.client.get(reverse('nodes_handler'), {
+            'count': True
+        })
+        parsed_result = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        self.assertEqual(http.client.OK, response.status_code)
+        self.expectThat(parsed_result, Equals(num_hosts * 2))
+
+    def test_GET_by_type_groups_by_type(self):
+        # The api allows for fetching the list of Machines.
+        machine = factory.make_Node()
+        device = factory.make_Device()
+        # These should be ignored since we're a normal user.
+        factory.make_RegionRackController()
+        factory.make_RackController()
+        factory.make_RegionController()
+        response = self.client.get(reverse('nodes_handler'), {
+            'op': "by_type",
+            'summary': False,
+        })
+        result = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        self.assertEqual(http.client.OK, response.status_code)
+        self.assertItemsEqual(
+            [
+                machine.system_id,
+            ],
+            extract_system_ids(result["machines"]))
+        self.assertItemsEqual(
+            [
+                device.system_id,
+            ],
+            extract_system_ids(result["devices"]))
+        # Since we're testing as a normal user, we won't be able to see
+        # controllers.
+        self.assertItemsEqual(
+            [
+            ],
+            extract_system_ids(result["region-controllers"]))
+        self.assertItemsEqual(
+            [
+            ],
+            extract_system_ids(result["rack-controllers"]))
+
+    def test_GET_by_type_with_summary_groups_by_type(self):
+        # The api allows for fetching the list of Machines.
+        machine = factory.make_Node()
+        device = factory.make_Device()
+        factory.make_RegionRackController()
+        factory.make_RackController()
+        factory.make_RegionController()
+        response = self.client.get(reverse('nodes_handler'), {
+            'op': "by_type",
+            'summary': False,
+        })
+        result = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        self.assertEqual(http.client.OK, response.status_code)
+        # Since we're testing as a normal user...
+        self.assertItemsEqual(
+            [
+                machine.system_id,
+            ],
+            extract_system_ids(result["machines"]))
+        self.assertItemsEqual(
+            [
+                device.system_id,
+            ],
+            extract_system_ids(result["devices"]))
+        self.assertItemsEqual(
+            [
+            ],
+            extract_system_ids(result["region-controllers"]))
+        self.assertItemsEqual(
+            [
+            ],
+            extract_system_ids(result["rack-controllers"]))
+
+    def test_GET_by_type_with_count_groups_by_type(self):
+        # The api allows for fetching the list of Machines.
+        factory.make_Node()
+        factory.make_Device()
+        factory.make_RegionRackController()
+        factory.make_RackController()
+        factory.make_RegionController()
+        response = self.client.get(reverse('nodes_handler'), {
+            'op': "by_type",
+            'count': True,
+        })
+        result = json.loads(
+            response.content.decode(settings.DEFAULT_CHARSET))
+        self.assertEqual(http.client.OK, response.status_code)
+        # Since we're testing as a normal user, they can only see the nodes
+        # and devices.
+        self.assertThat(result["nodes"], Equals(2))
+        self.assertThat(result["machines"], Equals(1))
+        self.assertThat(result["devices"], Equals(1))
+        self.assertThat(result["rack-controllers"], Equals(0))
+        self.assertThat(result["region-controllers"], Equals(0))
 
     def create_nodes(self, nodegroup, nb):
         for _ in range(nb):
 
=== modified file 'src/maasserver/testing/testclient.py'
--- src/maasserver/testing/testclient.py	2016-05-24 14:53:56 +0000
+++ src/maasserver/testing/testclient.py	2016-06-16 19:59:56 +0000
@@ -30,7 +30,7 @@
     """A derivative of Django's test client specially for MAAS.
 
     This ensures that requests are performed in a transaction, and that
-    post-commit hooks are alway fired or reset.
+    post-commit hooks are always fired or reset.
 
     It also permits logging-in using just a user object. A password will be
     configured and used to log-in.