Merged
Conversation
Contributor
Review Summary by QodoRemove exception handling wrappers and propagate errors
WalkthroughsDescription• Remove try-catch wrapper from GraphKnowledgeService.ExecuteQueryAsync • Propagate exceptions instead of catching and logging in MembaseGraphDb • Simplify error handling by letting exceptions bubble up naturally Diagramflowchart LR
A["GraphKnowledgeService.ExecuteQueryAsync"] -->|removed try-catch| B["Direct execution"]
C["MembaseGraphDb.ExecuteQueryAsync"] -->|changed catch behavior| D["throw instead of return empty"]
B --> E["Exceptions propagate up"]
D --> E
File Changes1. src/Plugins/BotSharp.Plugin.KnowledgeBase/Graph/GraphKnowledgeService.cs
|
Contributor
Code Review by Qodo
1. Removed query error fallback
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+23
to
+25
| var db = GetGraphDb(options?.Provider); | ||
| var result = await db.ExecuteQueryAsync(query, options); | ||
| return result; |
Contributor
There was a problem hiding this comment.
1. Removed query error fallback 📘 Rule violation ⛯ Reliability
ExecuteQueryAsync no longer catches provider failures, so exceptions can bubble to callers with no safe fallback result. This violates the requirement to return a safe fallback with clear logging at provider boundaries.
Agent Prompt
## Issue description
`GraphKnowledgeService.ExecuteQueryAsync` removed its try/catch and now lets provider/storage errors escape without returning a safe fallback or logging, violating the boundary guard/fallback requirement.
## Issue Context
This method is a provider/storage boundary (calls `GetGraphDb(...).ExecuteQueryAsync(...)`). The previous implementation returned a safe fallback `GraphQueryResult` and logged an error; the PR removed that behavior.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.KnowledgeBase/Graph/GraphKnowledgeService.cs[23-25]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| var argLogs = args.Select(x => (new KeyValue(x.Key, x.Value.ConvertToString(BotSharpOptions.defaultJsonOptions))).ToString()); | ||
| _logger.LogError(ex, $"Error when executing query in {Provider} graph db. (Query: {query}), (Argments: \r\n{string.Join("\r\n", argLogs)})"); | ||
| return new(); | ||
| throw; |
Contributor
There was a problem hiding this comment.
2. Rethrow breaks safe fallback 📘 Rule violation ⛯ Reliability
The graph DB implementation now rethrows exceptions instead of returning a safe fallback result, increasing the likelihood of runtime failures propagating across the provider boundary. This violates the requirement for safe fallback behavior with clear logging at provider/storage boundaries.
Agent Prompt
## Issue description
`MembaseGraphDb.ExecuteQueryAsync` now rethrows exceptions from the provider boundary instead of returning a safe fallback result.
## Issue Context
The catch block already logs the error; rethrowing defeats the safe fallback requirement and can cause upstream failures.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.Membase/GraphDb/MembaseGraphDb.cs[58-58]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+23
to
+25
| var db = GetGraphDb(options?.Provider); | ||
| var result = await db.ExecuteQueryAsync(query, options); | ||
| return result; |
Contributor
There was a problem hiding this comment.
3. Null graph db dereference 🐞 Bug ⛯ Reliability
GraphKnowledgeService.ExecuteQueryAsync can dereference a null IGraphDb when GetGraphDb returns null (no registered provider match), causing a NullReferenceException that now propagates to callers. This will break /knowledge/graph/search and any other callers because there is no controller- or middleware-level exception handling in production.
Agent Prompt
### Issue description
`GraphKnowledgeService.ExecuteQueryAsync` calls `db.ExecuteQueryAsync(...)` without verifying that `GetGraphDb(...)` actually found a provider, which can produce a `NullReferenceException`.
### Issue Context
`GetGraphDb` uses `FirstOrDefault` and can return null when configuration requests an unregistered provider.
### Fix Focus Areas
- src/Plugins/BotSharp.Plugin.KnowledgeBase/Graph/GraphKnowledgeService.cs[21-35]
- src/Infrastructure/BotSharp.OpenAPI/Controllers/KnowledgeBase/KnowledgeBaseController.cs[208-224]
- src/Infrastructure/BotSharp.OpenAPI/BotSharpOpenApiExtensions.cs[231-262]
### Implementation notes
- Add a null check after `GetGraphDb(...)`.
- Prefer preserving previous behavior (return empty `GraphQueryResult` and log) unless the API contract is explicitly changing; if changing to “throw”, add controller-level handling or a global exception handler middleware to return a controlled error response.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
No description provided.