API Model Classes

[coxley]

This commit introduces four (4) model classes, including the base class. These are derrived from collections.MutableMapping thus act like dicts and can instantiate with raw resource output from a resource.get() as well as supporting much simpler methods.

This also includes a test file under tests/test_models.py to try and make sure the supported use cases work.

Usage

In [1]: from pynsot.models import Network, Device, Interface
In [2]: from pynsot.client import get_api_client
In [3]: client = get_api_client()
In [4]: net = Network(raw=client.networks.get()[-1])

# Or also...
In [4]: net = Network(client=client, site_id=1, cidr='8.8.8.0/24')

In [5]: net.exists()
Out[5]: True

In [6]: net.existing_resource()
Out[6]: 
{u'attributes': {},
 u'id': 81,
 u'ip_version': u'4',
 u'is_ip': False,
 u'network_address': u'254.0.0.0',
 u'parent_id': 1,
 u'prefix_length': 24,
 u'site_id': 1,
 u'state': u'allocated'}

In [7]: net.purge()
Out[7]: True

In [8]: net.exists()
Out[8]: False

In [9]: net.ensure()
Out[9]: True

In [10]: net.exists()
Out[10]: True

In [11]: net['site_id']
Out[11]: 2

In [12]: net['site_id'] = 4

In [13]: net.exists()
Out[13]: False

In [14]: net['site_id'] = 2

In [15]: net.exists()
Out[15]: True

There are two tests failing which are a couple interface ones. The interfaces need redone a bit as noted by a few comments in addition to some usage docs before shipping.

Please review the code while I can accomplish those two things, though.

[coxley]

There were some changes necessary to get the ability to __setitem__. I was doing just a property method but am now doing the common scheme of:

[...]
  def __init__(self):
    self._payload = {}

  @property
  def payload(self):
    return self._payload

  @payload.setter
  def payload(self, value):
    self._payload = value

To get the resource specific metadata when raw isn't provided, though, I had to introduce init_payload. It works and there are tests around it. To see the diff of what it was before to now:

$ git diff HEAD^ HEAD | cat
diff --git a/pynsot/models.py b/pynsot/models.py
index d9e2a97..3c98a07 100644
--- a/pynsot/models.py
+++ b/pynsot/models.py
@@ -9,7 +9,7 @@ See the examples in the docstring for ``ApiModel``.
 from __future__ import unicode_literals
 import logging
 import collections
-from abc import abstractproperty, ABCMeta
+from abc import abstractproperty, abstractmethod, ABCMeta
 from netaddr import IPNetwork
 from pynsot.util import get_result
 from pynsot.client import get_api_client
@@ -54,11 +54,6 @@ class Resource(collections.MutableMapping):
     '''

     __metaclass__ = ABCMeta
-    logger = logging.getLogger(__name__)
-    errors = []
-    last_error = None
-    # Placeholder for .existing_resource() state
-    _existing_resource = {}

     def __init__(
         self,
@@ -68,6 +63,12 @@ class Resource(collections.MutableMapping):
         attributes={},
         **kwargs
     ):
+        self.logger = logging.getLogger(__name__)
+        self.errors = []
+        self.last_error = None
+        # Placeholder for .existing_resource() state
+        self._existing_resource = {}
+        self._payload = {}
         # Parameter validations

         # Site ID is required but can come from either kwarg or raw resource
@@ -84,8 +85,16 @@ class Resource(collections.MutableMapping):
         self.raw = raw
         self.attributes = attributes

-        if not raw:
+        if not self.raw:
             self.postinit(**kwargs)
+        else:
+            # This is done here because subclases do this in their postinit().
+            # If raw is passed, there's no reason to go through the postinit
+            # process which takes things like network_address, hostname, etc.
+            #
+            # Every subclasses init_payload method has a condition for 'if raw
+            # set raw' basically.
+            self.init_payload()

     def postinit(self, **kwargs):
         pass
@@ -110,17 +119,23 @@ class Resource(collections.MutableMapping):
         '''MUST be plural'''
         pass

-    @abstractproperty
+    @abstractmethod
+    def init_payload(self):
+        '''Initializes the _payload dictionary using resource specific data'''
+        pass
+
+    @property
     def payload(self):
         '''Represents exact payload sent to NSoT server

-        MUST be overloaded per subclass. __setitem__ is supported on object
-        instances which will update this dictionary to affect what is sent.
-
         Returns:
-            payload (dict)
+            _payload (dict)
         '''
-        pass
+        return self._payload
+
+    @payload.setter
+    def payload(self, value):
+        self._payload = value

     def __iter__(self):
         '''This allows dict(Resource) to return the payload to create'''
@@ -130,13 +145,17 @@ class Resource(collections.MutableMapping):
         return self.payload[key]

     def __setitem__(self, key, value):
+        # If we're changing items, it's not the same resource anymore
+        self.clear_cache()
         self.payload[key] = value

     def __delitem__(self, key):
+        # If we're changing items, it's not the same resource anymore
+        self.clear_cache()
         del self.payload[key]

     def __len__(self):
-        return len(self.payload)
+        return len(self._payload)

     def __repr__(self):
         '''Enable to be JSON serializable'''
@@ -186,6 +205,7 @@ class Resource(collections.MutableMapping):
         Returns:
             dict
         '''
+        self.ensure_client()
         if self._existing_resource:
             return self._existing_resource
         else:
@@ -193,7 +213,10 @@ class Resource(collections.MutableMapping):
             cur.pop('attributes', None)
             # We pop attributes because the query fails leaving them in
             try:
-                lookup = get_result(self.rclient.get(**cur))
+                # Site client of resource type because NSoT doesn't support
+                # passing site_id as a query parameter
+                s = eval("self.client.sites(self['site_id']).%s" % self.rtype)
+                lookup = get_result(s.get(**cur))
             except Exception as e:
                 self.log_error(e)
                 # There might be a better way to do this. If the NSoT server is
@@ -338,6 +361,7 @@ class Network(Resource):
         self.network_address = str(net.network)
         self.prefix_length = int(net.prefixlen)
         self.is_host = ver == 4 and net.prefixlen == 32 or net.prefixlen == 128
+        self.init_payload()

     @property
     def identifier(self):
@@ -352,12 +376,13 @@ class Network(Resource):
     def rtype(self):
         return 'networks'

-    @property
-    def payload(self):
+    def init_payload(self):
+        '''This will init the payload property'''
         if self.raw:
-            return self.raw
+            self.payload = self.raw
+            return

-        return {
+        self.payload = {
             'is_ip': self.is_host,
             'network_address': self.network_address,
             'prefix_length': self.prefix_length,
@@ -385,6 +410,7 @@ class Device(Resource):
         if not any([hostname, self.raw]):
             raise TypeError('Devices require a hostname')
         self.hostname = hostname
+        self.init_payload()

     @property
     def identifier(self):
@@ -399,12 +425,12 @@ class Device(Resource):
     def rtype(self):
         return 'devices'

-    @property
-    def payload(self):
+    def init_payload(self):
         if self.raw:
-            return self.raw
+            self.payload = self.raw
+            return

-        return {
+        self.payload = {
             'hostname': self.hostname,
             'site_id': self.site_id,
             'attributes': self.attributes,
@@ -453,6 +479,7 @@ class Interface(Resource):
             raise TypeError('Interfaces require both a name and device!')

         self.attempt_device()
+        self.init_payload()

     def attempt_device(self):
         '''Attempt to set ``device`` attribute to its ID if hostname was given
@@ -504,13 +531,18 @@ class Interface(Resource):
     def rtype(self):
         return 'interfaces'

-    @property
-    def payload(self):
+    def init_payload(self):
         if self.raw:
-            return self.raw
+            self.payload = self.raw
+            return

+        # TODO: This currently will only work on init and not later since
+        # init_payload is only called once. This is left over from when it was
+        # just payload
+        #
+        # attempt_device should instead set self.payload['device'] directly
         self.attempt_device()
-        return {
+        self.payload = {
             'addresses': self.addrs,
             'description': self.desc,
             'device': self.device,
diff --git a/tests/test_models.py b/tests/test_models.py
index 40470a6..099dbbc 100644
--- a/tests/test_models.py
+++ b/tests/test_models.py
@@ -10,6 +10,7 @@ import pytest
 from pytest import raises

 from pynsot.models import Resource, Network, Device, Interface
+from pynsot.util import get_result
 from .fixtures import config, client, site

 __all__ = ('client', 'config', 'pytest', 'site')
@@ -80,6 +81,52 @@ def test_existing(client, site):
     assert n.existing_resource() == n._existing_resource

+def test_dict():
+    n = Network(site_id=1, cidr='8.8.8.0/24')
+    assert n['site_id'] == 1
+    n['site_id'] = 2
+    assert n['site_id'] == 2
+
+    assert n.keys()
+    assert n.items()
+    assert dict(n)
+
+
+def test_payload_not_none_raw_and_not(client, site):
+    n = Network(client=client, site_id=site['id'], cidr='8.8.8.0/24')
+    assert n.payload
+    assert n.ensure()
+
+    n2 = Network(
+            raw=get_result(
+                client.sites(site['id']).networks('8.8.8.0/24').get()
+            )
+        )
+
+    assert n2.payload
+
+
+def test_clear_cache_on_change(client, site):
+    site_id = site['id']
+    c = client
+
+    n = Network(client=c, site_id=site_id, cidr='8.8.8.0/24')
+    assert n.ensure()
+    assert n.exists()
+
+    non_existing_site = get_result(c.sites.get())[-1]['id'] + 1000
+    n['site_id'] = non_existing_site
+    # First assert that the cache property was cleared
+    assert not n._existing_resource
+    assert not n.existing_resource()
+    # assert that the resource isn't successfully looked up if site doesn't
+    # match
+    assert not n.exists()
+    # Change site back and test
+    n['site_id'] = site_id
+    assert n.exists()
+
+
 def test_ip4_host():
     net = '8.8.8.8/32'
     n = Network(site_id=1, cidr=net)

[coxley]

I've resolved your issues and commented out the interface tests for now so travis is happy.

[coxley]

Updated