Transfer enhancement #181
Transfer enhancement #181
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @DDSDerek, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a significant enhancement to the 115 cloud storage integration, specifically focusing on improving file and folder management operations. It adds a new configuration option to enable a "transfer module enhancement" which, when active, utilizes a more robust approach for creating and retrieving file/folder information by intelligently combining requests to the 115 OpenAPI, Cookie API, and a local database cache.
Highlights
- New Configuration Option: A new switch labeled "整理模块增强" (Organization Module Enhancement) has been added to the frontend configuration, allowing users to enable or disable this new functionality.
- Enhanced File/Folder Operations: The core logic for interacting with 115 cloud storage has been upgraded to provide more reliable and efficient file and folder creation and retrieval. This includes new methods that prioritize fetching data from a local database cache, falling back to the 115 Cookie API, and finally the OpenAPI.
- Dynamic Method Patching: Key file management methods within the
U115Panclass (create_folder,get_item,get_folder) are now dynamically patched based on thetransfer_module_enhancementsetting, redirecting calls to the new, enhanced logic. - Frontend UI Updates: The user interface has been updated to include the new configuration option, and all associated frontend assets have been rebuilt.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a "transfer enhancement" feature by adding new configuration options and significantly refactoring the 115 API interaction logic. A new U115OpenHelper class is introduced with more robust methods for file operations, including caching and fallbacks, which are then monkey-patched into the existing U115Pan class. While the overall direction is good, I've identified a critical bug in the patch-reverting logic that could leave the application in an inconsistent state. Additionally, there are several medium-severity issues in the new helper class concerning non-deterministic behavior, performance, and code clarity that should be addressed.
| for key, status in cls._func_active.items(): | ||
| if status: | ||
| if cls._original_method[key] is not None: | ||
| U115Pan.upload = cls._original_method[key] | ||
|
|
||
| cls._func_active[key] = False | ||
| logger.info(f"【P115Open】{log_map[key]}") | ||
| logger.info(f"【P115Open】{cls.log_map[key]}接口恢复原始状态成功") |
There was a problem hiding this comment.
There is a critical bug in the disable method. Line 130 (U115Pan.upload = cls._original_method[key]) incorrectly restores the upload method for every patched function. It should restore the method corresponding to the current key in the loop (create_folder, get_item, etc.). This will break the patch restoration logic.
Please change line 130 to:
setattr(U115Pan, key, cls._original_method[key])| self, parent_item: schemas.FileItem, name: str | ||
| ) -> Optional[schemas.FileItem]: | ||
| """ | ||
| 创建目录 |
There was a problem hiding this comment.
The use of randint(0, 1) to switch between API implementations makes the application's behavior non-deterministic. This can complicate debugging, testing, and ensuring consistent performance. It would be better to use a deterministic approach, such as a configuration flag to select the desired API, or a primary/fallback mechanism where one API is tried first and the other is used if the primary one fails.
| pickcode=data.get("pickcode", ""), | ||
| size=data.get("size") if data.get("type") == "file" else None, | ||
| modify_time=data.get("mtime", 0), |
There was a problem hiding this comment.
This method, named database_get_item, has a side effect of writing to the database (upsert_batch). This is unexpected for a "get" method and can make the code harder to understand and maintain. Getters should generally not modify state. Consider separating the read and write operations, or renaming the method to reflect its behavior (e.g., get_and_update_item_from_db).
| } | ||
| resp = self.cookie_client.fs_dir_getid(payload) | ||
| if not resp.get("state", None): | ||
| return None |
There was a problem hiding this comment.
This hardcoded sleep(1) could introduce unnecessary delays and affect performance. If this is intended to handle API rate limiting, it should be documented with a comment. A better approach would be to implement a more robust rate-limiting strategy that reacts to API responses (like HTTP 429) instead of sleeping unconditionally.
| def get_folder(self, path: Path) -> Optional[schemas.FileItem]: | ||
| """ | ||
| 获取指定路径的文件夹，如不存在则创建 | ||
| """ | ||
| try: | ||
| file_item = self.databasehelper.get_by_path(path=str(path)) | ||
| if file_item: | ||
| return schemas.FileItem( | ||
| storage="u115", | ||
| fileid=str(file_item.get("id")), | ||
| path=path.as_posix() + "/", | ||
| type="dir", | ||
| name=file_item.get("name"), | ||
| basename=Path(file_item.get("name")).stem, | ||
| extension=None, | ||
| pickcode=file_item.get("pickcode", ""), | ||
| size=None, | ||
| modify_time=file_item.get("mtime", 0), | ||
| ) | ||
| except MultipleResultsFound: | ||
| pass |
There was a problem hiding this comment.
There is a redundant database lookup here. This method first calls self.databasehelper.get_by_path (lines 890-906), and then self.get_item(path) on line 908. The get_item method itself already queries the database as its first step. You can remove the initial database check to avoid this redundant operation and simplify the code.
folder = self.get_item(path)
if folder:
return folder|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a 'transfer enhancement' feature, which appears to leverage a cookie-based 115 client and a local database cache to improve file and folder operations. The changes span the frontend configuration UI, backend configuration, and the core logic for interacting with 115, including monkey-patching existing methods.
The implementation is comprehensive, but I've identified a couple of areas for improvement in plugins.v2/p115strmhelper/core/u115_open.py to enhance code quality, predictability, and maintainability. Specifically, I've commented on some non-deterministic behavior and a magic number that could be improved.
Overall, this is a significant enhancement. Addressing these points will make the new functionality more robust and easier to maintain.
| if randint(0, 1) == 0: | ||
| resp = self._request_api( | ||
| "POST", | ||
| "/open/folder/add", | ||
| data={"pid": int(parent_item.fileid or "0"), "file_name": name}, | ||
| ) | ||
| if not resp: | ||
| return None | ||
| if not resp.get("state"): | ||
| if resp.get("code") == 20004: | ||
| # 目录已存在 | ||
| return self.get_item(new_path) | ||
| logger.warn(f"【P115Open】创建目录失败: {resp.get('error')}") | ||
| return None | ||
| return schemas.FileItem( | ||
| storage="u115", | ||
| fileid=str(resp["data"]["file_id"]), | ||
| path=str(new_path) + "/", | ||
| name=name, | ||
| basename=name, | ||
| type="dir", | ||
| modify_time=int(time.time()), | ||
| ) | ||
| else: | ||
| resp = self.cookie_client.fs_mkdir(name, pid=int(parent_item.fileid or "0")) | ||
| if not resp.get("state"): | ||
| if resp.get("errno") == 20004: | ||
| return self.get_item(new_path) | ||
| logger.warn(f"【P115Open】创建目录失败: {resp}") | ||
| return None | ||
| return schemas.FileItem( | ||
| storage="u115", | ||
| fileid=str(resp["cid"]), | ||
| path=str(new_path) + "/", | ||
| name=name, | ||
| basename=name, | ||
| type="dir", | ||
| modify_time=int(time.time()), | ||
| ) |
There was a problem hiding this comment.
The use of randint(0, 1) to randomly select between the OpenAPI and the cookie-based API for creating a folder introduces non-deterministic behavior, which can make debugging and testing difficult. This seems like temporary code that might have been left over from development. It's better to choose a single, reliable implementation. Given that this PR is an enhancement using the cookie client, I'd suggest consistently using the cookie_client.fs_mkdir method. If there's a reason to keep both, this choice should be made configurable rather than random.
# Consistently use the cookie client for creating folders to ensure deterministic behavior.
resp = self.cookie_client.fs_mkdir(name, pid=int(parent_item.fileid or "0"))
if not resp.get("state"):
if resp.get("errno") == 20004:
return self.get_item(new_path)
logger.warn(f"【P115Open】创建目录失败: {resp}")
return None
return schemas.FileItem(
storage="u115",
fileid=str(resp["cid"]),
path=str(new_path) + "/",
name=name,
basename=name,
type="dir",
modify_time=int(time.time()),
)| folder_id = resp.get("id", None) | ||
| if not folder_id: | ||
| return None | ||
| sleep(1) |
There was a problem hiding this comment.
Using sleep(1) with a magic number can be problematic for performance and maintainability. If this delay is necessary to prevent API rate-limiting, it would be better to add a comment explaining its purpose. For even better maintainability, consider defining it as a named constant at the top of the module.
sleep(1) # Delay to avoid potential API rate-limiting
No description provided.