fix(config): reject ambiguous checklist comma format and propagate encode errors

ParseChecklistValue now accepts JSON arrays or single bare values and rejects
  legacy comma-separated input. Empty JSON segments are validated consistently,
  and FormatChecklistValue returns marshal errors instead of an empty string
This commit is contained in:
jamesread 2026-07-07 11:57:20 +01:00
parent a58e2fe0c3
commit 42f2572616
5 changed files with 53 additions and 38 deletions

View File

@ -7,7 +7,7 @@ import (
) )
// ParseChecklistValue parses a checklist argument wire value. // ParseChecklistValue parses a checklist argument wire value.
// New values are JSON arrays; legacy comma-separated values are still accepted. // Values must be JSON arrays, or a single choice without commas.
func ParseChecklistValue(value string) ([]string, error) { func ParseChecklistValue(value string) ([]string, error) {
trimmed := strings.TrimSpace(value) trimmed := strings.TrimSpace(value)
if trimmed == "" { if trimmed == "" {
@ -18,7 +18,11 @@ func ParseChecklistValue(value string) ([]string, error) {
return parseJSONChecklistValue(trimmed) return parseJSONChecklistValue(trimmed)
} }
return parseLegacyChecklistValue(value) if strings.Contains(trimmed, ",") {
return nil, fmt.Errorf("checklist value uses legacy comma-separated format; use a JSON array instead")
}
return []string{trimmed}, nil
} }
func parseJSONChecklistValue(value string) ([]string, error) { func parseJSONChecklistValue(value string) ([]string, error) {
@ -27,34 +31,25 @@ func parseJSONChecklistValue(value string) ([]string, error) {
return nil, fmt.Errorf("invalid checklist JSON value: %w", err) return nil, fmt.Errorf("invalid checklist JSON value: %w", err)
} }
return values, nil for _, segment := range values {
} if strings.TrimSpace(segment) == "" {
func parseLegacyChecklistValue(value string) ([]string, error) {
segments := strings.Split(value, ",")
values := make([]string, 0, len(segments))
for _, segment := range segments {
trimmedSegment := strings.TrimSpace(segment)
if trimmedSegment == "" {
return nil, fmt.Errorf("checklist value contains an empty segment") return nil, fmt.Errorf("checklist value contains an empty segment")
} }
values = append(values, trimmedSegment)
} }
return values, nil return values, nil
} }
// FormatChecklistValue serializes selected checklist values for API transport. // FormatChecklistValue serializes selected checklist values for API transport.
func FormatChecklistValue(values []string) string { func FormatChecklistValue(values []string) (string, error) {
if len(values) == 0 { if len(values) == 0 {
return "" return "", nil
} }
encoded, err := json.Marshal(values) encoded, err := json.Marshal(values)
if err != nil { if err != nil {
return "" return "", fmt.Errorf("encoding checklist value: %w", err)
} }
return string(encoded) return string(encoded), nil
} }

View File

@ -19,25 +19,40 @@ func TestParseChecklistValueJSON(t *testing.T) {
assert.Equal(t, []string{"kitchen,bedroom", "hallway"}, values) assert.Equal(t, []string{"kitchen,bedroom", "hallway"}, values)
} }
func TestParseChecklistValueLegacyCommaSeparated(t *testing.T) { func TestParseChecklistValueSingleValue(t *testing.T) {
t.Parallel() t.Parallel()
values, err := ParseChecklistValue("documents, photos") values, err := ParseChecklistValue("documents")
require.NoError(t, err) require.NoError(t, err)
assert.Equal(t, []string{"documents", "photos"}, values) assert.Equal(t, []string{"documents"}, values)
} }
func TestParseChecklistValueRejectsEmptyLegacySegment(t *testing.T) { func TestParseChecklistValueRejectsLegacyCommaSeparated(t *testing.T) {
t.Parallel() t.Parallel()
_, err := ParseChecklistValue("documents,,photos") _, err := ParseChecklistValue("documents, photos")
require.Error(t, err)
}
func TestParseChecklistValueRejectsEmptyJSONSegment(t *testing.T) {
t.Parallel()
_, err := ParseChecklistValue(`["documents","","photos"]`)
require.Error(t, err) require.Error(t, err)
} }
func TestFormatChecklistValueJSON(t *testing.T) { func TestFormatChecklistValueJSON(t *testing.T) {
t.Parallel() t.Parallel()
assert.Equal(t, `["documents","photos"]`, FormatChecklistValue([]string{"documents", "photos"})) encoded, err := FormatChecklistValue([]string{"documents", "photos"})
assert.Equal(t, `["kitchen,bedroom"]`, FormatChecklistValue([]string{"kitchen,bedroom"})) require.NoError(t, err)
assert.Empty(t, FormatChecklistValue(nil)) assert.Equal(t, `["documents","photos"]`, encoded)
encoded, err = FormatChecklistValue([]string{"kitchen,bedroom"})
require.NoError(t, err)
assert.Equal(t, `["kitchen,bedroom"]`, encoded)
encoded, err = FormatChecklistValue(nil)
require.NoError(t, err)
assert.Empty(t, encoded)
} }

View File

@ -499,16 +499,21 @@ func mangleChecklistValue(arg *config.ActionArgument, value string, actionTitle
return value return value
} }
return mangleChecklistSegments(arg, segments, actionTitle) return mangleChecklistSegments(arg, segments, value, actionTitle)
} }
func mangleChecklistSegments(arg *config.ActionArgument, segments []string, actionTitle string) string { func mangleChecklistSegments(arg *config.ActionArgument, segments []string, fallback string, actionTitle string) string {
mangled := make([]string, len(segments)) mangled := make([]string, len(segments))
for i, segment := range segments { for i, segment := range segments {
mangled[i] = mangleChecklistSegment(arg, segment, actionTitle) mangled[i] = mangleChecklistSegment(arg, segment, actionTitle)
} }
return config.FormatChecklistValue(mangled) formatted, err := config.FormatChecklistValue(mangled)
if err != nil {
return fallback
}
return formatted
} }
func mangleChecklistSegment(arg *config.ActionArgument, segment string, actionTitle string) string { func mangleChecklistSegment(arg *config.ActionArgument, segment string, actionTitle string) string {

View File

@ -136,10 +136,10 @@ func TestValidateArgumentChecklistSelections(t *testing.T) {
err := ValidateArgument(&arg, "documents", &action) err := ValidateArgument(&arg, "documents", &action)
assert.Nil(t, err) assert.Nil(t, err)
err = ValidateArgument(&arg, "documents,photos", &action) err = ValidateArgument(&arg, `["documents","photos"]`, &action)
assert.Nil(t, err) assert.Nil(t, err)
err = ValidateArgument(&arg, "documents,unknown", &action) err = ValidateArgument(&arg, `["documents","unknown"]`, &action)
assert.NotNil(t, err) assert.NotNil(t, err)
} }
@ -149,7 +149,7 @@ func TestValidateArgumentChecklistTitleMangling(t *testing.T) {
arg := checklistTestArg() arg := checklistTestArg()
action := config.Action{Title: "Test checklist title mangling"} action := config.Action{Title: "Test checklist title mangling"}
err := ValidateArgument(&arg, "Documents,Photos", &action) err := ValidateArgument(&arg, `["Documents","Photos"]`, &action)
assert.Nil(t, err) assert.Nil(t, err)
} }
@ -186,7 +186,7 @@ func TestValidateArgumentChecklistRejectsEmptySegment(t *testing.T) {
arg := checklistTestArg() arg := checklistTestArg()
action := config.Action{Title: "Test checklist empty segment"} action := config.Action{Title: "Test checklist empty segment"}
err := ValidateArgument(&arg, "documents,,photos", &action) err := ValidateArgument(&arg, `["documents","","photos"]`, &action)
assert.NotNil(t, err) assert.NotNil(t, err)
} }
@ -195,10 +195,10 @@ func TestMangleArgumentValueChecklist(t *testing.T) {
arg := checklistTestArg() arg := checklistTestArg()
out := MangleArgumentValue(&arg, "Documents,Music", "Test action") out := MangleArgumentValue(&arg, `["Documents","Music"]`, "Test action")
assert.Equal(t, `["documents","music"]`, out) assert.Equal(t, `["documents","music"]`, out)
out = MangleArgumentValue(&arg, "documents,photos", "Test action") out = MangleArgumentValue(&arg, `["documents","photos"]`, "Test action")
assert.Equal(t, `["documents","photos"]`, out) assert.Equal(t, `["documents","photos"]`, out)
} }
@ -225,10 +225,10 @@ func TestValidateArgumentChecklistEntitySelections(t *testing.T) {
err := ValidateArgument(&arg, "attic", &action) err := ValidateArgument(&arg, "attic", &action)
assert.Nil(t, err) assert.Nil(t, err)
err = ValidateArgument(&arg, "attic,basement", &action) err = ValidateArgument(&arg, `["attic","basement"]`, &action)
assert.Nil(t, err) assert.Nil(t, err)
err = ValidateArgument(&arg, "attic,unknown", &action) err = ValidateArgument(&arg, `["attic","unknown"]`, &action)
assert.NotNil(t, err) assert.NotNil(t, err)
} }
@ -247,7 +247,7 @@ func TestMangleArgumentValueChecklistEntityTitles(t *testing.T) {
}, },
} }
out := MangleArgumentValue(&arg, "attic room,basement room", "Test checklist entity titles") out := MangleArgumentValue(&arg, `["attic room","basement room"]`, "Test checklist entity titles")
assert.Equal(t, `["attic","basement"]`, out) assert.Equal(t, `["attic","basement"]`, out)
} }

View File

@ -92,7 +92,7 @@ func TestStorableArgumentsFromRequestStoresMangledChecklistValue(t *testing.T) {
}, },
} }
req.Arguments = map[string]string{ req.Arguments = map[string]string{
"directories": "Documents,Photos", "directories": `["Documents","Photos"]`,
} }
mangleInvalidArgumentValues(req) mangleInvalidArgumentValues(req)