PYTHON-4669 - Update Async GridFS APIs for Motor Compatibility #1821
Conversation
gridfs/asynchronous/grid_file.py
Outdated
| return super().__next__() | ||
| else: | ||
| async def __anext__(self) -> bytes: | ||
| return await self.readline() |
There was a problem hiding this comment.
We need to handle the end-of-stream and raise StopAsyncIteration here. Could you add a test for this?
gridfs/asynchronous/grid_file.py
Outdated
|
|
||
| # This is a duplicate definition of __next__ for the synchronous API | ||
| # due to the limitations of our synchro process | ||
| def __next__(self) -> bytes: # noqa: F811, RUF100 |
There was a problem hiding this comment.
Let's just remove __next__ altogether.
gridfs/asynchronous/grid_file.py
Outdated
| """Read one line or up to `size` bytes from the file. | ||
|
|
||
| :param size: the maximum number of bytes to read | ||
| """ | ||
| return await self._read_size_or_line(size=size, line=True) | ||
|
|
||
| async def readlines(self, size: int = -1) -> list[bytes]: |
There was a problem hiding this comment.
This should be async only too since we intentionally get it for free from IOBase in the sync version.
gridfs/asynchronous/grid_file.py
Outdated
| @@ -1194,19 +1194,9 @@ def __setattr__(self, name: str, value: Any) -> None: | |||
| ) | |||
There was a problem hiding this comment.
Rather than having __setattr__ always raise an error in async, we should allow it as long as the file is not closed and only raise an error if the file is closed:
# All other attributes are part of the document in db.fs.files.
# Store them to be sent to server on close() or if closed, send
# them now.
self._file[name] = value
if self._closed:
if _IS_SYNC:
self._coll.files.update_one({"_id": self._file["_id"]}, {"$set": {name: value}})
else:
raise AttributeError(
"AsyncGridIn does not support __setattr__ after being closed(). Set the attribute before closing the file or use AsyncGridIn.set() instead")
test/test_grid_file.py
Outdated
| # self.assertEqual(data, g.read(10) + g.read(10)) | ||
| # return True | ||
| # | ||
| # qcheck.check_unittest(self, helper, qcheck.gen_string(qcheck.gen_range(0, 20))) |
There was a problem hiding this comment.
We shouldn't disable this test in sync. It should only be skipped in async.
There was a problem hiding this comment.
Good catch, forgot to uncomment whoops.
test/test_grid_file.py
Outdated
| @@ -781,6 +848,7 @@ def test_zip(self): | |||
| self.assertEqual(1, self.db.fs.chunks.count_documents({})) | |||
|
|
|||
| g = GridOut(self.db.fs, f._id) | |||
| g.open() | |||
There was a problem hiding this comment.
Why add the open call here?
There was a problem hiding this comment.
Artifact from before I made this test sync-only, good catch.
test/test_grid_file.py
Outdated
| if _IS_SYNC: | ||
| a.filename = "my_file" | ||
| else: | ||
| a.set("filename", "my_file") |
There was a problem hiding this comment.
Most (all?) of these can be change back to the a.filename = "my_file" version now, right?
There was a problem hiding this comment.
Trying to use a.filename = "my_file" gives the following error:
AttributeError: property 'filename' of 'AsyncGridIn' object has no setter
This is because all those properties are _a_grid_in_property instances, which don't have an async setter since setting does file IO.
There was a problem hiding this comment.
We should make the setters work now. They will only raise an error on async when the file is closed.
There was a problem hiding this comment.
Ah I misunderstood--makes sense, will do.
test/test_grid_file.py
Outdated
| @@ -213,12 +226,14 @@ def test_grid_out_default_opts(self): | |||
|
|
|||
| gout = GridOut(self.db.fs, 5) | |||
| with self.assertRaises(NoFile): | |||
| gout.open() | |||
There was a problem hiding this comment.
There's a lot of added open() calls in this file. Is that intended or should they be changed to if not _IS_SYNC: open()
There was a problem hiding this comment.
They should all be only for the async tests, fixed.
|
LGTM but could you update the PR title and jira ticket to summarize the gridfs changes? |
NoahStapp
changed the title
No description provided.