feat(storage): support storage_class in _grpc_conversions - #18291
feat(storage): support storage_class in _grpc_conversions#18291chandra-siri wants to merge 3 commits into
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 the storage_class parameter during asynchronous object writes and within the Blob class, updating gRPC conversions and adding corresponding unit tests. The feedback recommends removing restrictive client-side validation of storage classes (such as limiting them to 'STANDARD' and 'RAPID') to ensure forward compatibility with other GCS storage classes, along with removing the associated validation tests. Additionally, it is suggested to use the existing _scalar_property helper in Blob instead of writing boilerplate getter and setter methods for storage_class.
| _SUPPORTED_STORAGE_CLASSES = ("STANDARD", "RAPID") | ||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Client-side validation of storage classes is an anti-pattern in Google Cloud client libraries. GCS supports multiple storage classes (e.g., STANDARD, NEARLINE, COLDLINE, ARCHIVE, RAPID, etc.), and new ones may be introduced in the future. Hardcoding a restricted list of supported storage classes like ('STANDARD', 'RAPID') prevents users from using other valid storage classes (such as NEARLINE or COLDLINE) and breaks forward compatibility when new storage classes are added. The backend already performs robust validation, so we should let the backend handle it.
| _SUPPORTED_STORAGE_CLASSES = ("STANDARD", "RAPID") | |
| logger = logging.getLogger(__name__) | |
| logger = logging.getLogger(__name__) |
| 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.
Remove the client-side validation of storage_class to allow all valid GCS storage classes and ensure forward compatibility.
| ) | ||
| from google.cloud.storage.asyncio.async_grpc_client import AsyncGrpcClient | ||
|
|
||
| _SUPPORTED_STORAGE_CLASSES = ("STANDARD", "RAPID") |
There was a problem hiding this comment.
Remove the restricted _SUPPORTED_STORAGE_CLASSES constant to avoid client-side validation of storage classes.
| 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.
Remove the client-side validation of storage_class to allow all valid GCS storage classes and ensure forward compatibility.
| @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.
Using _scalar_property is the established pattern in this file for scalar properties (like temporary_hold, event_based_hold, etc.). Replacing it with explicit getter/setter properties adds unnecessary boilerplate. We should keep _scalar_property("storageClass") and document the default value in its docstring instead.
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.
Remove the test verifying invalid storage class raises ValueError, as client-side validation has been removed.
| 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.
Remove the test verifying invalid storage class raises ValueError, as client-side validation has been removed.
1 participant
Description
Adds support for mapping
storage_classin_grpc_conversions.blob_to_proto(), enablingstorage_classfromBlobto properly propagate to the proto object during write stream initialization in_AsyncWriteObjectStream.open().Changes
"storage_class": "storage_class"to_BLOB_ATTR_TO_PROTO_FIELDingoogle/cloud/storage/_grpc_conversions.py.self.storage_classin_AsyncWriteObjectStream.open()ifself.blobdoes not have astorage_classspecified.tests/unit/test__grpc_conversions.pyverifying thatblob_to_protomapsstorage_classfor mock and realBlobinstances, as well as default handling.tests/unit/asyncio/test_async_write_object_stream.pyverifying thatBlob.storage_classis propagated to the initialBidiWriteObjectRequest.write_object_spec.resource.