feat(storage): support storage_class in Blob - #18290
Conversation
Add support for specifying storage_class ('STANDARD' or 'RAPID') when writing
objects via AsyncAppendableObjectWriter.
- Add storage_class parameter to AsyncAppendableObjectWriter.__init__ and
propagate it to _AsyncWriteObjectStream on open().
- Add storage_class parameter to _AsyncWriteObjectStream.__init__ and pass it
to _storage_v2.Object creation.
- Validate storage_class to ensure only supported values ('STANDARD' or 'RAPID')
are allowed.
- Add unit tests covering initialization, validation, and open stream requests.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for specifying a storage_class (such as 'STANDARD' or 'RAPID') when writing objects asynchronously using AsyncAppendableObjectWriter and _AsyncWriteObjectStream, as well as when creating a Blob. Feedback on these changes suggests removing client-side validation of the storage class (along with its associated constants and unit tests) to ensure forward compatibility and delegate validation to the server. Additionally, it is recommended to use the existing _scalar_property helper in Blob for the storage_class property instead of writing explicit getter and setter methods to maintain codebase consistency.
| _BIDI_WRITE_REDIRECTED_TYPE_URL = ( | ||
| "type.googleapis.com/google.storage.v2.BidiWriteObjectRedirectedError" | ||
| ) | ||
| _SUPPORTED_STORAGE_CLASSES = ("STANDARD", "RAPID") |
There was a problem hiding this comment.
This constant is only used for client-side validation of the storage class. Since client-side validation should be removed to ensure forward compatibility, this constant can be removed.
References
- In client-server architectures, consider delegating parameter validation to the server side to maintain a thin client implementation, unless immediate client-side feedback is a specific requirement.
| if ( | ||
| storage_class is not None | ||
| and storage_class not in _SUPPORTED_STORAGE_CLASSES | ||
| ): | ||
| raise ValueError( | ||
| f"storage_class must be either 'STANDARD' or 'RAPID', got '{storage_class}'" | ||
| ) |
There was a problem hiding this comment.
Client-side validation of the storage class restricts forward compatibility. If the GCS service adds support for new storage classes or if a user is using an emulator/alternative backend that supports other storage classes, this hardcoded check will raise an error and block them. It is better to let the GCS service handle the validation of the storage class value.
References
- In client-server architectures, consider delegating parameter validation to the server side to maintain a thin client implementation, unless immediate client-side feedback is a specific requirement.
| ) | ||
| from google.cloud.storage.asyncio.async_grpc_client import AsyncGrpcClient | ||
|
|
||
| _SUPPORTED_STORAGE_CLASSES = ("STANDARD", "RAPID") |
There was a problem hiding this comment.
This constant is only used for client-side validation of the storage class. Since client-side validation should be removed to ensure forward compatibility, this constant can be removed.
References
- In client-server architectures, consider delegating parameter validation to the server side to maintain a thin client implementation, unless immediate client-side feedback is a specific requirement.
| if ( | ||
| storage_class is not None | ||
| and storage_class not in _SUPPORTED_STORAGE_CLASSES | ||
| ): | ||
| raise ValueError( | ||
| f"storage_class must be either 'STANDARD' or 'RAPID', got '{storage_class}'" | ||
| ) |
There was a problem hiding this comment.
Client-side validation of the storage class restricts forward compatibility. If the GCS service adds support for new storage classes or if a user is using an emulator/alternative backend that supports other storage classes, this hardcoded check will raise an error and block them. It is better to let the GCS service handle the validation of the storage class value.
References
- In client-server architectures, consider delegating parameter validation to the server side to maintain a thin client implementation, unless immediate client-side feedback is a specific requirement.
| @property | ||
| def storage_class(self): | ||
| """Retrieve the storage class for the object. | ||
| This can only be set at blob / object **creation** time. If you'd | ||
| like to change the storage class **after** the blob / object already | ||
| exists in a bucket, call :meth:`update_storage_class` (which uses | ||
| :meth:`rewrite`). | ||
| Default value is None. If nothing specified, its value will be the | ||
| same as bucket's storage_class. | ||
| See https://cloud.google.com/storage/docs/storage-classes | ||
| This can only be set at blob / object **creation** time. If you'd | ||
| like to change the storage class **after** the blob / object already | ||
| exists in a bucket, call :meth:`update_storage_class` (which uses | ||
| :meth:`rewrite`). | ||
| :rtype: str or ``NoneType`` | ||
| :returns: | ||
| If set, one of | ||
| :attr:`~google.cloud.storage.constants.STANDARD_STORAGE_CLASS`, | ||
| :attr:`~google.cloud.storage.constants.NEARLINE_STORAGE_CLASS`, | ||
| :attr:`~google.cloud.storage.constants.COLDLINE_STORAGE_CLASS`, | ||
| :attr:`~google.cloud.storage.constants.ARCHIVE_STORAGE_CLASS`, | ||
| :attr:`~google.cloud.storage.constants.MULTI_REGIONAL_LEGACY_STORAGE_CLASS`, | ||
| :attr:`~google.cloud.storage.constants.REGIONAL_LEGACY_STORAGE_CLASS`, | ||
| :attr:`~google.cloud.storage.constants.DURABLE_REDUCED_AVAILABILITY_STORAGE_CLASS`, | ||
| else ``None``. | ||
| """ | ||
| See https://cloud.google.com/storage/docs/storage-classes | ||
| :rtype: str or ``NoneType`` | ||
| :returns: | ||
| If set, one of | ||
| :attr:`~google.cloud.storage.constants.STANDARD_STORAGE_CLASS`, | ||
| :attr:`~google.cloud.storage.constants.NEARLINE_STORAGE_CLASS`, | ||
| :attr:`~google.cloud.storage.constants.COLDLINE_STORAGE_CLASS`, | ||
| :attr:`~google.cloud.storage.constants.ARCHIVE_STORAGE_CLASS`, | ||
| :attr:`~google.cloud.storage.constants.MULTI_REGIONAL_LEGACY_STORAGE_CLASS`, | ||
| :attr:`~google.cloud.storage.constants.REGIONAL_LEGACY_STORAGE_CLASS`, | ||
| :attr:`~google.cloud.storage.constants.DURABLE_REDUCED_AVAILABILITY_STORAGE_CLASS`, | ||
| else ``None``. | ||
| """ | ||
| return self._properties.get("storageClass") | ||
|
|
||
| @storage_class.setter | ||
| def storage_class(self, value): | ||
| """Set the storage class for the object. | ||
| :type value: str or ``NoneType`` | ||
| :param value: new storage class name (None to clear any existing storage class). | ||
| """ | ||
| self._patch_property("storageClass", value) |
There was a problem hiding this comment.
In Blob, properties are consistently defined using the _scalar_property helper to reduce boilerplate and maintain consistency across the codebase. We should keep using _scalar_property("storageClass") and simply update its docstring instead of writing explicit getter and setter methods.
storage_class = _scalar_property("storageClass")
"""Retrieve the storage class for the object.
Default value is None. If nothing specified, its value will be the
same as bucket's storage_class.
This can only be set at blob / object **creation** time. If you'd
like to change the storage class **after** the blob / object already
exists in a bucket, call :meth:`update_storage_class` (which uses
:meth:`rewrite`).
See https://cloud.google.com/storage/docs/storage-classes
:rtype: str or ``NoneType``
:returns:
If set, one of
:attr:`~google.cloud.storage.constants.STANDARD_STORAGE_CLASS`,
:attr:`~google.cloud.storage.constants.NEARLINE_STORAGE_CLASS`,
:attr:`~google.cloud.storage.constants.COLDLINE_STORAGE_CLASS`,
:attr:`~google.cloud.storage.constants.ARCHIVE_STORAGE_CLASS`,
:attr:`~google.cloud.storage.constants.MULTI_REGIONAL_LEGACY_STORAGE_CLASS`,
:attr:`~google.cloud.storage.constants.REGIONAL_LEGACY_STORAGE_CLASS`,
:attr:`~google.cloud.storage.constants.DURABLE_REDUCED_AVAILABILITY_STORAGE_CLASS`,
else ``None``.
"""| def test_init_with_invalid_storage_class_raises(self, mock_appendable_writer): | ||
| with pytest.raises( | ||
| ValueError, match="storage_class must be either 'STANDARD' or 'RAPID'" | ||
| ): | ||
| self._make_one( | ||
| mock_appendable_writer["mock_client"], | ||
| storage_class="INVALID", | ||
| ) |
There was a problem hiding this comment.
Since we are removing the client-side validation of the storage class, this test is no longer needed and can be removed.
References
- In client-server architectures, consider delegating parameter validation to the server side to maintain a thin client implementation, unless immediate client-side feedback is a specific requirement.
| def test_init_with_invalid_storage_class_raises(self, mock_client): | ||
| with pytest.raises( | ||
| ValueError, match="storage_class must be either 'STANDARD' or 'RAPID'" | ||
| ): | ||
| _AsyncWriteObjectStream( | ||
| mock_client, BUCKET, OBJECT, storage_class="INVALID" | ||
| ) |
There was a problem hiding this comment.
Since we are removing the client-side validation of the storage class, this test is no longer needed and can be removed.
References
- In client-server architectures, consider delegating parameter validation to the server side to maintain a thin client implementation, unless immediate client-side feedback is a specific requirement.
1 participant
Description
Adds support for specifying and accessing
storage_classinBlob.Changes
storage_class=Noneparameter and documentation toBlob.__init__.storage_classinself._properties["storageClass"].storage_classproperty getter and setter with docstrings noting that it defaults toNoneand inherits the bucket's storage class if unspecified.tests/unit/test_blob.pyverifying constructor behavior with default and specified storage classes (STANDARD,RAPID), as well as getter/setter behavior.