fix(dotnet): correct ArgumentException constructor usage#5954
Conversation
The five `string.IsNullOrEmpty` guards in PrqlCompiler.cs threw `new ArgumentException(nameof(prqlQuery))`, passing the parameter name into the message slot. The result was an ArgumentException with `Message == "prqlQuery"` and `ParamName == null` — the opposite of what the call looked like it intended. Replace each guard with `ArgumentException.ThrowIfNullOrEmpty(...)`, which matches the modern style of the adjacent `ArgumentNullException.ThrowIfNull(options)` calls and sets ParamName correctly. Add regression tests asserting ParamName on each entry point; these would have caught the bug because the previous code left ParamName null. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
prql-bot
left a comment
There was a problem hiding this comment.
The fix and tests look correct — ArgumentException.ThrowIfNullOrEmpty is the right idiom and the new tests assert ParamName, which is exactly what would have caught the original bug.
One thing worth flagging that affects the validation here, not the diff itself: test-dotnet is being skipped on this PR (and has been silently skipped on any dotnet-only PR for a long time). The tests.yaml path filter watches prqlc/bindings/prql-dotnet/**, but the actual directory was renamed to prqlc/bindings/dotnet/** in #3683 — so the filter never matches dotnet-only changes, and the test plan checkbox here can't actually be ticked in CI. Worth a separate one-line PR to update the filter; happy to file it.
|
Follow-up on my review: I can't actually file the path-filter PR — pushing dotnet:
- - prqlc/bindings/prql-dotnet/**
+ - prqlc/bindings/dotnet/**
- prqlc/bindings/prqlc-c/**
- .github/workflows/test-dotnet.yaml |
|
@prql-bot try again |
|
Push worked this time — the token now has |
Summary
IsNullOrEmptyguards inPrqlCompiler.csthrewnew ArgumentException(nameof(prqlQuery)), which passes the parameter name into the message slot of the single-stringArgumentException(string message)constructor. The thrown exception ended up withMessage == "prqlQuery"(the literal param name) andParamName == null— the opposite of what the calls looked like they intended.ArgumentException.ThrowIfNullOrEmpty(prqlQuery)to match the adjacentArgumentNullException.ThrowIfNull(options)style and setParamNamecorrectly.ParamNamefor each entry point; they would have caught the bug since the previous code leftParamNamenull.Found during the nightly file survey (the related
ArgumentNullException-vs-ArgumentExceptionmismatch was item #5 in #5847, but this specific message-vs-paramName confusion wasn't covered there).Test plan
test-dotnetjob builds and runs all 10 testsThrowsArgumentException_When*tests pass🤖 Generated with Claude Code