fix: prevent exceptions when listing indices and constraints asynchronously#61
fix: prevent exceptions when listing indices and constraints asynchronously#61alexmg wants to merge 5 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralizes CALL ... YIELD query assembly in Graph via a new BuildQueryBodyForProcedureCall helper, extends CallProcedureAsync/CallProcedureReadOnlyAsync to accept optional args and kwargs, and adds synchronous and asynchronous tests in FalkorDBAPITest covering index helpers and various procedure-call yield scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@NFalkorDB/Graph.cs`:
- Around line 249-254: The new CallProcedureAsync public signature changed
parameter order and breaks existing callers; restore the previous overload that
accepted (string procedure, CommandFlags flags, IEnumerable<string> args = null,
Dictionary<string, List<string>> kwargs = null) and implement it to forward into
the current implementation (the overload that calls
BuildQueryBodyForProcedureCall and then QueryAsync) so old call sites like
CallProcedureAsync("db.labels", CommandFlags.None, ...) continue to work; keep
the existing implementation (which builds the query via
BuildQueryBodyForProcedureCall and calls QueryAsync) unchanged and simply add
the compatibility overload that delegates to it.
- Around line 292-297: The generated Cypher CALL string is missing the " YIELD "
keyword before appended kwargs, causing malformed SQL like "CALL proc()field";
update the code that builds queryBody (the logic around variables queryBody,
procedure, args, and kwargs/TryGetValue("y", out var kwargsList)) so that when
kwargsList is present you append " YIELD " followed by string.Join(",",
kwargsList) (ensuring proper spacing and no extra commas) rather than appending
the list directly after the closing parenthesis.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02f39ddc-b527-428b-a272-e7e96a68a78e
📒 Files selected for processing (2)
NFalkorDB.Tests/FalkorDBAPITest.csNFalkorDB/Graph.cs
| public Task<ResultSet> CallProcedureAsync(string procedure, IEnumerable<string> args = null, Dictionary<string, List<string>> kwargs = null, CommandFlags flags = CommandFlags.None) | ||
| { | ||
| args = args.Select(a => QuoteString(a)); | ||
|
|
||
| var queryBody = new StringBuilder(); | ||
|
|
||
| queryBody.Append($"CALL {procedure}({string.Join(",", args)})"); | ||
|
|
||
| if (kwargs.TryGetValue("y", out var kwargsList)) | ||
| { | ||
| queryBody.Append(string.Join(",", kwargsList)); | ||
| } | ||
| var queryBody = BuildQueryBodyForProcedureCall(procedure, ref args, kwargs); | ||
|
|
||
| return QueryAsync(queryBody.ToString(), flags: flags); | ||
| return QueryAsync(queryBody, flags: flags); | ||
| } |
There was a problem hiding this comment.
Preserve the previous CallProcedureAsync overload.
This public signature now puts flags after two optional parameters. Existing consumers that called CallProcedureAsync("db.labels", CommandFlags.None, ...) will break. Keep the old overload and forward it to this implementation instead of replacing it.
Compatibility overload
+ public Task<ResultSet> CallProcedureAsync(
+ string procedure,
+ CommandFlags flags,
+ IEnumerable<string> args = null,
+ Dictionary<string, List<string>> kwargs = null) =>
+ CallProcedureAsync(procedure, args, kwargs, flags);
+
public Task<ResultSet> CallProcedureAsync(string procedure, IEnumerable<string> args = null, Dictionary<string, List<string>> kwargs = null, CommandFlags flags = CommandFlags.None)
{
var queryBody = BuildQueryBodyForProcedureCall(procedure, ref args, kwargs);
return QueryAsync(queryBody, flags: flags);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Task<ResultSet> CallProcedureAsync(string procedure, IEnumerable<string> args = null, Dictionary<string, List<string>> kwargs = null, CommandFlags flags = CommandFlags.None) | |
| { | |
| args = args.Select(a => QuoteString(a)); | |
| var queryBody = new StringBuilder(); | |
| queryBody.Append($"CALL {procedure}({string.Join(",", args)})"); | |
| if (kwargs.TryGetValue("y", out var kwargsList)) | |
| { | |
| queryBody.Append(string.Join(",", kwargsList)); | |
| } | |
| var queryBody = BuildQueryBodyForProcedureCall(procedure, ref args, kwargs); | |
| return QueryAsync(queryBody.ToString(), flags: flags); | |
| return QueryAsync(queryBody, flags: flags); | |
| } | |
| public Task<ResultSet> CallProcedureAsync( | |
| string procedure, | |
| CommandFlags flags, | |
| IEnumerable<string> args = null, | |
| Dictionary<string, List<string>> kwargs = null) => | |
| CallProcedureAsync(procedure, args, kwargs, flags); | |
| public Task<ResultSet> CallProcedureAsync(string procedure, IEnumerable<string> args = null, Dictionary<string, List<string>> kwargs = null, CommandFlags flags = CommandFlags.None) | |
| { | |
| var queryBody = BuildQueryBodyForProcedureCall(procedure, ref args, kwargs); | |
| return QueryAsync(queryBody, flags: flags); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@NFalkorDB/Graph.cs` around lines 249 - 254, The new CallProcedureAsync public
signature changed parameter order and breaks existing callers; restore the
previous overload that accepted (string procedure, CommandFlags flags,
IEnumerable<string> args = null, Dictionary<string, List<string>> kwargs = null)
and implement it to forward into the current implementation (the overload that
calls BuildQueryBodyForProcedureCall and then QueryAsync) so old call sites like
CallProcedureAsync("db.labels", CommandFlags.None, ...) continue to work; keep
the existing implementation (which builds the query via
BuildQueryBodyForProcedureCall and calls QueryAsync) unchanged and simply add
the compatibility overload that delegates to it.
There was a problem hiding this comment.
The original CallProcedureAsync signature had args and kwargs parameters after the procedure parameter. Both parameters were non-nullable which was inconsistent with the other call procedure overloads that assign a default null value.
public Task<ResultSet> CallProcedureAsync(
string procedure,
IEnumerable<string> args,
Dictionary<string, List<string>> kwargs,
CommandFlags flags = CommandFlags.None)Invoking the CallProcedureAsync method with null values resulted in a NullReferenceException being thrown.
CallProcedureAsync("DB.CONSTRAINTS", null, null, flags);The cause being the args.Select() statement on line 1 attempting to enumerate a null instance.
public Task<ResultSet> CallProcedureAsync(
string procedure,
IEnumerable<string> args,
Dictionary<string, List<string>> kwargs,
CommandFlags flags = CommandFlags.None)
{
args = args.Select(a => QuoteString(a)); // NullReferenceException!
// ...
}It was mandatory to provide non-null values for those parameters when invoking the previous signature.
CallProcedureAsync("foo", [], [], CommandFlags.None);The method CallProcedureAsync("db.labels", CommandFlags.None, ...) was not a valid signature prior to the commit because of the non-nullable args and kwargs parameters.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
NFalkorDB/Graph.cs (1)
294-296:⚠️ Potential issue | 🟡 MinorGuard
kwargs["y"]before reading.Count.A dictionary entry with a
nullvalue still makesTryGetValuesucceed, so Line 294 can still throw aNullReferenceExceptionhere. That reintroduces the same class of failure this refactor is trying to remove for optional inputs.🩹 Proposed fix
- if (kwargs != null && kwargs.TryGetValue("y", out var kwargsList) && kwargsList.Count > 0) + if (kwargs != null && kwargs.TryGetValue("y", out var kwargsList) && kwargsList != null && kwargsList.Count > 0) { queryBody.Append($" YIELD {string.Join(",", kwargsList)}"); }A small regression test for
{ ["y"] = null }would lock this down too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NFalkorDB/Graph.cs` around lines 294 - 296, The TryGetValue call can succeed with a null value so accessing kwargsList.Count or string.Join can throw; update the guard in the Graph class where you build the query (the block that checks kwargs.TryGetValue("y", out var kwargsList) and calls queryBody.Append($" YIELD {string.Join(...)}")) to ensure kwargsList != null before reading .Count or joining (e.g., check kwargsList != null && kwargsList.Count > 0), and add a small regression test that passes a dictionary with ["y"] = null to verify no exception is thrown.
🧹 Nitpick comments (1)
NFalkorDB.Tests/FalkorDBAPITest.cs (1)
419-433: Seed the graph before these async list-helper calls.Unlike the sync coverage, both tests run against a freshly selected graph. Adding a tiny write first will keep any failure focused on the async
CALLpath instead of graph-initialization behavior.♻️ Suggested test hardening
[Fact] public async Task TestIndexHelpersListIndicesAsync() { + _api.Query("CREATE (:seed)"); var indicesResult = await _api.ListIndicesAsync(); // Shape is server-dependent; just ensure the call succeeds and returns a header Assert.NotNull(indicesResult); } [Fact] public async Task TestIndexHelpersListConstraintsAsync() { + _api.Query("CREATE (:seed)"); var constraints = await _api.ListConstraintsAsync(); // Shape is server-dependent; just ensure the call succeeds and returns a header Assert.NotNull(constraints); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@NFalkorDB.Tests/FalkorDBAPITest.cs` around lines 419 - 433, The two async tests TestIndexHelpersListIndicesAsync and TestIndexHelpersListConstraintsAsync should seed the selected graph with a tiny write before calling _api.ListIndicesAsync() and _api.ListConstraintsAsync(); add an awaited lightweight write using the existing _api (for example create a single node/property or call a small write helper like SeedGraphAsync on _api) immediately before each list call so the graph is initialized, then proceed to Assert.NotNull on the results.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@NFalkorDB/Graph.cs`:
- Around line 294-296: The TryGetValue call can succeed with a null value so
accessing kwargsList.Count or string.Join can throw; update the guard in the
Graph class where you build the query (the block that checks
kwargs.TryGetValue("y", out var kwargsList) and calls queryBody.Append($" YIELD
{string.Join(...)}")) to ensure kwargsList != null before reading .Count or
joining (e.g., check kwargsList != null && kwargsList.Count > 0), and add a
small regression test that passes a dictionary with ["y"] = null to verify no
exception is thrown.
---
Nitpick comments:
In `@NFalkorDB.Tests/FalkorDBAPITest.cs`:
- Around line 419-433: The two async tests TestIndexHelpersListIndicesAsync and
TestIndexHelpersListConstraintsAsync should seed the selected graph with a tiny
write before calling _api.ListIndicesAsync() and _api.ListConstraintsAsync();
add an awaited lightweight write using the existing _api (for example create a
single node/property or call a small write helper like SeedGraphAsync on _api)
immediately before each list call so the graph is initialized, then proceed to
Assert.NotNull on the results.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25fec618-dd17-4a5e-a608-aef736d59e0d
📒 Files selected for processing (2)
NFalkorDB.Tests/FalkorDBAPITest.csNFalkorDB/Graph.cs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@NFalkorDB/Graph.cs`:
- Around line 288-292: The code maps args through QuoteString which dereferences
its input, so a null element in args will cause a NullReferenceException; update
the code around args = args?.Select(QuoteString) to first validate that args
does not contain any null items (e.g., check args != null && args.Any(a => a ==
null)) and throw a clear ArgumentException (including the procedure name and
parameter name "args") if a null is found, or alternatively replace null
elements with a safe representation before calling QuoteString; keep references
to args, QuoteString, queryBody and procedure so the validation is applied just
before building the CALL string.
| args = args?.Select(QuoteString); | ||
|
|
||
| var queryBody = new StringBuilder(); | ||
|
|
||
| queryBody.Append($"CALL {procedure}({string.Join(",", args)})"); | ||
| queryBody.Append(args != null ? $"CALL {procedure}({string.Join(",", args)})" : $"CALL {procedure}()"); |
There was a problem hiding this comment.
Guard against null items inside args to avoid runtime NREs.
Line 288 calls QuoteString over each item, and QuoteString dereferences input; a null element in args will currently crash with NullReferenceException instead of a clear argument error.
Suggested fix
internal static string BuildQueryBodyForProcedureCall(string procedure, ref IEnumerable<string> args, Dictionary<string, List<string>> kwargs)
{
- args = args?.Select(QuoteString);
+ var quotedArgs = args?
+ .Select(a => a ?? throw new System.ArgumentException("Procedure args cannot contain null values.", nameof(args)))
+ .Select(QuoteString)
+ .ToArray();
+
+ args = quotedArgs;
var queryBody = new StringBuilder();
- queryBody.Append(args != null ? $"CALL {procedure}({string.Join(",", args)})" : $"CALL {procedure}()");
+ queryBody.Append(quotedArgs != null ? $"CALL {procedure}({string.Join(",", quotedArgs)})" : $"CALL {procedure}()");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@NFalkorDB/Graph.cs` around lines 288 - 292, The code maps args through
QuoteString which dereferences its input, so a null element in args will cause a
NullReferenceException; update the code around args = args?.Select(QuoteString)
to first validate that args does not contain any null items (e.g., check args !=
null && args.Any(a => a == null)) and throw a clear ArgumentException (including
the procedure name and parameter name "args") if a null is found, or
alternatively replace null elements with a safe representation before calling
QuoteString; keep references to args, QuoteString, queryBody and procedure so
the validation is applied just before building the CALL string.
|
@AviAvni It would be great if you could take a look at this PR? I believe the original issue is resolved along with another pre-existing issues identified by your bot. |
1 participant
Summary
This PR fixes the
ListIndicesAsyncandListConstraintsAsyncmethods so that calls no longer throw exceptions when invoked. The async and read-only procedure call paths now handle optional inputs consistently with sync behavior.What Changed
BuildQueryBodyForProcedureCallhelperBuildQueryBodyForProcedureCalllogic across sync, async, and read-only procedure callsCallProcedureAsyncmethod signature to accept optionalargsandkwargsmatching theCallProceduremethodargsquoting andkwargsprocessingTesting
Added tests:
Summary by CodeRabbit
New Features
Tests