Skip to content

remove try catch #1310

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
iceljc merged 1 commit into from
Mar 17, 2026
+4 −12
Merged

remove try catch #1310

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 3 additions & 11 deletions src/Plugins/BotSharp.Plugin.KnowledgeBase/Graph/GraphKnowledgeService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,9 @@ public GraphKnowledgeService(

public async Task<GraphQueryResult> ExecuteQueryAsync(string query, GraphQueryOptions? options = null)
{
try
{
var db = GetGraphDb(options?.Provider);
var result = await db.ExecuteQueryAsync(query, options);
return result;
}
catch (Exception ex)
{
_logger.LogError(ex, $"Error when searching graph knowledge (Query: {query}).");
return new GraphQueryResult();
}
var db = GetGraphDb(options?.Provider);
var result = await db.ExecuteQueryAsync(query, options);
return result;
Comment on lines +23 to +25
Copy link
Contributor

@qodo-code-review qodo-code-review bot Mar 17, 2026

Choose a reason for hiding this comment

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

Action required

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

Comment on lines +23 to +25
Copy link
Contributor

@qodo-code-review qodo-code-review bot Mar 17, 2026

Choose a reason for hiding this comment

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

Action required

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

}


Expand Down
2 changes: 1 addition & 1 deletion src/Plugins/BotSharp.Plugin.Membase/GraphDb/MembaseGraphDb.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public async Task<GraphQueryResult> ExecuteQueryAsync(string query, GraphQueryEx
{
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;
Copy link
Contributor

@qodo-code-review qodo-code-review bot Mar 17, 2026

Choose a reason for hiding this comment

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

Action required

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

}
}
}
Loading