From 5da3e65a6fbea4698223dec4deab033a34e37824 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Kr=C3=B6ger?= Date: Thu, 21 Aug 2025 17:00:48 +0200 Subject: [PATCH 1/3] Return commit id on successful commit Knowing the commit id of changes can be useful for example when wanting to compare to committed changes and knowing which one finished first and other features that we want in the future. --- adminapi/dataset.py | 4 ++++ serveradmin/api/views.py | 3 ++- serveradmin/dataset.py | 6 ++++-- serveradmin/serverdb/query_committer.py | 8 +++++--- serveradmin/serverdb/views.py | 4 ++-- serveradmin/servershell/views.py | 3 +-- 6 files changed, 18 insertions(+), 10 deletions(-) diff --git a/adminapi/dataset.py b/adminapi/dataset.py index 7962a21bb..c7edfb3df 100644 --- a/adminapi/dataset.py +++ b/adminapi/dataset.py @@ -282,6 +282,8 @@ def commit(self): for obj in self: obj._confirm_changes() + return result['commit_id'] + def _fetch_results(self): request_data = {'filters': self._filters} if self._restrict is not None: @@ -489,6 +491,8 @@ def commit(self): self._confirm_changes() + return result['commit_id'] + class MultiAttr(set): """This class must redefine all mutable methods of the set class diff --git a/serveradmin/api/views.py b/serveradmin/api/views.py index d07dcf928..48f01eb5e 100644 --- a/serveradmin/api/views.py +++ b/serveradmin/api/views.py @@ -99,7 +99,7 @@ def dataset_commit(request, app, data): kwargs[key] = value try: - commit_query(app=app, **kwargs) + _, commit_id = commit_query(app=app, **kwargs) except ValidationError as error: return { 'status': 'error', @@ -109,6 +109,7 @@ def dataset_commit(request, app, data): return { 'status': 'success', + 'commit_id': commit_id, } diff --git a/serveradmin/dataset.py b/serveradmin/dataset.py index 6fb35fad0..eedcc125f 100644 --- a/serveradmin/dataset.py +++ b/serveradmin/dataset.py @@ -18,8 +18,9 @@ def _fetch_new_object(self, servertype): def commit(self, app=None, user=None): commit_obj = self._build_commit_object() - commit_query(app=app, user=user, **commit_obj) + _, commit_id = commit_query(app=app, user=user, **commit_obj) self._confirm_changes() + return commit_id def _fetch_results(self): return execute_query(self._filters, self._restrict, self._order_by) @@ -28,5 +29,6 @@ def _fetch_results(self): class DatasetObject(ApiDatasetObject): def commit(self, app=None, user=None): commit_obj = self._build_commit_object() - commit_query(app=app, user=user, **commit_obj) + _, commit_id = commit_query(app=app, user=user, **commit_obj) self._confirm_changes() + return commit_id diff --git a/serveradmin/serverdb/query_committer.py b/serveradmin/serverdb/query_committer.py index 6b73c5e20..80ece7880 100644 --- a/serveradmin/serverdb/query_committer.py +++ b/serveradmin/serverdb/query_committer.py @@ -129,7 +129,7 @@ def commit_query(created=[], changed=[], deleted=[], app=None, user=None): created_objects, changed_objects, deleted_objects ) - _log_changes(user, app, changed, created_objects, deleted_objects) + commit_id = _log_changes(user, app, changed, created_objects, deleted_objects) post_commit.send_robust( commit_query, created=created, changed=changed, deleted=deleted @@ -139,7 +139,7 @@ def commit_query(created=[], changed=[], deleted=[], app=None, user=None): list(created_objects.values()), list(changed_objects.values()), list(deleted_objects.values()), - ) + ), commit_id def _validate(attribute_lookup, changed, changed_objects): @@ -467,7 +467,7 @@ def _acl_violations(touched_objects, pending_changes, acl): return violations or None -def _log_changes(user, app, changed, created_objects, deleted_objects): +def _log_changes(user, app, changed, created_objects, deleted_objects) -> int: changes = list() commit = ChangeCommit(user=user, app=app) @@ -504,6 +504,8 @@ def _log_changes(user, app, changed, created_objects, deleted_objects): commit.save() Change.objects.bulk_create(changes) + return commit.id + def _fetch_servers(object_ids): servers = { diff --git a/serveradmin/serverdb/views.py b/serveradmin/serverdb/views.py index 3825ec9d0..1e46d462e 100644 --- a/serveradmin/serverdb/views.py +++ b/serveradmin/serverdb/views.py @@ -183,8 +183,8 @@ def recreate(request, change_id): server_object.pop(attribute_id) try: - commit = commit_query([server_object], user=request.user) - object_id = str(commit.created[0]['object_id']) + changes_obj, _ = commit_query([server_object], user=request.user) + object_id = str(changes_obj.created[0]['object_id']) except (CommitError, ValidationError) as error: messages.error(request, str(error)) return redirect(reverse('serverdb_changes')) diff --git a/serveradmin/servershell/views.py b/serveradmin/servershell/views.py index 55f3bf665..e1706f70e 100644 --- a/serveradmin/servershell/views.py +++ b/serveradmin/servershell/views.py @@ -315,8 +315,7 @@ def _edit(request: HttpRequest, server, edit_mode=False, template='edit'): # NO messages.info(request, str('Nothing has changed.')) else: try: - commit_obj = commit_query(created, changed, - user=request.user) + commit_obj, _ = commit_query(created, changed, user=request.user) except (PermissionDenied, ValidationError) as err: messages.error(request, str(err)) else: From 357bab22000c5da260d0e2f14e997063cbdf0442 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Kr=C3=B6ger?= Date: Thu, 4 Sep 2025 09:24:57 +0200 Subject: [PATCH 2/3] Add type hints for return value of commit methods --- adminapi/dataset.py | 4 ++-- serveradmin/dataset.py | 5 +++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/adminapi/dataset.py b/adminapi/dataset.py index c7edfb3df..53c8db1ab 100644 --- a/adminapi/dataset.py +++ b/adminapi/dataset.py @@ -271,7 +271,7 @@ def _fetch_new_object(self, servertype): ) return _format_obj(response['result']) - def commit(self): + def commit(self) -> int: commit = self._build_commit_object() result = send_request(COMMIT_ENDPOINT, post_params=commit) @@ -482,7 +482,7 @@ def update(self, other, **kwargs): self[key] = value # XXX: Deprecated - def commit(self): + def commit(self) -> int: commit = self._build_commit_object() result = send_request(COMMIT_ENDPOINT, post_params=commit) diff --git a/serveradmin/dataset.py b/serveradmin/dataset.py index eedcc125f..5add117b7 100644 --- a/serveradmin/dataset.py +++ b/serveradmin/dataset.py @@ -16,7 +16,7 @@ class Query(BaseQuery): def _fetch_new_object(self, servertype): return DatasetObject(get_default_attribute_values(servertype)) - def commit(self, app=None, user=None): + def commit(self, app=None, user=None) -> int: commit_obj = self._build_commit_object() _, commit_id = commit_query(app=app, user=user, **commit_obj) self._confirm_changes() @@ -27,7 +27,8 @@ def _fetch_results(self): class DatasetObject(ApiDatasetObject): - def commit(self, app=None, user=None): + # XXX: Deprecated use Query().commit(). + def commit(self, app=None, user=None) -> int: commit_obj = self._build_commit_object() _, commit_id = commit_query(app=app, user=user, **commit_obj) self._confirm_changes() From d353d6cd51e145f15e7b61b2ed8d8b4e35239994 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Kr=C3=B6ger?= Date: Thu, 4 Sep 2025 09:47:09 +0200 Subject: [PATCH 3/3] Update tests to expect commit_id as return value --- serveradmin/access_control/tests/test_acl.py | 2 +- .../serverdb/tests/test_ip_addr_type.py | 58 +++++++++---------- 2 files changed, 30 insertions(+), 30 deletions(-) diff --git a/serveradmin/access_control/tests/test_acl.py b/serveradmin/access_control/tests/test_acl.py index 3454557dd..a8b9ccee5 100644 --- a/serveradmin/access_control/tests/test_acl.py +++ b/serveradmin/access_control/tests/test_acl.py @@ -27,7 +27,7 @@ def test_can_commit_related_via_attribute(self): vm = Query({"hostname": "vm-1"}, ["hostname", "hv"]) vm.update(hv="hv-2") - self.assertIsNone(vm.commit(app=Application.objects.get(name="test"))) + self.assertIsInstance(vm.commit(app=Application.objects.get(name="test")), int) # See https://github.com/innogames/serveradmin/pull/351 def test_cannot_commit_related_via_attribute_target(self): diff --git a/serveradmin/serverdb/tests/test_ip_addr_type.py b/serveradmin/serverdb/tests/test_ip_addr_type.py index 856cda922..e014be07c 100644 --- a/serveradmin/serverdb/tests/test_ip_addr_type.py +++ b/serveradmin/serverdb/tests/test_ip_addr_type.py @@ -49,7 +49,7 @@ class TestIpAddrTypeNullForInternIp(TestIpAddrType): def test_server_without_intern_ip(self): server = self._get_server("null") - self.assertIsNone(server.commit(user=User.objects.first())) + self.assertIsInstance(server.commit(user=User.objects.first()), int) def test_server_with_intern_ip(self): server = self._get_server("null") @@ -87,7 +87,7 @@ def test_server_without_value(self): def test_server_with_value(self): server = self._get_server("host") server["intern_ip"] = "10.0.0.1/32" - self.assertIsNone(server.commit(user=User.objects.first())) + self.assertIsInstance(server.commit(user=User.objects.first()), int) def test_server_with_invalid_value(self): server = self._get_server("host") @@ -132,7 +132,7 @@ def test_change_server_hostname(self): to_rename = Query({"hostname": server["hostname"]}, ["hostname"]) to_rename.update(hostname=self.faker.hostname()) - self.assertIsNone(to_rename.commit(user=User.objects.first())) + self.assertIsInstance(to_rename.commit(user=User.objects.first()), int) class TestIpAddrTypeHostForInetAttributes(TestIpAddrType): @@ -141,13 +141,13 @@ class TestIpAddrTypeHostForInetAttributes(TestIpAddrType): def test_server_without_value(self): server = self._get_server("host") server["intern_ip"] = "10.0.0.1/32" - self.assertIsNone(server.commit(user=User.objects.first())) + self.assertIsInstance(server.commit(user=User.objects.first()), int) def test_server_with_value(self): server = self._get_server("host") server["intern_ip"] = "10.0.0.1/32" server["ip_config_ipv4"] = "10.0.0.2/32" - self.assertIsNone(server.commit(user=User.objects.first())) + self.assertIsInstance(server.commit(user=User.objects.first()), int) def test_server_with_invalid_value(self): server = self._get_server("host") @@ -187,7 +187,7 @@ def test_server_overlaps_with_network(self): server = self._get_server("host") server["intern_ip"] = "10.0.0.1/32" server["ip_config_ipv4"] = "10.0.1.5/32" - self.assertIsNone(server.commit(user=User.objects.first())) + self.assertIsInstance(server.commit(user=User.objects.first()), int) def test_server_with_duplicate_intern_ip(self): first = self._get_server("host") @@ -211,7 +211,7 @@ def test_server_with_duplicate_inet_different_attrs(self): other_attribute = self._get_server("host") other_attribute["intern_ip"] = "10.0.0.3/32" other_attribute["ip_config_new"] = "10.0.0.2/32" - self.assertIsNone(other_attribute.commit(user=User.objects.first())) + self.assertIsInstance(other_attribute.commit(user=User.objects.first()), int) def test_server_with_duplicate_inet_ip(self): server = self._get_server("host") @@ -223,7 +223,7 @@ def test_server_with_duplicate_inet_ip(self): duplicate = self._get_server("loadbalancer") duplicate["intern_ip"] = "10.0.0.2/32" duplicate["ip_config_ipv4"] = "10.0.0.1/32" - self.assertIsNone(duplicate.commit(user=User.objects.first())) + self.assertIsInstance(duplicate.commit(user=User.objects.first()), int) def test_change_server_hostname(self): server = self._get_server("host") @@ -233,7 +233,7 @@ def test_change_server_hostname(self): to_rename = Query({"hostname": server["hostname"]}, ["hostname"]) to_rename.update(hostname=self.faker.hostname()) - self.assertIsNone(to_rename.commit(user=User.objects.first())) + self.assertIsInstance(to_rename.commit(user=User.objects.first()), int) class TestIpAddrTypeLoadbalancerForInternIp(TestIpAddrType): @@ -247,7 +247,7 @@ def test_server_without_value(self): def test_server_with_value(self): server = self._get_server("loadbalancer") server["intern_ip"] = "10.0.0.1/32" - self.assertIsNone(server.commit(user=User.objects.first())) + self.assertIsInstance(server.commit(user=User.objects.first()), int) def test_server_with_ip_network(self): server = self._get_server("loadbalancer") @@ -262,7 +262,7 @@ def test_server_with_duplicate_intern_ip(self): second = self._get_server("loadbalancer") second["intern_ip"] = "10.0.0.1/32" - self.assertIsNone(second.commit(user=User.objects.first())) + self.assertIsInstance(second.commit(user=User.objects.first()), int) def test_change_server_hostname(self): server = self._get_server("loadbalancer") @@ -271,7 +271,7 @@ def test_change_server_hostname(self): to_rename = Query({"hostname": server["hostname"]}, ["hostname"]) to_rename.update(hostname=self.faker.hostname()) - self.assertIsNone(to_rename.commit(user=User.objects.first())) + self.assertIsInstance(to_rename.commit(user=User.objects.first()), int) class TestIpAddrTypeLoadbalancerForInetAttributes(TestIpAddrType): @@ -280,13 +280,13 @@ class TestIpAddrTypeLoadbalancerForInetAttributes(TestIpAddrType): def test_server_without_value(self): server = self._get_server("loadbalancer") server["intern_ip"] = "10.0.0.1/32" - self.assertIsNone(server.commit(user=User.objects.first())) + self.assertIsInstance(server.commit(user=User.objects.first()), int) def test_server_with_value(self): server = self._get_server("loadbalancer") server["intern_ip"] = "10.0.0.1/32" server["ip_config_ipv4"] = "10.0.0.2/32" - self.assertIsNone(server.commit(user=User.objects.first())) + self.assertIsInstance(server.commit(user=User.objects.first()), int) def test_server_with_ip_network(self): server = self._get_server("loadbalancer") @@ -304,7 +304,7 @@ def test_server_with_duplicate_inet_attribute(self): second = self._get_server("loadbalancer") second["intern_ip"] = "10.0.0.1/32" second["ip_config_ipv4"] = "10.0.0.2/32" - self.assertIsNone(second.commit(user=User.objects.first())) + self.assertIsInstance(second.commit(user=User.objects.first()), int) def test_server_with_duplicate_inet_ip(self): first = self._get_server("loadbalancer") @@ -316,7 +316,7 @@ def test_server_with_duplicate_inet_ip(self): duplicate = self._get_server("host") duplicate["intern_ip"] = "10.0.0.2/32" duplicate["ip_config_ipv4"] = "10.0.0.1/32" - self.assertIsNone(duplicate.commit(user=User.objects.first())) + self.assertIsInstance(duplicate.commit(user=User.objects.first()), int) def test_server_with_duplicate_inet_different_attrs(self): server = self._get_server("loadbalancer") @@ -327,7 +327,7 @@ def test_server_with_duplicate_inet_different_attrs(self): duplicate = self._get_server("loadbalancer") duplicate["intern_ip"] = "10.0.0.3/32" duplicate["ip_config_new"] = "10.0.0.2/32" - self.assertIsNone(duplicate.commit(user=User.objects.first())) + self.assertIsInstance(duplicate.commit(user=User.objects.first()), int) def test_change_server_hostname(self): server = self._get_server("loadbalancer") @@ -337,7 +337,7 @@ def test_change_server_hostname(self): to_rename = Query({"hostname": server["hostname"]}, ["hostname"]) to_rename.update(hostname=self.faker.hostname()) - self.assertIsNone(to_rename.commit(user=User.objects.first())) + self.assertIsInstance(to_rename.commit(user=User.objects.first()), int) class TestIpAddrTypeNetworkForInternIp(TestIpAddrType): @@ -351,7 +351,7 @@ def test_server_without_value(self): def test_server_with_value(self): server = self._get_server("route_network") server["intern_ip"] = "10.0.0.0/16" - self.assertIsNone(server.commit(user=User.objects.first())) + self.assertIsInstance(server.commit(user=User.objects.first()), int) def test_server_with_invalid_value(self): server = self._get_server("host") @@ -369,7 +369,7 @@ def test_server_with_invalid_network(self): def test_server_with_ip_address(self): server = self._get_server("route_network") server["intern_ip"] = "10.0.0.1/32" # Just a very small network - self.assertIsNone(server.commit(user=User.objects.first())) + self.assertIsInstance(server.commit(user=User.objects.first()), int) def test_server_network_overlaps(self): first = self._get_server("route_network") @@ -389,7 +389,7 @@ def test_change_server_network_overlaps(self): host = Query({"hostname": first["hostname"]}, ["intern_ip"]) host.update(intern_ip=IPv4Network("10.0.0.0/28")) - self.assertIsNone(host.commit(user=User.objects.first())) + self.assertIsInstance(host.commit(user=User.objects.first()), int) def test_server_network_overlaps_inet(self): first = self._get_server("route_network") @@ -410,7 +410,7 @@ def test_server_network_overlaps_other_servertype(self): # A network can overlap with networks of other servertypes overlaps = self._get_server("provider_network") overlaps["intern_ip"] = "10.0.0.0/28" - self.assertIsNone(overlaps.commit(user=User.objects.first())) + self.assertIsInstance(overlaps.commit(user=User.objects.first()), int) def test_change_server_hostname(self): server = self._get_server("route_network") @@ -419,7 +419,7 @@ def test_change_server_hostname(self): to_rename = Query({"hostname": server["hostname"]}, ["hostname"]) to_rename.update(hostname=self.faker.hostname()) - self.assertIsNone(to_rename.commit(user=User.objects.first())) + self.assertIsInstance(to_rename.commit(user=User.objects.first()), int) class TestIpAddrTypeNetworkForInetAttributes(TestIpAddrType): @@ -428,13 +428,13 @@ class TestIpAddrTypeNetworkForInetAttributes(TestIpAddrType): def test_server_without_value(self): server = self._get_server("route_network") server["intern_ip"] = "10.0.0.0/16" - self.assertIsNone(server.commit(user=User.objects.first())) + self.assertIsInstance(server.commit(user=User.objects.first()), int) def test_server_with_value(self): server = self._get_server("route_network") server["intern_ip"] = "10.0.0.0/30" server["ip_config_ipv4"] = "10.0.1.0/30" - self.assertIsNone(server.commit(user=User.objects.first())) + self.assertIsInstance(server.commit(user=User.objects.first()), int) def test_server_with_invalid_value(self): server = self._get_server("host") @@ -455,7 +455,7 @@ def test_server_with_ip_address(self): server = self._get_server("route_network") server["intern_ip"] = "10.0.0.1/32" # Just a very small network server["ip_config_ipv4"] = "10.0.1.0/32" # Just a very small network - self.assertIsNone(server.commit(user=User.objects.first())) + self.assertIsInstance(server.commit(user=User.objects.first()), int) def test_server_network_overlaps(self): first = self._get_server("route_network") @@ -477,7 +477,7 @@ def test_change_server_network_overlaps(self): host = Query({"hostname": first["hostname"]}, ["ip_config_ipv4"]) host.update(ip_config_ipv4=IPv4Network("10.0.1.0/28")) - self.assertIsNone(host.commit(user=User.objects.first())) + self.assertIsInstance(host.commit(user=User.objects.first()), int) def test_server_network_overlaps_intern_ip(self): first = self._get_server("route_network") @@ -512,7 +512,7 @@ def test_server_network_overlaps_other_servertype(self): overlaps = self._get_server("provider_network") overlaps["intern_ip"] = "10.0.0.0/28" overlaps["ip_config_ipv4"] = "10.0.1.0/30" - self.assertIsNone(overlaps.commit(user=User.objects.first())) + self.assertIsInstance(overlaps.commit(user=User.objects.first()), int) def test_change_server_hostname(self): server = self._get_server("route_network") @@ -522,7 +522,7 @@ def test_change_server_hostname(self): to_rename = Query({"hostname": server["hostname"]}, ["hostname"]) to_rename.update(hostname=self.faker.hostname()) - self.assertIsNone(to_rename.commit(user=User.objects.first())) + self.assertIsInstance(to_rename.commit(user=User.objects.first()), int) class TestIpAddrTypeHostForSupernetAttr(TestIpAddrType):