optimize User，InstructionLog use async method

[yileicn]

PR Type

Enhancement

---

Description

• Convert User and InstructionLog repository methods to async

• Add Dashboard property to User model

• Update file I/O operations to use async methods

• Implement async/await patterns in service layers

---

Diagram Walkthrough

flowchart LR
  A["Sync Repository Methods"] -- "Convert to async" --> B["Task-based Methods"]
  C["File I/O Operations"] -- "Use async variants" --> D["Async File Methods"]
  E["Service Layer Calls"] -- "Add await" --> F["Async Service Calls"]
  B --> G["User Model Enhanced"]
  D --> G
  F --> G

Loading

File Walkthrough

Relevant files

Enhancement

16 files IFileStorageService.cs Convert user avatar methods to async                                          +2/-2      IBotSharpRepository.cs Convert User and InstructionLog methods to async                  +27/-27  User.cs Add Dashboard property to User model                                          +2/-0      AgentService.CreateAgent.cs Add await for async GetUserById call                                          +1/-1      ConversationService.cs Add await for async GetUserById call                                          +1/-1      LocalFileStorageService.User.cs Convert user avatar methods to async                                          +4/-4      LoggerService.Instruction.cs Add await for async GetInstructionLogSearchKeys                    +1/-1      FileRepository.Log.cs Convert instruction log methods to async                                  +6/-6      FileRepository.User.cs Convert all user repository methods to async                          +48/-42  UserService.Token.cs Add await for async user lookup methods                                    +10/-10  UserService.cs Convert user service methods to async                                        +47/-51  InstructionLogHook.cs Add await for async user and log methods                                  +4/-4      UserController.cs Convert avatar endpoints to async                                                +4/-4      UserDocument.cs Add Dashboard property mapping                                                      +1/-0      MongoRepository.Log.cs Convert instruction log methods to async                                  +5/-5      MongoRepository.User.cs Convert all user repository methods to async                          +65/-67

---

[qodo-code-review]

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Path traversal Description: Multiple async file-write operations build paths using user.Id/userId (e.g., Path.Combine(_dbSettings.FileRepository, USERS_FOLDER, userId) and then File.WriteAllTextAsync(...)) without validating that the identifier is a safe single path segment, which could allow path traversal (e.g., ../...) and arbitrary file overwrite if an attacker can influence userId/user.Id passed into methods like CreateUser, UpdateUserVerified, AddDashboardConversation, RemoveDashboardConversation, or UpdateDashboardConversation. FileRepository.User.cs [84-395] Referred Code public async Task CreateUser(User user) { var userId = Guid.NewGuid().ToString(); user.Id = userId; var dir = Path.Combine(_dbSettings.FileRepository, USERS_FOLDER, userId); if (!Directory.Exists(dir)) { Directory.CreateDirectory(dir); } var path = Path.Combine(dir, USER_FILE); await File.WriteAllTextAsync(path, JsonSerializer.Serialize(user, _options)); } public async Task UpdateExistUser(string userId, User user) { user.Id = userId; await CreateUser(user); } public async Task UpdateUserVerified(string userId) { ... (clipped 291 lines)
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: No audit logs: Newly-added dashboard mutation methods update user state (add/remove/update conversations) without recording who performed the action, what changed, and the outcome, preventing reconstruction of critical user-profile changes. Referred Code public async Task AddDashboardConversation(string userId, string conversationId) { var user = await GetUserById(userId); if (user == null) return; var curDash = user.Dashboard ?? new Dashboard(); curDash.ConversationList.Add(new DashboardConversation { Id = Guid.NewGuid().ToString(), ConversationId = conversationId }); var filter = Builders<UserDocument>.Filter.Eq(x => x.Id, userId); var update = Builders<UserDocument>.Update.Set(x => x.Dashboard, curDash) .Set(x => x.UpdatedTime, DateTime.UtcNow); } public async Task RemoveDashboardConversation(string userId, string conversationId) { var user = await GetUserById(userId); if (user == null || user.Dashboard == null || user.Dashboard.ConversationList.IsNullOrEmpty()) return; var curDash = user.Dashboard; ... (clipped 14 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Misleading identifiers: New async signatures introduce unclear/misspelled names (e.g., Iphone parameter and verficationCode) which reduce readability and can cause misuse. Referred Code Task UpdateUserVerificationCode(string userId, string verficationCode) => throw new NotImplementedException(); Task UpdateUserPassword(string userId, string password) => throw new NotImplementedException(); Task UpdateUserEmail(string userId, string email) => throw new NotImplementedException(); Task UpdateUserPhone(string userId, string Iphone, string regionCode) => throw new NotImplementedException(); Task UpdateUserIsDisable(string userId, bool isDisable) => throw new NotImplementedException(); Task UpdateUsersIsDisable(List<string> userIds, bool isDisable) => throw new NotImplementedException(); ValueTask<PagedItems<User>> GetUsers(UserFilter filter) => throw new NotImplementedException(); Task<List<User>> SearchLoginUsers(User filter, string source = UserSource.Internal) =>throw new NotImplementedException(); Task<User?> GetUserDetails(string userId, bool includeAgent = false) => throw new NotImplementedException(); Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Unhandled IO errors: Newly-converted async file operations (e.g., File.WriteAllTextAsync) have no exception handling or fallback behavior, so IO failures can crash the request path without actionable context/logging. Referred Code public async Task CreateUser(User user) { var userId = Guid.NewGuid().ToString(); user.Id = userId; var dir = Path.Combine(_dbSettings.FileRepository, USERS_FOLDER, userId); if (!Directory.Exists(dir)) { Directory.CreateDirectory(dir); } var path = Path.Combine(dir, USER_FILE); await File.WriteAllTextAsync(path, JsonSerializer.Serialize(user, _options)); } public async Task UpdateExistUser(string userId, User user) { user.Id = userId; await CreateUser(user); } public async Task UpdateUserVerified(string userId) { ... (clipped 8 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: PII in logs: A new/updated log line records user identifiers (record.Id, record.UserName) and region in a warning message, which can expose PII in application logs. Referred Code _logger.LogWarning($"Created new user account: {record.Id} {record.UserName}, RegionCode: {record.RegionCode}"); Utilities.ClearCache(); Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Null userId passed: New async calls pass user?.Id into repository methods that appear to require a non-null userId, which may lead to unexpected behavior or exceptions depending on nullable settings and repository implementations. Referred Code { var user = await GetUser(_user.Id); var db = _services.GetRequiredService<IBotSharpRepository>(); await db.AddDashboardConversation(user?.Id, conversationId); return true; } public async Task<bool> RemoveDashboardConversation(string conversationId) { var user = await GetUser(_user.Id); var db = _services.GetRequiredService<IBotSharpRepository>(); await db.RemoveDashboardConversation(user?.Id, conversationId); return true; } public async Task UpdateDashboardConversation(DashboardConversation newDashConv) { var db = _services.GetRequiredService<IBotSharpRepository>(); var user = await GetUser(_user.Id); var dashConv = (await db.GetDashboard(user?.Id))? ... (clipped 8 lines) Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

[qodo-code-review]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Execute the created database update operation Add the missing await _dc.Users.UpdateOneAsync(filter, update) call to ensure the dashboard conversation is actually saved to the database. src/Plugins/BotSharp.Plugin.MongoStorage/Repository/MongoRepository.User.cs [447-461] public async Task AddDashboardConversation(string userId, string conversationId) { var user = await GetUserById(userId); if (user == null) return; var curDash = user.Dashboard ?? new Dashboard(); curDash.ConversationList.Add(new DashboardConversation { Id = Guid.NewGuid().ToString(), ConversationId = conversationId }); var filter = Builders<UserDocument>.Filter.Eq(x => x.Id, userId); var update = Builders<UserDocument>.Update.Set(x => x.Dashboard, curDash) .Set(x => x.UpdatedTime, DateTime.UtcNow); + await _dc.Users.UpdateOneAsync(filter, update); } Apply / Chat Suggestion importance[1-10]: 9 __ Why: This suggestion identifies a critical bug where a database update operation is defined but never executed, leading to data not being saved.	High
	Validate retrieved user existence Add a null check for the user object after calling _db.GetUserById to prevent a potential NullReferenceException. src/Infrastructure/BotSharp.Core/Agents/Services/AgentService.CreateAgent.cs [29-46] -var user = await _db.GetUserById(userIdentity.Id); +var user = await _db.GetUserById(userIdentity.Id) + ?? throw new InvalidOperationException($"User '{userIdentity.Id}' not found."); ... _db.BulkInsertUserAgents(new List<UserAgent> { new UserAgent { UserId = user.Id, Actions = new List<string> { UserAction.Edit, ... }, CreatedTime = DateTime.UtcNow, UpdatedTime = DateTime.UtcNow } }); [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a potential NullReferenceException if user is null and provides a valid fix to handle this case, preventing a runtime crash.	Medium
	Replace synchronous file I/O with async Replace the synchronous File.ReadAllText with its asynchronous counterpart File.ReadAllTextAsync inside the UpdateInstructionLogStates method to avoid blocking the thread. src/Infrastructure/BotSharp.Core/Repository/FileRepository/FileRepository.Log.cs [207-235] public async Task<bool> UpdateInstructionLogStates(UpdateInstructionLogStatesModel updateInstructionStates) { if (string.IsNullOrWhiteSpace(updateInstructionStates?.LogId) || updateInstructionStates?.States?.Any() != true) { return false; } var baseDir = Path.Combine(_dbSettings.FileRepository, INSTRUCTION_LOG_FOLDER); if (!Directory.Exists(baseDir)) { return false; } - var logFile = Directory.EnumerateFiles(baseDir).FirstOrDefault(file => + string? logFile = null; + InstructionFileLogModel? log = null; + + foreach (var file in Directory.EnumerateFiles(baseDir)) { - var json = File.ReadAllText(file); - var log = JsonSerializer.Deserialize<InstructionFileLogModel>(json, _options); - return log?.Id == updateInstructionStates.LogId; - }); - - if (logFile == null) - { - return false; + var json = await File.ReadAllTextAsync(file); + var currentLog = JsonSerializer.Deserialize<InstructionFileLogModel>(json, _options); + if (currentLog?.Id == updateInstructionStates.LogId) + { + logFile = file; + log = currentLog; + break; + } } - var log = JsonSerializer.Deserialize<InstructionFileLogModel>(File.ReadAllText(logFile), _options); - if (log == null) + if (logFile == null || log == null) { return false; } ... [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that a synchronous I/O call (File.ReadAllText) is used within an async method, which is an anti-pattern, and provides the correct asynchronous fix.	Medium
High-level	Re-evaluate the async implementation for FileRepository The FileRepository uses Task.FromResult to wrap synchronous in-memory operations, creating misleading "fake async" methods. It is recommended to either implement true asynchronous file I/O or revert these methods to be synchronous if they are intended to use an in-memory cache. Examples: src/Infrastructure/BotSharp.Core/Repository/FileRepository/FileRepository.User.cs [55-59] public Task<User?> GetUserById(string? id = null) { var user = Users.FirstOrDefault(x => x.Id == id || (x.ExternalId != null && x.ExternalId == id)); return Task.FromResult(user); } src/Infrastructure/BotSharp.Core/Repository/FileRepository/FileRepository.User.cs [261-307] public async Task<bool> UpdateUser(User user, bool updateUserAgents = false) { if (string.IsNullOrEmpty(user?.Id)) { return false; } var dir = Path.Combine(_dbSettings.FileRepository, USERS_FOLDER, user.Id); if (!Directory.Exists(dir)) { ... (clipped 37 lines) Solution Walkthrough: Before: // src/Infrastructure/BotSharp.Core/Repository/FileRepository/FileRepository.User.cs public partial class FileRepository { // In-memory cache private List<User> Users => _users.Value; // "Fake async" method using in-memory cache public Task<User?> GetUserById(string? id = null) { var user = Users.FirstOrDefault(x => x.Id == id); return Task.FromResult(user); } // Inconsistent async method with sync file read public async Task<bool> UpdateUser(User user, bool updateUserAgents = false) { // ... var userJson = File.ReadAllText(userFile); // Synchronous I/O // ... await File.WriteAllTextAsync(userFile, ...); // Asynchronous I/O return true; } } After: // src/Infrastructure/BotSharp.Core/Repository/FileRepository/FileRepository.User.cs public partial class FileRepository { // Option 1: True async I/O for each call public async Task<User?> GetUserById(string? id = null) { // Logic to find the specific user file path var userFilePath = FindUserFileById(id); if (userFilePath == null) return null; var json = await File.ReadAllTextAsync(userFilePath); return JsonSerializer.Deserialize<User>(json); } // Consistent async method with async file read public async Task<bool> UpdateUser(User user, bool updateUserAgents = false) { // ... var userJson = await File.ReadAllTextAsync(userFile); // Asynchronous I/O // ... await File.WriteAllTextAsync(userFile, ...); // Asynchronous I/O return true; } } Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that many FileRepository methods are "fake async", wrapping synchronous in-memory lookups, which is misleading and inefficient. It also implicitly highlights inconsistencies where synchronous file I/O is still used in otherwise async methods, which is a significant design and correctness issue.	Medium
General	Use a bulk update operation Replace the loop that updates users one by one with a single UpdateManyAsync bulk operation to improve performance. src/Plugins/BotSharp.Plugin.MongoStorage/Repository/MongoRepository.User.cs [201-207] public async Task UpdateUsersIsDisable(List<string> userIds, bool isDisable) { - foreach (var userId in userIds) + if (userIds == null || !userIds.Any()) { - await UpdateUserIsDisable(userId, isDisable); + return; } + + var filter = Builders<UserDocument>.Filter.In(x => x.Id, userIds); + var update = Builders<UserDocument>.Update.Set(x => x.IsDisabled, isDisable) + .Set(x => x.UpdatedTime, DateTime.UtcNow); + await _dc.Users.UpdateManyAsync(filter, update); } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies an N+1 database problem and proposes an efficient bulk update using UpdateManyAsync, which significantly improves performance.	Medium
	Handle missing user in avatar retrieval Add a null check for the user object after it's retrieved from the database to prevent a NullReferenceException when constructing the avatar directory path. src/Infrastructure/BotSharp.Core/Files/Services/Storage/LocalFileStorageService.User.cs [7-11] public async Task<string> GetUserAvatar() { var db = _services.GetRequiredService<IBotSharpRepository>(); var user = await db.GetUserById(_user.Id); - var dir = GetUserAvatarDir(user?.Id); + if (user == null) + { + return string.Empty; + } + var dir = GetUserAvatarDir(user.Id); ... } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential NullReferenceException when user is null and proposes a safe way to handle it, improving the method's robustness.	Medium
Learned best practice	Persist MongoDB updates Execute the computed MongoDB updates (e.g., UpdateOneAsync) so dashboard changes are persisted; otherwise these methods silently do nothing. src/Plugins/BotSharp.Plugin.MongoStorage/Repository/MongoRepository.User.cs [447-489] public async Task AddDashboardConversation(string userId, string conversationId) { var user = await GetUserById(userId); if (user == null) return; var curDash = user.Dashboard ?? new Dashboard(); curDash.ConversationList.Add(new DashboardConversation { Id = Guid.NewGuid().ToString(), ConversationId = conversationId }); var filter = Builders<UserDocument>.Filter.Eq(x => x.Id, userId); var update = Builders<UserDocument>.Update.Set(x => x.Dashboard, curDash) .Set(x => x.UpdatedTime, DateTime.UtcNow); + + await _dc.Users.UpdateOneAsync(filter, update); } public async Task RemoveDashboardConversation(string userId, string conversationId) { var user = await GetUserById(userId); if (user == null || user.Dashboard == null || user.Dashboard.ConversationList.IsNullOrEmpty()) return; var curDash = user.Dashboard; var unpinConv = user.Dashboard.ConversationList.FirstOrDefault( x => string.Equals(x.ConversationId, conversationId, StringComparison.OrdinalIgnoreCase)); if (unpinConv == null) return; curDash.ConversationList.Remove(unpinConv); var filter = Builders<UserDocument>.Filter.Eq(x => x.Id, userId); var update = Builders<UserDocument>.Update.Set(x => x.Dashboard, curDash) .Set(x => x.UpdatedTime, DateTime.UtcNow); + + await _dc.Users.UpdateOneAsync(filter, update); } public async Task UpdateDashboardConversation(string userId, DashboardConversation dashConv) { var user = await GetUserById(userId); if (user == null || user.Dashboard == null || user.Dashboard.ConversationList.IsNullOrEmpty()) return; var curIdx = user.Dashboard.ConversationList.ToList().FindIndex( x => string.Equals(x.ConversationId, dashConv.ConversationId, StringComparison.OrdinalIgnoreCase)); if (curIdx < 0) return; var filter = Builders<UserDocument>.Filter.Eq(x => x.Id, userId); var update = Builders<UserDocument>.Update.Set(x => x.Dashboard.ConversationList[curIdx], dashConv) .Set(x => x.UpdatedTime, DateTime.UtcNow); + + await _dc.Users.UpdateOneAsync(filter, update); } Apply / Chat Suggestion importance[1-10]: 6 __ Why: Relevant best practice - Improve defensive coding with correct state/logic to prevent incorrect behavior (ensure DB updates are actually executed).	Low
	Always return a task Return a Task in all code paths (use Task.FromResult<User?>(null) for early exits) so callers never receive a null task and the async contract is preserved. src/Infrastructure/BotSharp.Core/Repository/FileRepository/FileRepository.User.cs [215-235] public Task<User?> GetUserDetails(string userId, bool includeAgent = false) { - if (string.IsNullOrWhiteSpace(userId)) return null; + if (string.IsNullOrWhiteSpace(userId)) return Task.FromResult<User?>(null); var user = Users.FirstOrDefault(x => x.Id == userId || x.ExternalId == userId); - if (user == null) return null; + if (user == null) return Task.FromResult<User?>(null); var agentActions = new List<UserAgentAction>(); var userAgents = UserAgents?.Where(x => x.UserId == userId)?.ToList() ?? []; if (!includeAgent) { agentActions = userAgents.Select(x => new UserAgentAction { Id = x.Id, AgentId = x.AgentId, Actions = x.Actions }).ToList(); user.AgentActions = agentActions; return Task.FromResult(user); } ... } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 5 __ Why: Relevant best practice - Use correct async patterns: methods returning Task<T> must always return a task (not null) and should not mix sync returns with async signatures.	Low
More

[JackJiang1234]

reviewed