Don't modify Resource fields in place

Attribute accesses in SDK pass through a type-conversion utility method
that is responsible for ensuring the following behavior:

  >>> class Demo(resource.Resource):
  ...     foo = resource.Body('foo', type=str)
  ...
  >>> demo = Demo(foo=123)
  >>> demo.foo
  '123'  # result is cast to a string

Unfortunately, because of how this is implemented, attribute accesses
can result result in a copy of the attribute being returned, rather than
the attribute itself. This means attempts to modify mutable attributes
in-place can end up modifying a copy of the attributes, rather than the
attribute itself.

  >>> class Demo(resource.Resource):
  ...     foo = resource.Body('foo', type=list, list_type=str)
  ...
  >>> demo = Demo(foo=[123])
  >>> demo.foo
  ['123']  # items are cast to strings
  >>> demo.foo.append(456)
  >>> demo.foo
  ['123']  # 456 is missing!

This was not previously an issue for horizon as we were not hitting any
of these cases, however, in in openstacksdk 4.3.0, a bug was addressed
where conversion would not happen if using 'type=list', if 'list_type'
was unset, and if the item was a set or tuple (i.e. not a list).

  >>> class Demo(resource.Resource):
  ...     foo = resource.Body('foo', type=list)
  ...
  >>> demo = Demo(foo={'123'})
  >>> demo.foo
  {'123'}  # should be a list!

This has now been fixed, however, the bugfix has exposed a case where we
were in fact modifying a mutable list in-place.

The long-term fix lies in changing how openstacksdk works, so that it
always return the original attribute rather potentially returning a
copy. However, this fix is likely going to be rather involved and not
something we want to cram into the end of the Epoxy release. Instead,
modify Horizon so that it updates the entire attribute rather than
modifying it in place.

Change-Id: I6e7814ea16ca84689b363a53f22de62800c1f0d8
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
Related-bug: #2100605

openstack_dashboard/test/unit/api/test_neutron.py

@@ -1594,15 +1594,16 @@ class NeutronApiTests(test.APIMockTestCase):
         router = self.api_routers_with_routes_sdk[0]
         router_id = self.api_routers_with_routes_sdk[0]['id']
         post_router = copy.deepcopy(router)
-        route = api.neutron.RouterStaticRoute(
-            post_router['routes'].pop())
+        routes = post_router.routes[:]
+        route = api.neutron.RouterStaticRoute(routes.pop())
+        post_router.routes = routes
 
         networkclient = mock_networkclient.return_value
         networkclient.get_router.return_value = router
         networkclient.update_router.return_value = post_router
 
-        api.neutron.router_static_route_remove(self.request,
-                                               router_id, route.id)
+        api.neutron.router_static_route_remove(
+            self.request, router_id, route.id)
 
         networkclient.get_router.assert_called_once_with(router_id)
         networkclient.update_router.assert_called_once_with(
@@ -1613,14 +1614,17 @@ class NeutronApiTests(test.APIMockTestCase):
         router = self.api_routers_with_routes_sdk[0]
         router_id = self.api_routers_with_routes_sdk[0]['id']
         post_router = copy.deepcopy(router)
+        routes = post_router.routes[:]
         route = {'nexthop': '10.0.0.5', 'destination': '40.0.1.0/24'}
-        post_router['routes'].insert(0, route)
+        routes.insert(0, route)
+        post_router.routes = routes
 
         networkclient = mock_networkclient.return_value
         networkclient.get_router.return_value = router
         networkclient.update_router.return_value = post_router
 
-        api.neutron.router_static_route_add(self.request, router_id, route)
+        api.neutron.router_static_route_add(
+            self.request, router_id, route)
 
         networkclient.get_router.assert_called_once_with(router_id)
         networkclient.update_router.assert_called_once_with(

tools/unit_tests.sh

@@ -1,5 +1,7 @@
 # Usage: unit_tests.sh [--coverage] <root_dir> [<test-file>, ...]
 
+set -o xtrace
+
 if [ "$1" = "--coverage" ]; then
   shift
   coverage=1