Skip to content

Warning for nonsense variables in config #7181

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

Merged
stephkyou merged 5 commits into from
Dec 19, 2023
Merged

Warning for nonsense variables in config #7181

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c54e6b7
restrict dolt config to accepted options
Dec 16, 2023
b19af8f
add check for nonsense variables in config
Dec 16, 2023
8c3f4c1
allow sqlserver prefix variables in dolt config
Dec 18, 2023
f91c9ab
remove nonsense variables from config tests
Dec 18, 2023
d06e827
PR comments
Dec 18, 2023
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
26 changes: 25 additions & 1 deletion go/cmd/dolt/commands/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,24 @@ const (
unsetOperationStr = "unset"
)

var ConfigOptions = map[string]string{
config.UserEmailKey: "",
config.UserNameKey: "",
config.UserCreds: "",
config.DoltEditor: "",
config.InitBranchName: "",
config.RemotesApiHostKey: "",
config.RemotesApiHostPortKey: "",
config.AddCredsUrlKey: "",
config.DoltLabInsecureKey: "",
config.MetricsDisabled: "",
config.MetricsHost: "",
config.MetricsPort: "",
config.MetricsInsecure: "",
config.PushAutoSetupRemote: "",
config.ProfileKey: "",
}

@fulghum fulghum Dec 18, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since anytime a new config value constant is added it'll need to be added to this map (i.e. high coupling), it's a good hint that these two pieces of code might benefit from being very close together. Try moving this map into the same file as where the config constants are defined.

There's another common Go trick for maps where you don't care about the values... you'll often see them written as:

var Foo = map[string]struct{}{
    "bar": {},
}

The advantage to that form is that it's unambiguous to future code readers (once you know this pattern at least) that the value of the map is ignored. If you use bool or string, then future code readers may think that the value has some meaning. It is also slightly more efficient for the runtime to store these, because there's an optimization where Go knows empty structs don't need to be allocated anywhere, as opposed to a bool or a string, which does require allocation, even if they are empty/false. It's a small efficiency gain, but worth knowing about, especially since you'll see this pattern fairly often.


var cfgDocs = cli.CommandDocumentationContent{
ShortDesc: `Get and set repository or global options`,
LongDesc: `You can query/set/replace/unset options with this command.
Expand Down Expand Up @@ -207,7 +225,13 @@ func addOperation(dEnv *env.DoltEnv, setCfgTypes *set.StrSet, args []string, usa

updates := make(map[string]string)
for i := 0; i < len(args); i += 2 {
updates[strings.ToLower(args[i])] = args[i+1]
option := strings.ToLower(args[i])
value := args[i+1]
if _, ok := ConfigOptions[option]; !ok && !strings.HasPrefix(option, env.SqlServerGlobalsPrefix) {
cli.Println("error: invalid config option, use dolt config --help to check valid configuration variables")
return 1
}
updates[option] = value
}

var cfgType string
Expand Down
60 changes: 30 additions & 30 deletions go/cmd/dolt/commands/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,25 +51,25 @@ func TestConfigAdd(t *testing.T) {
Name: "local",
CfgSet: localCfg,
Scope: env.LocalConfig,
Args: []string{"title", "senior dufus"},
Args: []string{"user.name", "senior dufus"},
},
{
Name: "global",
CfgSet: globalCfg,
Scope: env.GlobalConfig,
Args: []string{"title", "senior dufus"},
Args: []string{"user.name", "senior dufus"},
},
{
Name: "default",
CfgSet: &set.StrSet{},
Scope: env.LocalConfig,
Args: []string{"title", "senior dufus"},
Args: []string{"user.name", "senior dufus"},
},
{
Name: "multi error",
CfgSet: multiCfg,
Scope: env.LocalConfig,
Args: []string{"title", "senior dufus"},
Args: []string{"user.name", "senior dufus"},
Code: 1,
},
{
Expand All @@ -83,7 +83,7 @@ func TestConfigAdd(t *testing.T) {
Name: "odd args",
CfgSet: multiCfg,
Scope: env.LocalConfig,
Args: []string{"title"},
Args: []string{"user.name"},
Code: 1,
},
}
Expand All @@ -97,7 +97,7 @@ func TestConfigAdd(t *testing.T) {
assert.Equal(t, tt.Code, resCode)

} else if cfg, ok := dEnv.Config.GetConfig(tt.Scope); ok {
resVal := cfg.GetStringOrDefault("title", "")
resVal := cfg.GetStringOrDefault("user.name", "")
assert.Equal(t, "senior dufus", resVal)
} else {
t.Error("comparison config not found")
Expand Down Expand Up @@ -280,12 +280,12 @@ func TestConfig(t *testing.T) {

// test setting global config with --add
configCmd := ConfigCmd{}
ret := configCmd.Exec(ctx, "dolt config", []string{"-global", "--add", "name", "bheni"}, dEnv, nil)
ret += configCmd.Exec(ctx, "dolt config", []string{"-global", "--add", "title", "dufus"}, dEnv, nil)
ret := configCmd.Exec(ctx, "dolt config", []string{"-global", "--add", "user.name", "bheni"}, dEnv, nil)
ret += configCmd.Exec(ctx, "dolt config", []string{"-global", "--add", "user.email", "dufus@example.com"}, dEnv, nil)

expectedGlobal := map[string]string{
"name": "bheni",
"title": "dufus",
"user.name": "bheni",
"user.email": "dufus@example.com",
}

if ret != 0 {
Expand All @@ -295,11 +295,11 @@ func TestConfig(t *testing.T) {
}

// test setting global config with --set
ret = configCmd.Exec(ctx, "dolt config", []string{"-global", "--set", "name", "steph"}, dEnv, nil)
ret = configCmd.Exec(ctx, "dolt config", []string{"-global", "--set", "user.name", "steph"}, dEnv, nil)

expectedGlobal = map[string]string{
"name": "steph",
"title": "dufus",
"user.name": "steph",
"user.email": "dufus@example.com",
}

if ret != 0 {
Expand All @@ -309,40 +309,40 @@ func TestConfig(t *testing.T) {
}

// test setting local config with --add
ret = configCmd.Exec(ctx, "dolt config", []string{"-local", "--add", "title", "senior dufus"}, dEnv, nil)
ret = configCmd.Exec(ctx, "dolt config", []string{"-local", "--add", "user.name", "senior dufus"}, dEnv, nil)

expectedLocal := map[string]string{
"title": "senior dufus",
"user.name": "senior dufus",
}

if ret != 0 {
t.Error("Failed to set local config")
} else if cfg, ok := dEnv.Config.GetConfig(env.LocalConfig); !ok || !config.Equals(cfg, expectedLocal) {
t.Error("config -add did not yield expected local results")
} else if val, err := cfg.GetString("title"); err != nil || val != "senior dufus" {
t.Error("Unexpected value of \"title\" retrieved from the config hierarchy")
} else if val, err := cfg.GetString("user.name"); err != nil || val != "senior dufus" {
t.Error("Unexpected value of \"user.name\" retrieved from the config hierarchy")
}

// test setting local config with --set
ret = configCmd.Exec(ctx, "dolt config", []string{"-local", "--set", "name", "steph"}, dEnv, nil)
ret = configCmd.Exec(ctx, "dolt config", []string{"-local", "--set", "user.email", "dufus@example.com"}, dEnv, nil)

expectedLocal = map[string]string{
"name": "steph",
"title": "senior dufus",
"user.name": "senior dufus",
"user.email": "dufus@example.com",
}

if ret != 0 {
t.Error("Failed to set local config")
} else if cfg, ok := dEnv.Config.GetConfig(env.LocalConfig); !ok || !config.Equals(cfg, expectedLocal) {
t.Error("config -set did not yield expected local results")
} else if val, err := cfg.GetString("name"); err != nil || val != "steph" {
t.Error("Unexpected value of \"name\" retrieved from the config hierarchy")
} else if val, err := cfg.GetString("user.email"); err != nil || val != "dufus@example.com" {
t.Error("Unexpected value of \"user.email\" retrieved from the config hierarchy")
}

ret = configCmd.Exec(ctx, "dolt config", []string{"-global", "--unset", "name"}, dEnv, nil)
ret = configCmd.Exec(ctx, "dolt config", []string{"-global", "--unset", "user.name"}, dEnv, nil)

expectedGlobal = map[string]string{
"title": "dufus",
"user.email": "dufus@example.com",
}

if ret != 0 {
Expand All @@ -351,7 +351,7 @@ func TestConfig(t *testing.T) {
t.Error("config -add did not yield expected global results")
}

expectedGlobal = map[string]string{"title": "dufus"}
expectedGlobal = map[string]string{"user.email": "dufus@example.com"}
globalProperties := map[string]string{}
ret = listOperation(dEnv, globalCfg, []string{}, func() {}, func(k string, v string) {
globalProperties[k] = v
Expand All @@ -363,7 +363,7 @@ func TestConfig(t *testing.T) {
t.Error("listOperation did not yield expected global results")
}

expectedLocal = map[string]string{"name": "steph", "title": "senior dufus"}
expectedLocal = map[string]string{"user.name": "senior dufus", "user.email": "dufus@example.com"}
localProperties := map[string]string{}
ret = listOperation(dEnv, localCfg, []string{}, func() {}, func(k string, v string) {
localProperties[k] = v
Expand All @@ -375,8 +375,8 @@ func TestConfig(t *testing.T) {
t.Error("listOperation did not yield expected local results")
}

ret = getOperation(dEnv, globalCfg, []string{"title"}, func(k string, v *string) {
if v == nil || *v != "dufus" {
ret = getOperation(dEnv, globalCfg, []string{"user.email"}, func(k string, v *string) {
if v == nil || *v != "dufus@example.com" {
t.Error("Failed to get expected value for title.")
}
})
Expand All @@ -385,9 +385,9 @@ func TestConfig(t *testing.T) {
t.Error("get operation failed")
}

ret = getOperation(dEnv, globalCfg, []string{"name"}, func(k string, v *string) {
ret = getOperation(dEnv, globalCfg, []string{"user.name"}, func(k string, v *string) {
if v != nil {
t.Error("Failed to get expected value for \"name\" which should not be set in the config.")
t.Error("Failed to get expected value for \"user.name\" which should not be set in the config.")
}
})

Expand Down
19 changes: 19 additions & 0 deletions go/cmd/dolt/dolt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"os/exec"
"path/filepath"
"strconv"
"strings"
"time"

"github.com/dolthub/go-mysql-server/sql"
Expand Down Expand Up @@ -461,6 +462,24 @@ func runMain() int {
return 1
}

globalConfig.Iter(func(name, val string) (stop bool) {
if _, ok := commands.ConfigOptions[name]; !ok && !strings.HasPrefix(name, env.SqlServerGlobalsPrefix) {

@fulghum fulghum Dec 18, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

(minor) Might be worth running strings.ToLower on name here and for the local config section below. It does look like the current code always lowercases the names, but older versions may not have, plus people could always manually edit the config file.

cli.Println(color.YellowString("Warning: Unknown global config option '%s'. Use `dolt config --global --unset %s` to remove.", name, name))
}
return false
})

// try verifying contents of local config
localConfig, ok := dEnv.Config.GetConfig(env.LocalConfig)
if ok {
localConfig.Iter(func(name, val string) (stop bool) {
if _, ok := commands.ConfigOptions[name]; !ok && !strings.HasPrefix(name, env.SqlServerGlobalsPrefix) {
cli.Println(color.YellowString("Warning: Unknown local config option '%s'. Use `dolt config --local --unset %s` to remove.", name, name))
}
return false
})
}

apr, remainingArgs, subcommandName, err := parseGlobalArgsAndSubCommandName(globalConfig, args)
if err == argparser.ErrHelp {
doltCommand.PrintUsage("dolt")
Expand Down
2 changes: 2 additions & 0 deletions go/libraries/utils/config/keys.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,3 +41,5 @@ const MetricsPort = "metrics.port"
const MetricsInsecure = "metrics.insecure"

const PushAutoSetupRemote = "push.autosetupremote"

const ProfileKey = "profile"
28 changes: 14 additions & 14 deletions integration-tests/bats/config-home.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,41 +37,41 @@ teardown() {
HOME="$BATS_TMPDIR/config-test$$/homeA"

HOME=$HOME dolt config --global --add metrics.disabled true > /dev/null 2>&1
HOME=$HOME run dolt config --global --add test testA
HOME=$HOME run dolt config --global --add user.name "Your Name"
[ "$status" -eq 0 ]
[[ "$output" =~ "Config successfully updated" ]] || false
HOME=$HOME run dolt config --list
[ "$status" -eq 0 ]
[[ "$output" =~ "test = testA" ]] || false
HOME=$HOME run dolt config --get test
[[ "$output" =~ "user.name = Your Name" ]] || false
HOME=$HOME run dolt config --get user.name
[ "$status" -eq 0 ]
[ "$output" = "testA" ]
[ "$output" = "Your Name" ]

mkdir "$BATS_TMPDIR/config-test$$/homeB"
HOME="$BATS_TMPDIR/config-test$$/homeB"

HOME=$HOME dolt config --global --add metrics.disabled true > /dev/null 2>&1
HOME=$HOME run dolt config --global --add test testB
HOME=$HOME run dolt config --global --add core.editor foo
[ "$status" -eq 0 ]
[[ "$output" =~ "Config successfully updated" ]] || false
HOME=$HOME run dolt config --list
[ "$status" -eq 0 ]
[[ "$output" =~ "test = testB" ]] || false
[[ ! "$output" =~ "test = testA" ]] || false
HOME=$HOME run dolt config --get test
[[ "$output" =~ "core.editor = foo" ]] || false
[[ ! "$output" =~ "user.name = Your Name" ]] || false
HOME=$HOME run dolt config --get core.editor
[ "$status" -eq 0 ]
[ "$output" = "testB" ]
[ "$output" = "foo" ]

HOME="$BATS_TMPDIR/config-test$$/homeA"

HOME=$HOME run dolt config --global --add test testA
HOME=$HOME run dolt config --global --add user.email "you@example.com"
[ "$status" -eq 0 ]
[[ "$output" =~ "Config successfully updated" ]] || false
HOME=$HOME run dolt config --list
[ "$status" -eq 0 ]
[[ "$output" =~ "test = testA" ]] || false
[[ ! "$output" =~ "test = testB" ]] || false
HOME=$HOME run dolt config --get test
[[ "$output" =~ "user.email = you@example.com" ]] || false
[[ ! "$output" =~ "core.editor = foo" ]] || false
HOME=$HOME run dolt config --get user.email
[ "$status" -eq 0 ]
[ "$output" = "testA" ]
[ "$output" = "you@example.com" ]
}
Loading