fix(orm): upsert concurrency-safe to avoid UNIQUE constraint on GroupSetting.group_id #189
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a concurrency issue in the AsyncORM.insert_or_update method to prevent SQLAlchemy IntegrityError on UNIQUE constraint violations for GroupSetting.group_id during concurrent operations.
- Replaces the existence-check-then-insert pattern with a more robust update-first-then-insert approach
- Adds proper exception handling for IntegrityError to handle race conditions
- Imports IntegrityError from sqlalchemy.exc to support the new error handling
Reviewed Changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| core/orm/init.py | Updated insert_or_update method to be concurrency-safe and added IntegrityError import |
| CLAUDE.md | Added new documentation file providing project overview and development guidance |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
core/orm/__init__.py
Outdated
| stmt = stmt.execution_options(synchronize_session="fetch") | ||
| result = await self.execute(stmt) | ||
| try: | ||
| if result is not None and getattr(result, "rowcount", 0) > 0: |
There was a problem hiding this comment.
The check result is not None is redundant. SQLAlchemy's execute() method will always return a result object for UPDATE statements, never None. Consider simplifying to if getattr(result, 'rowcount', 0) > 0:.
| if result is not None and getattr(result, "rowcount", 0) > 0: | |
| if getattr(result, "rowcount", 0) > 0: |
There was a problem hiding this comment.
✅ 已修复: 感谢您的建议！您说得对，SQLAlchemy 的 execute() 方法总是返回结果对象，不会返回 None。我已经移除了冗余的 result is not None 检查，简化为：
if getattr(result, "rowcount", 0) > 0:这样代码更简洁且逻辑更清晰。
core/orm/__init__.py
Outdated
| except IntegrityError: | ||
| # 竞争条件：在我们尝试 INSERT 前，其他协程/进程已插入相同唯一键 | ||
| # 回退到 UPDATE，确保数据按预期更新 | ||
| stmt2 = update(table).where(*condition).values(**data) | ||
| stmt2 = stmt2.execution_options(synchronize_session="fetch") | ||
| return await self.execute(stmt2) |
There was a problem hiding this comment.
The UPDATE statement construction is duplicated from lines 219-221. Consider extracting this into a helper function or variable to avoid code duplication and improve maintainability.
There was a problem hiding this comment.
✅ 已修复: 非常好的建议！代码重复确实会影响可维护性。我已经提取了一个辅助方法 _build_update_stmt() 来构造 UPDATE 语句，消除了代码重复：
def _build_update_stmt(self, table, data, condition):
"""构造 UPDATE 语句的辅助方法"""
stmt = update(table).where(*condition).values(**data)
stmt = stmt.execution_options(synchronize_session="fetch")
return stmt现在两个 UPDATE 操作都使用这个辅助方法，代码更加简洁且易于维护。
- Use UPDATE-first strategy; if no rows affected, try INSERT - Catch IntegrityError to handle UNIQUE conflicts and retry UPDATE - Import IntegrityError from sqlalchemy.exc This fixes UNIQUE constraint failures on GroupSetting.group_id when multiple callers race to initialize group settings (e.g., AccountController.init_group).
g1331
force-pushed
the
fix/orm-insert-or-update-constraint
branch
from
6214ca4 to
7d29cca
Compare
- Remove redundant `result is not None` check in insert_or_update - Extract _build_update_stmt helper method to eliminate code duplication - Improve code maintainability and readability Addresses review comments from Copilot in PR #189.
📝 Code Review 反馈处理完成我已经处理了所有的 code review 评论，并在新的提交中实现了改进： ✅ 已修复的问题1. 移除冗余的
|
🎉 Code Review 处理完成 - 所有检查通过我已经完成了所有 code review 评论的处理，并且所有 CI 检查都已成功通过！ ✅ 已处理的 Review Comments
📊 CI 检查状态 - 全部通过 ✅
🔧 具体修复内容1. 移除冗余检查 ✅- if result is not None and getattr(result, "rowcount", 0) > 0:
+ if getattr(result, "rowcount", 0) > 0:原因: SQLAlchemy 的 2. 提取辅助方法 ✅def _build_update_stmt(self, table, data, condition):
"""构造 UPDATE 语句的辅助方法"""
stmt = update(table).where(*condition).values(**data)
stmt = stmt.execution_options(synchronize_session="fetch")
return stmt效果: 消除了 UPDATE 语句构造的代码重复，提高可维护性 📝 提交历史
🚀 准备就绪✅ 所有 review comments 已处理 💯 该 PR 已经准备好进行最终审查和合并！ 感谢所有参与 code review 的同事，您们的建议使代码质量得到了显著提升！🙏 |
1 similar comment
🎉 Code Review 处理完成 - 所有检查通过我已经完成了所有 code review 评论的处理，并且所有 CI 检查都已成功通过！ ✅ 已处理的 Review Comments
📊 CI 检查状态 - 全部通过 ✅
🔧 具体修复内容1. 移除冗余检查 ✅- if result is not None and getattr(result, "rowcount", 0) > 0:
+ if getattr(result, "rowcount", 0) > 0:原因: SQLAlchemy 的 2. 提取辅助方法 ✅def _build_update_stmt(self, table, data, condition):
"""构造 UPDATE 语句的辅助方法"""
stmt = update(table).where(*condition).values(**data)
stmt = stmt.execution_options(synchronize_session="fetch")
return stmt效果: 消除了 UPDATE 语句构造的代码重复，提高可维护性 📝 提交历史
🚀 准备就绪✅ 所有 review comments 已处理 💯 该 PR 已经准备好进行最终审查和合并！ 感谢所有参与 code review 的同事，您们的建议使代码质量得到了显著提升！🙏 |
🎉 Code Review 处理完成 - 所有检查通过我已经完成了所有 code review 评论的处理，并且所有 CI 检查都已成功通过！ ✅ 已处理的 Review Comments
📊 CI 检查状态 - 全部通过 ✅
🔧 具体修复内容1. 移除冗余检查 ✅- if result is not None and getattr(result, "rowcount", 0) > 0:
+ if getattr(result, "rowcount", 0) > 0:原因: SQLAlchemy 的 2. 提取辅助方法 ✅def _build_update_stmt(self, table, data, condition):
"""构造 UPDATE 语句的辅助方法"""
stmt = update(table).where(*condition).values(**data)
stmt = stmt.execution_options(synchronize_session="fetch")
return stmt效果: 消除了 UPDATE 语句构造的代码重复，提高可维护性 3. Copilot PR 概述 ✅
📝 提交历史
🚀 准备就绪✅ 所有 review comments 已处理 💯 该 PR 已经准备好进行最终审查和合并！ 感谢所有参与 code review 的同事和工具，您们的建议使代码质量得到了显著提升！🙏 |
|
@claude check this |
|
Claude finished @g1331's task —— View job PR Review Complete ✅I've thoroughly reviewed this PR and verified all the changes. Here's my assessment: Tasks
SummaryThis PR successfully fixes the SQLAlchemy UNIQUE constraint error in the ORM layer with a well-implemented concurrency-safe solution. All code review feedback has been properly addressed, and the implementation follows best practices. Key Changes Reviewed1. Concurrency-Safe
|
|
@claude 解决冲突 |
|
Claude finished @g1331's task —— View job Conflict Resolution Complete ✅Tasks
What I DidI've successfully resolved the merge conflicts between your feature branch and main: 1.
|
Co-authored-by: Umaru <[email protected]>
代码审查总结 - PR #189
|
|
@claude 根据审查结果进行修复 |
|
Claude encountered an error —— View job ✅ 代码审查问题修复完成修复清单
已完成的修复1. 修复
|
… race - Move UPDATE result check outside try block in insert_or_update to prevent catching UPDATE exceptions incorrectly - Fix TOCTOU race condition in insert_or_ignore by using INSERT-first approach instead of check-then-insert - Remove broad Exception catching, only catch IntegrityError - Use bare 'raise' instead of 'raise e' for better traceback Addresses blocking and high-priority issues from code review. Co-authored-by: Umaru <[email protected]>
代码审查总结✅ 总体评价：可合并，建议修复高优先级问题后再上线 核心改进本次 PR 有效解决了并发场景下
|
问题描述
修复 SQLAlchemy 数据库完整性约束错误：
根本原因
core/orm/__init__.py第224行的insert_or_update方法GroupSetting表插入已存在的group_id，违反了 UNIQUE 约束AccountController.init_group→AsyncORM.insert_or_update→ SQLAlchemy INSERT 失败修复内容
1. 重构
insert_or_update方法（并发安全）IntegrityError异常，处理竞态条件2. 修复
insert_or_update_batch方法insert_or_update方法3. 修复
insert_or_ignore方法4. 代码质量改进
sqlalchemy.exc.IntegrityError导入影响范围
修复影响以下调用链：
AccountController.init_group→AsyncORM.insert_or_updateinsert_or_update、insert_or_update_batch、insert_or_ignore的地方测试验证
后续测试建议
group_id的场景AccountController.init_group的正常工作相关 Issue
解决了原始的 UNIQUE 约束冲突问题，使系统能够优雅地处理重复的群组初始化请求，同时保持了数据的一致性和完整性。
Pull Request 由 Augment Code 在 PR 作者指导下创建