Improve flag and environment config overrides (#1898)

* Separate overrides from config
* Don't allow changing overridden value
* Write default host and port to config file
* Use existing library value. Hide generated if set

pkg/api/resolver_mutation_configure.go

@@ -13,6 +13,8 @@ import (
 	"github.com/stashapp/stash/pkg/utils"
 )
 
+var ErrOverriddenConfig = errors.New("cannot set overridden value")
+
 func (r *mutationResolver) Setup(ctx context.Context, input models.SetupInput) (bool, error) {
 	err := manager.GetInstance().Setup(ctx, input)
 	return err == nil, err
@@ -25,6 +27,7 @@ func (r *mutationResolver) Migrate(ctx context.Context, input models.MigrateInpu
 
 func (r *mutationResolver) ConfigureGeneral(ctx context.Context, input models.ConfigGeneralInput) (*models.ConfigGeneralResult, error) {
 	c := config.GetInstance()
+
 	existingPaths := c.GetStashPaths()
 	if len(input.Stashes) > 0 {
 		for _, s := range input.Stashes {
@@ -46,7 +49,20 @@ func (r *mutationResolver) ConfigureGeneral(ctx context.Context, input models.Co
 		c.Set(config.Stash, input.Stashes)
 	}
 
-	if input.DatabasePath != nil {
+	checkConfigOverride := func(key string) error {
+		if c.HasOverride(key) {
+			return fmt.Errorf("%w: %s", ErrOverriddenConfig, key)
+		}
+
+		return nil
+	}
+
+	existingDBPath := c.GetDatabasePath()
+	if input.DatabasePath != nil && existingDBPath != *input.DatabasePath {
+		if err := checkConfigOverride(config.Database); err != nil {
+			return makeConfigGeneralResult(), err
+		}
+
 		ext := filepath.Ext(*input.DatabasePath)
 		if ext != ".db" && ext != ".sqlite" && ext != ".sqlite3" {
 			return makeConfigGeneralResult(), fmt.Errorf("invalid database path, use extension db, sqlite, or sqlite3")
@@ -54,14 +70,24 @@ func (r *mutationResolver) ConfigureGeneral(ctx context.Context, input models.Co
 		c.Set(config.Database, input.DatabasePath)
 	}
 
-	if input.GeneratedPath != nil {
+	existingGeneratedPath := c.GetGeneratedPath()
+	if input.GeneratedPath != nil && existingGeneratedPath != *input.GeneratedPath {
+		if err := checkConfigOverride(config.Generated); err != nil {
+			return makeConfigGeneralResult(), err
+		}
+
 		if err := utils.EnsureDir(*input.GeneratedPath); err != nil {
 			return makeConfigGeneralResult(), err
 		}
 		c.Set(config.Generated, input.GeneratedPath)
 	}
 
-	if input.MetadataPath != nil {
+	existingMetadataPath := c.GetMetadataPath()
+	if input.MetadataPath != nil && existingMetadataPath != *input.MetadataPath {
+		if err := checkConfigOverride(config.Metadata); err != nil {
+			return makeConfigGeneralResult(), err
+		}
+
 		if *input.MetadataPath != "" {
 			if err := utils.EnsureDir(*input.MetadataPath); err != nil {
 				return makeConfigGeneralResult(), err
@@ -70,7 +96,12 @@ func (r *mutationResolver) ConfigureGeneral(ctx context.Context, input models.Co
 		c.Set(config.Metadata, input.MetadataPath)
 	}
 
-	if input.CachePath != nil {
+	existingCachePath := c.GetCachePath()
+	if input.CachePath != nil && existingCachePath != *input.CachePath {
+		if err := checkConfigOverride(config.Metadata); err != nil {
+			return makeConfigGeneralResult(), err
+		}
+
 		if *input.CachePath != "" {
 			if err := utils.EnsureDir(*input.CachePath); err != nil {
 				return makeConfigGeneralResult(), err

pkg/api/resolver_query_configuration.go

@@ -64,7 +64,7 @@ func makeConfigGeneralResult() *models.ConfigGeneralResult {
 		DatabasePath:                 config.GetDatabasePath(),
 		GeneratedPath:                config.GetGeneratedPath(),
 		MetadataPath:                 config.GetMetadataPath(),
-		ConfigFilePath:               config.GetConfigFilePath(),
+		ConfigFilePath:               config.GetConfigFile(),
 		ScrapersPath:                 config.GetScrapersPath(),
 		CachePath:                    config.GetCachePath(),
 		CalculateMd5:                 config.IsCalculateMD5(),
@@ -108,7 +108,7 @@ func makeConfigInterfaceResult() *models.ConfigInterfaceResult {
 	soundOnPreview := config.GetSoundOnPreview()
 	wallShowTitle := config.GetWallShowTitle()
 	wallPlayback := config.GetWallPlayback()
-	noBrowser := config.GetNoBrowserFlag()
+	noBrowser := config.GetNoBrowser()
 	maximumLoopDuration := config.GetMaximumLoopDuration()
 	autostartVideo := config.GetAutostartVideo()
 	autostartVideoOnPlaySelected := config.GetAutostartVideoOnPlaySelected()

pkg/api/server.go

@@ -252,7 +252,7 @@ func Start(uiBox embed.FS, loginUIBox embed.FS) {
 
 		// This can be done before actually starting the server, as modern browsers will
 		// automatically reload the page if a local port is closed at page load and then opened.
-		if !c.GetNoBrowserFlag() && manager.GetInstance().IsDesktop() {
+		if !c.GetNoBrowser() && manager.GetInstance().IsDesktop() {
 			err = browser.OpenURL(displayAddress)
 			if err != nil {
 				logger.Error("Could not open browser: " + err.Error())

pkg/manager/config/config_concurrency_test.go

@@ -21,12 +21,15 @@ func TestConcurrentConfigAccess(t *testing.T) {
 				}
 
 				i.HasCredentials()
+				i.ValidateCredentials("", "")
 				i.GetCPUProfilePath()
 				i.GetConfigFile()
 				i.GetConfigPath()
 				i.GetDefaultDatabaseFilePath()
 				i.GetStashPaths()
-				i.GetConfigFilePath()
+				_ = i.ValidateStashBoxes(nil)
+				_ = i.Validate()
+				_ = i.ActivatePublicAccessTripwire("")
 				i.Set(Cache, i.GetCachePath())
 				i.Set(Generated, i.GetGeneratedPath())
 				i.Set(Metadata, i.GetMetadataPath())
@@ -94,6 +97,15 @@ func TestConcurrentConfigAccess(t *testing.T) {
 				i.Set(MaxUploadSize, i.GetMaxUploadSize())
 				i.Set(FunscriptOffset, i.GetFunscriptOffset())
 				i.Set(DefaultIdentifySettings, i.GetDefaultIdentifySettings())
+				i.Set(DeleteGeneratedDefault, i.GetDeleteGeneratedDefault())
+				i.Set(DeleteFileDefault, i.GetDeleteFileDefault())
+				i.Set(TrustedProxies, i.GetTrustedProxies())
+				i.Set(dangerousAllowPublicWithoutAuth, i.GetDangerousAllowPublicWithoutAuth())
+				i.Set(SecurityTripwireAccessedFromPublicInternet, i.GetSecurityTripwireAccessedFromPublicInternet())
+				i.Set(DisableDropdownCreatePerformer, i.GetDisableDropdownCreate().Performer)
+				i.Set(DisableDropdownCreateStudio, i.GetDisableDropdownCreate().Studio)
+				i.Set(DisableDropdownCreateTag, i.GetDisableDropdownCreate().Tag)
+				i.SetChecksumDefaultValues(i.GetVideoFileNamingAlgorithm(), i.IsCalculateMD5())
 				i.Set(AutostartVideoOnPlaySelected, i.GetAutostartVideoOnPlaySelected())
 				i.Set(ContinuePlaylistDefault, i.GetContinuePlaylistDefault())
 			}

pkg/manager/config/init.go

@@ -14,7 +14,10 @@ import (
 	"github.com/stashapp/stash/pkg/utils"
 )
 
-var once sync.Once
+var (
+	initOnce     sync.Once
+	instanceOnce sync.Once
+)
 
 type flagStruct struct {
 	configFilePath string
@@ -22,18 +25,29 @@ type flagStruct struct {
 	nobrowser      bool
 }
 
+func GetInstance() *Instance {
+	instanceOnce.Do(func() {
+		instance = &Instance{
+			main:      viper.New(),
+			overrides: viper.New(),
+		}
+	})
+	return instance
+}
+
 func Initialize() (*Instance, error) {
 	var err error
-	once.Do(func() {
+	initOnce.Do(func() {
 		flags := initFlags()
-		instance = &Instance{
+		overrides := makeOverrideConfig()
-			cpuProfilePath: flags.cpuProfilePath,
-		}
 
-		if err = initConfig(flags); err != nil {
+		_ = GetInstance()
+		instance.overrides = overrides
+		instance.cpuProfilePath = flags.cpuProfilePath
+
+		if err = initConfig(instance, flags); err != nil {
 			return
 		}
-		initEnvs()
 
 		if instance.isNewSystem {
 			if instance.Validate() == nil {
@@ -43,20 +57,23 @@ func Initialize() (*Instance, error) {
 		}
 
 		if !instance.isNewSystem {
-			setExistingSystemDefaults(instance)
+			err = instance.setExistingSystemDefaults()
+			if err == nil {
 				err = instance.SetInitialConfig()
 			}
+		}
 	})
 	return instance, err
 }
 
-func initConfig(flags flagStruct) error {
+func initConfig(instance *Instance, flags flagStruct) error {
+	v := instance.main
 
 	// The config file is called config.  Leave off the file extension.
-	viper.SetConfigName("config")
+	v.SetConfigName("config")
 
-	viper.AddConfigPath(".")            // Look for config in the working directory
+	v.AddConfigPath(".")            // Look for config in the working directory
-	viper.AddConfigPath("$HOME/.stash") // Look for the config in the home directory
+	v.AddConfigPath("$HOME/.stash") // Look for the config in the home directory
 
 	configFile := ""
 	envConfigFile := os.Getenv("STASH_CONFIG_FILE")
@@ -68,7 +85,7 @@ func initConfig(flags flagStruct) error {
 	}
 
 	if configFile != "" {
-		viper.SetConfigFile(configFile)
+		v.SetConfigFile(configFile)
 
 		// if file does not exist, assume it is a new system
 		if exists, _ := utils.FileExists(configFile); !exists {
@@ -86,7 +103,7 @@ func initConfig(flags flagStruct) error {
 		}
 	}
 
-	err := viper.ReadInConfig() // Find and read the config file
+	err := v.ReadInConfig() // Find and read the config file
 	// if not found, assume its a new system
 	var notFoundErr viper.ConfigFileNotFoundError
 	if errors.As(err, &notFoundErr) {
@@ -99,28 +116,6 @@ func initConfig(flags flagStruct) error {
 	return nil
 }
 
-// setExistingSystemDefaults sets config options that are new and unset in an existing install,
-// but should have a separate default than for brand-new systems, to maintain behavior.
-func setExistingSystemDefaults(instance *Instance) {
-	if !instance.isNewSystem {
-		configDirtied := false
-
-		// Existing systems as of the introduction of auto-browser open should retain existing
-		// behavior and not start the browser automatically.
-		if !viper.InConfig("nobrowser") {
-			configDirtied = true
-			viper.Set("nobrowser", "true")
-		}
-
-		if configDirtied {
-			err := viper.WriteConfig()
-			if err != nil {
-				logger.Errorf("Could not save existing system defaults: %s", err.Error())
-			}
-		}
-	}
-}
-
 func initFlags() flagStruct {
 	flags := flagStruct{}
 
@@ -131,30 +126,35 @@ func initFlags() flagStruct {
 	pflag.BoolVar(&flags.nobrowser, "nobrowser", false, "Don't open a browser window after launch")
 
 	pflag.Parse()
-	if err := viper.BindPFlags(pflag.CommandLine); err != nil {
-		logger.Infof("failed to bind flags: %s", err.Error())
-	}
 
 	return flags
 }
 
-func initEnvs() {
+func initEnvs(viper *viper.Viper) {
 	viper.SetEnvPrefix("stash")     // will be uppercased automatically
-	bindEnv("host")             // STASH_HOST
+	bindEnv(viper, "host")          // STASH_HOST
-	bindEnv("port")             // STASH_PORT
+	bindEnv(viper, "port")          // STASH_PORT
-	bindEnv("external_host")    // STASH_EXTERNAL_HOST
+	bindEnv(viper, "external_host") // STASH_EXTERNAL_HOST
-	bindEnv("generated")        // STASH_GENERATED
+	bindEnv(viper, "generated")     // STASH_GENERATED
-	bindEnv("metadata")         // STASH_METADATA
+	bindEnv(viper, "metadata")      // STASH_METADATA
-	bindEnv("cache")            // STASH_CACHE
+	bindEnv(viper, "cache")         // STASH_CACHE
-
+	bindEnv(viper, "stash")         // STASH_STASH
-	// only set stash config flag if not already set
-	if instance.GetStashPaths() == nil {
-		bindEnv("stash") // STASH_STASH
-	}
 }
 
-func bindEnv(key string) {
+func bindEnv(viper *viper.Viper, key string) {
 	if err := viper.BindEnv(key); err != nil {
 		panic(fmt.Sprintf("unable to set environment key (%v): %v", key, err))
 	}
 }
+
+func makeOverrideConfig() *viper.Viper {
+	viper := viper.New()
+
+	if err := viper.BindPFlags(pflag.CommandLine); err != nil {
+		logger.Infof("failed to bind flags: %s", err.Error())
+	}
+
+	initEnvs(viper)
+
+	return viper
+}

pkg/manager/manager.go

@@ -302,19 +302,15 @@ func setSetupDefaults(input *models.SetupInput) {
 
 func (s *singleton) Setup(ctx context.Context, input models.SetupInput) error {
 	setSetupDefaults(&input)
+	c := s.Config
 
 	// create the config directory if it does not exist
+	// don't do anything if config is already set in the environment
+	if !config.FileEnvSet() {
 		configDir := filepath.Dir(input.ConfigLocation)
 		if exists, _ := utils.DirExists(configDir); !exists {
 			if err := os.Mkdir(configDir, 0755); err != nil {
-			return fmt.Errorf("abc: %v", err)
+				return fmt.Errorf("error creating config directory: %v", err)
-		}
-	}
-
-	// create the generated directory if it does not exist
-	if exists, _ := utils.DirExists(input.GeneratedLocation); !exists {
-		if err := os.Mkdir(input.GeneratedLocation, 0755); err != nil {
-			return fmt.Errorf("error creating generated directory: %v", err)
 			}
 		}
 
@@ -323,10 +319,24 @@ func (s *singleton) Setup(ctx context.Context, input models.SetupInput) error {
 		}
 
 		s.Config.SetConfigFile(input.ConfigLocation)
+	}
+
+	// create the generated directory if it does not exist
+	if !c.HasOverride(config.Generated) {
+		if exists, _ := utils.DirExists(input.GeneratedLocation); !exists {
+			if err := os.Mkdir(input.GeneratedLocation, 0755); err != nil {
+				return fmt.Errorf("error creating generated directory: %v", err)
+			}
+		}
+
+		s.Config.Set(config.Generated, input.GeneratedLocation)
+	}
 
 	// set the configuration
-	s.Config.Set(config.Generated, input.GeneratedLocation)
+	if !c.HasOverride(config.Database) {
 		s.Config.Set(config.Database, input.DatabaseFile)
+	}
+
 	s.Config.Set(config.Stash, input.Stashes)
 	if err := s.Config.Write(); err != nil {
 		return fmt.Errorf("error writing configuration file: %v", err)

ui/v2.5/src/components/Setup/Setup.tsx

@@ -1,4 +1,4 @@
-import React, { useEffect, useState } from "react";
+import React, { useEffect, useState, useContext } from "react";
 import { FormattedMessage, useIntl } from "react-intl";
 import {
   Alert,
@@ -11,11 +11,16 @@ import {
 import * as GQL from "src/core/generated-graphql";
 import { mutateSetup, useSystemStatus } from "src/core/StashService";
 import { Link } from "react-router-dom";
+import { ConfigurationContext } from "src/hooks/Config";
 import StashConfiguration from "../Settings/StashConfiguration";
 import { Icon, LoadingIndicator } from "../Shared";
 import { FolderSelectDialog } from "../Shared/FolderSelect/FolderSelectDialog";
 
 export const Setup: React.FC = () => {
+  const { configuration, loading: configLoading } = useContext(
+    ConfigurationContext
+  );
+
   const [step, setStep] = useState(0);
   const [configLocation, setConfigLocation] = useState("");
   const [stashes, setStashes] = useState<GQL.StashConfig[]>([]);
@@ -36,6 +41,23 @@ export const Setup: React.FC = () => {
     }
   }, [systemStatus]);
 
+  useEffect(() => {
+    if (configuration) {
+      const { stashes: configStashes, generatedPath } = configuration.general;
+      if (configStashes.length > 0) {
+        setStashes(
+          configStashes.map((s) => {
+            const { __typename, ...withoutTypename } = s;
+            return withoutTypename;
+          })
+        );
+      }
+      if (generatedPath) {
+        setGeneratedLocation(generatedPath);
+      }
+    }
+  }, [configuration]);
+
   const discordLink = (
     <a href="https://discord.gg/2TsNFKt" target="_blank" rel="noreferrer">
       Discord
@@ -179,6 +201,47 @@ export const Setup: React.FC = () => {
     return <FolderSelectDialog onClose={onGeneratedClosed} />;
   }
 
+  function maybeRenderGenerated() {
+    if (!configuration?.general.generatedPath) {
+      return (
+        <Form.Group id="generated">
+          <h3>
+            <FormattedMessage id="setup.paths.where_can_stash_store_its_generated_content" />
+          </h3>
+          <p>
+            <FormattedMessage
+              id="setup.paths.where_can_stash_store_its_generated_content_description"
+              values={{
+                code: (chunks: string) => <code>{chunks}</code>,
+              }}
+            />
+          </p>
+          <InputGroup>
+            <Form.Control
+              className="text-input"
+              value={generatedLocation}
+              placeholder={intl.formatMessage({
+                id: "setup.paths.path_to_generated_directory_empty_for_default",
+              })}
+              onChange={(e: React.ChangeEvent<HTMLInputElement>) =>
+                setGeneratedLocation(e.currentTarget.value)
+              }
+            />
+            <InputGroup.Append>
+              <Button
+                variant="secondary"
+                className="text-input"
+                onClick={() => setShowGeneratedDialog(true)}
+              >
+                <Icon icon="ellipsis-h" />
+              </Button>
+            </InputGroup.Append>
+          </InputGroup>
+        </Form.Group>
+      );
+    }
+  }
+
   function renderSetPaths() {
     return (
       <>
@@ -228,41 +291,7 @@ export const Setup: React.FC = () => {
               }
             />
           </Form.Group>
-          <Form.Group id="generated">
+          {maybeRenderGenerated()}
-            <h3>
-              <FormattedMessage id="setup.paths.where_can_stash_store_its_generated_content" />
-            </h3>
-            <p>
-              <FormattedMessage
-                id="setup.paths.where_can_stash_store_its_generated_content_description"
-                values={{
-                  code: (chunks: string) => <code>{chunks}</code>,
-                }}
-              />
-            </p>
-            <InputGroup>
-              <Form.Control
-                className="text-input"
-                value={generatedLocation}
-                placeholder={intl.formatMessage({
-                  id:
-                    "setup.paths.path_to_generated_directory_empty_for_default",
-                })}
-                onChange={(e: React.ChangeEvent<HTMLInputElement>) =>
-                  setGeneratedLocation(e.currentTarget.value)
-                }
-              />
-              <InputGroup.Append>
-                <Button
-                  variant="secondary"
-                  className="text-input"
-                  onClick={() => setShowGeneratedDialog(true)}
-                >
-                  <Icon icon="ellipsis-h" />
-                </Button>
-              </InputGroup.Append>
-            </InputGroup>
-          </Form.Group>
         </section>
         <section className="mt-5">
           <div className="d-flex justify-content-center">
@@ -524,7 +553,7 @@ export const Setup: React.FC = () => {
   }
 
   // only display setup wizard if system is not setup
-  if (statusLoading) {
+  if (statusLoading || configLoading) {
     return <LoadingIndicator />;
   }