Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 9 additions & 14 deletions packages/google-cloud-storage/google/cloud/storage/blob.py
Original file line number Diff line number Diff line change
Expand Up @@ -5368,20 +5368,10 @@ class ObjectCustomContextPayload(dict):

:type value: str or ``NoneType``
:param value: (Optional) The value of the custom context.

:type create_time: :class:`datetime.datetime` or ``NoneType``
:param create_time: (Optional) Creation time of the custom context.

:type update_time: :class:`datetime.datetime` or ``NoneType``
:param update_time: (Optional) Last update time of the custom context.
"""

def __init__(self, value=None, create_time=None, update_time=None):
def __init__(self, value=None):
data = {"value": value}
if create_time is not None:
data["createTime"] = _datetime_to_rfc3339(create_time)
if update_time is not None:
data["updateTime"] = _datetime_to_rfc3339(update_time)
super(ObjectCustomContextPayload, self).__init__(data)
self._contexts = None

Expand Down Expand Up @@ -5426,6 +5416,8 @@ def update_time(self):
class ObjectContexts(dict):
"""Container for an object's contexts.

See: https://docs.cloud.google.com/storage/docs/object-contexts

:type blob: :class:`Blob`
:param blob: blob for which these contexts apply to.

Expand All @@ -5445,12 +5437,10 @@ def __init__(self, blob, custom=None):
raise ValueError(
"All values in custom must be ObjectCustomContextPayload instances"
)
payload._contexts = self
data["custom"] = custom
super(ObjectContexts, self).__init__(data)
self._blob = blob
if custom is not None:
for payload in custom.values():
payload._contexts = self

@classmethod
def from_api_repr(cls, resource, blob):
Expand Down Expand Up @@ -5497,9 +5487,14 @@ def custom(self):

@custom.setter
def custom(self, value):
if value is None:
value = {}
if not isinstance(value, dict):
raise ValueError(
"custom must be a dictionary mapping keys to ObjectCustomContextPayload instances"
)
for payload in value.values():
if isinstance(payload, ObjectCustomContextPayload):
payload._contexts = self
Comment on lines +5496 to +5498
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The custom setter should strictly validate that all values in the dictionary are ObjectCustomContextPayload instances, matching the validation logic in ObjectContexts.__init__. The current implementation silently skips setting back-references for invalid types while still allowing them to be stored in the dictionary, which is inconsistent with the class's design and the error message on line 5493.

Suggested change
for payload in value.values():
if isinstance(payload, ObjectCustomContextPayload):
payload._contexts = self
for payload in value.values():
if not isinstance(payload, ObjectCustomContextPayload):
raise ValueError(
"All values in custom must be ObjectCustomContextPayload instances"
)
payload._contexts = self
References
  1. When a function receives parameters of an unsupported type, it should raise an error instead of silently returning empty values to ensure fail-fast behavior.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jules make changes according to what is suggested here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have implemented the requested changes in the custom setter for ObjectContexts in blob.py, as well as the other cleanups and test updates detailed in the implementation plan. This includes proper back-reference management and handling of None values.

self["custom"] = value
self.blob._patch_property("contexts", self)
Original file line number Diff line number Diff line change
Expand Up @@ -1518,7 +1518,7 @@ def list_blobs(
Note ``soft_deleted`` and ``versions`` cannot be set to True simultaneously. See:
https://cloud.google.com/storage/docs/soft-delete

:type filter_: str
:type filter_: str or None
:param filter_:
(Optional) Filter string used to filter objects. See:
https://docs.cloud.google.com/storage/docs/listing-objects#filter-by-object-contexts-syntax
Expand Down
3 changes: 2 additions & 1 deletion packages/google-cloud-storage/tests/system/test_blob.py
Original file line number Diff line number Diff line change
Expand Up @@ -1308,7 +1308,7 @@ def test_blob_contexts(shared_bucket, blobs_to_delete):

blob.reload()
assert blob.contexts.custom["k1"].value == "v1-updated"
assert "k2" not in blob.contexts.custom or blob.contexts.custom["k2"].value is None
assert "k2" not in blob.contexts.custom

# 3. Clear all
blob.contexts = None
Expand Down Expand Up @@ -1344,3 +1344,4 @@ def test_blob_contexts_custom_setter(shared_bucket, blobs_to_delete):

blob.reload()
assert blob.contexts.custom["k1"].value == "v1-updated"
assert blob.contexts.custom["k2"].value == "v2"
21 changes: 20 additions & 1 deletion packages/google-cloud-storage/tests/system/test_bucket.py
Original file line number Diff line number Diff line change
Expand Up @@ -775,21 +775,40 @@ def test_bucket_list_blobs_w_filter(
buckets_to_delete.append(bucket)

payload = b"helloworld"
blob_names = ["foo", "bar", "baz"]
blob_names = ["foo", "bar", "baz", "qux"]
for name in blob_names:
blob = bucket.blob(name)
blob.upload_from_string(payload)
if name == "bar":
custom = {"target": ObjectCustomContextPayload(value="match")}
blob.contexts = ObjectContexts(blob, custom=custom)
blob.patch()
if name == "qux":
custom = {"target": ObjectCustomContextPayload(value="nomatch")}
blob.contexts = ObjectContexts(blob, custom=custom)
blob.patch()
blobs_to_delete.append(blob)

# List with filter matching only 'bar'
blob_iter = bucket.list_blobs(filter_='contexts."target"="match"')
blobs = list(blob_iter)
assert [blob.name for blob in blobs] == ["bar"]

# List with filter matching bar and qux
blob_iter = bucket.list_blobs(filter_='contexts."target":*')
blobs = list(blob_iter)
assert sorted([blob.name for blob in blobs]) == ["bar", "qux"]

# List with filter matching all except 'bar'
blob_iter = bucket.list_blobs(filter_='-contexts."target"="match"')
blobs = list(blob_iter)
assert sorted([blob.name for blob in blobs]) == ["baz", "foo", "qux"]

# List with filter matching 'foo' and 'baz' (those without target context)
blob_iter = bucket.list_blobs(filter_='-contexts."target":*')
blobs = list(blob_iter)
assert sorted([blob.name for blob in blobs]) == ["baz", "foo"]


def test_bucket_list_blobs_include_managed_folders(
storage_client,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,8 +145,7 @@ def test_blob_to_proto_contexts():

from google.cloud.storage.blob import ObjectContexts, ObjectCustomContextPayload

create_time = datetime.datetime(2025, 1, 1, tzinfo=datetime.timezone.utc)
payload = ObjectCustomContextPayload(value="val", create_time=create_time)
payload = ObjectCustomContextPayload(value="val")
blob.contexts = ObjectContexts(blob, custom={"key": payload})

blob.custom_time = None
Expand Down