Skip to content

Commit ca976d6

Browse files
committed
Fixed auth issue when using token="anon"
added unit tests for zonal file methods changed zonal_mocks to mock _get_bucket_type instead of mocking two methods
1 parent 0c719f5 commit ca976d6

File tree

4 files changed

+171
-47
lines changed

4 files changed

+171
-47
lines changed

gcsfs/extended_gcsfs.py

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
from google.api_core import exceptions as api_exceptions
66
from google.api_core import gapic_v1
77
from google.api_core.client_info import ClientInfo
8+
from google.auth.credentials import AnonymousCredentials
89
from google.cloud import storage_control_v2
910
from google.cloud.storage._experimental.asyncio.async_grpc_client import AsyncGrpcClient
1011
from google.cloud.storage._experimental.asyncio.async_multi_range_downloader import (
@@ -48,6 +49,9 @@ def __init__(self, *args, **kwargs):
4849
super().__init__(*args, **kwargs)
4950
self.grpc_client = None
5051
self.storage_control_client = None
52+
self.credential = self.credentials.credentials
53+
if self.credentials.token == "anon":
54+
self.credential = AnonymousCredentials()
5155
# initializing grpc and storage control client for Hierarchical and
5256
# zonal bucket operations
5357
self.grpc_client = asyn.sync(self.loop, self._create_grpc_client)
@@ -59,6 +63,7 @@ def __init__(self, *args, **kwargs):
5963
async def _create_grpc_client(self):
6064
if self.grpc_client is None:
6165
return AsyncGrpcClient(
66+
credentials=self.credential,
6267
client_info=ClientInfo(user_agent=f"{USER_AGENT}/{version}"),
6368
).grpc_client
6469
else:
@@ -71,7 +76,7 @@ async def _create_control_plane_client(self):
7176
user_agent=f"{USER_AGENT}/{version}"
7277
)
7378
return storage_control_v2.StorageControlAsyncClient(
74-
credentials=self.credentials.credentials, client_info=client_info
79+
credentials=self.credential, client_info=client_info
7580
)
7681

7782
async def _lookup_bucket_type(self, bucket):

gcsfs/tests/conftest.py

Lines changed: 17 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,6 @@
33
import shlex
44
import subprocess
55
import time
6-
from contextlib import nullcontext
7-
from unittest.mock import patch
86

97
import fsspec
108
import pytest
@@ -144,32 +142,23 @@ def extended_gcsfs(gcs_factory, populate=True):
144142
os.environ.get("STORAGE_EMULATOR_HOST") == "https://storage.googleapis.com"
145143
)
146144

147-
# Mock authentication if not using a real GCS endpoint,
148-
# since grpc client in extended_gcsfs does not work with anon access
149-
mock_authentication_manager = (
150-
patch("google.auth.default", return_value=(None, "fake-project"))
151-
if not is_real_gcs
152-
else nullcontext()
153-
)
154-
155-
with mock_authentication_manager:
156-
extended_gcsfs = gcs_factory()
157-
try:
158-
# Only create/delete/populate the bucket if we are NOT using the real GCS endpoint
159-
if not is_real_gcs:
160-
try:
161-
extended_gcsfs.rm(TEST_BUCKET, recursive=True)
162-
except FileNotFoundError:
163-
pass
164-
extended_gcsfs.mkdir(TEST_BUCKET)
165-
if populate:
166-
extended_gcsfs.pipe(
167-
{TEST_BUCKET + "/" + k: v for k, v in allfiles.items()}
168-
)
169-
extended_gcsfs.invalidate_cache()
170-
yield extended_gcsfs
171-
finally:
172-
_cleanup_gcs(extended_gcsfs, is_real_gcs)
145+
extended_gcsfs = gcs_factory()
146+
try:
147+
# Only create/delete/populate the bucket if we are NOT using the real GCS endpoint
148+
if not is_real_gcs:
149+
try:
150+
extended_gcsfs.rm(TEST_BUCKET, recursive=True)
151+
except FileNotFoundError:
152+
pass
153+
extended_gcsfs.mkdir(TEST_BUCKET)
154+
if populate:
155+
extended_gcsfs.pipe(
156+
{TEST_BUCKET + "/" + k: v for k, v in allfiles.items()}
157+
)
158+
extended_gcsfs.invalidate_cache()
159+
yield extended_gcsfs
160+
finally:
161+
_cleanup_gcs(extended_gcsfs, is_real_gcs)
173162

174163

175164
@pytest.fixture

gcsfs/tests/test_extended_gcsfs.py

Lines changed: 8 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -34,11 +34,8 @@ def _zonal_mocks_factory(file_data):
3434
if is_real_gcs:
3535
yield None
3636
return
37-
patch_target_lookup_bucket_type = (
38-
"gcsfs.extended_gcsfs.ExtendedGcsFileSystem._lookup_bucket_type"
39-
)
40-
patch_target_sync_lookup_bucket_type = (
41-
"gcsfs.extended_gcsfs.ExtendedGcsFileSystem._sync_lookup_bucket_type"
37+
patch_target_get_bucket_type = (
38+
"gcsfs.extended_gcsfs.ExtendedGcsFileSystem._get_bucket_type"
4239
)
4340
patch_target_create_mrd = (
4441
"google.cloud.storage._experimental.asyncio.async_multi_range_downloader"
@@ -63,20 +60,16 @@ async def download_side_effect(read_requests, **kwargs):
6360
mock_create_mrd = mock.AsyncMock(return_value=mock_downloader)
6461
with (
6562
mock.patch(
66-
patch_target_sync_lookup_bucket_type,
67-
return_value=BucketType.ZONAL_HIERARCHICAL,
68-
) as mock_sync_lookup_bucket_type,
69-
mock.patch(
70-
patch_target_lookup_bucket_type,
63+
patch_target_get_bucket_type,
7164
return_value=BucketType.ZONAL_HIERARCHICAL,
72-
),
65+
) as mock_get_bucket_type,
7366
mock.patch(patch_target_create_mrd, mock_create_mrd),
7467
mock.patch(
7568
patch_target_gcsfs_cat_file, new_callable=mock.AsyncMock
7669
) as mock_cat_file,
7770
):
7871
mocks = {
79-
"sync_lookup_bucket_type": mock_sync_lookup_bucket_type,
72+
"get_bucket_type": mock_get_bucket_type,
8073
"create_mrd": mock_create_mrd,
8174
"downloader": mock_downloader,
8275
"cat_file": mock_cat_file,
@@ -119,9 +112,6 @@ def test_read_block_zb(extended_gcsfs, zonal_mocks, subtests):
119112

120113
assert result == expected_data
121114
if mocks:
122-
mocks["sync_lookup_bucket_type"].assert_called_once_with(
123-
TEST_BUCKET
124-
)
125115
if expected_data:
126116
mocks["downloader"].download_ranges.assert_called_with(
127117
[(offset, mock.ANY, mock.ANY)]
@@ -149,7 +139,7 @@ def test_read_small_zb(extended_gcsfs, zonal_mocks):
149139
# cache drop
150140
assert len(f.cache.cache) < len(out)
151141
if mocks:
152-
mocks["sync_lookup_bucket_type"].assert_called_once_with(TEST_BUCKET)
142+
mocks["get_bucket_type"].assert_called_once_with(TEST_BUCKET)
153143

154144

155145
def test_readline_zb(extended_gcsfs, zonal_mocks):
@@ -194,7 +184,7 @@ def test_readline_empty_zb(extended_gcsfs, zonal_mocks):
194184
data = b""
195185
if not extended_gcsfs.on_google:
196186
with mock.patch.object(
197-
extended_gcsfs, "_sync_lookup_bucket_type", return_value=BucketType.UNKNOWN
187+
extended_gcsfs, "_get_bucket_type", return_value=BucketType.UNKNOWN
198188
):
199189
with extended_gcsfs.open(b, "wb") as f:
200190
f.write(data)
@@ -208,7 +198,7 @@ def test_readline_blocksize_zb(extended_gcsfs, zonal_mocks):
208198
data = b"ab\n" + b"a" * (2**18) + b"\nab"
209199
if not extended_gcsfs.on_google:
210200
with mock.patch.object(
211-
extended_gcsfs, "_sync_lookup_bucket_type", return_value=BucketType.UNKNOWN
201+
extended_gcsfs, "_get_bucket_type", return_value=BucketType.UNKNOWN
212202
):
213203
with extended_gcsfs.open(c, "wb") as f:
214204
f.write(data)

gcsfs/tests/test_zonal_file.py

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
"""Tests for ZonalFile write operations."""
2+
3+
from unittest import mock
4+
5+
import pytest
6+
7+
from gcsfs.extended_gcsfs import BucketType
8+
from gcsfs.tests.settings import TEST_BUCKET
9+
10+
file_path = f"{TEST_BUCKET}/zonal-file-test"
11+
test_data = b"hello world"
12+
13+
14+
@pytest.fixture
15+
def zonal_write_mocks():
16+
"""A fixture for mocking Zonal bucket write functionality."""
17+
18+
patch_target_get_bucket_type = (
19+
"gcsfs.extended_gcsfs.ExtendedGcsFileSystem._get_bucket_type"
20+
)
21+
patch_target_init_aaow = "gcsfs.zb_hns_utils.init_aaow"
22+
23+
mock_aaow = mock.AsyncMock()
24+
mock_init_aaow = mock.AsyncMock(return_value=mock_aaow)
25+
with (
26+
mock.patch(
27+
patch_target_get_bucket_type,
28+
return_value=BucketType.ZONAL_HIERARCHICAL,
29+
),
30+
mock.patch(patch_target_init_aaow, mock_init_aaow),
31+
):
32+
mocks = {
33+
"aaow": mock_aaow,
34+
"init_aaow": mock_init_aaow,
35+
}
36+
yield mocks
37+
38+
39+
@pytest.mark.parametrize(
40+
"setup_action, error_match",
41+
[
42+
(lambda f: setattr(f, "mode", "rb"), "File not in write mode"),
43+
(lambda f: setattr(f, "closed", True), "I/O operation on closed file"),
44+
(
45+
lambda f: setattr(f, "forced", True),
46+
"This file has been force-flushed, can only close",
47+
),
48+
],
49+
ids=["not_writable", "closed", "force_flushed"],
50+
)
51+
def test_zonal_file_write_value_errors(
52+
extended_gcsfs, zonal_write_mocks, setup_action, error_match # noqa: F841
53+
):
54+
"""Test ZonalFile.write raises ValueError for invalid states."""
55+
with extended_gcsfs.open(file_path, "wb") as f:
56+
setup_action(f)
57+
with pytest.raises(ValueError, match=error_match):
58+
f.write(test_data)
59+
60+
61+
def test_zonal_file_write_success(extended_gcsfs, zonal_write_mocks):
62+
"""Test that writing to a ZonalFile calls the underlying writer's append method."""
63+
with extended_gcsfs.open(file_path, "wb") as f:
64+
f.write(test_data)
65+
66+
zonal_write_mocks["aaow"].append.assert_awaited_once_with(test_data)
67+
68+
69+
def test_zonal_file_open_write_mode(extended_gcsfs, zonal_write_mocks):
70+
"""Test that opening a ZonalFile in write mode initializes the writer."""
71+
bucket, key, _ = extended_gcsfs.split_path(file_path)
72+
with extended_gcsfs.open(file_path, "wb"):
73+
pass
74+
75+
zonal_write_mocks["init_aaow"].assert_called_once_with(
76+
extended_gcsfs.grpc_client, bucket, key
77+
)
78+
79+
80+
def test_zonal_file_flush(extended_gcsfs, zonal_write_mocks):
81+
"""Test that flush calls the underlying writer's flush method."""
82+
with extended_gcsfs.open(file_path, "wb") as f:
83+
f.flush()
84+
85+
zonal_write_mocks["aaow"].flush.assert_awaited()
86+
87+
88+
def test_zonal_file_commit(extended_gcsfs, zonal_write_mocks):
89+
"""Test that commit finalizes the write and sets autocommit to True."""
90+
with extended_gcsfs.open(file_path, "wb") as f:
91+
f.commit()
92+
93+
zonal_write_mocks["aaow"].finalize.assert_awaited_once()
94+
assert f.autocommit is True
95+
96+
97+
def test_zonal_file_discard(extended_gcsfs, zonal_write_mocks): # noqa: F841
98+
"""Test that discard on a ZonalFile logs a warning."""
99+
with mock.patch("gcsfs.zonal_file.logger") as mock_logger:
100+
with extended_gcsfs.open(file_path, "wb") as f:
101+
f.discard()
102+
mock_logger.warning.assert_called_once()
103+
assert (
104+
"Discard is unavailable for Zonal Buckets"
105+
in mock_logger.warning.call_args[0][0]
106+
)
107+
108+
109+
def test_zonal_file_close(extended_gcsfs, zonal_write_mocks):
110+
"""Test that close finalizes the write by default (autocommit=True)."""
111+
with extended_gcsfs.open(file_path, "wb"):
112+
pass
113+
zonal_write_mocks["aaow"].close.assert_awaited_once_with(finalize_on_close=True)
114+
115+
116+
def test_zonal_file_close_with_autocommit_false(extended_gcsfs, zonal_write_mocks):
117+
"""Test that close does not finalize the write when autocommit is False."""
118+
119+
with extended_gcsfs.open(file_path, "wb", autocommit=False):
120+
pass # close is called on exit
121+
122+
zonal_write_mocks["aaow"].close.assert_awaited_once_with(finalize_on_close=False)
123+
124+
125+
@pytest.mark.parametrize(
126+
"method_name",
127+
[
128+
("_initiate_upload"),
129+
("_simple_upload"),
130+
("_upload_chunk"),
131+
],
132+
)
133+
def test_zonal_file_not_implemented_methods(
134+
extended_gcsfs, zonal_write_mocks, method_name # noqa: F841
135+
):
136+
"""Test that some GCSFile methods are not implemented for ZonalFile."""
137+
with extended_gcsfs.open(file_path, "wb") as f:
138+
method_to_call = getattr(f, method_name)
139+
with pytest.raises(NotImplementedError):
140+
method_to_call()

0 commit comments

Comments
 (0)