Skip to content

fix: support multiple values for StringArray flags via env vars #679

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

Open
emilic wants to merge 1 commit into
base: main
Choose a base branch
from
+133 −2
Open

fix: support multiple values for StringArray flags via env vars #679

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
37 changes: 35 additions & 2 deletions internal/cmdutils/bind-viper-to-flags.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ limitations under the License.
package cmdutils

import (
"encoding/json"
"fmt"
"reflect"
"strings"

"github.com/spf13/cobra"
"github.com/spf13/pflag"
Expand All @@ -31,12 +34,42 @@ func BindViperToFlags(cmd *cobra.Command, viperInstance *viper.Viper) {

if !flag.Changed && viperInstance.IsSet(configName) {
value := viperInstance.Get(configName)
err := cmd.Flags().Set(flag.Name, fmt.Sprintf("%v", value))
cobra.CheckErr(err)
for _, strVal := range viperValueToStrings(value) {
cobra.CheckErr(cmd.Flags().Set(flag.Name, strVal))
}
}
})

for _, subcmd := range cmd.Commands() {
BindViperToFlags(subcmd, viperInstance)
}
}

// viperValueToStrings converts a viper config value to a slice of strings for
// 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
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

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).

Suggested change
// FGA_CUSTOM_HEADERS='["X-Foo: bar","X-Baz: qux"]'. Other scalars produce a
// FGA_API_SCOPES='["scope1","scope2"]'. Other scalars produce a

Copilot uses AI. Check for mistakes.
// single-element slice.
func viperValueToStrings(value any) []string {
reflectValue := reflect.ValueOf(value)
Comment on lines +54 to +55
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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{}
}

Copilot uses AI. Check for mistakes.

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}
}
Comment on lines +54 to +75
Copy link
Copy Markdown

@senojj senojj Apr 17, 2026

Choose a reason for hiding this comment

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

Instead of using this function with viperInstance.Get(configName), why wouldn't we just use viperInstance.GetStringSlice(configName) and call it a day?

Copy link
Copy Markdown

@senojj senojj Apr 17, 2026

Choose a reason for hiding this comment

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

See my comment here as well: #670 (comment)

Comment on lines +54 to +75
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Apr 17, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

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).

98 changes: 98 additions & 0 deletions internal/cmdutils/bind-viper-to-flags_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
/*
Copyright © 2023 OpenFGA

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package cmdutils

import (
"testing"

"github.com/stretchr/testify/assert"
)

func TestViperValueToStrings(t *testing.T) {
t.Parallel()

testcases := []struct {
name string
value any
expected []string
}{
{
name: "scalar string produces single-element slice",
value: "X-Custom-Header: value1",
expected: []string{"X-Custom-Header: value1"},
},
{
name: "typed string slice produces one string per element",
value: []string{"X-Custom-Header: value1", "X-Request-ID: abc123"},
expected: []string{"X-Custom-Header: value1", "X-Request-ID: abc123"},
},
{
name: "any slice produces one string per element",
value: []any{"X-Custom-Header: value1", "X-Request-ID: abc123"},
expected: []string{"X-Custom-Header: value1", "X-Request-ID: abc123"},
},
{
name: "empty slice",
value: []any{},
expected: []string{},
},
{
name: "JSON array string is parsed into multiple entries",
value: `["X-Foo: bar","X-Baz: qux"]`,
expected: []string{"X-Foo: bar", "X-Baz: qux"},
},
{
name: "JSON array with single element",
value: `["X-Foo: bar"]`,
expected: []string{"X-Foo: bar"},
},
{
name: "string starting with [ but not ending with ] is treated as scalar",
value: "[not-an-array",
expected: []string{"[not-an-array"},
},
{
name: "string ending with ] but not starting with [ is treated as scalar",
value: "not-an-array]",
expected: []string{"not-an-array]"},
},
{
name: "invalid JSON array is treated as scalar",
value: "[not valid json]",
expected: []string{"[not valid json]"},
},
{
name: "boolean value is stringified",
value: true,
expected: []string{"true"},
},
{
name: "integer value is stringified",
value: 42,
expected: []string{"42"},
},
}

Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
t.Run("nil value panics", func(t *testing.T) {
t.Parallel()
assert.Panics(t, func() {
_ = viperValueToStrings(nil)
})
})

Copilot uses AI. Check for mistakes.
for _, test := range testcases {
t.Run(test.name, func(t *testing.T) {
t.Parallel()

result := viperValueToStrings(test.value)
assert.Equal(t, test.expected, result)
})
}
}
Loading