Fix failed tests test_attrs() and test_mkdir_with_path()

[shuuji3]

Hello, this PR is a trial to fix test_attrs(), one of the current pytest failures.

I noticed this PATCH call didn't actually update the metadata field but PUT did:

gcsfs/gcsfs/core.py

Lines 837 to 845 in c2d2d89

	o_json = await self._call(
	"PATCH",
	"b/{}/o/{}",
	bucket,
	key,
	fields="metadata",
	json=i_json,
	json_out=True,
	)

I'm not sure this is the best way to fix it since the official documentation recommends us to use a PATCH request for the update of user-defined metadata: Objects: update  |  Cloud Storage  |  Google Cloud. Maybe this is an issue of fsouza/fake-gcs-server.

I also delete the following line as the assigned values seem not used anywhere:

gcsfs/gcsfs/core.py

Line 846 in c2d2d89

(await self._info(path))["metadata"] = o_json.get("metadata", {})

Here is a test GitHub Actions run on my fork branch: shuuji3/pull/1

[shuuji3]

Another test failure for test_mkdir_with_path is caused by this new feature of fsouza/fake-gcs-server: fsouza/fake-gcs-server#805. The behavior reflects an actual Cloud Storage behavior so I think we should use another unique directory name for testing.

[martindurant]

Would you mind testing PUT versus PATCH on real google?

[shuuji3]

I created a new bucket named shuuji3-gcsfs-test and run the below code.

To summarize, the original PATCH method can modify metadata against real Google Cloud Storage objects as expected but the PUT method changes the current behavior. It will remove all other metadata key-value if we don't put all pairs to setxattrs() arguments.

It looks like this is only an issue in testing so I think I should revert my change that replaces PATCH with PUT.

original PATCH method

> cd gcsfs/
> pip install .
> pip list
Package                  Version
------------------------ -------------------
aiohttp                  3.8.1
aiosignal                1.2.0
async-timeout            4.0.2
attrs                    21.4.0
cachetools               5.2.0
certifi                  2022.5.18.1
charset-normalizer       2.0.12
decorator                5.1.1
frozenlist               1.3.0
fsspec                   2022.5.0
gcsfs                    2022.5.0+1.gd323bc3
(...)

> ipython

In [1]: from gcsfs import GCSFileSystem
   ...: gcs = GCSFileSystem()
   ...: hello_patch='gs://shuuji3-gcsfs-test/hello-patch'
   ...: hello_put='gs://shuuji3-gcsfs-test/hello-put'


In [2]: gcs.touch(hello_patch)

In [3]: gcs.setxattrs(hello_patch, key1='abc', key2='xyz')
@_setxattrs: method=PATCH
Out[3]: {'key1': 'abc', 'key2': 'xyz'}

In [4]: gcs.getxattr(hello_patch, 'key1')
Out[4]: 'abc'

In [5]: gcs.getxattr(hello_patch, 'key2')
Out[5]: 'xyz'

In [6]: gcs.setxattrs(hello_patch, key1='abc-modified')
@_setxattrs: method=PATCH
Out[6]: {'key1': 'abc-modified', 'key2': 'xyz'}

In [8]: gcs.getxattr(hello_patch, 'key1')
Out[8]: 'abc-modified'

In [9]: gcs.getxattr(hello_patch, 'key2')
Out[9]: 'xyz'

In [10]: gcs.getxattr(hello_patch, 'key3')
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
Input In [10], in <cell line: 1>()
----> 1 gcs.getxattr(hello_patch, 'key3')

File ~/src/gcsfs/venv/lib/python3.9/site-packages/fsspec/asyn.py:86, in sync_wrapper.<locals>.wrapper(*args, **kwargs)
     83 @functools.wraps(func)
     84 def wrapper(*args, **kwargs):
     85     self = obj or args[0]
---> 86     return sync(self.loop, func, *args, **kwargs)

File ~/src/gcsfs/venv/lib/python3.9/site-packages/fsspec/asyn.py:66, in sync(loop, func, timeout, *args, **kwargs)
     64     raise FSTimeoutError from return_result
     65 elif isinstance(return_result, BaseException):
---> 66     raise return_result
     67 else:
     68     return return_result

File ~/src/gcsfs/venv/lib/python3.9/site-packages/fsspec/asyn.py:26, in _runner(event, coro, result, timeout)
     24     coro = asyncio.wait_for(coro, timeout=timeout)
     25 try:
---> 26     result[0] = await coro
     27 except Exception as ex:
     28     result[0] = ex

File ~/src/gcsfs/venv/lib/python3.9/site-packages/gcsfs/core.py:787, in GCSFileSystem._getxattr(self, path, attr)
    785 """Get user-defined metadata attribute"""
    786 meta = (await self._info(path)).get("metadata", {})
--> 787 return meta[attr]

KeyError: 'key3'

PUT method

After modifying core.py:

    async def _setxattrs(

(...)

        bucket, key = self.split_path(path)
+       print('@_setxattrs: method=PUT')
        o_json = await self._call(
-           "PATCH",
+           "PUT",
            "b/{}/o/{}",
            bucket,
            key,
            fields="metadata",
            json=i_json,
            json_out=True,
        )

tried similar modifications:

In [4]: gcs.touch(hello_put)

In [5]: gcs.setxattrs(hello_put, key1='abc-put', key2='xyz-put')
@_setxattrs: method=PUT
Out[5]: {'key1': 'abc-put', 'key2': 'xyz-put'}

In [6]: gcs.setxattrs(hello_put, key1='abc-put-modified')
@_setxattrs: method=PUT
Out[6]: {'key1': 'abc-put-modified'}

In [7]: gcs.getxattr(hello, 'key1')
Out[7]: 'abc-modified'

In [8]: gcs.getxattr(hello, 'key2')
Out[8]: 'xyz'

I also confirmed that the returned values of getxattr are the same as metadata shown in GCP Console.

[martindurant]

It makes sense to me that PUT should replace, and PATCH amend the attrs. In our usage, we are after amending.

[martindurant]

So should we set xfail on these until the GCS emulator agrees, or add a replace= flag to setattrs to control whether we use PUT or PATCH?

[shuuji3]

Thank you for tidying up my incomplete PR.