Skip to content

PYTHON-4946 - Add GridFSBucket.rename_by_name #2219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Mar 31, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 2 additions & 0 deletions doc/changelog.rst
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ PyMongo 4.12 brings a number of changes including:
- Support for configuring DEK cache lifetime via the ``key_expiration_ms`` argument to
:class:`~pymongo.encryption_options.AutoEncryptionOpts`.
- Support for $lookup in CSFLE and QE supported on MongoDB 8.1+.
- Added :meth:`gridfs.asynchronous.grid_file.AsyncGridFSBucket.rename_by_name` and :meth:`gridfs.grid_file.GridFSBucket.rename_by_name`
for more performant renaming of file revisions.
Copy link
Member

Choose a reason for hiding this comment

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

of file revisions. -> of a file with multiple revisions.


Issues Resolved
...............
Expand Down
30 changes: 30 additions & 0 deletions gridfs/asynchronous/grid_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -1021,6 +1021,36 @@ async def rename(
"matched file_id %i" % (new_filename, file_id)
)

async def rename_by_name(
Copy link
Member

@ShaneHarvey ShaneHarvey Mar 24, 2025

Choose a reason for hiding this comment

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

Instead of adding a new method for this, should we consider overloading the existing rename method? For example:

>>> fs.rename(file_id, "new") # rename by id
>>> fs.rename(filename="old", new_filename="new") # rename by name

The pro is we have less apis to maintain. I guess a con is that the user could accidentally mix up the file_id and filename params.

Note I'm not advocating for either, I just think we should consider it and maybe ask the spec author about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like this suggestion--both methods have nearly identical bodies, so combining them makes sense. For #2218, the two methods have slightly different bodies, but still similar enough that we could do the same thing there. Thoughts?

Copy link
Member

@ShaneHarvey ShaneHarvey Mar 25, 2025

Choose a reason for hiding this comment

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

What would the method signatures looks like (including type hints)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After trying this out a bit, I think it's a little too confusing/unclear to combine them.

Copy link
Member

Choose a reason for hiding this comment

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

Could you share the signatures for posterity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

async def rename(file_id: Optional[Any], new_filename: str, filename: Optional[str] = None,...)

self, filename: str, new_filename: str, session: Optional[AsyncClientSession] = None
) -> None:
"""Renames the stored file with the specified filename.

For example::

my_db = AsyncMongoClient().test
fs = AsyncGridFSBucket(my_db)
await fs.upload_from_stream("test_file", "data I want to store!")
await fs.rename_by_name("test_file", "new_test_name")

Raises :exc:`~gridfs.errors.NoFile` if no file with file_id exists.
Copy link
Member

Choose a reason for hiding this comment

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

with file_id -> with the given filename


:param filename: The filename of the file to be renamed.
:param new_filename: The new name of the file.
:param session: a
:class:`~pymongo.client_session.AsyncClientSession`
Copy link
Member

Choose a reason for hiding this comment

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

our line length allows this to be one line now right?

        :param session: a :class:`~pymongo.client_session.AsyncClientSession`


.. versionadded:: 4.12
"""
_disallow_transactions(session)
result = await self._files.update_many(
{"filename": filename}, {"$set": {"filename": new_filename}}, session=session
)
if not result.matched_count:
raise NoFile(
f"no files could be renamed {new_filename} because none matched filename {filename}"
Copy link
Member

Choose a reason for hiding this comment

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

Suggest: f"no files could be renamed {new_filename!r} because none matched filename {filename!r}" to make the strings more obvious.

)


class AsyncGridIn:
"""Class to write data to GridFS."""
Expand Down
30 changes: 30 additions & 0 deletions gridfs/synchronous/grid_file.py
Original file line number Diff line number Diff line change
Expand Up @@ -1015,6 +1015,36 @@ def rename(
"matched file_id %i" % (new_filename, file_id)
)

def rename_by_name(
Copy link
Member

Choose a reason for hiding this comment

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

Let's add some hand written tests for this too like test_rename in test_gridfs_bucket. That way we get some signal on typing and the UX.

self, filename: str, new_filename: str, session: Optional[ClientSession] = None
) -> None:
"""Renames the stored file with the specified filename.

For example::

my_db = MongoClient().test
fs = GridFSBucket(my_db)
fs.upload_from_stream("test_file", "data I want to store!")
fs.rename_by_name("test_file", "new_test_name")

Raises :exc:`~gridfs.errors.NoFile` if no file with file_id exists.

:param filename: The filename of the file to be renamed.
:param new_filename: The new name of the file.
:param session: a
:class:`~pymongo.client_session.ClientSession`

.. versionadded:: 4.12
"""
_disallow_transactions(session)
result = self._files.update_many(
{"filename": filename}, {"$set": {"filename": new_filename}}, session=session
)
if not result.matched_count:
raise NoFile(
f"no files could be renamed {new_filename} because none matched filename {filename}"
)


class GridIn:
"""Class to write data to GridFS."""
Expand Down
5 changes: 4 additions & 1 deletion test/asynchronous/unified_format.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
from bson import SON, json_util
from bson.codec_options import DEFAULT_CODEC_OPTIONS
from bson.objectid import ObjectId
from gridfs import AsyncGridFSBucket, GridOut
from gridfs import AsyncGridFSBucket, GridOut, NoFile
from pymongo import ASCENDING, AsyncMongoClient, CursorType, _csot
from pymongo.asynchronous.change_stream import AsyncChangeStream
from pymongo.asynchronous.client_session import AsyncClientSession, TransactionOptions, _TxnState
Expand Down Expand Up @@ -630,6 +630,9 @@ def process_error(self, exception, spec):
self.assertNotIsInstance(error, NotPrimaryError)
elif isinstance(error, (InvalidOperation, ConfigurationError, EncryptionError)):
pass
# gridfs NoFile errors are considered client errors.
elif isinstance(error, NoFile):
Copy link
Member

Choose a reason for hiding this comment

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

Suggest adding this to the previous elif: InvalidOperation, ConfigurationError, EncryptionError, NoFile)):

pass
else:
self.assertNotIsInstance(error, PyMongoError)

Expand Down
Loading
Loading