From a11c3d316195f3d19c76c7040e312eb1869fe768 Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Fri, 28 Feb 2025 13:07:12 +0000 Subject: [PATCH] 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 Related-bug: #2100605 --- .../test/unit/api/test_neutron.py | 16 ++++++++++------ tools/unit_tests.sh | 2 ++ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/openstack_dashboard/test/unit/api/test_neutron.py b/openstack_dashboard/test/unit/api/test_neutron.py index 62fa8e56a9..46db82d8fa 100644 --- a/openstack_dashboard/test/unit/api/test_neutron.py +++ b/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( diff --git a/tools/unit_tests.sh b/tools/unit_tests.sh index 90dd0e7a0c..ace2d9add9 100755 --- a/tools/unit_tests.sh +++ b/tools/unit_tests.sh @@ -1,5 +1,7 @@ # Usage: unit_tests.sh [--coverage] [, ...] +set -o xtrace + if [ "$1" = "--coverage" ]; then shift coverage=1