From 613d5646fc798822b832743d9fe85616092a48d6 Mon Sep 17 00:00:00 2001 From: Satya Prakash G Date: Thu, 22 Jun 2023 08:14:24 +0000 Subject: [PATCH] feat(default_retention_period): Changes to backup.create API to support null expire time With the default retention period feature, null value for expiration time is a valid value. If the expiration time is not specified (null) at the time of create backup then the backup will be retained for the duration of maximum retention period. In this commit, the precondition check on expiration time in the create backup API path is removed. The condition remains intact for copy backup API and that would be addressed in a follow-up commit. --- google/cloud/spanner_v1/backup.py | 10 +++-- google/cloud/spanner_v1/instance.py | 3 +- tests/unit/test_backup.py | 57 +++++++++++++++++++++++++++-- tests/unit/test_instance.py | 18 +++++++++ 4 files changed, 79 insertions(+), 9 deletions(-) diff --git a/google/cloud/spanner_v1/backup.py b/google/cloud/spanner_v1/backup.py index 2f54cf2167..5abe127c91 100644 --- a/google/cloud/spanner_v1/backup.py +++ b/google/cloud/spanner_v1/backup.py @@ -53,8 +53,9 @@ class Backup(object): :type expire_time: :class:`datetime.datetime` :param expire_time: (Optional) The expire time that will be used to - create the backup. Required if the create method - needs to be called. + create the backup. If the expire time is not specified + then the backup will be retained for the duration of + maximum retention period. :type version_time: :class:`datetime.datetime` :param version_time: (Optional) The version time that was specified for @@ -271,8 +272,6 @@ def create(self): :raises BadRequest: if the database or expire_time values are invalid or expire_time is not set """ - if not self._expire_time: - raise ValueError("expire_time not set") if not self._database and not self._source_backup: raise ValueError("database and source backup both not set") @@ -295,6 +294,9 @@ def create(self): metadata = _metadata_with_prefix(self.name) if self._source_backup: + if not self._expire_time: + raise ValueError("expire_time not set") + request = CopyBackupRequest( parent=self._instance.name, backup_id=self.backup_id, diff --git a/google/cloud/spanner_v1/instance.py b/google/cloud/spanner_v1/instance.py index 1b426f8cc2..2614484603 100644 --- a/google/cloud/spanner_v1/instance.py +++ b/google/cloud/spanner_v1/instance.py @@ -532,7 +532,8 @@ def backup( :type expire_time: :class:`datetime.datetime` :param expire_time: Optional. The expire time that will be used when creating the backup. - Required if the create method needs to be called. + If the expire time is not specified then the backup will be retained + for the duration of maximum retention period. :type version_time: :class:`datetime.datetime` :param version_time: diff --git a/tests/unit/test_backup.py b/tests/unit/test_backup.py index 00621c2148..17f104134a 100644 --- a/tests/unit/test_backup.py +++ b/tests/unit/test_backup.py @@ -27,6 +27,8 @@ class _BaseTest(unittest.TestCase): DATABASE_NAME = INSTANCE_NAME + "/databases/" + DATABASE_ID BACKUP_ID = "backup_id" BACKUP_NAME = INSTANCE_NAME + "/backups/" + BACKUP_ID + DST_BACKUP_ID = "dst_backup_id" + DST_BACKUP_NAME = INSTANCE_NAME + "/backups" + DST_BACKUP_ID def _make_one(self, *args, **kwargs): return self._get_target_class()(*args, **kwargs) @@ -329,11 +331,45 @@ def test_create_instance_not_found(self): ) def test_create_expire_time_not_set(self): - instance = _Instance(self.INSTANCE_NAME) - backup = self._make_one(self.BACKUP_ID, instance, database=self.DATABASE_NAME) + from google.cloud.spanner_admin_database_v1 import Backup + from google.cloud.spanner_admin_database_v1 import CreateBackupRequest + from datetime import datetime + from datetime import timedelta + from datetime import timezone - with self.assertRaises(ValueError): - backup.create() + op_future = object() + client = _Client() + api = client.database_admin_api = self._make_database_admin_api() + api.create_backup.return_value = op_future + + instance = _Instance(self.INSTANCE_NAME, client=client) + version_timestamp = datetime.utcnow() - timedelta(minutes=5) + version_timestamp = version_timestamp.replace(tzinfo=timezone.utc) + backup = self._make_one( + self.BACKUP_ID, + instance, + database=self.DATABASE_NAME, + version_time=version_timestamp, + ) + + backup_pb = Backup( + database=self.DATABASE_NAME, + version_time=version_timestamp, + ) + + future = backup.create() + self.assertIs(future, op_future) + + request = CreateBackupRequest( + parent=self.INSTANCE_NAME, + backup_id=self.BACKUP_ID, + backup=backup_pb, + ) + + api.create_backup.assert_called_once_with( + request=request, + metadata=[("google-cloud-resource-prefix", backup.name)], + ) def test_create_database_not_set(self): instance = _Instance(self.INSTANCE_NAME) @@ -413,6 +449,19 @@ def test_create_w_invalid_encryption_config(self): with self.assertRaises(ValueError): backup.create() + def test_copy_expire_time_not_set(self): + client = _Client() + client.database_admin_api = self._make_database_admin_api() + instance = _Instance(self.INSTANCE_NAME, client=client) + backup = self._make_one( + self.DST_BACKUP_ID, + instance, + source_backup=self.BACKUP_ID, + ) + + with self.assertRaises(ValueError): + backup.create() + def test_exists_grpc_error(self): from google.api_core.exceptions import Unknown diff --git a/tests/unit/test_instance.py b/tests/unit/test_instance.py index f9d1fec6b8..2357183eae 100644 --- a/tests/unit/test_instance.py +++ b/tests/unit/test_instance.py @@ -695,6 +695,24 @@ def test_backup_factory_explicit(self): self.assertIs(backup._expire_time, timestamp) self.assertEqual(backup._encryption_config, encryption_config) + def test_backup_factory_without_expiration_time(self): + from google.cloud.spanner_v1.backup import Backup + + client = _Client(self.PROJECT) + instance = self._make_one(self.INSTANCE_ID, client, self.CONFIG_NAME) + BACKUP_ID = "backup-id" + DATABASE_NAME = "database-name" + + backup = instance.backup( + BACKUP_ID, + database=DATABASE_NAME, + ) + + self.assertIsInstance(backup, Backup) + self.assertEqual(backup.backup_id, BACKUP_ID) + self.assertIs(backup._instance, instance) + self.assertEqual(backup._database, DATABASE_NAME) + def test_list_backups_defaults(self): from google.cloud.spanner_admin_database_v1 import Backup as BackupPB from google.cloud.spanner_admin_database_v1 import DatabaseAdminClient