Merge lp:~jtv/maas/bug-1372735 into lp:~maas-committers/maas/trunk

[Jeroen T. Vermeulen]

Fix DeprecationWarning in Bin class. It was showing up in Twisted logs, which in turn broke Node model tests sometimes (because they checked for the log to be empty).

The warning was about object.__init__ not wanting to receive arguments during the super() upcall from Bin.__init__. Even making just a direct upcall to bytes.__init__ (because Bin is derived from bytes, not from object directly) did not fix it. And so I had to replace use of the Bin constructor with a little factory.

I did want to keep the safety of checking against the wrong values being passed in, especially unicode or None. I don't see any way to check that _after_ construction, and I couldn't write my own constructor, so I had to get in _before_ construction.

Most of the diff will be pointless context around tiny mechanical changes in usage. It's actually quite a small change.

Jeroen

[Newell Jensen (newell-jensen)]

review: Approve

[Gavin Panella (allenap)]

I think there's a much simpler fix, but have a look and check that it does what you need.

review: Needs Information

[Jeroen T. Vermeulen (jtv)]

Proposing your version as a fresh branch.

=== modified file 'src/maasserver/api/commissioning_scripts.py'
--- src/maasserver/api/commissioning_scripts.py	2014-08-17 01:33:52 +0000
+++ src/maasserver/api/commissioning_scripts.py	2014-09-23 04:43:27 +0000
@@ -21,7 +21,7 @@
 from django.shortcuts import get_object_or_404
 from maasserver.api.support import OperationsHandler
 from maasserver.api.utils import get_mandatory_param
-from metadataserver.fields import Bin
+from metadataserver.fields import wrap_bin
 from metadataserver.models import CommissioningScript
 from piston.utils import rc
 
@@ -80,7 +80,7 @@
             is ignored; MAAS will know it by the name you pass to the request.
         """
         name = get_mandatory_param(request.data, 'name')
-        content = Bin(get_content_parameter(request))
+        content = wrap_bin(get_content_parameter(request))
         return CommissioningScript.objects.create(name=name, content=content)
 
     @classmethod
@@ -114,7 +114,7 @@
 
     def update(self, request, name):
         """Update a commissioning script."""
-        content = Bin(get_content_parameter(request))
+        content = wrap_bin(get_content_parameter(request))
         script = get_object_or_404(CommissioningScript, name=name)
         script.content = content
         script.save()

=== modified file 'src/maasserver/api/tests/test_nodegroup.py'
--- src/maasserver/api/tests/test_nodegroup.py	2014-09-15 14:28:28 +0000
+++ src/maasserver/api/tests/test_nodegroup.py	2014-09-23 04:43:27 +0000
@@ -48,7 +48,7 @@
 from maastesting.celery import CeleryFixture
 from maastesting.matchers import MockCalledOnceWith
 from metadataserver.enum import RESULT_TYPE
-from metadataserver.fields import Bin
+from metadataserver.fields import wrap_bin
 from metadataserver.models import (
     commissioningscript,
     NodeResult,
@@ -416,7 +416,7 @@
         NodeResult.objects.store_data(
             node, commissioningscript.LSHW_OUTPUT_NAME,
             script_result=0, result_type=RESULT_TYPE.COMMISSIONING,
-            data=Bin(data))
+            data=wrap_bin(data))
 
     example_lldp_details = dedent("""\
         <?xml version="1.0" encoding="UTF-8"?>
@@ -429,7 +429,7 @@
         NodeResult.objects.store_data(
             node, commissioningscript.LLDP_OUTPUT_NAME,
             script_result=0, result_type=RESULT_TYPE.COMMISSIONING,
-            data=Bin(data))
+            data=wrap_bin(data))
 
     def test_nodegroup_requires_authentication(self):
         nodegroup = factory.make_NodeGroup()

=== modified file 'src/maasserver/forms.py'
--- src/maasserver/forms.py	2014-09-22 05:36:10 +0000
+++ src/maasserver/forms.py	2014-09-23 04:43:27 +0000
@@ -142,7 +142,7 @@
     list_osystem_choices,
     list_release_choices,
     )
-from metadataserver.fields import Bin
+from metadataserver.fields import wrap_bin
 from metadataserver.models import CommissioningScript
 from netaddr import IPAddress
 from provisioningserver.drivers.osystem import OperatingSystemRegistry
@@ -1620,7 +1620,7 @@
         content = self.cleaned_data['content']
         CommissioningScript.objects.create(
             name=content.name,
-            content=Bin(content.read()))
+            content=wrap_bin(content.read()))
 
 
 class UnconstrainedMultipleChoiceField(MultipleChoiceField):

=== modified file 'src/maasserver/models/filestorage.py'
--- src/maasserver/models/filestorage.py	2013-10-07 09:12:40 +0000
+++ src/maasserver/models/filestorage.py	2014-09-23 04:43:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Storage for uploaded files."""
@@ -31,8 +31,8 @@
 from maasserver import DefaultMeta
 from maasserver.models.cleansave import CleanSave
 from metadataserver.fields import (
-    Bin,
     BinaryField,
+    wrap_bin,
     )
 
 
@@ -59,7 +59,7 @@
         """
         # This probably ought to read in chunks but large files are
         # not expected.
-        content = Bin(file_object.read())
+        content = wrap_bin(file_object.read())
         storage, created = self.get_or_create(
             filename=filename, owner=owner, defaults={'content': content})
         if not created:

=== modified file 'src/maasserver/models/tests/test_node.py'
--- src/maasserver/models/tests/test_node.py	2014-09-22 06:29:06 +0000
+++ src/maasserver/models/tests/test_node.py	2014-09-23 04:43:27 +0000
@@ -78,7 +78,7 @@
 from maastesting.testcase import MAASTestCase
 from metadataserver import commissioning
 from metadataserver.enum import RESULT_TYPE
-from metadataserver.fields import Bin
+from metadataserver.fields import wrap_bin
 from metadataserver.models import (
     NodeResult,
     NodeUserData,
@@ -1050,7 +1050,7 @@
             node, factory.make_string(),
             random.randint(0, 10),
             RESULT_TYPE.COMMISSIONING,
-            Bin(factory.make_bytes()))
+            wrap_bin(factory.make_bytes()))
         node.start_commissioning(factory.make_admin())
         self.assertItemsEqual([], node.noderesult_set.all())
 
@@ -1061,7 +1061,7 @@
         script_result = random.randint(0, 10)
         NodeResult.objects.store_data(
             node, filename, script_result, RESULT_TYPE.COMMISSIONING,
-            Bin(data))
+            wrap_bin(data))
         other_node = factory.make_Node(status=NODE_STATUS.NEW)
         other_node.start_commissioning(factory.make_admin())
         self.assertEqual(

=== modified file 'src/maasserver/testing/factory.py'
--- src/maasserver/testing/factory.py	2014-09-19 12:48:48 +0000
+++ src/maasserver/testing/factory.py	2014-09-23 04:43:27 +0000
@@ -76,7 +76,7 @@
 import maastesting.factory
 from maastesting.factory import NO_VALUE
 from metadataserver.enum import RESULT_TYPE
-from metadataserver.fields import Bin
+from metadataserver.fields import wrap_bin
 from metadataserver.models import (
     CommissioningScript,
     NodeResult,
@@ -445,7 +445,7 @@
             script_result = random.randint(0, 10)
         ncr = NodeResult(
             node=node, name=name, script_result=script_result,
-            result_type=RESULT_TYPE.COMMISSIONING, data=Bin(data))
+            result_type=RESULT_TYPE.COMMISSIONING, data=wrap_bin(data))
         ncr.save()
         return ncr
 
@@ -462,7 +462,7 @@
             script_result = random.randint(0, 10)
         ncr = NodeResult(
             node=node, name=name, script_result=script_result,
-            result_type=RESULT_TYPE.INSTALLING, data=Bin(data))
+            result_type=RESULT_TYPE.INSTALLING, data=wrap_bin(data))
         ncr.save()
         return ncr
 
@@ -707,7 +707,7 @@
         if content is None:
             content = b'content:' + self.make_string().encode('ascii')
         return CommissioningScript.objects.create(
-            name=name, content=Bin(content))
+            name=name, content=wrap_bin(content))
 
     def make_DownloadProgress(self, nodegroup=None, filename=None,
                               size=NO_VALUE, bytes_downloaded=NO_VALUE,

=== modified file 'src/maasserver/tests/test_third_party_drivers.py'
--- src/maasserver/tests/test_third_party_drivers.py	2014-09-10 16:20:31 +0000
+++ src/maasserver/tests/test_third_party_drivers.py	2014-09-23 04:43:27 +0000
@@ -28,7 +28,7 @@
 from maastesting import root
 from maastesting.testcase import MAASTestCase
 from metadataserver.enum import RESULT_TYPE
-from metadataserver.fields import Bin
+from metadataserver.fields import wrap_bin
 from metadataserver.models import (
     commissioningscript,
     NodeResult,
@@ -42,7 +42,7 @@
         node = factory.make_Node()
         NodeResult.objects.store_data(
             node, commissioningscript.LIST_MODALIASES_OUTPUT_NAME,
-            0, RESULT_TYPE.COMMISSIONING, Bin(test_data))
+            0, RESULT_TYPE.COMMISSIONING, wrap_bin(test_data))
 
         aliases = node_modaliases(node)
         self.assertEqual(['hulla', 'baloo'], aliases)

=== modified file 'src/metadataserver/api.py'
--- src/metadataserver/api.py	2014-08-22 16:25:29 +0000
+++ src/metadataserver/api.py	2014-09-23 04:43:27 +0000
@@ -67,7 +67,7 @@
     RESULT_TYPE,
     SIGNAL_STATUS,
     )
-from metadataserver.fields import Bin
+from metadataserver.fields import wrap_bin
 from metadataserver.models import (
     CommissioningScript,
     NodeKey,
@@ -210,7 +210,7 @@
             raw_content = uploaded_file.read()
             NodeResult.objects.store_data(
                 node, name, script_result=0,
-                result_type=RESULT_TYPE.INSTALLING, data=Bin(raw_content))
+                result_type=RESULT_TYPE.INSTALLING, data=wrap_bin(raw_content))
 
     def _store_commissioning_results(self, node, request):
         """Store commissioning result files for `node`."""
@@ -224,7 +224,8 @@
                     exit_status=script_result)
             NodeResult.objects.store_data(
                 node, name, script_result,
-                result_type=RESULT_TYPE.COMMISSIONING, data=Bin(raw_content))
+                result_type=RESULT_TYPE.COMMISSIONING,
+                data=wrap_bin(raw_content))
 
     @operation(idempotent=False)
     def signal(self, request, version=None, mac=None):

=== modified file 'src/metadataserver/fields.py'
--- src/metadataserver/fields.py	2013-10-21 05:31:09 +0000
+++ src/metadataserver/fields.py	2014-09-23 04:43:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Custom field types for the metadata server."""
@@ -14,6 +14,7 @@
 __metaclass__ = type
 __all__ = [
     'BinaryField',
+    'wrap_bin',
     ]
 
 from base64 import (
@@ -29,6 +30,26 @@
 from south.modelsinspector import add_introspection_rules
 
 
+def wrap_bin(binary_data):
+    """Wrap a `bytes` into a `Bin`.
+
+    Use this to construct a `Bin`, a marker class with which we wrap binary
+    data.  Otherwise Django field conversions won't be able to tell a binary
+    value as found in the database from one that's already been "converted" to
+    a Python object.
+
+    :param binary_data: A `bytes` object.  Nothing else is accepted.
+    :return: A `Bin` containing `binary_data`.
+    """
+    # We can't give `Bin` its own constructor to check this, because the
+    # upcall to bytes.__init__ triggers a DeprecationWarning about
+    # object.__init__ no longer accepting arguments.  (This happens even with
+    # a direct upcall; it's not just super() that causes it.)
+    assert isinstance(binary_data, bytes), (
+        "Not a binary string: '%s'." % repr(binary_data))
+    return Bin(binary_data)
+
+
 class Bin(bytes):
     """Wrapper class to convince django that a string is really binary.
 
@@ -39,24 +60,12 @@
     can stay as it is).  The line between bytes and unicode is dangerously
     thin.
 
-    So, to store a value in a BinaryField, wrap it in a Bin:
+    So, to store a value in a BinaryField, wrap it in a Bin.  Use the
+    `wrap_bin` factory:
 
-        my_model_object.binary_data = Bin(b"\x01\x02\x03")
+        my_model_object.binary_data = wrap_bin(b"\x01\x02\x03")
     """
 
-    def __init__(self, initializer):
-        """Wrap a bytes.
-
-        :param initializer: Binary string of data for this Bin.  This must
-            be a bytes.  Anything else is almost certainly a mistake, so e.g.
-            this constructor will refuse to render None as b'None'.
-        :type initializer: bytes
-        """
-        if not isinstance(initializer, bytes):
-            raise AssertionError(
-                "Not a binary string: '%s'" % repr(initializer))
-        super(Bin, self).__init__(initializer)
-
     def __emittable__(self):
         """Emit base-64 encoded bytes.
 

=== modified file 'src/metadataserver/migrations/0012_commission_result_binary_data_recode.py'
--- src/metadataserver/migrations/0012_commission_result_binary_data_recode.py	2014-03-27 04:15:45 +0000
+++ src/metadataserver/migrations/0012_commission_result_binary_data_recode.py	2014-09-23 04:43:27 +0000
@@ -1,7 +1,6 @@
 # -*- coding: utf-8 -*-
 import datetime
 
-from django.db import models
 from south.db import db
 from south.v2 import DataMigration
 
@@ -10,9 +9,9 @@
 
     def forwards(self, orm):
         "Write your forwards methods here."
-        from metadataserver.fields import Bin
+        from metadataserver.fields import wrap_bin
         for result in orm.NodeCommissionResult.objects.all():
-            result.data_bin = Bin(result.data.encode("utf-8"))
+            result.data_bin = wrap_bin(result.data.encode("utf-8"))
             result.save()
 
     def backwards(self, orm):

=== modified file 'src/metadataserver/models/commissioningscript.py'
--- src/metadataserver/models/commissioningscript.py	2014-08-13 21:49:35 +0000
+++ src/metadataserver/models/commissioningscript.py	2014-09-23 04:43:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012, 2013 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Custom commissioning scripts, and their database backing."""
@@ -47,8 +47,8 @@
 from metadataserver import DefaultMeta
 from metadataserver.enum import RESULT_TYPE
 from metadataserver.fields import (
-    Bin,
     BinaryField,
+    wrap_bin,
     )
 from metadataserver.models.noderesult import NodeResult
 
@@ -467,7 +467,7 @@
     assert isinstance(output, bytes)
     NodeResult.objects.store_data(
         node, name, script_result=exit_status,
-        result_type=RESULT_TYPE.COMMISSIONING, data=Bin(output))
+        result_type=RESULT_TYPE.COMMISSIONING, data=wrap_bin(output))
     if name in BUILTIN_COMMISSIONING_SCRIPTS:
         postprocess_hook = BUILTIN_COMMISSIONING_SCRIPTS[name]['hook']
         postprocess_hook(node=node, output=output, exit_status=exit_status)

=== modified file 'src/metadataserver/models/nodeuserdata.py'
--- src/metadataserver/models/nodeuserdata.py	2014-09-02 12:34:10 +0000
+++ src/metadataserver/models/nodeuserdata.py	2014-09-23 04:43:27 +0000
@@ -1,4 +1,4 @@
-# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2014 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Node user-data for cloud-init's use."""
@@ -25,8 +25,8 @@
 from maasserver.models.cleansave import CleanSave
 from metadataserver import DefaultMeta
 from metadataserver.fields import (
-    Bin,
     BinaryField,
+    wrap_bin,
     )
 
 
@@ -53,7 +53,7 @@
 
     def _set(self, node, data):
         """Set actual user data for a node.  Not usable if data is None."""
-        wrapped_data = Bin(data)
+        wrapped_data = wrap_bin(data)
         (existing_entry, created) = self.get_or_create(
             node=node, defaults={'data': wrapped_data})
         if not created:
@@ -72,7 +72,7 @@
         self.filter(node__in=nodes).delete()
         if data is not None:
             self.bulk_create((
-                self.model(node=node, data=Bin(data))
+                self.model(node=node, data=wrap_bin(data))
                 for node in nodes
             ))
 

=== modified file 'src/metadataserver/models/tests/test_nodecommissionresult.py'
--- src/metadataserver/models/tests/test_nodecommissionresult.py	2014-09-15 14:28:28 +0000
+++ src/metadataserver/models/tests/test_nodecommissionresult.py	2014-09-23 04:43:27 +0000
@@ -22,7 +22,7 @@
 from maasserver.utils.converters import XMLToYAML
 from maastesting.djangotestcase import DjangoTestCase
 from metadataserver.enum import RESULT_TYPE
-from metadataserver.fields import Bin
+from metadataserver.fields import wrap_bin
 from metadataserver.models import NodeResult
 from metadataserver.models.commissioningscript import (
     LLDP_OUTPUT_NAME,
@@ -133,7 +133,7 @@
         script_result = randint(0, 10)
         result = NodeResult.objects.store_data(
             node, name=name, script_result=script_result,
-            result_type=RESULT_TYPE.COMMISSIONING, data=Bin(data))
+            result_type=RESULT_TYPE.COMMISSIONING, data=wrap_bin(data))
         result_in_db = NodeResult.objects.get(node=node)
 
         self.assertAttributes(result_in_db, dict(name=name, data=data))
@@ -148,7 +148,7 @@
         data = factory.make_bytes(1024 * 1024)
         NodeResult.objects.store_data(
             node, name=name, script_result=script_result,
-            result_type=RESULT_TYPE.COMMISSIONING, data=Bin(data))
+            result_type=RESULT_TYPE.COMMISSIONING, data=wrap_bin(data))
 
         self.assertAttributes(
             NodeResult.objects.get(node=node),

=== modified file 'src/metadataserver/models/tests/test_noderesults.py'
--- src/metadataserver/models/tests/test_noderesults.py	2014-09-10 16:20:31 +0000
+++ src/metadataserver/models/tests/test_noderesults.py	2014-09-23 04:43:27 +0000
@@ -45,7 +45,7 @@
 from maastesting.matchers import MockCalledOnceWith
 from maastesting.utils import sample_binary_data
 from metadataserver.enum import RESULT_TYPE
-from metadataserver.fields import Bin
+from metadataserver.fields import wrap_bin
 from metadataserver.models import (
     CommissioningScript,
     commissioningscript as cs_module,
@@ -153,7 +153,7 @@
     def test_scripts_may_be_binary(self):
         name = make_script_name()
         CommissioningScript.objects.create(
-            name=name, content=Bin(sample_binary_data))
+            name=name, content=wrap_bin(sample_binary_data))
         stored_script = CommissioningScript.objects.get(name=name)
         self.assertEqual(sample_binary_data, stored_script.content)
 

=== modified file 'src/metadataserver/tests/test_fields.py'
--- src/metadataserver/tests/test_fields.py	2014-07-18 17:05:57 +0000
+++ src/metadataserver/tests/test_fields.py	2014-09-23 04:43:27 +0000
@@ -22,6 +22,7 @@
 from metadataserver.fields import (
     Bin,
     BinaryField,
+    wrap_bin,
     )
 from metadataserver.tests.models import BinaryFieldModel
 
@@ -29,24 +30,27 @@
 class TestBin(MAASServerTestCase):
     """Test Bin helper class."""
 
-    def test_is_basically_bytes(self):
-        self.assertEqual(b"Hello", Bin(b"Hello"))
-
-    def test_refuses_to_construct_from_unicode(self):
-        self.assertRaises(AssertionError, Bin, "Hello")
-
-    def test_refuses_to_construct_from_None(self):
-        self.assertRaises(AssertionError, Bin, None)
-
-    def test_emits_base64(self):
+    def test__is_what_wrap_bin_returns(self):
+        self.assertIsInstance(wrap_bin(b"Hi"), Bin)
+
+    def test__is_basically_bytes(self):
+        self.assertEqual(b"Hello", wrap_bin(b"Hello"))
+
+    def test__refuses_to_construct_from_unicode(self):
+        self.assertRaises(AssertionError, wrap_bin, "Hello")
+
+    def test__refuses_to_construct_from_None(self):
+        self.assertRaises(AssertionError, wrap_bin, None)
+
+    def test__emits_base64(self):
         # Piston hooks onto an __emittable__() method, if present.
         # Bin() returns a base-64 encoded string so that it can be
         # transmitted in JSON.
-        self.assertEqual(b"", Bin(b"").__emittable__())
+        self.assertEqual(b"", wrap_bin(b"").__emittable__())
         example_bytes = factory.make_bytes()
         self.assertEqual(
             b64encode(example_bytes),
-            Bin(example_bytes).__emittable__())
+            wrap_bin(example_bytes).__emittable__())
 
 
 class TestBinaryField(TestModelMixin, MAASServerTestCase):
@@ -62,7 +66,7 @@
             BinaryFieldModel.objects.get(id=binary_item.id).data)
 
     def test_stores_and_retrieves_empty_data(self):
-        binary_item = BinaryFieldModel(data=Bin(b''))
+        binary_item = BinaryFieldModel(data=wrap_bin(b''))
         self.assertEqual(b'', binary_item.data)
         binary_item.save()
         self.assertEqual(
@@ -70,7 +74,7 @@
 
     def test_does_not_truncate_at_zero_bytes(self):
         data = b"BEFORE THE ZERO\x00AFTER THE ZERO"
-        binary_item = BinaryFieldModel(data=Bin(data))
+        binary_item = BinaryFieldModel(data=wrap_bin(data))
         self.assertEqual(data, binary_item.data)
         binary_item.save()
         self.assertEqual(
@@ -78,24 +82,24 @@
 
     def test_stores_and_retrieves_binary_data(self):
         data = b"\x01\x02\xff\xff\xfe\xff\xff\xfe"
-        binary_item = BinaryFieldModel(data=Bin(data))
+        binary_item = BinaryFieldModel(data=wrap_bin(data))
         self.assertEqual(data, binary_item.data)
         binary_item.save()
         self.assertEqual(
             data, BinaryFieldModel.objects.get(id=binary_item.id).data)
 
     def test_returns_bytes_not_text(self):
-        binary_item = BinaryFieldModel(data=Bin(b"Data"))
+        binary_item = BinaryFieldModel(data=wrap_bin(b"Data"))
         binary_item.save()
         retrieved_data = BinaryFieldModel.objects.get(id=binary_item.id).data
         self.assertIsInstance(retrieved_data, bytes)
 
     def test_looks_up_data(self):
         data = b"Binary item"
-        binary_item = BinaryFieldModel(data=Bin(data))
+        binary_item = BinaryFieldModel(data=wrap_bin(data))
         binary_item.save()
         self.assertEqual(
-            binary_item, BinaryFieldModel.objects.get(data=Bin(data)))
+            binary_item, BinaryFieldModel.objects.get(data=wrap_bin(data)))
 
     def test_get_default_returns_None(self):
         field = BinaryField(null=True)
@@ -104,10 +108,10 @@
 
     def test_get_default_returns_Bin(self):
         field = BinaryField(null=True)
-        self.patch(field, "default", Bin(b"wotcha"))
-        self.assertEqual(Bin(b"wotcha"), field.get_default())
+        self.patch(field, "default", wrap_bin(b"wotcha"))
+        self.assertEqual(wrap_bin(b"wotcha"), field.get_default())
 
     def test_get_default_returns_Bin_from_bytes(self):
         field = BinaryField(null=True)
         self.patch(field, "default", b"wotcha")
-        self.assertEqual(Bin(b"wotcha"), field.get_default())
+        self.assertEqual(wrap_bin(b"wotcha"), field.get_default())