Merge lp:~lamont/maas/bug-1543707-1.9 into lp:maas/1.9
- bug-1543707-1.9
- Merge into 1.9
| Status: | Merged |
|---|---|
| Approved by: | LaMont Jones |
| Approved revision: | no longer in the source branch. |
| Merged at revision: | 4553 |
| Proposed branch: | lp:~lamont/maas/bug-1543707-1.9 |
| Merge into: | lp:maas/1.9 |
| Diff against target: |
320 lines (+158/-20) 7 files modified
docs/changelog.rst (+4/-0) src/maasserver/models/fabric.py (+24/-6) src/maasserver/models/space.py (+27/-6) src/maasserver/models/tests/test_fabric.py (+27/-0) src/maasserver/models/tests/test_space.py (+27/-0) src/maasserver/testing/factory.py (+0/-2) src/maasserver/websockets/tests/test_listener.py (+49/-6) |
| To merge this branch: | bzr merge lp:~lamont/maas/bug-1543707-1.9 |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Blake Rouse (community) | Approve | ||
|
Review via email:
|
|||
Commit message
Disallow blanks in fabric/space names. Disallow duplicate names. No schema migration (for upgrade compatibility), so no data migration needed.
Description of the change
Disallow blanks in fabric/space names. Disallow duplicate names. No schema migration (for upgrade compatibility), so no data migration needed.
| MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~lamont/maas/bug-1543707-1.9 into lp:maas/1.9 failed. Below is the output from the failed tests.
Get:1 http://
Ign http://
Get:2 http://
Hit http://
Hit http://
Hit http://
Get:3 http://
Get:4 http://
Get:5 http://
Get:6 http://
Get:7 http://
Get:8 http://
Get:9 http://
Get:10 http://
Get:11 http://
Hit http://
Hit http://
Get:12 http://
Get:13 http://
Get:14 http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
| MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~lamont/maas/bug-1543707-1.9 into lp:maas/1.9 failed. Below is the output from the failed tests.
Get:1 http://
Ign http://
Get:2 http://
Hit http://
Hit http://
Hit http://
Get:3 http://
Get:4 http://
Get:5 http://
Get:6 http://
Get:7 http://
Get:8 http://
Get:9 http://
Get:10 http://
Get:11 http://
Hit http://
Get:12 http://
Hit http://
Get:13 http://
Get:14 http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
| MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~lamont/maas/bug-1543707-1.9 into lp:maas/1.9 failed. Below is the output from the failed tests.
Ign http://
Get:1 http://
Get:2 http://
Hit http://
Hit http://
Hit http://
Get:3 http://
Get:4 http://
Get:5 http://
Get:6 http://
Get:7 http://
Get:8 http://
Get:9 http://
Get:10 http://
Get:11 http://
Get:12 http://
Get:13 http://
Hit http://
Get:14 http://
Hit http://
Get:15 http://
Get:16 http://
Get:17 http://
Get:18 http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
| MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~lamont/maas/bug-1543707-1.9 into lp:maas/1.9 failed. Below is the output from the failed tests.
Get:1 http://
Ign http://
Get:2 http://
Hit http://
Hit http://
Hit http://
Get:3 http://
Get:4 http://
Get:5 http://
Get:6 http://
Get:7 http://
Get:8 http://
Get:9 http://
Get:10 http://
Get:11 http://
Hit http://
Hit http://
Get:12 http://
Get:13 http://
Get:14 http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
| MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~lamont/maas/bug-1543707-1.9 into lp:maas/1.9 failed. Below is the output from the failed tests.
Ign http://
Get:1 http://
Get:2 http://
Hit http://
Hit http://
Hit http://
Get:3 http://
Get:4 http://
Get:5 http://
Get:6 http://
Get:7 http://
Get:8 http://
Get:9 http://
Get:10 http://
Get:11 http://
Get:12 http://
Get:13 http://
Get:14 http://
Get:15 http://
Get:16 http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
Hit http://
| MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~lamont/maas/bug-1543707-1.9 into lp:maas/1.9 failed. Below is the output from the failed tests.
Ign http://
Get:1 http://
Get:2 http://
Get:3 http://
Hit http://
Hit http://
Get:4 http://
Get:5 http://
Get:6 http://
Get:7 http://
Get:8 http://
Get:9 http://
Get:10 http://
Get:11 http://
Get:12 http://
Hit http://
Hit http://
Get:13 http://
Get:14 http://
Get:15 http://
Get:16 http://
Get:17 http://
Get:18 http://
Get:19 http://
Get:20 http://
Get:21 http://
Get:22 http://
Get:23 http://
Get:24 http://
Get:25 http://
Get:26 http://
Get:27 http://
Get:28 http://
| MAAS Lander (maas-lander) wrote : | # |
The attempt to merge lp:~lamont/maas/bug-1543707-1.9 into lp:maas/1.9 failed. Below is the output from the failed tests.
Get:1 http://
Ign http://
Get:2 http://
Get:3 http://
Hit http://
Hit http://
Get:4 http://
Get:5 http://
Get:6 http://
Get:7 http://
Get:8 http://
Get:9 http://
Get:10 http://
Hit http://
Get:11 http://
Get:12 http://
Hit http://
Get:13 http://
Get:14 http://
Get:15 http://
Get:16 http://
Get:17 http://
Get:18 http://
Get:19 http://
Get:20 http://
Get:21 http://
Get:22 http://
Get:23 http://
Get:24 http://
Get:25 http://
Get:26 http://
Get:27 http://
Hit http://
Hit http:/...
Preview Diff
| 1 | === modified file 'docs/changelog.rst' |
| 2 | --- docs/changelog.rst 2016-03-04 20:09:45 +0000 |
| 3 | +++ docs/changelog.rst 2016-04-08 16:06:53 +0000 |
| 4 | @@ -13,6 +13,10 @@ |
| 5 | |
| 6 | #1499934 Power state could not be queried (vmware) |
| 7 | |
| 8 | +#1543707 MAAS 1.9+ should not allow whitespace characters in space names |
| 9 | + |
| 10 | +#1543968 MAAS 1.9.0 allows non-unique space names |
| 11 | + |
| 12 | |
| 13 | 1.9.1 |
| 14 | ===== |
| 15 | |
| 16 | === modified file 'src/maasserver/models/fabric.py' |
| 17 | --- src/maasserver/models/fabric.py 2015-11-09 22:20:01 +0000 |
| 18 | +++ src/maasserver/models/fabric.py 2016-04-08 16:06:53 +0000 |
| 19 | @@ -42,7 +42,7 @@ |
| 20 | """Django validator: `value` must be either `None`, or valid.""" |
| 21 | if value is None: |
| 22 | return |
| 23 | - namespec = re.compile(r'^[ \w-]+$') |
| 24 | + namespec = re.compile(r'^[\w-]+$') |
| 25 | if not namespec.search(value): |
| 26 | raise ValidationError("Invalid fabric name: %s." % value) |
| 27 | |
| 28 | @@ -161,6 +161,8 @@ |
| 29 | |
| 30 | objects = FabricManager() |
| 31 | |
| 32 | + # We don't actually allow blank or null name, but that is enforced in |
| 33 | + # clean() and save(). Ditto for unique. |
| 34 | name = CharField( |
| 35 | max_length=256, editable=True, null=True, blank=True, unique=False, |
| 36 | validators=[validate_fabric_name]) |
| 37 | @@ -211,18 +213,34 @@ |
| 38 | name=DEFAULT_VLAN_NAME, vid=DEFAULT_VID, fabric=self) |
| 39 | |
| 40 | def save(self, *args, **kwargs): |
| 41 | + # Name will get set by clean_name() if None or empty, and there is an |
| 42 | + # id. We just need to handle names here for creation. |
| 43 | created = self.id is None |
| 44 | super(Fabric, self).save(*args, **kwargs) |
| 45 | + if self.name is None or self.name == '': |
| 46 | + # If we got here, then we have a newly created fabric that needs a |
| 47 | + # default name. |
| 48 | + self.name = "fabric-%d" % self.id |
| 49 | + fabric = Fabric.objects.get(id=self.id) |
| 50 | + fabric.save() |
| 51 | # Create default VLAN if this is a fabric creation. |
| 52 | if created: |
| 53 | self._create_default_vlan() |
| 54 | |
| 55 | def clean_name(self): |
| 56 | - reserved = re.compile('fabric-\d+$') |
| 57 | - if self.name is not None and reserved.search(self.name): |
| 58 | - if (self.id is None or self.name != 'fabric-%d' % self.id): |
| 59 | - raise ValidationError( |
| 60 | - {'name': ["Reserved fabric name."]}) |
| 61 | + reserved = re.compile('^fabric-\d+$') |
| 62 | + if self.name is not None and self.name != '': |
| 63 | + if reserved.search(self.name): |
| 64 | + if (self.id is None or self.name != 'fabric-%d' % self.id): |
| 65 | + raise ValidationError( |
| 66 | + {'name': ["Reserved fabric name."]}) |
| 67 | + elif self.id is not None: |
| 68 | + # Since we are not creating the fabric, force the (null or empty) |
| 69 | + # name to be the default name. |
| 70 | + self.name = "fabric-%d" % self.id |
| 71 | + name_count = Fabric.objects.filter(name=self.name).count() |
| 72 | + if name_count > 0 + (self.id is not None): |
| 73 | + raise ValidationError("Duplicate name: %s" % self.name) |
| 74 | |
| 75 | def clean(self, *args, **kwargs): |
| 76 | super(Fabric, self).clean(*args, **kwargs) |
| 77 | |
| 78 | === modified file 'src/maasserver/models/space.py' |
| 79 | --- src/maasserver/models/space.py 2015-11-09 22:20:01 +0000 |
| 80 | +++ src/maasserver/models/space.py 2016-04-08 16:06:53 +0000 |
| 81 | @@ -39,7 +39,7 @@ |
| 82 | """Django validator: `value` must be either `None`, or valid.""" |
| 83 | if value is None: |
| 84 | return |
| 85 | - namespec = re.compile('^[ \w-]+$') |
| 86 | + namespec = re.compile('^[\w-]+$') |
| 87 | if not namespec.search(value): |
| 88 | raise ValidationError("Invalid space name: %s." % value) |
| 89 | |
| 90 | @@ -134,6 +134,8 @@ |
| 91 | |
| 92 | objects = SpaceManager() |
| 93 | |
| 94 | + # We don't actually allow blank or null name, but that is enforced in |
| 95 | + # clean() and save(). Ditto for unique. |
| 96 | name = CharField( |
| 97 | max_length=256, editable=True, null=True, blank=True, unique=False, |
| 98 | validators=[validate_space_name]) |
| 99 | @@ -153,11 +155,30 @@ |
| 100 | return "space-%s" % self.id |
| 101 | |
| 102 | def clean_name(self): |
| 103 | - reserved = re.compile('space-\d+$') |
| 104 | - if self.name is not None and reserved.search(self.name): |
| 105 | - if (self.id is None or self.name != 'space-%d' % self.id): |
| 106 | - raise ValidationError( |
| 107 | - {'name': ["Reserved space name."]}) |
| 108 | + reserved = re.compile('^space-\d+$') |
| 109 | + if self.name is not None and self.name != '': |
| 110 | + if reserved.search(self.name): |
| 111 | + if (self.id is None or self.name != 'space-%d' % self.id): |
| 112 | + raise ValidationError( |
| 113 | + {'name': ["Reserved space name."]}) |
| 114 | + elif self.id is not None: |
| 115 | + # Since we are not creating the space, force the (null or empty) |
| 116 | + # name to be the default name. |
| 117 | + self.name = "space-%d" % self.id |
| 118 | + name_count = Space.objects.filter(name=self.name).count() |
| 119 | + if name_count > 0 + (self.id is not None): |
| 120 | + raise ValidationError("Duplicate name: %s" % self.name) |
| 121 | + |
| 122 | + def save(self, *args, **kwargs): |
| 123 | + # Name will get set by clean_name() if None or empty, and there is an |
| 124 | + # id. We just need to handle names here for creation. |
| 125 | + super(Space, self).save(*args, **kwargs) |
| 126 | + if self.name is None or self.name == '': |
| 127 | + # If we got here, then we have a newly created space that needs a |
| 128 | + # default name. |
| 129 | + self.name = "space-%d" % self.id |
| 130 | + space = Space.objects.get(id=self.id) |
| 131 | + space.save() |
| 132 | |
| 133 | def clean(self, *args, **kwargs): |
| 134 | super(Space, self).clean(*args, **kwargs) |
| 135 | |
| 136 | === modified file 'src/maasserver/models/tests/test_fabric.py' |
| 137 | --- src/maasserver/models/tests/test_fabric.py 2015-11-05 21:05:08 +0000 |
| 138 | +++ src/maasserver/models/tests/test_fabric.py 2016-04-08 16:06:53 +0000 |
| 139 | @@ -173,6 +173,33 @@ |
| 140 | default_fabric = Fabric.objects.get_default_fabric() |
| 141 | self.assertEqual(0, default_fabric.id) |
| 142 | self.assertEqual(DEFAULT_FABRIC_NAME, default_fabric.get_name()) |
| 143 | + self.assertEqual(DEFAULT_FABRIC_NAME, default_fabric.name) |
| 144 | + |
| 145 | + def test_create_sets_name(self): |
| 146 | + fabric = Fabric.objects.create(name=None) |
| 147 | + self.assertEqual("fabric-%d" % fabric.id, fabric.name) |
| 148 | + |
| 149 | + def test_create_does_not_override_name(self): |
| 150 | + name = factory.make_name() |
| 151 | + fabric = factory.make_Fabric(name=name) |
| 152 | + self.assertEqual(name, fabric.name) |
| 153 | + |
| 154 | + def test_nonreserved_name_does_not_raise_exception(self): |
| 155 | + fabric = factory.make_Fabric(name='myfabric-33') |
| 156 | + self.assertEqual("myfabric-33", fabric.name) |
| 157 | + |
| 158 | + def test_rejects_names_with_blanks(self): |
| 159 | + self.assertRaises( |
| 160 | + ValidationError, |
| 161 | + factory.make_Fabric, |
| 162 | + name=factory.make_name("Fabric ")) |
| 163 | + |
| 164 | + def test_rejects_duplicate_names(self): |
| 165 | + fabric1 = factory.make_Fabric() |
| 166 | + self.assertRaises( |
| 167 | + ValidationError, |
| 168 | + factory.make_Fabric, |
| 169 | + name=fabric1.name) |
| 170 | |
| 171 | def test_get_default_fabric_is_idempotent(self): |
| 172 | default_fabric = Fabric.objects.get_default_fabric() |
| 173 | |
| 174 | === modified file 'src/maasserver/models/tests/test_space.py' |
| 175 | --- src/maasserver/models/tests/test_space.py 2015-11-05 21:40:32 +0000 |
| 176 | +++ src/maasserver/models/tests/test_space.py 2016-04-08 16:06:53 +0000 |
| 177 | @@ -148,6 +148,7 @@ |
| 178 | default_space = Space.objects.get_default_space() |
| 179 | self.assertEqual(0, default_space.id) |
| 180 | self.assertEqual(DEFAULT_SPACE_NAME, default_space.get_name()) |
| 181 | + self.assertEqual(DEFAULT_SPACE_NAME, default_space.name) |
| 182 | |
| 183 | def test_invalid_name_raises_exception(self): |
| 184 | self.assertRaises( |
| 185 | @@ -161,6 +162,32 @@ |
| 186 | factory.make_Space, |
| 187 | name='space-1999') |
| 188 | |
| 189 | + def test_create_sets_name(self): |
| 190 | + space = Space.objects.create(name=None) |
| 191 | + self.assertEqual("space-%d" % space.id, space.name) |
| 192 | + |
| 193 | + def test_create_does_not_override_name(self): |
| 194 | + name = factory.make_name() |
| 195 | + space = factory.make_Space(name=name) |
| 196 | + self.assertEqual(name, space.name) |
| 197 | + |
| 198 | + def test_nonreserved_name_does_not_raise_exception(self): |
| 199 | + space = factory.make_Space(name='myspace-1999') |
| 200 | + self.assertEqual("myspace-1999", space.name) |
| 201 | + |
| 202 | + def test_rejects_names_with_blanks(self): |
| 203 | + self.assertRaises( |
| 204 | + ValidationError, |
| 205 | + factory.make_Space, |
| 206 | + name=factory.make_name("Space ")) |
| 207 | + |
| 208 | + def test_rejects_duplicate_names(self): |
| 209 | + space1 = factory.make_Space() |
| 210 | + self.assertRaises( |
| 211 | + ValidationError, |
| 212 | + factory.make_Space, |
| 213 | + name=space1.name) |
| 214 | + |
| 215 | def test_get_default_space_is_idempotent(self): |
| 216 | default_space = Space.objects.get_default_space() |
| 217 | default_space2 = Space.objects.get_default_space() |
| 218 | |
| 219 | === modified file 'src/maasserver/testing/factory.py' |
| 220 | --- src/maasserver/testing/factory.py 2016-04-05 15:38:30 +0000 |
| 221 | +++ src/maasserver/testing/factory.py 2016-04-08 16:06:53 +0000 |
| 222 | @@ -698,8 +698,6 @@ |
| 223 | return key |
| 224 | |
| 225 | def make_Space(self, name=None): |
| 226 | - if name is None: |
| 227 | - name = self.make_name('space--') |
| 228 | space = Space(name=name) |
| 229 | space.save() |
| 230 | return space |
| 231 | |
| 232 | === modified file 'src/maasserver/websockets/tests/test_listener.py' |
| 233 | --- src/maasserver/websockets/tests/test_listener.py 2015-11-05 00:32:43 +0000 |
| 234 | +++ src/maasserver/websockets/tests/test_listener.py 2016-04-08 16:06:53 +0000 |
| 235 | @@ -84,6 +84,7 @@ |
| 236 | from twisted.internet.defer import ( |
| 237 | CancelledError, |
| 238 | Deferred, |
| 239 | + DeferredList, |
| 240 | inlineCallbacks, |
| 241 | ) |
| 242 | from twisted.python.failure import Failure |
| 243 | @@ -1985,14 +1986,35 @@ |
| 244 | |
| 245 | @wait_for_reactor |
| 246 | @inlineCallbacks |
| 247 | - def test__calls_handler_on_create_notification(self): |
| 248 | + def test__calls_handler_on_create_notification_with_blank_name(self): |
| 249 | yield deferToDatabase(register_all_triggers) |
| 250 | listener = self.make_listener_without_delay() |
| 251 | - dv = DeferredValue() |
| 252 | - listener.register("fabric", lambda *args: dv.set(args)) |
| 253 | + dvs = [DeferredValue(), DeferredValue()] |
| 254 | + save_dvs = dvs[:] |
| 255 | + listener.register("fabric", lambda *args: dvs.pop().set(args)) |
| 256 | yield listener.start() |
| 257 | try: |
| 258 | fabric = yield deferToDatabase(self.create_fabric) |
| 259 | + results = yield DeferredList( |
| 260 | + (dv.get(timeout=2) for dv in save_dvs)) |
| 261 | + self.assertItemsEqual( |
| 262 | + [('create', '%s' % fabric.id), ('update', '%s' % fabric.id)], |
| 263 | + [res for (suc, res) in results]) |
| 264 | + finally: |
| 265 | + yield listener.stop() |
| 266 | + |
| 267 | + @wait_for_reactor |
| 268 | + @inlineCallbacks |
| 269 | + def test__calls_handler_on_create_notification_with_name(self): |
| 270 | + yield deferToDatabase(register_all_triggers) |
| 271 | + listener = self.make_listener_without_delay() |
| 272 | + dv = DeferredValue() |
| 273 | + listener.register("fabric", lambda *args: dv.set(args)) |
| 274 | + yield listener.start() |
| 275 | + try: |
| 276 | + fabric = yield deferToDatabase( |
| 277 | + self.create_fabric, |
| 278 | + {'name': factory.make_name('name')}) |
| 279 | yield dv.get(timeout=2) |
| 280 | self.assertEqual(('create', '%s' % fabric.id), dv.value) |
| 281 | finally: |
| 282 | @@ -2159,14 +2181,35 @@ |
| 283 | |
| 284 | @wait_for_reactor |
| 285 | @inlineCallbacks |
| 286 | - def test__calls_handler_on_create_notification(self): |
| 287 | + def test__calls_handler_on_create_notification_with_blank_name(self): |
| 288 | yield deferToDatabase(register_all_triggers) |
| 289 | listener = self.make_listener_without_delay() |
| 290 | - dv = DeferredValue() |
| 291 | - listener.register("space", lambda *args: dv.set(args)) |
| 292 | + dvs = [DeferredValue(), DeferredValue()] |
| 293 | + save_dvs = dvs[:] |
| 294 | + listener.register("space", lambda *args: dvs.pop().set(args)) |
| 295 | yield listener.start() |
| 296 | try: |
| 297 | space = yield deferToDatabase(self.create_space) |
| 298 | + results = yield DeferredList( |
| 299 | + (dv.get(timeout=2) for dv in save_dvs)) |
| 300 | + self.assertItemsEqual( |
| 301 | + [('create', '%s' % space.id), ('update', '%s' % space.id)], |
| 302 | + [res for (suc, res) in results]) |
| 303 | + finally: |
| 304 | + yield listener.stop() |
| 305 | + |
| 306 | + @wait_for_reactor |
| 307 | + @inlineCallbacks |
| 308 | + def test__calls_handler_on_create_notification_with_name(self): |
| 309 | + yield deferToDatabase(register_all_triggers) |
| 310 | + listener = self.make_listener_without_delay() |
| 311 | + dv = DeferredValue() |
| 312 | + listener.register("space", lambda *args: dv.set(args)) |
| 313 | + yield listener.start() |
| 314 | + try: |
| 315 | + space = yield deferToDatabase( |
| 316 | + self.create_space, |
| 317 | + {'name': factory.make_name('name')}) |
| 318 | yield dv.get(timeout=2) |
| 319 | self.assertEqual(('create', '%s' % space.id), dv.value) |
| 320 | finally: |
Looks good.