fix: support multiple values for StringArray flags via env vars#679
fix: support multiple values for StringArray flags via env vars#679
Conversation
Env vars for StringArray flags (e.g. FGA_CUSTOM_HEADERS, FGA_API_SCOPES) now support multiple values using JSON array syntax, e.g. FGA_CUSTOM_HEADERS='["X-Foo: bar","X-Baz: qux"]'. Also handles YAML list values from config files by iterating slice values individually. Closes #678
WalkthroughUpdated Viper configuration binding to handle multiple values from environment variables and config files. A new helper function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
| func viperValueToStrings(value any) []string { | ||
| reflectValue := reflect.ValueOf(value) | ||
|
|
||
| if reflectValue.Kind() == reflect.Slice || reflectValue.Kind() == reflect.Array { | ||
| result := make([]string, 0, reflectValue.Len()) | ||
| for i := range reflectValue.Len() { | ||
| result = append(result, fmt.Sprintf("%v", reflectValue.Index(i).Interface())) | ||
| } | ||
|
|
||
| return result | ||
| } | ||
|
|
||
| str := fmt.Sprintf("%v", value) | ||
| if strings.HasPrefix(str, "[") && strings.HasSuffix(str, "]") { | ||
| var parsed []string | ||
| if err := json.Unmarshal([]byte(str), &parsed); err == nil { | ||
| return parsed | ||
| } | ||
| } | ||
|
|
||
| return []string{str} | ||
| } |
There was a problem hiding this comment.
Instead of using this function with viperInstance.Get(configName), why wouldn't we just use viperInstance.GetStringSlice(configName) and call it a day?
There was a problem hiding this comment.
See my comment here as well: #670 (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: 1
🧹 Nitpick comments (2)
internal/cmdutils/bind-viper-to-flags_test.go (1)
28-88: Consider a couple of additional edge-case cases.Good coverage overall. A few worth adding:
- Empty JSON-array string
"[]"→ expect[]string{}(documents intended behavior and guards against regressions).nilinput (defensive —reflect.ValueOf(nil).Kind()isInvalid, falls through tofmt.Sprintf("%v", nil)→"<nil>"; confirming/asserting this behavior prevents surprise).- A JSON array containing non-string elements (e.g.
"[1,2]") to document that it falls back to the scalar path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmdutils/bind-viper-to-flags_test.go` around lines 28 - 88, Add three edge-case entries to the testcases slice in bind-viper-to-flags_test.go: one where value is the JSON empty-array string "[]" with expected empty []string{}, one where value is nil with expected []string{"<nil>"} (to lock in current fmt.Sprintf behavior), and one where value is a JSON array of non-strings like "[1,2]" with expected []string{"[1,2]"} (confirming it falls back to scalar). Locate and append these cases in the existing testcases variable so they run with the other table-driven tests.internal/cmdutils/bind-viper-to-flags.go (1)
66-72: Nit: consider trimming whitespace before the bracket check.A user-provided env var value with surrounding whitespace (e.g.
' ["a","b"] ') would bypass the JSON-array path. Optional hardening:♻️ Proposed refinement
- str := fmt.Sprintf("%v", value) - if strings.HasPrefix(str, "[") && strings.HasSuffix(str, "]") { + str := fmt.Sprintf("%v", value) + trimmed := strings.TrimSpace(str) + if strings.HasPrefix(trimmed, "[") && strings.HasSuffix(trimmed, "]") { var parsed []string - if err := json.Unmarshal([]byte(str), &parsed); err == nil { + if err := json.Unmarshal([]byte(trimmed), &parsed); err == nil { return parsed } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmdutils/bind-viper-to-flags.go` around lines 66 - 72, The code currently converts the value to str and checks strings.HasPrefix/HasSuffix for "["/"]" without trimming, so inputs with surrounding whitespace (e.g. ' ["a","b"] ') are missed; update the logic in the block that creates str (the local variable named str) to first run strings.TrimSpace(str) (and use the trimmed value for the bracket checks and the json.Unmarshal call that populates parsed) so the HasPrefix/HasSuffix checks and the JSON unmarshalling handle padded input correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cmdutils/bind-viper-to-flags.go`:
- Around line 54-75: viperValueToStrings currently treats any value that looks
like a JSON array as a string slice, which causes scalar flags to be split when
supplied as env vars; restrict JSON-array parsing to only flag types that are
actual slices by checking the pflag Value implementation or name before
splitting: in the code paths that call viperValueToStrings (and inside
viperValueToStrings itself), detect whether the target flag's pflag.Value
implements pflag.SliceValue or has a type/name like "stringArray"/"stringSlice"
and only then attempt the json.Unmarshal branch; for all other flag types (plain
String, Bool, Int, etc.) return the scalar string unmodified so Set is called
once as before (reference viperValueToStrings, pflag.SliceValue, and
pflag.Value/Set).
---
Nitpick comments:
In `@internal/cmdutils/bind-viper-to-flags_test.go`:
- Around line 28-88: Add three edge-case entries to the testcases slice in
bind-viper-to-flags_test.go: one where value is the JSON empty-array string "[]"
with expected empty []string{}, one where value is nil with expected
[]string{"<nil>"} (to lock in current fmt.Sprintf behavior), and one where value
is a JSON array of non-strings like "[1,2]" with expected []string{"[1,2]"}
(confirming it falls back to scalar). Locate and append these cases in the
existing testcases variable so they run with the other table-driven tests.
In `@internal/cmdutils/bind-viper-to-flags.go`:
- Around line 66-72: The code currently converts the value to str and checks
strings.HasPrefix/HasSuffix for "["/"]" without trimming, so inputs with
surrounding whitespace (e.g. ' ["a","b"] ') are missed; update the logic in the
block that creates str (the local variable named str) to first run
strings.TrimSpace(str) (and use the trimmed value for the bracket checks and the
json.Unmarshal call that populates parsed) so the HasPrefix/HasSuffix checks and
the JSON unmarshalling handle padded input correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38fdc7c0-5c3c-4675-b847-b9c5ef7bdc71
📒 Files selected for processing (2)
internal/cmdutils/bind-viper-to-flags.gointernal/cmdutils/bind-viper-to-flags_test.go
| func viperValueToStrings(value any) []string { | ||
| reflectValue := reflect.ValueOf(value) | ||
|
|
||
| if reflectValue.Kind() == reflect.Slice || reflectValue.Kind() == reflect.Array { | ||
| result := make([]string, 0, reflectValue.Len()) | ||
| for i := range reflectValue.Len() { | ||
| result = append(result, fmt.Sprintf("%v", reflectValue.Index(i).Interface())) | ||
| } | ||
|
|
||
| return result | ||
| } | ||
|
|
||
| str := fmt.Sprintf("%v", value) | ||
| if strings.HasPrefix(str, "[") && strings.HasSuffix(str, "]") { | ||
| var parsed []string | ||
| if err := json.Unmarshal([]byte(str), &parsed); err == nil { | ||
| return parsed | ||
| } | ||
| } | ||
|
|
||
| return []string{str} | ||
| } |
There was a problem hiding this comment.
JSON-array parsing is applied to every flag type, not just StringArray.
viperValueToStrings is invoked for all flags (String, Bool, Int, StringArray, etc.). If a scalar String flag is supplied via env var with a value that happens to start with [ and end with ] and also parses as a JSON string array (e.g. ["foo"]), it will now be split and Set called once per element. For non-array flags, the last value wins, which silently changes the effective value compared to prior behavior.
Impact is likely low given typical inputs, but consider gating the JSON-array path on the flag type (e.g. only when the pflag Value implements SliceValue or has type stringArray/stringSlice) to avoid surprising scalar-flag consumers. At minimum, it may be worth calling this out in docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cmdutils/bind-viper-to-flags.go` around lines 54 - 75,
viperValueToStrings currently treats any value that looks like a JSON array as a
string slice, which causes scalar flags to be split when supplied as env vars;
restrict JSON-array parsing to only flag types that are actual slices by
checking the pflag Value implementation or name before splitting: in the code
paths that call viperValueToStrings (and inside viperValueToStrings itself),
detect whether the target flag's pflag.Value implements pflag.SliceValue or has
a type/name like "stringArray"/"stringSlice" and only then attempt the
json.Unmarshal branch; for all other flag types (plain String, Bool, Int, etc.)
return the scalar string unmodified so Set is called once as before (reference
viperValueToStrings, pflag.SliceValue, and pflag.Value/Set).
There was a problem hiding this comment.
Pull request overview
This PR improves how Viper-provided config values are bound into Cobra/PFlag flags so StringArray flags can be populated with multiple entries from (a) YAML list config values and (b) environment variables using JSON array syntax (issue #678).
Changes:
- Iterate slice/array config values and call
flag.Set(...)once per element instead of stringifying the entire slice. - Add JSON-array parsing for scalar string env var values that look like
[...]to support multiple entries. - Add unit tests for the new
viperValueToStringshelper.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/cmdutils/bind-viper-to-flags.go | Introduces viperValueToStrings and uses it when binding Viper values into flags, enabling multi-value StringArray behavior. |
| internal/cmdutils/bind-viper-to-flags_test.go | Adds unit tests validating conversion/parsing behavior for scalar, slice, and JSON-array-like values. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func viperValueToStrings(value any) []string { | ||
| reflectValue := reflect.ValueOf(value) |
There was a problem hiding this comment.
viperValueToStrings will panic if value is nil because reflect.ValueOf(nil) returns an invalid reflect.Value and calling Kind() on it panics. Viper can return nil for explicitly-null config values; consider guarding value == nil / !reflectValue.IsValid() and returning an empty slice (so no Flags().Set calls) or otherwise handling it explicitly.
| func viperValueToStrings(value any) []string { | |
| reflectValue := reflect.ValueOf(value) | |
| func viperValueToStrings(value any) []string { | |
| if value == nil { | |
| return []string{} | |
| } | |
| reflectValue := reflect.ValueOf(value) | |
| if !reflectValue.IsValid() { | |
| return []string{} | |
| } |
| // pflag.Set calls. Slice values (e.g. from YAML lists) produce one string per | ||
| // element. Scalar strings that look like a JSON array (start with "[" and end | ||
| // with "]") are parsed as JSON to support multiple values via env vars, e.g. | ||
| // FGA_CUSTOM_HEADERS='["X-Foo: bar","X-Baz: qux"]'. Other scalars produce a |
There was a problem hiding this comment.
The doc comment example references FGA_CUSTOM_HEADERS, but there doesn’t appear to be a custom-headers flag/env binding in this repo (only api-scopes, contextual-tuple, etc.). Using a non-existent flag/env var in the example is misleading; please update the example to a flag that actually exists (e.g. FGA_API_SCOPES).
| // FGA_CUSTOM_HEADERS='["X-Foo: bar","X-Baz: qux"]'. Other scalars produce a | |
| // FGA_API_SCOPES='["scope1","scope2"]'. Other scalars produce a |
| expected: []string{"42"}, | ||
| }, | ||
| } | ||
|
|
There was a problem hiding this comment.
Tests cover several value shapes, but there’s no case for a nil value. Since viperValueToStrings(nil) currently panics (due to reflect.ValueOf(nil).Kind()), adding an explicit nil testcase would help prevent regressions once nil handling is added.
| t.Run("nil value panics", func(t *testing.T) { | |
| t.Parallel() | |
| assert.Panics(t, func() { | |
| _ = viperValueToStrings(nil) | |
| }) | |
| }) |
Env vars for StringArray flags (e.g. FGA_CUSTOM_HEADERS, FGA_API_SCOPES) now support multiple values using JSON array syntax, e.g. FGA_CUSTOM_HEADERS='["X-Foo: bar","X-Baz: qux"]'. Also handles YAML list values from config files by iterating slice values individually.
Closes #678
Description
What problem is being solved?
How is it being solved?
What changes are made to solve it?
References
Review Checklist
mainSummary by CodeRabbit
New Features
Tests