Merge lp:~allenap/maas/fix-resource-uri-model-mismatch into lp:~maas-committers/maas/trunk

[Gavin Panella]

Ensure that all Web API handlers render the fields required to reconstruct a self-referential URI.

In addition, make it an error to define a handler that omits the required fields.

[Mike Pontillo (mpontillo)]

You'll have to excuse my ignorance on this one.

How did you test this change and what was the requirement to drive this?

For the vast majority of these changes, I don't see any obviously-modified unit tests that would cover the changes.

Are there any potential side effects (such as API compatibility, and/or integration with other products) we need to watch out for if we rename 'fields' to 'subfields'?

review: Needs Information

[Mike Pontillo (mpontillo)]

I went back and read the bug, so I now better-understand the need for this change.

But I sill am not sure if current testing covers all these changes; thoughts?

[Gavin Panella (allenap)]

> Are there any potential side effects (such as API compatibility,
> and/or integration with other products) we need to watch out for if we
> rename 'fields' to 'subfields'?

No, there shouldn't be. That particular handler renders the response
itself so doesn't trigger Piston's emitters. The use of `fields` was an
unfortunate naming clash, but it was never noticed because only an
emitter would use it.

> For the vast majority of these changes, I don't see any
> obviously-modified unit tests that would cover the changes.
...
> I still am not sure if current testing covers all these changes;
> thoughts?

You're right. The has_fields/has_resource_uri checks ensure that we're
asking Piston to emit all of the fields we need, the lack of failures in
tests imply that these fields are being correctly rendered, but there
aren't any tests to check that those fields contain the right
information. I'll add those.

Thanks!

[Mike Pontillo (mpontillo)]

> No, there shouldn't be. That particular handler renders the response
> itself so doesn't trigger Piston's emitters. The use of `fields` was an
> unfortunate naming clash, but it was never noticed because only an
> emitter would use it.

Ah. Some comments around this would be nice, for future readers.

[Gavin Panella (allenap)]

Tests added, and a couple of DHCPSnippet tests fixed (they were using assertItemsEqual to compare dicts, so were in fact only comparing keys). Please take another look!

[Mike Pontillo (mpontillo)]

Thanks for the fixes!

review: Approve

=== modified file 'src/maasserver/api/bcache.py'
--- src/maasserver/api/bcache.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/api/bcache.py	2016-12-08 16:59:50 +0000
@@ -25,6 +25,7 @@
 
 
 DISPLAYED_BCACHE_FIELDS = (
+    'system_id',
     'id',
     'uuid',
     'name',
@@ -96,7 +97,7 @@
     def resource_uri(cls, bcache=None):
         # See the comment in NodeHandler.resource_uri.
         system_id = "system_id"
-        bcache_id = "bcache_id"
+        bcache_id = "id"
         if bcache is not None:
             bcache_id = bcache.id
             node = bcache.get_node()
@@ -105,6 +106,11 @@
         return ('bcache_device_handler', (system_id, bcache_id))
 
     @classmethod
+    def system_id(cls, bcache):
+        node = bcache.get_node()
+        return None if node is None else node.system_id
+
+    @classmethod
     def size(cls, bcache):
         return bcache.get_size()
 

=== modified file 'src/maasserver/api/bcache_cacheset.py'
--- src/maasserver/api/bcache_cacheset.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/api/bcache_cacheset.py	2016-12-08 16:59:50 +0000
@@ -25,6 +25,7 @@
 
 
 DISPLAYED_CACHE_SET_FIELDS = (
+    'system_id',
     'id',
     'name',
     'cache_device',
@@ -85,7 +86,7 @@
     def resource_uri(cls, cache_set=None):
         # See the comment in NodeHandler.resource_uri.
         system_id = "system_id"
-        cache_set_id = "cache_set_id"
+        cache_set_id = "id"
         if cache_set is not None:
             cache_set_id = cache_set.id
             node = cache_set.get_node()
@@ -94,6 +95,11 @@
         return ('bcache_cache_set_handler', (system_id, cache_set_id))
 
     @classmethod
+    def system_id(cls, cache_set):
+        node = cache_set.get_node()
+        return None if node is None else node.system_id
+
+    @classmethod
     def cache_device(cls, cache_set):
         """Return the cache device for this cache set."""
         return cache_set.get_device()

=== modified file 'src/maasserver/api/blockdevices.py'
--- src/maasserver/api/blockdevices.py	2016-10-20 16:04:24 +0000
+++ src/maasserver/api/blockdevices.py	2016-12-08 16:59:50 +0000
@@ -38,6 +38,7 @@
 
 
 DISPLAYED_BLOCKDEVICE_FIELDS = (
+    'system_id',
     'id',
     'name',
     'uuid',
@@ -125,13 +126,17 @@
         # See the comment in NodeHandler.resource_uri.
         if block_device is None:
             system_id = "system_id"
-            device_id = "device_id"
+            device_id = "id"
         else:
             device_id = block_device.id
             system_id = block_device.node.system_id
         return ('blockdevice_handler', (system_id, device_id))
 
     @classmethod
+    def system_id(cls, block_device):
+        return block_device.node.system_id
+
+    @classmethod
     def name(cls, block_device):
         return block_device.actual_instance.get_name()
 

=== modified file 'src/maasserver/api/boot_source_selections.py'
--- src/maasserver/api/boot_source_selections.py	2016-04-12 17:36:35 +0000
+++ src/maasserver/api/boot_source_selections.py	2016-12-08 16:59:50 +0000
@@ -21,6 +21,7 @@
 
 
 DISPLAYED_BOOTSOURCESELECTION_FIELDS = (
+    'boot_source_id',
     'id',
     'os',
     'release',

=== modified file 'src/maasserver/api/dhcpsnippets.py'
--- src/maasserver/api/dhcpsnippets.py	2016-04-11 16:23:26 +0000
+++ src/maasserver/api/dhcpsnippets.py	2016-12-08 16:59:50 +0000
@@ -48,7 +48,7 @@
         if dhcp_snippet is not None:
             dhcp_snippet_id = dhcp_snippet.id
         else:
-            dhcp_snippet_id = "dhcp_snippet_id"
+            dhcp_snippet_id = "id"
         return ('dhcp_snippet_handler', (dhcp_snippet_id,))
 
     @classmethod

=== modified file 'src/maasserver/api/dnsresourcerecords.py'
--- src/maasserver/api/dnsresourcerecords.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/api/dnsresourcerecords.py	2016-12-08 16:59:50 +0000
@@ -153,7 +153,7 @@
     @classmethod
     def resource_uri(cls, dnsresourcerecord=None):
         # See the comment in NodeHandler.resource_uri.
-        dnsresourcerecord_id = "dnsresourcerecord_id"
+        dnsresourcerecord_id = "id"
         if dnsresourcerecord is not None:
             dnsresourcerecord_id = dnsresourcerecord.id
         return ('dnsresourcerecord_handler', (dnsresourcerecord_id,))

=== modified file 'src/maasserver/api/dnsresources.py'
--- src/maasserver/api/dnsresources.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/api/dnsresources.py	2016-12-08 16:59:50 +0000
@@ -124,7 +124,7 @@
     @classmethod
     def resource_uri(cls, dnsresource=None):
         # See the comment in NodeHandler.resource_uri.
-        dnsresource_id = "dnsresource_id"
+        dnsresource_id = "id"
         if dnsresource is not None:
             dnsresource_id = dnsresource.id
         return ('dnsresource_handler', (dnsresource_id,))

=== modified file 'src/maasserver/api/domains.py'
--- src/maasserver/api/domains.py	2016-06-14 18:31:07 +0000
+++ src/maasserver/api/domains.py	2016-12-08 16:59:50 +0000
@@ -96,7 +96,7 @@
     @classmethod
     def resource_uri(cls, domain=None):
         # See the comment in NodeHandler.resource_uri.
-        domain_id = "domain_id"
+        domain_id = "id"
         if domain is not None:
             domain_id = domain.id
         return ('domain_handler', (domain_id,))

=== modified file 'src/maasserver/api/fabrics.py'
--- src/maasserver/api/fabrics.py	2016-04-27 20:18:20 +0000
+++ src/maasserver/api/fabrics.py	2016-12-08 16:59:50 +0000
@@ -62,7 +62,7 @@
     @classmethod
     def resource_uri(cls, fabric=None):
         # See the comment in NodeHandler.resource_uri.
-        fabric_id = "fabric_id"
+        fabric_id = "id"
         if fabric is not None:
             fabric_id = fabric.id
         return ('fabric_handler', (fabric_id,))

=== modified file 'src/maasserver/api/fannetworks.py'
--- src/maasserver/api/fannetworks.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/api/fannetworks.py	2016-12-08 16:59:50 +0000
@@ -70,7 +70,7 @@
     @classmethod
     def resource_uri(cls, fannetwork=None):
         # See the comment in NodeHandler.resource_uri.
-        fannetwork_id = "fannetwork_id"
+        fannetwork_id = "id"
         if fannetwork is not None:
             fannetwork_id = fannetwork.id
         return ('fannetwork_handler', (fannetwork_id,))

=== modified file 'src/maasserver/api/interfaces.py'
--- src/maasserver/api/interfaces.py	2016-12-07 12:46:14 +0000
+++ src/maasserver/api/interfaces.py	2016-12-08 16:59:50 +0000
@@ -55,6 +55,7 @@
 BLANK_FIELD = "This field cannot be blank."
 
 DISPLAYED_INTERFACE_FIELDS = (
+    'system_id',
     'id',
     'name',
     'type',
@@ -340,7 +341,7 @@
     def resource_uri(cls, interface=None):
         # See the comment in NodeHandler.resource_uri.
         system_id = "system_id"
-        interface_id = "interface_id"
+        interface_id = "id"
         if interface is not None:
             interface_id = interface.id
             node = interface.get_node()
@@ -349,6 +350,11 @@
         return ('interface_handler', (system_id, interface_id))
 
     @classmethod
+    def system_id(cls, interface):
+        node = interface.get_node()
+        return None if node is None else node.system_id
+
+    @classmethod
     def mac_address(cls, interface):
         if interface.mac_address is not None:
             return "%s" % interface.mac_address

=== modified file 'src/maasserver/api/ipranges.py'
--- src/maasserver/api/ipranges.py	2016-10-17 20:45:44 +0000
+++ src/maasserver/api/ipranges.py	2016-12-08 16:59:50 +0000
@@ -81,7 +81,7 @@
     @classmethod
     def resource_uri(cls, iprange=None):
         # See the comment in NodeHandler.resource_uri.
-        iprange_id = "iprange_id"
+        iprange_id = "id"
         if iprange is not None:
             iprange_id = iprange.id
         return ('iprange_handler', (iprange_id,))

=== modified file 'src/maasserver/api/machines.py'
--- src/maasserver/api/machines.py	2016-10-28 21:36:43 +0000
+++ src/maasserver/api/machines.py	2016-12-08 16:59:50 +0000
@@ -811,8 +811,9 @@
     fields = DISPLAYED_ANON_MACHINE_FIELDS
 
     @classmethod
-    def resource_uri(cls, machine):
-        return ('machine_handler', (machine.system_id, ))
+    def resource_uri(cls, machine=None):
+        system_id = "system_id" if machine is None else machine.system_id
+        return ('machine_handler', (system_id, ))
 
 
 class AnonMachinesHandler(AnonNodesHandler):

=== modified file 'src/maasserver/api/nodes.py'
--- src/maasserver/api/nodes.py	2016-12-07 12:46:14 +0000
+++ src/maasserver/api/nodes.py	2016-12-08 16:59:50 +0000
@@ -246,6 +246,8 @@
     read = create = update = delete = None
     model = Node
 
+    resource_uri = NodeHandler.resource_uri
+
 
 class AnonNodesHandler(AnonymousOperationsHandler):
     """Anonymous access to Nodes."""

=== modified file 'src/maasserver/api/packagerepositories.py'
--- src/maasserver/api/packagerepositories.py	2016-08-18 15:42:50 +0000
+++ src/maasserver/api/packagerepositories.py	2016-12-08 16:59:50 +0000
@@ -45,7 +45,7 @@
         if package_repository is not None:
             package_repository_id = package_repository.id
         else:
-            package_repository_id = "package_repository_id"
+            package_repository_id = "id"
         return ('package_repository_handler', (package_repository_id,))
 
     def read(self, request, package_repository_id):

=== modified file 'src/maasserver/api/partitions.py'
--- src/maasserver/api/partitions.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/api/partitions.py	2016-12-08 16:59:50 +0000
@@ -33,6 +33,8 @@
 
 
 DISPLAYED_PARTITION_FIELDS = (
+    'system_id',
+    'device_id',
     'id',
     'uuid',
     'path',
@@ -145,7 +147,7 @@
         if partition is None:
             system_id = "system_id"
             device_id = "device_id"
-            partition_id = "partition_id"
+            partition_id = "id"
         else:
             partition_id = partition.id
             block_device = partition.partition_table.block_device
@@ -154,6 +156,15 @@
         return (
             'partition_handler', (system_id, device_id, partition_id))
 
+    @classmethod
+    def system_id(cls, partition):
+        block_device = partition.partition_table.block_device
+        return block_device.node.system_id
+
+    @classmethod
+    def device_id(cls, partition):
+        return partition.partition_table.block_device.id
+
     def read(self, request, system_id, device_id, partition_id):
         """Read partition.
 

=== modified file 'src/maasserver/api/raid.py'
--- src/maasserver/api/raid.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/api/raid.py	2016-12-08 16:59:50 +0000
@@ -26,6 +26,7 @@
 
 
 DISPLAYED_RAID_FIELDS = (
+    'system_id',
     'id',
     'uuid',
     'name',
@@ -95,7 +96,7 @@
     def resource_uri(cls, raid=None):
         # See the comment in NodeHandler.resource_uri.
         system_id = "system_id"
-        raid_id = "raid_id"
+        raid_id = "id"
         if raid is not None:
             raid_id = raid.id
             node = raid.get_node()
@@ -104,6 +105,11 @@
         return ('raid_device_handler', (system_id, raid_id))
 
     @classmethod
+    def system_id(cls, raid):
+        node = raid.get_node()
+        return None if node is None else node.system_id
+
+    @classmethod
     def level(cls, raid):
         """Return the level of RAID for device."""
         # group_type holds the raid level, we just expose it over the API

=== modified file 'src/maasserver/api/spaces.py'
--- src/maasserver/api/spaces.py	2016-12-06 13:53:47 +0000
+++ src/maasserver/api/spaces.py	2016-12-08 16:59:50 +0000
@@ -60,7 +60,7 @@
     @classmethod
     def resource_uri(cls, space=None):
         # See the comment in NodeHandler.resource_uri.
-        space_id = "space_id"
+        space_id = "id"
         if space is not None:
             space_id = space.id
         return ('space_handler', (space_id,))

=== modified file 'src/maasserver/api/ssh_keys.py'
--- src/maasserver/api/ssh_keys.py	2016-09-23 19:21:31 +0000
+++ src/maasserver/api/ssh_keys.py	2016-12-08 16:59:50 +0000
@@ -141,7 +141,7 @@
 
     @classmethod
     def resource_uri(cls, sshkey=None):
-        keyid = "keyid"
+        keyid = "id"
         if sshkey is not None:
             keyid = sshkey.id
         return ('sshkey_handler', (keyid, ))

=== modified file 'src/maasserver/api/ssl_keys.py'
--- src/maasserver/api/ssl_keys.py	2016-04-11 16:23:26 +0000
+++ src/maasserver/api/ssl_keys.py	2016-12-08 16:59:50 +0000
@@ -102,7 +102,7 @@
 
     @classmethod
     def resource_uri(cls, sslkey=None):
-        keyid = "keyid"
+        keyid = "id"
         if sslkey is not None:
             keyid = sslkey.id
         return ('sslkey_handler', (keyid, ))

=== modified file 'src/maasserver/api/staticroutes.py'
--- src/maasserver/api/staticroutes.py	2016-08-17 15:29:26 +0000
+++ src/maasserver/api/staticroutes.py	2016-12-08 16:59:50 +0000
@@ -64,7 +64,7 @@
     @classmethod
     def resource_uri(cls, staticroute=None):
         # See the comment in NodeHandler.resource_uri.
-        staticroute_id = "staticroute_id"
+        staticroute_id = "id"
         if staticroute is not None:
             staticroute_id = staticroute.id
         return ('staticroute_handler', (staticroute_id,))

=== modified file 'src/maasserver/api/subnets.py'
--- src/maasserver/api/subnets.py	2016-12-07 12:46:14 +0000
+++ src/maasserver/api/subnets.py	2016-12-08 16:59:50 +0000
@@ -138,7 +138,7 @@
     @classmethod
     def resource_uri(cls, subnet=None):
         # See the comment in NodeHandler.resource_uri.
-        subnet_id = "subnet_id"
+        subnet_id = "id"
         if subnet is not None:
             subnet_id = subnet.id
         return ('subnet_handler', (subnet_id,))

=== 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-12-08 16:59:50 +0000
@@ -28,6 +28,10 @@
     HttpStatusCode,
     rc,
 )
+from provisioningserver.logger import LegacyLogger
+
+
+log = LegacyLogger()
 
 
 class OperationsResource(Resource):
@@ -225,9 +229,42 @@
         cls.allowed_methods = frozenset(
             http_method for http_method, name in exports)
 
+        # Flags used later.
+        has_fields = cls.fields is not BaseHandler.fields
+        has_resource_uri = hasattr(cls, "resource_uri")
+        is_internal_only = cls.__module__ in {__name__, "metadataserver.api"}
+
+        # Reject handlers which omit fields required for self-referential
+        # URIs. See bug 1643552. We ignore handlers that don't define `fields`
+        # because we assume they are doing custom object rendering and we have
+        # no way to check here for compliance.
+        if has_fields and has_resource_uri:
+            _, uri_params, *_ = cls.resource_uri()
+            missing = set(uri_params).difference(cls.fields)
+            if len(missing) != 0:
+                raise OperationsHandlerMisconfigured(
+                    "{handler.__module__}.{handler.__name__} does not render "
+                    "all fields required to construct a self-referential URI. "
+                    "Fields missing: {missing}.".format(
+                        handler=cls, missing=" ".join(sorted(missing))))
+
+        # Piston uses `resource_uri` even for handlers without models in order
+        # to generate documentation. We ignore those modules we consider "for
+        # internal use only" since we do not intend to generate documentation
+        # for these.
+        if not has_resource_uri and not is_internal_only:
+            log.warn(
+                "{handler.__module__}.{handler.__name__} does not have "
+                "`resource_uri`. This means it may be omitted from generated "
+                "documentation. Please investigate.", handler=cls)
+
         return cls
 
 
+class OperationsHandlerMisconfigured(Exception):
+    """Handler has been misconfigured; see the error message for details."""
+
+
 class OperationsHandlerMixin:
     """Handler mixin for operations dispatch.
 

=== modified file 'src/maasserver/api/tests/test_bcache.py'
--- src/maasserver/api/tests/test_bcache.py	2016-05-24 21:29:53 +0000
+++ src/maasserver/api/tests/test_bcache.py	2016-12-08 16:59:50 +0000
@@ -228,6 +228,7 @@
             "backing_device": ContainsDict({
                 "id": Equals(backing_block_device.id),
                 }),
+            "system_id": Equals(bcache.get_node().system_id),
             }))
 
     def test_read_404_when_not_bcache(self):

=== modified file 'src/maasserver/api/tests/test_bcache_cacheset.py'
--- src/maasserver/api/tests/test_bcache_cacheset.py	2016-05-24 22:05:45 +0000
+++ src/maasserver/api/tests/test_bcache_cacheset.py	2016-12-08 16:59:50 +0000
@@ -149,6 +149,7 @@
             "cache_device": ContainsDict({
                 "id": Equals(cache_block_device.id),
                 }),
+            "system_id": Equals(cache_set.get_node().system_id),
             }))
 
     def test_read_404_when_invalid_id(self):

=== modified file 'src/maasserver/api/tests/test_blockdevice.py'
--- src/maasserver/api/tests/test_blockdevice.py	2016-10-20 16:04:24 +0000
+++ src/maasserver/api/tests/test_blockdevice.py	2016-12-08 16:59:50 +0000
@@ -18,6 +18,7 @@
 from maasserver.models.blockdevice import MIN_BLOCK_DEVICE_SIZE
 from maasserver.testing.api import APITestCase
 from maasserver.testing.factory import factory
+from maasserver.testing.matchers import HasStatusCode
 from maasserver.utils.converters import json_load_bytes
 from maasserver.utils.orm import reload_object
 from testtools.matchers import (
@@ -383,6 +384,17 @@
             "mount_options": filesystem2.mount_options,
             }, parsed_device['partitions'][1]['filesystem'])
 
+    def test_read_returns_system_id(self):
+        node = factory.make_Node()
+        block_device = factory.make_PhysicalBlockDevice(node=node)
+        uri = get_blockdevice_uri(block_device)
+        response = self.client.get(uri)
+        self.assertThat(response, HasStatusCode(http.client.OK))
+        parsed_device = json_load_bytes(response.content)
+        self.assertThat(parsed_device, ContainsDict({
+            "system_id": Equals(node.system_id),
+        }))
+
     def test_delete_returns_403_when_not_admin(self):
         block_device = factory.make_PhysicalBlockDevice()
         uri = get_blockdevice_uri(block_device)

=== modified file 'src/maasserver/api/tests/test_dhcpsnippets.py'
--- src/maasserver/api/tests/test_dhcpsnippets.py	2016-05-24 21:29:53 +0000
+++ src/maasserver/api/tests/test_dhcpsnippets.py	2016-12-08 16:59:50 +0000
@@ -19,6 +19,7 @@
 from maasserver.testing.api import APITestCase
 from maasserver.testing.factory import factory
 from maasserver.utils.orm import reload_object
+from testtools.matchers import Equals
 
 
 class TestDHCPSnippetAPI(APITestCase.ForUser):
@@ -44,22 +45,31 @@
         self.assertEqual(
             http.client.OK, response.status_code, response.content)
         parsed_dhcp_snippet = json.loads(response.content.decode())
-        self.assertItemsEqual({
+        self.assertThat(parsed_dhcp_snippet, Equals({
             'id': dhcp_snippet.id,
             'name': dhcp_snippet.name,
-            'value': dhcp_snippet.value,
+            'value': dhcp_snippet.value.data,
             'description': dhcp_snippet.description,
-            'history': [{
-                'id': dhcp_snippet.value.previous_version.id,
-                'value': dhcp_snippet.value.previous_version.data,
-                'created': format_datetime(dhcp_snippet.value.created),
-            }],
+            'history': [
+                {
+                    'id': dhcp_snippet.value.id,
+                    'value': dhcp_snippet.value.data,
+                    'created': format_datetime(
+                        dhcp_snippet.value.created),
+                },
+                {
+                    'id': dhcp_snippet.value.previous_version.id,
+                    'value': dhcp_snippet.value.previous_version.data,
+                    'created': format_datetime(
+                        dhcp_snippet.value.previous_version.created),
+                },
+            ],
             'enabled': dhcp_snippet.enabled,
             'node': None,
             'subnet': None,
             'global_snippet': True,
             'resource_uri': self.get_dhcp_snippet_uri(dhcp_snippet),
-        }, parsed_dhcp_snippet)
+        }))
 
     def test_read_by_name(self):
         dhcp_snippet = factory.make_DHCPSnippet()
@@ -71,22 +81,31 @@
         self.assertEqual(
             http.client.OK, response.status_code, response.content)
         parsed_dhcp_snippet = json.loads(response.content.decode())
-        self.assertItemsEqual({
+        self.assertThat(parsed_dhcp_snippet, Equals({
             'id': dhcp_snippet.id,
             'name': dhcp_snippet.name,
-            'value': dhcp_snippet.value,
+            'value': dhcp_snippet.value.data,
             'description': dhcp_snippet.description,
-            'history': [{
-                'id': dhcp_snippet.value.previous_version.id,
-                'value': dhcp_snippet.value.previous_version.data,
-                'created': format_datetime(dhcp_snippet.value.created),
-            }],
+            'history': [
+                {
+                    'id': dhcp_snippet.value.id,
+                    'value': dhcp_snippet.value.data,
+                    'created': format_datetime(
+                        dhcp_snippet.value.created),
+                },
+                {
+                    'id': dhcp_snippet.value.previous_version.id,
+                    'value': dhcp_snippet.value.previous_version.data,
+                    'created': format_datetime(
+                        dhcp_snippet.value.previous_version.created),
+                },
+            ],
             'enabled': dhcp_snippet.enabled,
             'node': None,
             'subnet': None,
             'global_snippet': True,
             'resource_uri': self.get_dhcp_snippet_uri(dhcp_snippet),
-        }, parsed_dhcp_snippet)
+        }))
 
     def test_read_404_when_bad_id(self):
         response = self.client.get(

=== modified file 'src/maasserver/api/tests/test_interfaces.py'
--- src/maasserver/api/tests/test_interfaces.py	2016-10-20 16:04:24 +0000
+++ src/maasserver/api/tests/test_interfaces.py	2016-12-08 16:59:50 +0000
@@ -831,6 +831,7 @@
             "resource_uri": Equals(get_interface_uri(bond)),
             "params": Equals(bond.params),
             "effective_mtu": Equals(bond.get_effective_mtu()),
+            "system_id": Equals(node.system_id),
         }))
         self.assertEqual(sorted(
             nic.name

=== modified file 'src/maasserver/api/tests/test_partitions.py'
--- src/maasserver/api/tests/test_partitions.py	2016-05-24 22:05:45 +0000
+++ src/maasserver/api/tests/test_partitions.py	2016-12-08 16:59:50 +0000
@@ -165,9 +165,13 @@
 
         parsed_partition = json.loads(
             response.content.decode(settings.DEFAULT_CHARSET))
-        self.assertTrue(parsed_partition['bootable'])
-        self.assertEqual(partition.id, parsed_partition['id'])
-        self.assertEqual(partition.size, parsed_partition['size'])
+        self.assertThat(parsed_partition, ContainsDict({
+            "bootable": Is(True),
+            "id": Equals(partition.id),
+            "size": Equals(partition.size),
+            "system_id": Equals(device.node.system_id),
+            "device_id": Equals(device.id),
+        }))
 
     def test_read_partition_by_name(self):
         device = factory.make_PhysicalBlockDevice(

=== modified file 'src/maasserver/api/tests/test_raid.py'
--- src/maasserver/api/tests/test_raid.py	2016-05-24 21:29:53 +0000
+++ src/maasserver/api/tests/test_raid.py	2016-12-08 16:59:50 +0000
@@ -818,6 +818,7 @@
             "human_size": Equals(
                 human_readable_bytes(raid.get_size())),
             "resource_uri": Equals(get_raid_device_uri(raid)),
+            "system_id": Equals(node.system_id),
             }))
         self.assertItemsEqual(
             block_device_ids + partitions_ids, parsed_device_ids)

=== modified file 'src/maasserver/api/tests/test_user.py'
--- src/maasserver/api/tests/test_user.py	2016-05-24 22:05:45 +0000
+++ src/maasserver/api/tests/test_user.py	2016-12-08 16:59:50 +0000
@@ -27,6 +27,11 @@
 )
 
 
+def get_user_uri(user):
+    """Return a User URI."""
+    return reverse('user_handler', args=[user.username])
+
+
 class TestUsers(APITestCase.ForUser):
 
     def test_handler_path(self):
@@ -169,6 +174,7 @@
         self.assertEqual(user.username, returned_user['username'])
         self.assertEqual(user.email, returned_user['email'])
         self.assertFalse(returned_user['is_superuser'])
+        self.assertEqual(get_user_uri(user), returned_user['resource_uri'])
 
     def test_GET_shows_expected_fields(self):
         user = factory.make_User()
@@ -181,7 +187,7 @@
         returned_user = json.loads(
             response.content.decode(settings.DEFAULT_CHARSET))
         self.assertItemsEqual(
-            ['username', 'email', 'is_superuser'],
+            ['username', 'email', 'resource_uri', 'is_superuser'],
             returned_user.keys())
 
     def test_GET_identifies_superuser_as_such(self):

=== modified file 'src/maasserver/api/tests/test_vlans.py'
--- src/maasserver/api/tests/test_vlans.py	2016-12-07 16:40:35 +0000
+++ src/maasserver/api/tests/test_vlans.py	2016-12-08 16:59:50 +0000
@@ -199,6 +199,7 @@
             "name": Equals(vlan.get_name()),
             "vid": Equals(vlan.vid),
             "fabric": Equals(fabric.get_name()),
+            "fabric_id": Equals(fabric.id),
             "resource_uri": Equals(get_vlan_uri(vlan)),
             }))
 

=== modified file 'src/maasserver/api/tests/test_volume_groups.py'
--- src/maasserver/api/tests/test_volume_groups.py	2016-05-24 21:29:53 +0000
+++ src/maasserver/api/tests/test_volume_groups.py	2016-12-08 16:59:50 +0000
@@ -249,6 +249,7 @@
             "human_used_size": Equals(
                 human_readable_bytes(volume_group.get_lvm_allocated_size())),
             "resource_uri": Equals(get_volume_group_uri(volume_group)),
+            "system_id": Equals(node.system_id),
             }))
         self.assertItemsEqual(
             block_device_ids + partitions_ids, parsed_device_ids)

=== modified file 'src/maasserver/api/users.py'
--- src/maasserver/api/users.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/api/users.py	2016-12-08 16:59:50 +0000
@@ -112,3 +112,8 @@
                 user.delete()
 
         return rc.DELETED
+
+    @classmethod
+    def resource_uri(cls, user=None):
+        username = "username" if user is None else user.username
+        return ('user_handler', [username])

=== modified file 'src/maasserver/api/vlans.py'
--- src/maasserver/api/vlans.py	2016-12-07 16:40:35 +0000
+++ src/maasserver/api/vlans.py	2016-12-08 16:59:50 +0000
@@ -21,6 +21,7 @@
     'name',
     'vid',
     'fabric',
+    'fabric_id',
     'mtu',
     'primary_rack',
     'secondary_rack',

=== modified file 'src/maasserver/api/volume_groups.py'
--- src/maasserver/api/volume_groups.py	2016-03-28 13:54:47 +0000
+++ src/maasserver/api/volume_groups.py	2016-12-08 16:59:50 +0000
@@ -31,6 +31,7 @@
 
 
 DISPLAYED_VOLUME_GROUP_FIELDS = (
+    'system_id',
     'id',
     'uuid',
     'name',
@@ -99,7 +100,7 @@
     def resource_uri(cls, volume_group=None):
         # See the comment in NodeHandler.resource_uri.
         system_id = "system_id"
-        volume_group_id = "volume_group_id"
+        volume_group_id = "id"
         if volume_group is not None:
             volume_group_id = volume_group.id
             node = volume_group.get_node()
@@ -108,6 +109,14 @@
         return ('volume_group_handler', (system_id, volume_group_id))
 
     @classmethod
+    def system_id(cls, volume_group):
+        if volume_group is None:
+            return None
+        else:
+            node = volume_group.get_node()
+            return None if node is None else node.system_id
+
+    @classmethod
     def size(cls, filesystem_group):
         return filesystem_group.get_size()
 

=== modified file 'src/metadataserver/api.py'
--- src/metadataserver/api.py	2016-09-13 20:58:15 +0000
+++ src/metadataserver/api.py	2016-12-08 16:59:50 +0000
@@ -196,13 +196,13 @@
     create = update = delete = None
 
     def read(self, request, mac=None):
-        return make_list_response(sorted(self.fields))
+        return make_list_response(sorted(self.subfields))
 
 
 class IndexHandler(MetadataViewHandler):
     """Top-level metadata listing."""
 
-    fields = ('latest', '2012-03-01')
+    subfields = ('latest', '2012-03-01')
 
 
 class StatusHandler(MetadataViewHandler):
@@ -394,7 +394,7 @@
 class VersionIndexHandler(MetadataViewHandler):
     """Listing for a given metadata version."""
     create = update = delete = None
-    fields = ('maas-commissioning-scripts', 'meta-data', 'user-data')
+    subfields = ('maas-commissioning-scripts', 'meta-data', 'user-data')
 
     # States in which a node is allowed to signal
     # commissioning/installing/entering-rescue-mode status.
@@ -432,11 +432,11 @@
         check_version(version)
         node = get_queried_node(request, for_mac=mac)
         if NodeUserData.objects.has_user_data(node):
-            shown_fields = self.fields
+            shown_subfields = self.subfields
         else:
-            shown_fields = list(self.fields)
-            shown_fields.remove('user-data')
-        return make_list_response(sorted(shown_fields))
+            shown_subfields = list(self.subfields)
+            shown_subfields.remove('user-data')
+        return make_list_response(sorted(shown_subfields))
 
     def _store_installation_results(self, node, request):
         """Store installation result file for `node`."""
@@ -608,7 +608,7 @@
 class MetaDataHandler(VersionIndexHandler):
     """Meta-data listing for a given version."""
 
-    fields = (
+    subfields = (
         'instance-id',
         'local-hostname',
         'public-keys',
@@ -627,9 +627,9 @@
             returns an HttpResponse.
         :rtype: Callable
         """
-        field = item.split('/')[0]
-        if field not in self.fields:
-            raise MAASAPINotFound("Unknown metadata attribute: %s" % field)
+        subfield = item.split('/')[0]
+        if subfield not in self.subfields:
+            raise MAASAPINotFound("Unknown metadata attribute: %s" % subfield)
 
         producers = {
             'instance-id': self.instance_id,
@@ -639,7 +639,7 @@
             'x509': self.ssl_certs,
         }
 
-        return producers[field]
+        return producers[subfield]
 
     def read(self, request, version, mac=None, item=None):
         check_version(version)
@@ -648,7 +648,7 @@
         # Requesting the list of attributes, not any particular
         # attribute.
         if item is None or len(item) == 0:
-            fields = list(self.fields)
+            subfields = list(self.subfields)
             commissioning_without_ssh = (
                 node.status == NODE_STATUS.COMMISSIONING and
                 not node.enable_ssh)
@@ -656,8 +656,8 @@
             # node has registered SSH keys.
             keys = SSHKey.objects.get_keys_for_user(user=node.owner)
             if not keys or commissioning_without_ssh:
-                fields.remove('public-keys')
-            return make_list_response(sorted(fields))
+                subfields.remove('public-keys')
+            return make_list_response(sorted(subfields))
 
         producer = self.get_attribute_producer(item)
         return producer(node, version, item)
@@ -787,10 +787,10 @@
 
 class EnlistVersionIndexHandler(OperationsHandler):
     create = update = delete = None
-    fields = ('meta-data', 'user-data')
+    subfields = ('meta-data', 'user-data')
 
     def read(self, request, version):
-        return make_list_response(sorted(self.fields))
+        return make_list_response(sorted(self.subfields))
 
 
 class AnonMetaDataHandler(VersionIndexHandler):

=== modified file 'src/metadataserver/tests/test_api.py'
--- src/metadataserver/tests/test_api.py	2016-09-22 02:53:33 +0000
+++ src/metadataserver/tests/test_api.py	2016-12-08 16:59:50 +0000
@@ -345,7 +345,7 @@
         response = client.get(url)
         self.assertIn('text/plain', response['Content-Type'])
         self.assertItemsEqual(
-            MetaDataHandler.fields, [
+            MetaDataHandler.subfields, [
                 field.decode(settings.DEFAULT_CHARSET)
                 for field in response.content.split()
             ])