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
| """ | ||
| 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
| else: | ||
| raise AttributeError( | ||
| "AsyncGridIn does not support __setattr__. Use AsyncGridIn.set() instead" | ||
| ) |
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
| 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
| | ||
| 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.