Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 我发现了 1 个问题，并留下了一些高层次的反馈：
- 新的
Data抽象用自定义的_db连接替换了TShock.DB，但从未显式打开或释放连接，并在所有调用之间共享单个IDbConnection实例，这可能带来连接生命周期和并发方面的问题（尤其是对 SQLite）；建议直接使用TShock.DB，或者为每次操作创建/释放连接，而不是使用长生命周期的静态连接。 - 在
CGive.Execute中，你先调用Save()，然后通过匹配executer/cmd/who查询最后插入的id，但没有使用事务或任何唯一约束，如果同时有多个相似的命令被并发添加，就会产生竞争条件；使用数据库自增主键获取机制（例如LAST_INSERT_ID()/last_insert_rowid()）或事务会让 ID 分配更可靠。 OnGreetPlayer现在通过CGive.GetCGive()加载所有记录，然后在内存中按玩家过滤，而之前的 API 是按who查询；对于大表来说，每次登录都这样会比较昂贵，所以可能值得恢复按条件过滤的查询，或者添加一个只返回相关命令的GetCGiveForPlayer(string who)。
面向 AI 代理的提示
Please address the comments from this code review:
## Overall Comments
- The new `Data` abstraction replaces `TShock.DB` with a custom `_db` connection but never explicitly opens or disposes it and shares a single `IDbConnection` instance across all calls, which can introduce connection‑lifecycle and concurrency issues (especially for SQLite); consider using `TShock.DB` directly or creating/disposing a per‑operation connection instead of a long‑lived static one.
- In `CGive.Execute` you call `Save()` and then query the last inserted `id` by matching `executer/cmd/who` without any transaction or uniqueness, which can race if multiple similar commands are added concurrently; using database identity retrieval (e.g., `LAST_INSERT_ID()`/`last_insert_rowid()`) or a transaction would make the ID assignment more reliable.
- `OnGreetPlayer` now uses `CGive.GetCGive()` to load all records and then filters per player in memory, whereas the previous API queried by `who`; for large tables this can become expensive on each login, so it may be worth restoring a filtered query or adding a `GetCGiveForPlayer(string who)` that only returns relevant commands.
## Individual Comments
### Comment 1
<location path="src/CGive/CGive.cs" line_range="51" />
<code_context>
- var list2 = TSPlayer.FindByNameOrID(this.who);
- if (list2.Count > 0)
+
+ // personal ģʽ����ȷƥ���������
+ var target = TShock.Players.FirstOrDefault(p =>
+ p != null && p.Active && p.Name == this.who);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Personal execution uses a case-sensitive name comparison, inconsistent with other case-insensitive comparisons.
Here `target` is resolved with `p.Name == this.who`, which is case-sensitive, while other checks (e.g. `Executer.Equals("server", StringComparison.OrdinalIgnoreCase)` and the name comparison in `ExecuteForPlayer`) are case-insensitive. This inconsistency can cause commands for `Foo` to fail for `foo`. Please switch to a case-insensitive comparison here (e.g. `StringComparison.OrdinalIgnoreCase`) for consistency and correctness.
</issue_to_address>帮我变得更有用！请对每条评论点 👍 或 👎，我会根据你的反馈改进后续评审。
Original comment in English
Hey - I've found 1 issue, and left some high level feedback:
- The new
Dataabstraction replacesTShock.DBwith a custom_dbconnection but never explicitly opens or disposes it and shares a singleIDbConnectioninstance across all calls, which can introduce connection‑lifecycle and concurrency issues (especially for SQLite); consider usingTShock.DBdirectly or creating/disposing a per‑operation connection instead of a long‑lived static one. - In
CGive.Executeyou callSave()and then query the last insertedidby matchingexecuter/cmd/whowithout any transaction or uniqueness, which can race if multiple similar commands are added concurrently; using database identity retrieval (e.g.,LAST_INSERT_ID()/last_insert_rowid()) or a transaction would make the ID assignment more reliable. OnGreetPlayernow usesCGive.GetCGive()to load all records and then filters per player in memory, whereas the previous API queried bywho; for large tables this can become expensive on each login, so it may be worth restoring a filtered query or adding aGetCGiveForPlayer(string who)that only returns relevant commands.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `Data` abstraction replaces `TShock.DB` with a custom `_db` connection but never explicitly opens or disposes it and shares a single `IDbConnection` instance across all calls, which can introduce connection‑lifecycle and concurrency issues (especially for SQLite); consider using `TShock.DB` directly or creating/disposing a per‑operation connection instead of a long‑lived static one.
- In `CGive.Execute` you call `Save()` and then query the last inserted `id` by matching `executer/cmd/who` without any transaction or uniqueness, which can race if multiple similar commands are added concurrently; using database identity retrieval (e.g., `LAST_INSERT_ID()`/`last_insert_rowid()`) or a transaction would make the ID assignment more reliable.
- `OnGreetPlayer` now uses `CGive.GetCGive()` to load all records and then filters per player in memory, whereas the previous API queried by `who`; for large tables this can become expensive on each login, so it may be worth restoring a filtered query or adding a `GetCGiveForPlayer(string who)` that only returns relevant commands.
## Individual Comments
### Comment 1
<location path="src/CGive/CGive.cs" line_range="51" />
<code_context>
- var list2 = TSPlayer.FindByNameOrID(this.who);
- if (list2.Count > 0)
+
+ // personal ģʽ����ȷƥ���������
+ var target = TShock.Players.FirstOrDefault(p =>
+ p != null && p.Active && p.Name == this.who);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Personal execution uses a case-sensitive name comparison, inconsistent with other case-insensitive comparisons.
Here `target` is resolved with `p.Name == this.who`, which is case-sensitive, while other checks (e.g. `Executer.Equals("server", StringComparison.OrdinalIgnoreCase)` and the name comparison in `ExecuteForPlayer`) are case-insensitive. This inconsistency can cause commands for `Foo` to fail for `foo`. Please switch to a case-insensitive comparison here (e.g. `StringComparison.OrdinalIgnoreCase`) for consistency and correctness.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
该 PR 主要围绕 CGive 插件的离线命令工作流做整理与可靠性增强：将原 personal/all 子命令合并为统一的 /cgive add，并尝试通过独立数据连接与辅助方法缓解 SQLite 锁库问题，同时调整离线命令在玩家登录时的执行路径与占位符格式（{name}）。
Changes:
- 将
/cgive personal与/cgive all合并为/cgive add，并更新帮助信息与 README 示例/更新日志 - 调整离线命令登录触发执行逻辑，引入按玩家查询的
GetCGiveForPlayer - 引入
Data辅助类封装查询、并调整 Given 表写入逻辑以记录 (name, id)
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/CGive/README.md | 更新命令文档为 /cgive add，补充参数说明与更新日志 |
| src/CGive/Main.cs | 登录触发执行路径重构；指令入口更新为 add/del/list/reset，并补充未知子命令提示 |
| src/CGive/CGive.cs | 执行逻辑改为 {name} 占位符；新增按玩家查询接口；GetCGive 接口调整 |
| src/CGive/Data.cs | 新增独立 DB 连接与 Query/QueryReader/LastInsertId 辅助方法；Given 表结构调整 |
| src/CGive/Given.cs | Given 写入/查询改为走 Data，并改为插入 (name, id) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/CGive/CGive.cs
Outdated
Comment on lines
79
to
82
| /// <summary> | ||
| /// ֻ��ѯ��ָ���������صļ�¼��personal ģʽ���� all ģʽ��¼������ȫ��ɨ�� | ||
| /// </summary> | ||
| public static IEnumerable<CGive> GetCGiveForPlayer(string playerName) |
There was a problem hiding this comment.
XML <summary> 内容同样是乱码，生成文档或在 IDE 中悬浮提示时不可读。建议改为可读文本，并统一该文件的编码/字体以避免再次出现乱码。
src/CGive/Main.cs
Outdated
Comment on lines
129
to
136
| var temp = new CGive | ||
| { | ||
| Executer = "Server", | ||
| who = player.Name, | ||
| cmd = item.cmd, | ||
| id = item.id | ||
| }; | ||
| if (temp.Execute()) |
There was a problem hiding this comment.
item.who == "-1" 的离线指令在玩家登录时被包装成 temp 执行，但这里把 Executer 硬编码为 Server，会忽略数据库里原本保存的执行者身份，并可能导致指令以更高权限执行（与 /cgive add [执行者] ... 的语义不一致）。建议改为沿用 item.Executer（或明确限制 all(-1) 只能用 Server 并在入库/帮助里校验与说明）。
src/CGive/Data.cs
Outdated
Comment on lines
11
to
52
| @@ -16,17 +24,31 @@ public static void Init() | |||
| ); | |||
|
|
|||
| var givenTable = new SqlTable("Given", | |||
| new SqlColumn("id", MySqlDbType.Int32) { Primary = true, AutoIncrement = true }, | |||
| new SqlColumn("name", MySqlDbType.Text)); | |||
|
|
|||
| new SqlColumn("name", MySqlDbType.Text), | |||
| new SqlColumn("id", MySqlDbType.Int32) | |||
| ); | |||
|
|
|||
| SqlTableCreator creator = new(TShock.DB, TShock.DB.GetSqlQueryBuilder()); | |||
| var creator = new SqlTableCreator(_db, _db.GetSqlQueryBuilder()); | |||
| creator.EnsureTableStructure(cgiveTable); | |||
| creator.EnsureTableStructure(givenTable); | |||
| } | |||
|
|
|||
| public static void Command(string cmd, params object[] args) | |||
| { | |||
| TShock.DB.Query(cmd, args); | |||
| _db!.Query(cmd, args); | |||
| } | |||
|
|
|||
| public static QueryResult QueryReader(string query, params object[] args) | |||
| { | |||
| return _db!.QueryReader(query, args); | |||
| } | |||
|
|
|||
| public static int GetLastInsertId() | |||
| { | |||
| var sql = TShock.Config.Settings.StorageType.Equals("mysql", StringComparison.OrdinalIgnoreCase) | |||
| ? "SELECT LAST_INSERT_ID()" | |||
| : "SELECT last_insert_rowid()"; | |||
| using var re = _db!.QueryReader(sql); | |||
| return re.Read() ? re.Reader.GetInt32(0) : 0; | |||
There was a problem hiding this comment.
Data 使用单例 _db 连接并在 Command/QueryReader/GetLastInsertId 中直接复用；在 REST 请求、玩家登录事件、命令处理等可能并发的场景下，这会导致连接的并发访问问题（多数 IDbConnection/Provider 都不是线程安全的），同时也会让 GetLastInsertId() 读到其他并发插入的结果。建议为所有 DB 调用加全局锁/队列串行化，或改为“每次操作创建新连接(依赖连接池)”并把 last insert id 获取与插入放在同一连接/事务范围内。
Comment on lines
26
to
+29
| var givenTable = new SqlTable("Given", | ||
| new SqlColumn("id", MySqlDbType.Int32) { Primary = true, AutoIncrement = true }, | ||
| new SqlColumn("name", MySqlDbType.Text)); | ||
|
|
||
| new SqlColumn("name", MySqlDbType.Text), | ||
| new SqlColumn("id", MySqlDbType.Int32) | ||
| ); |
There was a problem hiding this comment.
Given 表结构从“自增主键 id + name”改成了“name + id(外键)”且不再有主键/唯一约束。对于已经存在旧表结构的数据，EnsureTableStructure 通常不会重命名/迁移冲突列名，这可能导致运行时插入失败或与旧 id 值冲突；同时缺少 (name,id) 唯一约束会让去重完全依赖应用层。建议：保留旧自增主键（例如改列名为 cgive_id），并为 (name, cgive_id) 加唯一索引/主键；如需变更结构，加入显式迁移逻辑。
Comment on lines
13
to
+18
| public static void Init() | ||
| { | ||
| _db = TShock.Config.Settings.StorageType.Equals("mysql", StringComparison.OrdinalIgnoreCase) | ||
| ? new MySqlConnection(TShock.Config.Settings.MySqlConnectionString) | ||
| : new SqliteConnection("Data Source=" + Path.Combine(TShock.SavePath, "CGive.sqlite")); | ||
|
|
There was a problem hiding this comment.
这里创建了新的 _db 连接，但当前类没有任何关闭/释放逻辑；如果插件支持热重载/卸载或服务器长时间运行，这可能导致连接泄漏或 SQLite 文件句柄长期占用。建议增加 Data.Dispose/Close 并在插件 Dispose 时调用（同时避免在 Init 多次调用时重复创建连接）。
src/CGive/CGive.cs
Outdated
Comment on lines
44
to
46
| // personal ģʽ����Сд�����о�ȷƥ�� | ||
| var target = TShock.Players.FirstOrDefault(p => | ||
| p != null && p.Active && p.Name.Equals(this.who, StringComparison.OrdinalIgnoreCase)); |
There was a problem hiding this comment.
新增的注释/文档字符串出现乱码（例如 personal ģʽ...、XML summary），这会降低可维护性且影响后续阅读/生成文档。建议将该注释改为正常的 UTF-8 文本（中文或英文均可），并确认源码文件编码为 UTF-8。
| # 玩家leader上线就kill她一次（leader 离线时保存，上线自动执行） | ||
| /cgive add server leader /kill {name} | ||
|
|
||
| # 任意玩家上线就kill他一次（包括未来登录的玩家，每人只一次） |
There was a problem hiding this comment.
README 示例注释写的是“上线就kill他一次”，但实际示例命令是 /give {name} 4956；这会误导使用者。建议统一示例说明与命令内容（要么改注释，要么把命令改成 /kill {name}）。
Suggested change
| # 任意玩家上线就kill他一次（包括未来登录的玩家，每人只一次） | |
| # 任意玩家上线就送他一次物品 4956（包括未来登录的玩家，每人只一次） |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
ACaiCat
requested changes
Mar 1, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
更新插件/修复BUG
其他
Summary by Sourcery
将 CGive 子命令统一为单一的基于 add 的工作流，并提升在不同存储后端下离线指令执行的可靠性。
新功能：
/cgive add子命令，以更清晰的参数语义同时覆盖个人和全体玩家的离线指令。错误修复：
{name}格式。增强改进：
/cgive的指令帮助信息、用法校验以及未知子命令的反馈。Original summary in English
Summary by Sourcery
Unify CGive subcommands into a single add-based workflow and improve offline command execution reliability across storage backends.
New Features:
Bug Fixes:
Enhancements: