validating resource id; checking for duplicate id when adding to resource group

orchestrator/model/resources.py

 import copy
 import json
 import uuid
-
 from enum import Enum
-from voluptuous import Schema, Invalid, MultipleInvalid, Any, ALLOW_EXTRA, PREVENT_EXTRA
+
+from voluptuous import Schema, Invalid, MultipleInvalid, Any, ALLOW_EXTRA, PREVENT_EXTRA, re
 
 
 class Resource(object):
@@ -37,17 +37,17 @@ class Resource(object):
 
     @classmethod
     def __schema(cls, resource_type=None, extend=False):
-        try:
-            string_validator = Any(str, unicode)
-        except NameError:
-            string_validator = Any(str)
-
-        schema = Schema(schema={'id': string_validator, 'type': ResourceType.validate}, extra=ALLOW_EXTRA)
+        schema = Schema(schema={'id': cls.__id_validator, 'type': ResourceType.validate}, extra=ALLOW_EXTRA)
         if extend:
             additional = resource_type.ADDITIONAL_SCHEMA if resource_type else cls.ADDITIONAL_SCHEMA
             schema = schema.extend(additional, extra=PREVENT_EXTRA)
         return schema
 
+    @staticmethod
+    def __id_validator(resource_id):
+        if not re.compile('^[a-zA-Z0-9_-]{2,42}$').match(resource_id):
+            raise Invalid("'%s' is invalid" % resource_id)
+
     @property
     def type(self):
         return str(self._attributes['type'])
@@ -122,7 +122,10 @@ class ResourceGroup(Resource):
                 resource = Resource(resource)
             except InvalidResourceException:
                 raise InvalidResourceException('ResourceGroup can only store resources')
-        self.__resources[resource.id] = resource
+        if resource.id not in self.__resources.keys():
+            self.__resources[resource.id] = resource
+        else:
+            raise KeyError("ResourceGroup '%s' already contains a resource with id '%s'" % (self.id, resource.id))
 
     def remove(self, resource):
         try:
@@ -167,7 +170,7 @@ class ResourceType(Enum):
         except KeyError:
             return cls(value)
         except ValueError:
-            raise Invalid('The given resource type (%s) is not valid' % value)
+            raise Invalid("'%s' is invalid" % value)
 
     def __str__(self):
         return str(self.name)

tests/test_model.py

@@ -45,6 +45,24 @@ class ResourceTest(TestCase):
         with self.assertRaises(InvalidResourceException):
             Resource(invalid_attributes)
 
+    def test_resource_id_validation(self):
+        invalid_ids = [
+            'resource_id_which_is_way_too_long_to_be_valid',
+            'some.resource.id',
+            'some resource id',
+            'a'
+        ]
+
+        for invalid_id in invalid_ids:
+            with self.assertRaises(InvalidResourceException):
+                Instance(id=invalid_id)
+
+        self.assertIsInstance(Instance(id='valid_id'), Instance)
+        self.assertIsInstance(Instance(id='valid-id'), Instance)
+        self.assertIsInstance(Instance(id='VALID'), Instance)
+        self.assertIsInstance(Instance(id='123456'), Instance)
+        self.assertIsInstance(Instance(id='_id-42'), Instance)
+
     def test_resource_type_validation(self):
         self.assertTrue(ResourceType.validate('instance'))
         self.assertTrue(ResourceType.validate(Instance))
@@ -108,7 +126,7 @@ class ResourceTest(TestCase):
         # then
         self.assertEqual(expected_dict, resource_dict)
 
-    def test_dynamic_resource_type(self):
+    def test_resource_dynamic_type(self):
         # when
         resource = Resource(type='instance')
 
@@ -139,7 +157,6 @@ class ResourceGroupTest(TestCase):
 
         # then
         self.assertEqual([resource1], ResourceGroup(resources=[resource1]).resources)
-        self.assertEqual([resource1], ResourceGroup(resources=[resource1, resource1]).resources)
         for resources in [[resource1, resource2], [resource2, resource1]]:
             self.assertEqual(2, len(ResourceGroup(resources=resources).resources))
             self.assertIn(resource1, ResourceGroup(resources=resources).resources)
@@ -158,7 +175,6 @@ class ResourceGroupTest(TestCase):
 
         # then
         self.assertEqual([expected1], ResourceGroup(resources=[dict1]).resources)
-        self.assertEqual([expected1], ResourceGroup(resources=[dict1, dict1]).resources)
         for resources in [[expected1, expected2], [dict2, dict1]]:
             self.assertEqual(2, len(ResourceGroup(resources=resources).resources))
             self.assertIn(expected1, ResourceGroup(resources=resources).resources)
@@ -208,7 +224,16 @@ class ResourceGroupTest(TestCase):
         # then
         self.assertEqual([], group.resources)
 
-    # @skip('works same way as resource addition')
+    def test_resource_group_duplicate_resource_id(self):
+        # given
+        group = ResourceGroup()
+        resource = Instance()
+
+        # then
+        with self.assertRaises(KeyError):
+            group.add(resource)
+            group.add(resource)
+
     def test_resource_group_addition(self):
         # given
         group1 = ResourceGroup()