Empty user config values should not override original config value / default (#2993)

* User should be able to clear the original config value but not the default

Author: sgalsaleh

api/pkg/template/config.go

@@ -195,8 +195,9 @@ func (e *Engine) resolveConfigItem(name string) (*resolvedConfigItem, error) {
 	}
 
 	// Priority: user value > config value > config default
+	// User value is only used if it differs from the original config value (user made a change) - reason: sc-129708
 	var userVal *string
-	if v, exists := e.configValues[name]; exists {
+	if v, exists := e.configValues[name]; exists && v.Value != templatedValue {
 		userVal = &v.Value
 	}
 
@@ -208,8 +209,9 @@ func (e *Engine) resolveConfigItem(name string) (*resolvedConfigItem, error) {
 		effectiveValue = templatedDefault
 	}
 
+	// User filename is only used if it differs from the original config filename (user made a change) - reason: sc-129708
 	var userFilename *string
-	if v, exists := e.configValues[name]; exists {
+	if v, exists := e.configValues[name]; exists && v.Filename != templatedFilename {
 		userFilename = &v.Filename
 	}
 
@@ -277,19 +279,11 @@ func (e *Engine) configValueChanged(itemName string) bool {
 		return true
 	}
 
-	return prevVal.Value != currentVal.Value
-}
-
-func (e *Engine) getItemFilename(configItem *kotsv1beta1.ConfigItem) string {
-	// Priority: user value
-	if userVal, exists := e.configValues[configItem.Name]; exists {
-		return userVal.Filename
+	if prevVal.Filename != currentVal.Filename {
+		return true
 	}
 
-	// Do not use the config item's filename for KOTS parity
-
-	// If still empty, return empty string
-	return ""
+	return prevVal.Value != currentVal.Value
 }
 
 func (e *Engine) findConfigItem(name string) *kotsv1beta1.ConfigItem {

api/pkg/template/config_test.go

@@ -248,9 +248,9 @@ func TestEngine_templateConfigItems(t *testing.T) {
 								{
 									Name:     "user_cleared",
 									Type:     "file",
-									Value:    multitype.FromString(""),
+									Value:    multitype.FromString(""), // Empty differs from config value = user cleared it
 									Default:  multitype.FromString("user_cleared_default"),
-									Filename: "",
+									Filename: "", // Empty differs from config filename = user cleared it
 								},
 								{
 									Name:     "no_user_value",
@@ -544,143 +544,3 @@ func TestEngine_ShouldInvalidateItem(t *testing.T) {
 	engine.depsTree = map[string][]string{}
 	assert.False(t, engine.shouldInvalidateItem("item1"), "should not invalidate item1 as it doesn't exist in either config values")
 }
-
-func TestEngine_GetItemFilename(t *testing.T) {
-	config := &kotsv1beta1.Config{
-		Spec: kotsv1beta1.ConfigSpec{
-			Groups: []kotsv1beta1.ConfigGroup{
-				{
-					Name: "test",
-					Items: []kotsv1beta1.ConfigItem{
-						{Name: "file_with_user_filename"},
-						{Name: "file_without_user_filename"},
-						{Name: "file_with_empty_user_filename"},
-						{Name: "non_file_item"},
-					},
-				},
-			},
-		},
-	}
-
-	engine := NewEngine(config)
-
-	// Test 1: User provides filename - should use user filename
-	engine.configValues = types.AppConfigValues{
-		"file_with_user_filename": {Filename: "user-provided.txt", Value: "content"},
-	}
-	filename := engine.getItemFilename(&kotsv1beta1.ConfigItem{Name: "file_with_user_filename"})
-	assert.Equal(t, "user-provided.txt", filename, "should use user-provided filename")
-
-	// Test 2: User doesn't provide filename - should return empty string
-	engine.configValues = types.AppConfigValues{
-		"file_without_user_filename": {Value: "content"},
-	}
-	filename = engine.getItemFilename(&kotsv1beta1.ConfigItem{Name: "file_without_user_filename"})
-	assert.Equal(t, "", filename, "should return empty string when user doesn't provide filename")
-
-	// Test 3: User provides empty filename - should return empty string
-	engine.configValues = types.AppConfigValues{
-		"file_with_empty_user_filename": {Filename: "", Value: "content"},
-	}
-	filename = engine.getItemFilename(&kotsv1beta1.ConfigItem{Name: "file_with_empty_user_filename"})
-	assert.Equal(t, "", filename, "should return empty string when user provides empty filename")
-
-	// Test 4: Item not in config values - should return empty string
-	filename = engine.getItemFilename(&kotsv1beta1.ConfigItem{Name: "non_file_item"})
-	assert.Equal(t, "", filename, "should return empty string for item not in config values")
-
-	// Test 5: Empty config values - should return empty string
-	engine.configValues = types.AppConfigValues{}
-	filename = engine.getItemFilename(&kotsv1beta1.ConfigItem{Name: "file_with_user_filename"})
-	assert.Equal(t, "", filename, "should return empty string when config values is empty")
-}
-
-func TestEngine_FilenamePreservationInTemplateConfigItems(t *testing.T) {
-	config := &kotsv1beta1.Config{
-		Spec: kotsv1beta1.ConfigSpec{
-			Groups: []kotsv1beta1.ConfigGroup{
-				{
-					Name: "filename_test",
-					Items: []kotsv1beta1.ConfigItem{
-						{
-							Name:    "preserved_file",
-							Type:    "file",
-							Value:   multitype.FromString(""),
-							Default: multitype.FromString("default.txt"),
-						},
-						{
-							Name:    "overwritten_file",
-							Type:    "file",
-							Value:   multitype.FromString(""),
-							Default: multitype.FromString("default.txt"),
-						},
-						{
-							Name:    "text_item",
-							Type:    "text",
-							Value:   multitype.FromString(""),
-							Default: multitype.FromString("default"),
-						},
-					},
-				},
-			},
-		},
-	}
-
-	engine := NewEngine(config, WithMode(ModeConfig))
-	engine.configValues = types.AppConfigValues{
-		"preserved_file":   {Filename: "preserved.txt", Value: "content1"},
-		"overwritten_file": {Filename: "overwritten.txt", Value: "content2"},
-		"text_item":        {Value: "text content"},
-	}
-
-	result, err := engine.templateConfigItems()
-	require.NoError(t, err)
-	assert.NotNil(t, result)
-
-	// Verify filename is preserved for file items
-	preservedItem := result.Spec.Groups[0].Items[0]
-	assert.Equal(t, "preserved.txt", preservedItem.Filename, "filename should be preserved for preserved_file")
-
-	overwrittenItem := result.Spec.Groups[0].Items[1]
-	assert.Equal(t, "overwritten.txt", overwrittenItem.Filename, "filename should be preserved for overwritten_file")
-
-	// Verify text items don't have filename field set
-	textItem := result.Spec.Groups[0].Items[2]
-	assert.Equal(t, "", textItem.Filename, "text items should not have filename field set")
-}
-
-func TestEngine_GetItemFilenameDirect(t *testing.T) {
-	// Test the getItemFilename function directly
-	config := &kotsv1beta1.Config{
-		Spec: kotsv1beta1.ConfigSpec{
-			Groups: []kotsv1beta1.ConfigGroup{
-				{
-					Name: "test",
-					Items: []kotsv1beta1.ConfigItem{
-						{Name: "test_file"},
-					},
-				},
-			},
-		},
-	}
-
-	engine := NewEngine(config)
-
-	// Test with user filename
-	engine.configValues = types.AppConfigValues{
-		"test_file": {Filename: "test.txt", Value: "content"},
-	}
-
-	configItem := &kotsv1beta1.ConfigItem{Name: "test_file"}
-	filename := engine.getItemFilename(configItem)
-
-	t.Logf("User filename test: expected 'test.txt', got '%s'", filename)
-	assert.Equal(t, "test.txt", filename, "should return user filename when provided")
-
-	// Test without user filename
-	engine.configValues = types.AppConfigValues{}
-	filename = engine.getItemFilename(configItem)
-
-	t.Logf("No user filename test: expected '', got '%s'", filename)
-	assert.Equal(t, "", filename, "should return empty string when no user filename")
-}

api/pkg/template/execute_test.go

@@ -174,16 +174,16 @@ func TestEngine_ValuePriority(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, "fallback_default", result)
 
-	// Test with empty user value (should use empty string, not fall back to config value)
+	// Test with empty user value - user cleared the value (differs from config value)
 	emptyConfigValues := types.AppConfigValues{
-		"database_host": {Value: ""}, // Empty user value should be used as-is
+		"database_host": {Value: ""}, // Empty differs from config value "db-internal.company.com" = user cleared it
 		"database_port": {Value: "5433"},
 	}
 	err = engine.Parse("{{repl ConfigOption \"database_url\" }}")
 	require.NoError(t, err)
 	result, err = engine.Execute(emptyConfigValues, WithProxySpec(&ecv1beta1.ProxySpec{}))
 	require.NoError(t, err)
-	assert.Equal(t, "postgres://:5433/app", result) // Empty host should result in empty string, not config fallback
+	assert.Equal(t, "postgres://:5433/app", result) // Empty host = user cleared it
 }
 
 func TestEngine_ConfigOptionEquals(t *testing.T) {
@@ -412,6 +412,13 @@ func TestEngine_ConfigOptionFilename(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, "user_file.txt", result)
 
+	// Test with user providing empty filename - should return empty string
+	result, err = engine.Execute(types.AppConfigValues{
+		"a_file": {Value: userContentEncoded, Filename: ""},
+	}, WithProxySpec(&ecv1beta1.ProxySpec{}))
+	require.NoError(t, err)
+	assert.Equal(t, "", result)
+
 	// Test with an unknown item - an error is returned and empty string is returned
 	err = engine.Parse("{{repl ConfigOptionFilename \"notfound\" }}")
 	require.NoError(t, err)

api/types/app.go

@@ -1,8 +1,6 @@
 package types
 
 import (
-	"strings"
-
 	kotsv1beta1 "github.com/replicatedhq/kotskinds/apis/kots/v1beta1"
 )
 
@@ -38,16 +36,9 @@ func ConvertToAppConfigValues(kotsConfigValues *kotsv1beta1.ConfigValues) AppCon
 
 	configValues := make(AppConfigValues)
 	for key, value := range kotsConfigValues.Spec.Values {
-		// Temporary fix for https://app.shortcut.com/replicated/story/129708/template-execution-fails-when-empty-user-config-values-override-generated-defaults
-		// TODO: Remove this block of code and add a unit test for this function once a fix has been implemented
-		temporaryFixValue := value.Value
-		if strings.HasSuffix(key, "_json") && temporaryFixValue == "" {
-			temporaryFixValue = "{}"
-		}
-
 		configValues[key] = AppConfigValue{
 			Default:        value.Default,
-			Value:          temporaryFixValue,
+			Value:          value.Value,
 			Data:           value.Data,
 			ValuePlaintext: value.ValuePlaintext,
 			DataPlaintext:  value.DataPlaintext,