Skip to content

Conversation

@JohnRichard4096
Copy link
Member

@JohnRichard4096 JohnRichard4096 commented Oct 24, 2025

Sourcery 总结

引入一个用于货币、账户和交易数据的内存LRU缓存,并将其集成到API操作中,以提高性能和一致性。

新特性:

  • 添加 Cache 和 CacheManager 类,实现LRU淘汰机制以进行内存缓存。
  • 在API方法中公开缓存控制标志(no_cache_use, no_cache_refresh, no_cache_update)。
  • 通过新的 value_pre_build_cache 配置选项支持在启动时预构建缓存。

改进:

  • 在余额和货币API端点中集成缓存更新和过期调用。
  • 移除 AccountExecutor 中冗余的 data_map 存储,转而依赖全局缓存。
  • 将插件版本提升至 0.1.7。

文档:

  • 更新 README,以记录缓存功能、其配置和行为。

测试:

  • 为 CacheManager 和 Cache 功能添加单元测试,包括更新、获取、过期和清除操作。
Original summary in English

Summary by Sourcery

Introduce an in-memory LRU cache for currency, account, and transaction data and integrate it into API operations to improve performance and consistency.

New Features:

  • Add Cache and CacheManager classes implementing LRU eviction for in-memory caching.
  • Expose cache control flags (no_cache_use, no_cache_refresh, no_cache_update) in API methods.
  • Support pre-building caches at startup via a new value_pre_build_cache configuration option.

Enhancements:

  • Integrate cache update and expiration calls across balance and currency API endpoints.
  • Remove redundant data_map storage in AccountExecutor and rely on the global cache.
  • Bump plugin version to 0.1.7.

Documentation:

  • Update README to document the caching feature, its configuration, and behavior.

Tests:

  • Add unit tests for CacheManager and Cache functionality, including update, get, expire, and clear operations.

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Oct 24, 2025

审阅者指南

此 PR 引入了一个应用程序级别的 LRU 缓存,用于存储货币、账户和交易数据,将缓存更新和过期调用集成到 API 端点中,以支持 CRUD 和余额操作,移除了每个执行器级别的缓存,并添加了一个可配置的预构建缓存选项,附带文档和版本升级。

API 缓存更新和过期集成时序图

sequenceDiagram
participant API as "API Endpoint"
participant CacheManager
participant Cache
participant DB as "Database"
API->>CacheManager: update_cache(category, data)
CacheManager->>Cache: update(data)
API->>DB: Perform DB operation
API->>CacheManager: expire_cache(category, data_id)
CacheManager->>Cache: delete(data_id)
Loading

新增和更新的缓存类图

classDiagram
class CacheManager {
  +get_cache(category: CacheCategoryEnum, max_size: int = 1000) Cache[Any]
  +update_cache(category: CacheCategoryEnum, data: BaseData, max_size: int = 1000) Self
  +expire_cache(category: CacheCategoryEnum, data_id: str | None = None) Self
  +expire_all_cache() Self
}
class Cache {
  +max_size: int
  +update(data: BaseData) bool
  +get(data_id: str) BaseData | None
  +get_all() list[BaseData]
  +delete(data_id: str)
  +clear()
}
class CacheCategoryEnum {
  CURRENCY
  ACCOUNT
  TRANSACTION
}
class Config {
  +value_pre_build_cache: bool = True
}
CacheManager --> CacheCategoryEnum
CacheManager --> Cache
Cache --> BaseData
Cache --> UserAccountData
Cache --> CurrencyData
Cache --> TransactionData
Loading

缓存重构后 AccountExecutor 的类图

classDiagram
class AccountExecutor {
  +currency_id: str
  +user_id: str
  +__call__(event: Event) Self
  +get_data(currency_id: str | None = None) UserAccountData
  +get_balance(currency_id: str | None = None) float
  +add_balance(amount: float, source: str = "_transfer", currency_id: str | None = None) Self
  +decrease_balance(amount: float, source: str = "_transfer", currency_id: str | None = None) Self
}
AccountExecutor --> UserAccountData
Loading

文件级别更改

Change Details Files
引入 LRU 缓存管理器
  • 使用 LRU 淘汰策略和 asyncio 锁创建了 Cache 和 CacheManager
  • 定义了用于货币、账户和交易的 CacheCategoryEnum
  • 实现了 update/get/delete/expire/clear 操作
  • 为每个 data_id 提供了线程安全锁
  • 为缓存行为添加了全面的单元测试
nonebot_plugin_value/_cache.py
tests/value_tests/cache_test.py
将缓存操作集成到账户和货币 API 中
  • 在所有数据修改函数周围插入了 expire_cache 调用
  • 在数据检索函数之后添加了 update_cache 调用
  • 引入了 no_cache_use 和 no_cache_refresh 标志来控制缓存行为
  • 调整了函数签名和返回类型以反映缓存使用情况
nonebot_plugin_value/api/api_balance.py
nonebot_plugin_value/api/api_currency.py
通过移除内部缓存简化 AccountExecutor
  • 移除了 data_map 字段和相关逻辑
  • 将所有数据检索直接委托给 get_or_create_account
nonebot_plugin_value/api/executor.py
在启动时添加可配置的预构建缓存
  • 创建了带有 value_pre_build_cache 标志的 Config 模型
  • 在启用时,在插件初始化中预填充货币和账户缓存
  • 更新了 README,增加了新的配置和缓存功能描述
  • 将项目版本升级到 0.1.7
nonebot_plugin_value/config.py
nonebot_plugin_value/__init__.py
README.md
pyproject.toml
次要清理和格式修复
  • 规范化了 balance_pyd 中的空白和类型注解
  • 移除了 hooks_type 和 currency 仓库中多余的空行
  • 对 uuid_lib 和 VSCode 设置进行了次要格式调整
nonebot_plugin_value/pyd_models/balance_pyd.py
nonebot_plugin_value/hook/hooks_type.py
nonebot_plugin_value/repositories/currency.py
nonebot_plugin_value/uuid_lib.py
.vscode/settings.json

提示和命令

与 Sourcery 交互

  • 触发新审查: 在拉取请求上评论 @sourcery-ai review
  • 继续讨论: 直接回复 Sourcery 的审查评论。
  • 从审查评论生成 GitHub issue: 通过回复 Sourcery 的审查评论,要求它创建 issue。您也可以回复审查评论并带上 @sourcery-ai issue 来创建 issue。
  • 生成拉取请求标题: 随时在拉取请求标题中的任意位置写入 @sourcery-ai 来生成标题。您也可以在拉取请求上评论 @sourcery-ai title 来随时(重新)生成标题。
  • 生成拉取请求摘要: 随时在拉取请求正文中的任意位置写入 @sourcery-ai summary 来生成 PR 摘要,位置由您决定。您也可以在拉取请求上评论 @sourcery-ai summary 来随时(重新)生成摘要。
  • 生成审阅者指南: 在拉取请求上评论 @sourcery-ai guide 来随时(重新)生成审阅者指南。
  • 解决所有 Sourcery 评论: 在拉取请求上评论 @sourcery-ai resolve 来解决所有 Sourcery 评论。如果您已经处理了所有评论并且不想再看到它们,这会很有用。
  • 驳回所有 Sourcery 审查: 在拉取请求上评论 @sourcery-ai dismiss 来驳回所有现有的 Sourcery 审查。如果您想从头开始进行新的审查,这会特别有用——别忘了评论 @sourcery-ai review 来触发新的审查!

定制您的体验

访问您的 仪表盘 以:

  • 启用或禁用审查功能,例如 Sourcery 生成的拉取请求摘要、审阅者指南等。
  • 更改审查语言。
  • 添加、移除或编辑自定义审查说明。
  • 调整其他审查设置。

获取帮助

Original review guide in English

Reviewer's Guide

This PR introduces an application‐level LRU cache for currency, account, and transaction data, integrates cache update and expire calls into API endpoints for CRUD and balance operations, removes per‐executor caching, and adds a configurable pre‐build cache option with documentation and version bump.

Sequence diagram for API cache update and expire integration

sequenceDiagram
participant API as "API Endpoint"
participant CacheManager
participant Cache
participant DB as "Database"
API->>CacheManager: update_cache(category, data)
CacheManager->>Cache: update(data)
API->>DB: Perform DB operation
API->>CacheManager: expire_cache(category, data_id)
CacheManager->>Cache: delete(data_id)
Loading

Class diagram for new and updated cache classes

classDiagram
class CacheManager {
  +get_cache(category: CacheCategoryEnum, max_size: int = 1000) Cache[Any]
  +update_cache(category: CacheCategoryEnum, data: BaseData, max_size: int = 1000) Self
  +expire_cache(category: CacheCategoryEnum, data_id: str | None = None) Self
  +expire_all_cache() Self
}
class Cache {
  +max_size: int
  +update(data: BaseData) bool
  +get(data_id: str) BaseData | None
  +get_all() list[BaseData]
  +delete(data_id: str)
  +clear()
}
class CacheCategoryEnum {
  CURRENCY
  ACCOUNT
  TRANSACTION
}
class Config {
  +value_pre_build_cache: bool = True
}
CacheManager --> CacheCategoryEnum
CacheManager --> Cache
Cache --> BaseData
Cache --> UserAccountData
Cache --> CurrencyData
Cache --> TransactionData
Loading

Class diagram for AccountExecutor after cache refactor

classDiagram
class AccountExecutor {
  +currency_id: str
  +user_id: str
  +__call__(event: Event) Self
  +get_data(currency_id: str | None = None) UserAccountData
  +get_balance(currency_id: str | None = None) float
  +add_balance(amount: float, source: str = "_transfer", currency_id: str | None = None) Self
  +decrease_balance(amount: float, source: str = "_transfer", currency_id: str | None = None) Self
}
AccountExecutor --> UserAccountData
Loading

File-Level Changes

Change Details Files
Introduce LRU cache manager
  • Created Cache and CacheManager with LRU eviction and asyncio locks
  • Defined CacheCategoryEnum for currency, account, and transaction
  • Implemented update/get/delete/expire/clear operations
  • Provided thread-safe locking per data_id
  • Added comprehensive unit tests for cache behavior
nonebot_plugin_value/_cache.py
tests/value_tests/cache_test.py
Integrate cache operations into account and currency APIs
  • Inserted expire_cache calls around all data‐mutating functions
  • Added update_cache calls after data retrieval functions
  • Introduced no_cache_use and no_cache_refresh flags to control cache behavior
  • Adjusted function signatures and return types to reflect cache usage
nonebot_plugin_value/api/api_balance.py
nonebot_plugin_value/api/api_currency.py
Simplify AccountExecutor by removing internal caching
  • Removed data_map field and related logic
  • Delegated all data retrieval to get_or_create_account directly
nonebot_plugin_value/api/executor.py
Add configurable pre-build cache on startup
  • Created Config model with value_pre_build_cache flag
  • Pre-populate currency and account caches in plugin init when enabled
  • Updated README with new config and cache feature description
  • Bumped project version to 0.1.7
nonebot_plugin_value/config.py
nonebot_plugin_value/__init__.py
README.md
pyproject.toml
Minor cleanup and formatting fixes
  • Normalized whitespace and type annotations in balance_pyd
  • Removed extraneous blank lines in hooks_type and currency repository
  • Minor formatting adjustments in uuid_lib and VSCode settings
nonebot_plugin_value/pyd_models/balance_pyd.py
nonebot_plugin_value/hook/hooks_type.py
nonebot_plugin_value/repositories/currency.py
nonebot_plugin_value/uuid_lib.py
.vscode/settings.json

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

你好 - 我已经审查了你的更改 - 这里有一些反馈:

  • 各种缓存控制标志(no_cache_use, no_cache_refresh, no_cache_update)在不同函数中有所差异——请考虑统一它们的名称和行为以保持一致性和易用性。
  • 许多修改方法(add_balance, del_balance, transfer_funds等)现在返回 None 而不是更新后的数据对象,这可能会破坏现有消费者——请验证下游兼容性或重新考虑返回类型。
  • 确保所有写入路径都适当地调用 expire_cache(包括批量操作),并考虑添加并发测试以防止在高负载下提供陈旧的缓存条目。
给AI代理的提示
请处理此代码审查中的评论:

## 总体评论
- 各种缓存控制标志(no_cache_use, no_cache_refresh, no_cache_update)在不同函数中有所差异——请考虑统一它们的名称和行为以保持一致性和易用性。
- 许多修改方法(add_balance, del_balance, transfer_funds等)现在返回 None 而不是更新后的数据对象,这可能会破坏现有消费者——请验证下游兼容性或重新考虑返回类型。
- 确保所有写入路径都适当地调用 expire_cache(包括批量操作),并考虑添加并发测试以防止在高负载下提供陈旧的缓存条目。

## 单独评论

### 评论 1
<location> `nonebot_plugin_value/api/api_balance.py:25-31` </location>
<code_context>
         frozen (bool): 是否冻结
     """
     async with get_session() as session:
+        await CacheManager().expire_cache(
+            category=CacheCategoryEnum.ACCOUNT, data_id=account_id
+        )
         await _set_frozen_all(account_id, frozen, session)
</code_context>

<issue_to_address>
**suggestion:** 缓存过期使用 account_id,但其他地方使用 get_uni_id。

请使用 get_uni_id(account_id, currency_id) 进行缓存过期,以匹配其他地方使用的方法并防止缓存键不一致。

```suggestion
async def set_frozen_all(account_id: str, frozen: bool, currency_id: str = DEFAULT_CURRENCY_UUID) -> None:
    """
    frozen (bool): 是否冻结
    """
    async with get_session() as session:
        await CacheManager().expire_cache(
            category=CacheCategoryEnum.ACCOUNT, data_id=get_uni_id(account_id, currency_id)
        )
        await _set_frozen_all(account_id, frozen, session)
```
</issue_to_address>

### 评论 2
<location> `nonebot_plugin_value/api/api_balance.py:114-120` </location>
<code_context>
-        list[UserAccountData]: 用户账户数据列表
+        None: None
     """
-    data_list: list[UserAccountData] = []
     if currency_id is None:
         currency_id = DEFAULT_CURRENCY_UUID.hex
     await _batch_del(updates, currency_id, source, return_all_on_fail=True)
     for user_id, _ in updates:
-        data_list.append(await get_or_create_account(user_id, currency_id))
-    return data_list
+        await CacheManager().expire_cache(
+            category=CacheCategoryEnum.ACCOUNT, data_id=get_uni_id(user_id, currency_id)
</code_context>

<issue_to_address>
**suggestion:** 返回类型已更改为 None,但 docstring 仍然是 list[UserAccountData]。

请更新 batch_del_balance 和 batch_add_balance 的 docstring,以表明返回类型现在是 None。

建议的实现:

```python
) -> None:
    """批量减少账户余额

    Args:
        updates (list[tuple[str, float]]): 用户ID和减少金额的列表
        currency_id (str, optional): 币种ID,默认为默认币种
        source (str, optional): 源说明,默认为 "batch_update"

    Returns:
        None: 无返回值
    """

```

```python
async def batch_add_balance(
    updates: list[tuple[str, float]],
    currency_id: str | None = None,
    source: str = "batch_update",
) -> None:
    """批量添加账户余额

    Args:
        updates (list[tuple[str, float]]): 用户ID和添加金额的列表
        currency_id (str, optional): 币种ID,默认为默认币种
        source (str, optional): 源说明,默认为 "batch_update"

    Returns:
        None: 无返回值
    """

```
</issue_to_address>

### 评论 3
<location> `nonebot_plugin_value/api/api_balance.py:51` </location>
<code_context>
-async def list_accounts(currency_id: str | None = None) -> list[UserAccountData]:
-    """获取指定货币(或默认)的账户列表
+async def list_accounts(
+    currency_id: str | None = None, no_cache_refresh: bool = False
+) -> list[UserAccountData]:
+    """获取指定货币(或默认)的账户列表(无法命中缓存,但是可以更新缓存)
</code_context>

<issue_to_address>
**nitpick:** 参数 no_cache_refresh 已添加但未在文档中说明。

请更新 docstring 以描述 no_cache_refresh 参数并阐明其对缓存行为的影响。
</issue_to_address>

### 评论 4
<location> `nonebot_plugin_value/api/api_balance.py:102-103` </location>
<code_context>

 async def get_or_create_account(
-    user_id: str, currency_id: str | None = None
+    user_id: str,
+    currency_id: str | None = None,
+    no_cache_use: bool = False,
+    no_cache_refresh: bool = False,
</code_context>

<issue_to_address>
**nitpick:** 新参数 no_cache_use 和 no_cache_refresh 未在文档中说明。

请更新 docstring 以包含这些参数的描述。
</issue_to_address>

### 评论 5
<location> `nonebot_plugin_value/api/api_currency.py:48` </location>
<code_context>

-async def list_currencies() -> list[CurrencyData]:
-    """获取所有已存在货币
+async def list_currencies(no_cache_update: bool = False) -> list[CurrencyData]:
+    """获取所有已存在货币(无法命中缓存)

</code_context>

<issue_to_address>
**nitpick:** 参数 no_cache_update 已添加但未在文档中说明。

请更新 docstring 以描述 no_cache_update 参数及其对缓存行为的影响。
</issue_to_address>

### 评论 6
<location> `nonebot_plugin_value/api/api_currency.py:69` </location>
<code_context>

-async def get_currency(currency_id: str) -> CurrencyData | None:
+
+async def get_currency(currency_id: str, no_cache: bool = False) -> CurrencyData | None:
     """获取一个货币信息

</code_context>

<issue_to_address>
**nitpick:** 参数 no_cache 已添加但未在文档中说明。

请更新 docstring 以描述 no_cache 参数并阐明其如何影响缓存行为。
</issue_to_address>

### 评论 7
<location> `nonebot_plugin_value/_cache.py:42-45` </location>
<code_context>
+        if self.max_size <= 0:
+            self.max_size = 1000
+
+    async def update(self, *, data: BaseData) -> bool:
+        data_id = data.uni_id if isinstance(data, UserAccountData) else data.id
+        async with self._lock(data_id):
+            existing = self._cache.get(data_id)
</code_context>

<issue_to_address>
**suggestion:** 不同数据类型的缓存键逻辑可能不一致。

请标准化缓存键选择或记录其基本原理,以避免如果其他模型引入 uni_id 时造成混淆。

```suggestion
    def _get_cache_key(self, data: BaseData) -> str:
        """
        获取用于缓存的唯一键。默认使用 data.id。
        若为 UserAccountData,则使用 data.uni_id。
        如需支持更多类型,请在此方法中扩展。
        """
        if isinstance(data, UserAccountData):
            return data.uni_id
        return data.id

    async def update(self, *, data: BaseData) -> bool:
        data_id = self._get_cache_key(data)
        async with self._lock(data_id):
            existing = self._cache.get(data_id)
```
</issue_to_address>

### 评论 8
<location> `nonebot_plugin_value/_cache.py:83-85` </location>
<code_context>
+        async with self._lock(0):
+            self._cache.clear()
+
+    @staticmethod
+    @lru_cache(1024)
+    def _lock(*args: Hashable) -> Lock:
+        return Lock()
+
</code_context>

<issue_to_address>
**issue (bug_risk):** lru_cache 用于锁,但可能对 asyncio 不安全。

通过 lru_cache 共享锁可能会导致 asyncio 中的并发问题。请改用专门的锁管理方法。
</issue_to_address>

### 评论 9
<location> `tests/value_tests/cache_test.py:5-14` </location>
<code_context>
[email protected]
</code_context>

<issue_to_address>
**suggestion (testing):** 缓存测试涵盖了基本的 CRUD 操作,但缺少边缘情况场景。

请添加针对重复更新、max_size 溢出触发驱逐以及并发访问等场景的测试,以验证线程安全性。
</issue_to_address>

### 评论 10
<location> `tests/value_tests/cache_test.py:29-45` </location>
<code_context>
+        category=CacheCategoryEnum.TRANSACTION,
+        data=TEST_TRANSACTION,
+    )
+    assert (
+        await (await cache_manager.get_cache(CacheCategoryEnum.CURRENCY)).get(
+            data_id="1"
+        )
+        == TEST_CURRENCY
+    )
+    assert (
+        await (await cache_manager.get_cache(CacheCategoryEnum.ACCOUNT)).get(
+            data_id="1"
+        )
+    ) == TEST_USER
+    assert (
+        await (await cache_manager.get_cache(CacheCategoryEnum.TRANSACTION)).get(
</code_context>

<issue_to_address>
**suggestion (testing):** 测试直接使用模型实例的相等性,这可能不可靠。

请使用比较模型数据的断言,例如 `assert model.dict() == expected.dict()`,以确保准确性,无论对象标识或相等性实现如何。

```suggestion
    currency = await (await cache_manager.get_cache(CacheCategoryEnum.CURRENCY)).get(
        data_id="1"
    )
    assert currency.dict() == TEST_CURRENCY.dict()

    account = await (await cache_manager.get_cache(CacheCategoryEnum.ACCOUNT)).get(
        data_id="1"
    )
    assert account.dict() == TEST_USER.dict()

    transaction = await (await cache_manager.get_cache(CacheCategoryEnum.TRANSACTION)).get(
        data_id="1"
    )
    assert transaction.dict() == TEST_TRANSACTION.dict()
```
</issue_to_address>

### 评论 11
<location> `nonebot_plugin_value/_cache.py:45-46` </location>
<code_context>
    async def update(self, *, data: BaseData) -> bool:
        data_id = data.uni_id if isinstance(data, UserAccountData) else data.id
        async with self._lock(data_id):
            existing = self._cache.get(data_id)
            if existing:
                existing.model_validate(data, from_attributes=True)
                self._cache.move_to_end(data_id)
                return True

            # 添加新数据
            self._cache[data_id] = data
            self._cache.move_to_end(data_id)

            # 如果超出最大大小,删除最久未使用的项(第一个)
            if len(self._cache) > self.max_size:
                self._cache.popitem(last=False)

            return False

</code_context>

<issue_to_address>
**suggestion (code-quality):** 使用命名表达式简化赋值和条件 ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))

```suggestion
            if existing := self._cache.get(data_id):
```
</issue_to_address>

### 评论 12
<location> `nonebot_plugin_value/_cache.py:71-73` </location>
<code_context>
    async def get_all(self) -> list[BaseData]:
        async with self._lock():
            # 返回所有缓存项的副本
            items = list(self._cache.values())
            return items

</code_context>

<issue_to_address>
**suggestion (code-quality): 后者是 inline-immediately-returned-variable 的建议**内联立即返回的变量 ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/inline-immediately-returned-variable/))

```suggestion
            return list(self._cache.values())
```
</issue_to_address>

Sourcery 对开源免费 - 如果您喜欢我们的评论,请考虑分享它们 ✨
帮助我更有用!请点击每个评论上的 👍 或 👎,我将使用反馈来改进您的评论。
Original comment in English

Hey there - I've reviewed your changes - here's some feedback:

  • The various cache control flags (no_cache_use, no_cache_refresh, no_cache_update) differ across functions—consider unifying their names and behaviors for consistency and ease of use.
  • Many mutating methods (add_balance, del_balance, transfer_funds, etc.) now return None instead of the updated data object, which might break existing consumers—please verify downstream compatibility or revisit the return types.
  • Ensure all write paths invoke expire_cache appropriately (including batch operations) and consider adding concurrency tests to guard against serving stale cache entries under load.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- The various cache control flags (no_cache_use, no_cache_refresh, no_cache_update) differ across functions—consider unifying their names and behaviors for consistency and ease of use.
- Many mutating methods (add_balance, del_balance, transfer_funds, etc.) now return None instead of the updated data object, which might break existing consumers—please verify downstream compatibility or revisit the return types.
- Ensure all write paths invoke expire_cache appropriately (including batch operations) and consider adding concurrency tests to guard against serving stale cache entries under load.

## Individual Comments

### Comment 1
<location> `nonebot_plugin_value/api/api_balance.py:25-31` </location>
<code_context>
         frozen (bool): 是否冻结
     """
     async with get_session() as session:
+        await CacheManager().expire_cache(
+            category=CacheCategoryEnum.ACCOUNT, data_id=account_id
+        )
         await _set_frozen_all(account_id, frozen, session)
</code_context>

<issue_to_address>
**suggestion:** Cache expiration uses account_id, but other places use get_uni_id.

Use get_uni_id(account_id, currency_id) for cache expiration to match the approach used elsewhere and prevent cache key inconsistencies.

```suggestion
async def set_frozen_all(account_id: str, frozen: bool, currency_id: str = DEFAULT_CURRENCY_UUID) -> None:
    """
    frozen (bool): 是否冻结
    """
    async with get_session() as session:
        await CacheManager().expire_cache(
            category=CacheCategoryEnum.ACCOUNT, data_id=get_uni_id(account_id, currency_id)
        )
        await _set_frozen_all(account_id, frozen, session)
```
</issue_to_address>

### Comment 2
<location> `nonebot_plugin_value/api/api_balance.py:114-120` </location>
<code_context>
-        list[UserAccountData]: 用户账户数据列表
+        None: None
     """
-    data_list: list[UserAccountData] = []
     if currency_id is None:
         currency_id = DEFAULT_CURRENCY_UUID.hex
     await _batch_del(updates, currency_id, source, return_all_on_fail=True)
     for user_id, _ in updates:
-        data_list.append(await get_or_create_account(user_id, currency_id))
-    return data_list
+        await CacheManager().expire_cache(
+            category=CacheCategoryEnum.ACCOUNT, data_id=get_uni_id(user_id, currency_id)
</code_context>

<issue_to_address>
**suggestion:** Return type changed to None, but docstring still says list[UserAccountData].

Please update the docstrings for batch_del_balance and batch_add_balance to indicate the return type is now None.

Suggested implementation:

```python
) -> None:
    """批量减少账户余额

    Args:
        updates (list[tuple[str, float]]): 用户ID和减少金额的列表
        currency_id (str, optional): 币种ID,默认为默认币种
        source (str, optional): 源说明,默认为 "batch_update"

    Returns:
        None: 无返回值
    """

```

```python
async def batch_add_balance(
    updates: list[tuple[str, float]],
    currency_id: str | None = None,
    source: str = "batch_update",
) -> None:
    """批量添加账户余额

    Args:
        updates (list[tuple[str, float]]): 用户ID和添加金额的列表
        currency_id (str, optional): 币种ID,默认为默认币种
        source (str, optional): 源说明,默认为 "batch_update"

    Returns:
        None: 无返回值
    """

```
</issue_to_address>

### Comment 3
<location> `nonebot_plugin_value/api/api_balance.py:51` </location>
<code_context>
-async def list_accounts(currency_id: str | None = None) -> list[UserAccountData]:
-    """获取指定货币(或默认)的账户列表
+async def list_accounts(
+    currency_id: str | None = None, no_cache_refresh: bool = False
+) -> list[UserAccountData]:
+    """获取指定货币(或默认)的账户列表(无法命中缓存,但是可以更新缓存)
</code_context>

<issue_to_address>
**nitpick:** Parameter no_cache_refresh is added but not documented.

Please update the docstring to describe the no_cache_refresh parameter and clarify its impact on cache behavior.
</issue_to_address>

### Comment 4
<location> `nonebot_plugin_value/api/api_balance.py:102-103` </location>
<code_context>

 async def get_or_create_account(
-    user_id: str, currency_id: str | None = None
+    user_id: str,
+    currency_id: str | None = None,
+    no_cache_use: bool = False,
+    no_cache_refresh: bool = False,
</code_context>

<issue_to_address>
**nitpick:** New parameters no_cache_use and no_cache_refresh are undocumented.

Please update the docstring to include descriptions for these parameters.
</issue_to_address>

### Comment 5
<location> `nonebot_plugin_value/api/api_currency.py:48` </location>
<code_context>

-async def list_currencies() -> list[CurrencyData]:
-    """获取所有已存在货币
+async def list_currencies(no_cache_update: bool = False) -> list[CurrencyData]:
+    """获取所有已存在货币(无法命中缓存)

</code_context>

<issue_to_address>
**nitpick:** Parameter no_cache_update is added but not documented.

Please update the docstring to describe the no_cache_update parameter and its impact on cache behavior.
</issue_to_address>

### Comment 6
<location> `nonebot_plugin_value/api/api_currency.py:69` </location>
<code_context>

-async def get_currency(currency_id: str) -> CurrencyData | None:
+
+async def get_currency(currency_id: str, no_cache: bool = False) -> CurrencyData | None:
     """获取一个货币信息

</code_context>

<issue_to_address>
**nitpick:** Parameter no_cache is added but not documented.

Please update the docstring to describe the no_cache parameter and clarify how it affects cache behavior.
</issue_to_address>

### Comment 7
<location> `nonebot_plugin_value/_cache.py:42-45` </location>
<code_context>
+        if self.max_size <= 0:
+            self.max_size = 1000
+
+    async def update(self, *, data: BaseData) -> bool:
+        data_id = data.uni_id if isinstance(data, UserAccountData) else data.id
+        async with self._lock(data_id):
+            existing = self._cache.get(data_id)
</code_context>

<issue_to_address>
**suggestion:** Cache key logic may be inconsistent for different data types.

Standardize cache key selection or document the rationale to avoid confusion if other models introduce uni_id.

```suggestion
    def _get_cache_key(self, data: BaseData) -> str:
        """
        获取用于缓存的唯一键。默认使用 data.id。
        若为 UserAccountData,则使用 data.uni_id。
        如需支持更多类型,请在此方法中扩展。
        """
        if isinstance(data, UserAccountData):
            return data.uni_id
        return data.id

    async def update(self, *, data: BaseData) -> bool:
        data_id = self._get_cache_key(data)
        async with self._lock(data_id):
            existing = self._cache.get(data_id)
```
</issue_to_address>

### Comment 8
<location> `nonebot_plugin_value/_cache.py:83-85` </location>
<code_context>
+        async with self._lock(0):
+            self._cache.clear()
+
+    @staticmethod
+    @lru_cache(1024)
+    def _lock(*args: Hashable) -> Lock:
+        return Lock()
+
</code_context>

<issue_to_address>
**issue (bug_risk):** lru_cache used for locks, but may not be safe for asyncio.

Sharing locks via lru_cache may cause concurrency issues in asyncio. Use a dedicated lock management approach instead.
</issue_to_address>

### Comment 9
<location> `tests/value_tests/cache_test.py:5-14` </location>
<code_context>
[email protected]
</code_context>

<issue_to_address>
**suggestion (testing):** Cache tests cover basic CRUD operations but lack edge case scenarios.

Please add tests for scenarios like repeated updates, max_size overflow triggering eviction, and concurrent access to verify thread safety.
</issue_to_address>

### Comment 10
<location> `tests/value_tests/cache_test.py:29-45` </location>
<code_context>
+        category=CacheCategoryEnum.TRANSACTION,
+        data=TEST_TRANSACTION,
+    )
+    assert (
+        await (await cache_manager.get_cache(CacheCategoryEnum.CURRENCY)).get(
+            data_id="1"
+        )
+        == TEST_CURRENCY
+    )
+    assert (
+        await (await cache_manager.get_cache(CacheCategoryEnum.ACCOUNT)).get(
+            data_id="1"
+        )
+    ) == TEST_USER
+    assert (
+        await (await cache_manager.get_cache(CacheCategoryEnum.TRANSACTION)).get(
</code_context>

<issue_to_address>
**suggestion (testing):** Tests use direct equality for model instances, which may not be reliable.

Use assertions that compare model data, such as `assert model.dict() == expected.dict()`, to ensure accuracy regardless of object identity or equality implementation.

```suggestion
    currency = await (await cache_manager.get_cache(CacheCategoryEnum.CURRENCY)).get(
        data_id="1"
    )
    assert currency.dict() == TEST_CURRENCY.dict()

    account = await (await cache_manager.get_cache(CacheCategoryEnum.ACCOUNT)).get(
        data_id="1"
    )
    assert account.dict() == TEST_USER.dict()

    transaction = await (await cache_manager.get_cache(CacheCategoryEnum.TRANSACTION)).get(
        data_id="1"
    )
    assert transaction.dict() == TEST_TRANSACTION.dict()
```
</issue_to_address>

### Comment 11
<location> `nonebot_plugin_value/_cache.py:45-46` </location>
<code_context>
    async def update(self, *, data: BaseData) -> bool:
        data_id = data.uni_id if isinstance(data, UserAccountData) else data.id
        async with self._lock(data_id):
            existing = self._cache.get(data_id)
            if existing:
                existing.model_validate(data, from_attributes=True)
                self._cache.move_to_end(data_id)
                return True

            # 添加新数据
            self._cache[data_id] = data
            self._cache.move_to_end(data_id)

            # 如果超出最大大小,删除最久未使用的项(第一个)
            if len(self._cache) > self.max_size:
                self._cache.popitem(last=False)

            return False

</code_context>

<issue_to_address>
**suggestion (code-quality):** Use named expression to simplify assignment and conditional ([`use-named-expression`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/use-named-expression/))

```suggestion
            if existing := self._cache.get(data_id):
```
</issue_to_address>

### Comment 12
<location> `nonebot_plugin_value/_cache.py:71-73` </location>
<code_context>
    async def get_all(self) -> list[BaseData]:
        async with self._lock():
            # 返回所有缓存项的副本
            items = list(self._cache.values())
            return items

</code_context>

<issue_to_address>
**suggestion (code-quality):** Inline variable that is immediately returned ([`inline-immediately-returned-variable`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/inline-immediately-returned-variable/))

```suggestion
            return list(self._cache.values())
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@JohnRichard4096 JohnRichard4096 merged commit d67519e into master Oct 24, 2025
2 checks passed
@JohnRichard4096 JohnRichard4096 deleted the feat-cache branch October 24, 2025 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants