browser-flag: wrapped bool type denotes browser on/off flag. (#3963)

Statically typed BrowserFlag prevents any arbitrary string value
usage. The wrapped bool marshals/unmarshals JSON according to the
typed value ie string value "on" represents boolean true and "off" as
boolean false.

cmd/browser-flag.go

@@ -0,0 +1,68 @@
+/*
+ * Minio Cloud Storage, (C) 2017 Minio, Inc.
+ *
+ * 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 cmd
+
+import (
+	"encoding/json"
+	"fmt"
+)
+
+// BrowserFlag - wrapper bool type.
+type BrowserFlag bool
+
+// String - returns string of BrowserFlag.
+func (bf BrowserFlag) String() string {
+	if bf {
+		return "on"
+	}
+
+	return "off"
+}
+
+// MarshalJSON - converts BrowserFlag into JSON data.
+func (bf BrowserFlag) MarshalJSON() ([]byte, error) {
+	return json.Marshal(bf.String())
+}
+
+// UnmarshalJSON - parses given data into BrowserFlag.
+func (bf *BrowserFlag) UnmarshalJSON(data []byte) (err error) {
+	var s string
+	if err = json.Unmarshal(data, &s); err == nil {
+		b := BrowserFlag(true)
+		if s == "" {
+			// Empty string is treated as valid.
+			*bf = b
+		} else if b, err = ParseBrowserFlag(s); err == nil {
+			*bf = b
+		}
+	}
+
+	return err
+}
+
+// ParseBrowserFlag - parses string into BrowserFlag.
+func ParseBrowserFlag(s string) (bf BrowserFlag, err error) {
+	if s == "on" {
+		bf = true
+	} else if s == "off" {
+		bf = false
+	} else {
+		err = fmt.Errorf("invalid value ‘%s’ for BrowserFlag", s)
+	}
+
+	return bf, err
+}

cmd/browser-flag_test.go

@@ -0,0 +1,137 @@
+/*
+ * Minio Cloud Storage, (C) 2017 Minio, Inc.
+ *
+ * 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 cmd
+
+import (
+	"errors"
+	"testing"
+)
+
+// Test BrowserFlag.String()
+func TestBrowserFlagString(t *testing.T) {
+	var bf BrowserFlag
+
+	testCases := []struct {
+		flag           BrowserFlag
+		expectedResult string
+	}{
+		{bf, "off"},
+		{BrowserFlag(true), "on"},
+		{BrowserFlag(false), "off"},
+	}
+
+	for _, testCase := range testCases {
+		str := testCase.flag.String()
+		if testCase.expectedResult != str {
+			t.Fatalf("expected: %v, got: %v", testCase.expectedResult, str)
+		}
+	}
+}
+
+// Test BrowserFlag.MarshalJSON()
+func TestBrowserFlagMarshalJSON(t *testing.T) {
+	var bf BrowserFlag
+
+	testCases := []struct {
+		flag           BrowserFlag
+		expectedResult string
+	}{
+		{bf, `"off"`},
+		{BrowserFlag(true), `"on"`},
+		{BrowserFlag(false), `"off"`},
+	}
+
+	for _, testCase := range testCases {
+		data, _ := testCase.flag.MarshalJSON()
+		if testCase.expectedResult != string(data) {
+			t.Fatalf("expected: %v, got: %v", testCase.expectedResult, string(data))
+		}
+	}
+}
+
+// Test BrowserFlag.UnmarshalJSON()
+func TestBrowserFlagUnmarshalJSON(t *testing.T) {
+	testCases := []struct {
+		data           []byte
+		expectedResult BrowserFlag
+		expectedErr    error
+	}{
+		{[]byte(`{}`), BrowserFlag(false), errors.New("json: cannot unmarshal object into Go value of type string")},
+		{[]byte(`["on"]`), BrowserFlag(false), errors.New("json: cannot unmarshal array into Go value of type string")},
+		{[]byte(`"junk"`), BrowserFlag(false), errors.New("invalid value ‘junk’ for BrowserFlag")},
+		{[]byte(`"true"`), BrowserFlag(false), errors.New("invalid value ‘true’ for BrowserFlag")},
+		{[]byte(`"false"`), BrowserFlag(false), errors.New("invalid value ‘false’ for BrowserFlag")},
+		{[]byte(`"ON"`), BrowserFlag(false), errors.New("invalid value ‘ON’ for BrowserFlag")},
+		{[]byte(`"OFF"`), BrowserFlag(false), errors.New("invalid value ‘OFF’ for BrowserFlag")},
+		{[]byte(`""`), BrowserFlag(true), nil},
+		{[]byte(`"on"`), BrowserFlag(true), nil},
+		{[]byte(`"off"`), BrowserFlag(false), nil},
+	}
+
+	for _, testCase := range testCases {
+		var flag BrowserFlag
+		err := (&flag).UnmarshalJSON(testCase.data)
+		if testCase.expectedErr == nil {
+			if err != nil {
+				t.Fatalf("error: expected = <nil>, got = %v", err)
+			}
+		} else if err == nil {
+			t.Fatalf("error: expected = %v, got = <nil>", testCase.expectedErr)
+		} else if testCase.expectedErr.Error() != err.Error() {
+			t.Fatalf("error: expected = %v, got = %v", testCase.expectedErr, err)
+		}
+
+		if err == nil && testCase.expectedResult != flag {
+			t.Fatalf("result: expected: %v, got: %v", testCase.expectedResult, flag)
+		}
+	}
+}
+
+// Test ParseBrowserFlag()
+func TestParseBrowserFlag(t *testing.T) {
+	testCases := []struct {
+		flagStr        string
+		expectedResult BrowserFlag
+		expectedErr    error
+	}{
+		{"", BrowserFlag(false), errors.New("invalid value ‘’ for BrowserFlag")},
+		{"junk", BrowserFlag(false), errors.New("invalid value ‘junk’ for BrowserFlag")},
+		{"true", BrowserFlag(false), errors.New("invalid value ‘true’ for BrowserFlag")},
+		{"false", BrowserFlag(false), errors.New("invalid value ‘false’ for BrowserFlag")},
+		{"ON", BrowserFlag(false), errors.New("invalid value ‘ON’ for BrowserFlag")},
+		{"OFF", BrowserFlag(false), errors.New("invalid value ‘OFF’ for BrowserFlag")},
+		{"on", BrowserFlag(true), nil},
+		{"off", BrowserFlag(false), nil},
+	}
+
+	for _, testCase := range testCases {
+		bf, err := ParseBrowserFlag(testCase.flagStr)
+		if testCase.expectedErr == nil {
+			if err != nil {
+				t.Fatalf("error: expected = <nil>, got = %v", err)
+			}
+		} else if err == nil {
+			t.Fatalf("error: expected = %v, got = <nil>", testCase.expectedErr)
+		} else if testCase.expectedErr.Error() != err.Error() {
+			t.Fatalf("error: expected = %v, got = %v", testCase.expectedErr, err)
+		}
+
+		if err == nil && testCase.expectedResult != bf {
+			t.Fatalf("result: expected: %v, got: %v", testCase.expectedResult, bf)
+		}
+	}
+}

cmd/config-migrate.go

@@ -886,7 +886,7 @@ func migrateV13ToV14() error {
 	}
 
 	// Set the new browser parameter to true by default
-	srvConfig.Browser = "on"
+	srvConfig.Browser = true
 
 	if err = quick.Save(configFile, srvConfig); err != nil {
 		return fmt.Errorf("Failed to migrate config from ‘%s’ to ‘%s’. %v", cv13.Version, srvConfig.Version, err)

cmd/config-old.go

@@ -388,7 +388,7 @@ type serverConfigV14 struct {
 	// S3 API configuration.
 	Credential credential  `json:"credential"`
 	Region     string      `json:"region"`
-	Browser    string     `json:"browser"`
+	Browser    BrowserFlag `json:"browser"`
 
 	// Additional error logging configuration.
 	Logger *loggerV7 `json:"logger"`
@@ -405,7 +405,7 @@ type serverConfigV15 struct {
 	// S3 API configuration.
 	Credential credential  `json:"credential"`
 	Region     string      `json:"region"`
-	Browser    string     `json:"browser"`
+	Browser    BrowserFlag `json:"browser"`
 
 	// Additional error logging configuration.
 	Logger *loggerV7 `json:"logger"`

cmd/config-v16.go

@@ -20,7 +20,6 @@ import (
 	"errors"
 	"fmt"
 	"io/ioutil"
-	"strings"
 	"sync"
 
 	"github.com/minio/minio/pkg/quick"
@@ -30,7 +29,7 @@ import (
 // Read Write mutex for safe access to ServerConfig.
 var serverConfigMu sync.RWMutex
 
-var v16 = "16"
+const v16 = "16"
 
 // serverConfigV16 server configuration version '16' which is like
 // version '15' except it removes log level field and renames `fileName`
@@ -41,7 +40,7 @@ type serverConfigV16 struct {
 	// S3 API configuration.
 	Credential credential  `json:"credential"`
 	Region     string      `json:"region"`
-	Browser    string     `json:"browser"`
+	Browser    BrowserFlag `json:"browser"`
 
 	// Additional error logging configuration.
 	Logger *loggers `json:"logger"`
@@ -53,12 +52,13 @@ type serverConfigV16 struct {
 func newServerConfigV16() *serverConfigV16 {
 	srvCfg := &serverConfigV16{
 		Version:    v16,
+		Credential: mustGetNewCredential(),
 		Region:     globalMinioDefaultRegion,
+		Browser:    true,
 		Logger:     &loggers{},
 		Notify:     &notifier{},
 	}
-	srvCfg.SetCredential(mustGetNewCredential())
+
-	srvCfg.SetBrowser("on")
 	// Enable console logger by default on a fresh run.
 	srvCfg.Logger.Console = NewConsoleLogger()
 
@@ -117,10 +117,12 @@ func newConfig(envParams envParams) error {
 // loadConfig - loads a new config from disk, overrides params from env
 // if found and valid
 func loadConfig(envParams envParams) error {
-	configFile := getConfigFile()
+	srvCfg := &serverConfigV16{
+		Region:  globalMinioDefaultRegion,
+		Browser: true,
+	}
 
-	srvCfg := &serverConfigV16{}
+	if _, err := quick.Load(getConfigFile(), srvCfg); err != nil {
-	if _, err := quick.Load(configFile, srvCfg); err != nil {
 		return err
 	}
 	if srvCfg.Version != v16 {
@@ -136,7 +138,7 @@ func loadConfig(envParams envParams) error {
 		srvCfg.SetBrowser(envParams.browser)
 	}
 
-	if strings.ToLower(srvCfg.GetBrowser()) == "off" {
+	if !srvCfg.GetBrowser() {
 		globalIsBrowserEnabled = false
 	}
 
@@ -201,9 +203,12 @@ func checkDupJSONKeys(json string) error {
 
 // validateConfig checks for
 func validateConfig() error {
-	configFile := getConfigFile()
+	srvCfg := &serverConfigV16{
+		Region:  globalMinioDefaultRegion,
+		Browser: true,
+	}
 
-	srvCfg := &serverConfigV16{}
+	configFile := getConfigFile()
 	if _, err := quick.Load(configFile, srvCfg); err != nil {
 		return err
 	}
@@ -227,11 +232,6 @@ func validateConfig() error {
 		return errors.New("Region config value cannot be empty")
 	}
 
-	// Validate browser field
-	if b := strings.ToLower(srvCfg.GetBrowser()); b != "on" && b != "off" {
-		return fmt.Errorf("Browser config value %s is invalid", b)
-	}
-
 	// Validate credential fields only when
 	// they are not set via the environment
 	if !globalIsEnvCreds {
@@ -307,30 +307,20 @@ func (s serverConfigV16) GetCredential() credential {
 }
 
 // SetBrowser set if browser is enabled.
-func (s *serverConfigV16) SetBrowser(v string) {
+func (s *serverConfigV16) SetBrowser(b BrowserFlag) {
 	serverConfigMu.Lock()
 	defer serverConfigMu.Unlock()
 
-	// Set browser param
-	if v == "" {
-		v = "on" // Browser is on by default.
-	}
-
 	// Set the new value.
-	s.Browser = v
+	s.Browser = b
 }
 
 // GetCredentials get current credentials.
-func (s serverConfigV16) GetBrowser() string {
+func (s serverConfigV16) GetBrowser() BrowserFlag {
 	serverConfigMu.RLock()
 	defer serverConfigMu.RUnlock()
 
-	if s.Browser != "" {
 	return s.Browser
-	} // empty browser.
-
-	// Empty browser means "on" by default.
-	return "on"
 }
 
 // Save config.

cmd/config-v16_test.go

@@ -159,7 +159,7 @@ func TestServerConfigWithEnvs(t *testing.T) {
 	defer removeAll(rootPath)
 
 	// Check if serverConfig has
-	if serverConfig.GetBrowser() != "off" {
+	if serverConfig.GetBrowser() {
 		t.Errorf("Expecting browser `off` found %s", serverConfig.GetBrowser())
 	}
 

cmd/server-main.go

@@ -96,7 +96,7 @@ func checkUpdate(mode string) {
 // envParams holds all env parameters
 type envParams struct {
 	creds   credential
-	browser string
+	browser BrowserFlag
 }
 
 func migrate() {
@@ -139,20 +139,21 @@ func initConfig() {
 		globalIsEnvCreds = true
 	}
 
-	browser := os.Getenv("MINIO_BROWSER")
+	envs := envParams{
-	if browser != "" {
+		creds:   cred,
-		if !(strings.EqualFold(browser, "off") || strings.EqualFold(browser, "on")) {
+		browser: BrowserFlag(true),
-			fatalIf(errors.New("invalid value"), "‘%s’ in MINIO_BROWSER environment variable.", browser)
 	}
 
-		// browser Envs are set globally, this doesn't represent
+	if browser := os.Getenv("MINIO_BROWSER"); browser != "" {
-		// if browser is turned off or on.
+		browserFlag, err := ParseBrowserFlag(browser)
-		globalIsEnvBrowser = true
+		if err != nil {
+			fatalIf(errors.New("invalid value"), "Unknown value ‘%s’ in MINIO_BROWSER environment variable.", browser)
 		}
 
-	envs := envParams{
+		// browser Envs are set globally, this does not represent
-		creds:   cred,
+		// if browser is turned off or on.
-		browser: browser,
+		globalIsEnvBrowser = true
+		envs.browser = browserFlag
 	}
 
 	// Config file does not exist, we create it fresh and return upon success.