-
Notifications
You must be signed in to change notification settings - Fork 83
Invalid handling of empty values in sqlcmd #708
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.
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
base: main
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree [company="Adaptive"] |
@microsoft-github-policy-service agree |
@microsoft-github-policy-service agree company="Adaptive" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Fixes a panic in the legacy cmd/sqlcmd argument-rewriting logic when a flag is followed by an empty string value (e.g., -d ""), ensuring sqlcmd can accept intentionally empty values without crashing.
Changes:
- Make flag detection safe for empty-string arguments by guarding index access and reusing
isFlagincheckDefaultValue. - Add a unit test case covering a flag with an explicit empty value (
-d "") to prevent regression.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/sqlcmd/sqlcmd.go | Prevents panics by making isFlag safe on empty strings and using it in checkDefaultValue when checking the next arg. |
| cmd/sqlcmd/sqlcmd_test.go | Adds a regression test ensuring empty flag values (e.g., -d "") are preserved and don’t crash conversion. |
| "flag with empty value", | ||
| []string{"-S", "server", "-U", "sa", "-d", "", "-Q", "SELECT 1", "-b"}, | ||
| []string{"-S", "server", "-U", "sa", "-d", "", "-Q", "SELECT 1", "-b"}, | ||
| }, |
Copilot
AI
Feb 9, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In TestConvertOsArgs, the argument order is semantically important for Cobra/pflag parsing, but the test currently uses assert.ElementsMatch (order-insensitive). This can allow regressions in convertOsArgs ordering to pass undetected; use an order-sensitive assertion (e.g., assert.Equal) for the converted args.
The built sqlcmd is throwing panic errors, for the empty values. For eg.
Throws:
After the fix:
name
pubs
I have validated for rest of the flags too.