Return commit id on successful commit (#408)

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.

Author: kofrezo

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)
 
@@ -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:
@@ -480,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)
 
@@ -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

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):

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,
     }
 
 

serveradmin/dataset.py

@@ -16,17 +16,20 @@ 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_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)
 
 
 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_query(app=app, user=user, **commit_obj)
+        _, commit_id = commit_query(app=app, user=user, **commit_obj)
         self._confirm_changes()
+        return commit_id

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 = {

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):

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'))

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: