diff --git a/internal/cli/config_test.go b/internal/cli/config_test.go index 6eb45cb..3553c0c 100644 --- a/internal/cli/config_test.go +++ b/internal/cli/config_test.go @@ -252,7 +252,7 @@ func TestConfigSetKeepsFileThatWouldNotLoad(t *testing.T) { {configWithoutRecipients, "snapshots.home.paths", "/srv", "into []string"}, {configWithoutRecipients, "chunk_size", "fast", - "invalid size format"}, + `chunk_size: "fast" is not a size`}, // Values of the right type that fail validation. {configWithoutRecipients, "compression_level", "99", "compression_level must be between 1 and 19"}, diff --git a/internal/config/config.go b/internal/config/config.go index a615400..c04e464 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -290,7 +290,8 @@ func Load(path string) (*Config, error) { } // Parse reads YAML config data as Load does, without validating it. It -// refuses a key that no setting has, applies default values for optional +// refuses a key that no setting has and a size setting whose value is not a +// size, naming the setting. It applies default values for optional // fields, fills in ${...} references using smartconfig, and expands ~ in // paths. func Parse(data []byte) (*Config, error) { @@ -315,8 +316,9 @@ func Parse(data []byte) (*Config, error) { return nil, fmt.Errorf("failed to load config file: %w", err) } - // A ${...} reference can bring in keys that are not in the file. - err = checkFilledKeys(sc.Data()) + // A ${...} reference can bring in keys that are not in the file, and + // is the value of a size setting only once it is filled in. + err = checkFilledConfig(sc.Data()) if err != nil { return nil, err } diff --git a/internal/config/keys.go b/internal/config/keys.go index 8e51543..b9ff171 100644 --- a/internal/config/keys.go +++ b/internal/config/keys.go @@ -129,10 +129,11 @@ func checkKeys(node *yaml.Node, typ reflect.Type, path string, return nil } -// checkFilledKeys is checkKeys for data, the config after smartconfig has -// filled in its ${...} references. A key that a reference brings in is not -// in the file, and the copy that Encode makes has no line numbers. -func checkFilledKeys(data map[string]any) error { +// checkFilledConfig checks data, the config after smartconfig has filled in +// its ${...} references: its keys with checkKeys, then its size settings +// with checkSizes. A key that a reference brings in is not in the file, and +// the copy that Encode makes has no line numbers. +func checkFilledConfig(data map[string]any) error { var filled yaml.Node err := filled.Encode(data) @@ -140,8 +141,13 @@ func checkFilledKeys(data map[string]any) error { return fmt.Errorf("failed to marshal config data: %w", err) } - return checkKeys(&filled, reflect.TypeFor[Config](), "", + err = checkKeys(&filled, reflect.TypeFor[Config](), "", map[*yaml.Node]bool{}, map[nodeAndType]bool{}) + if err != nil { + return err + } + + return checkSizes(&filled, nil) } // checkMappingKeys is checkKeys for a map. diff --git a/internal/config/size.go b/internal/config/size.go index 23b77b6..12cc5b0 100644 --- a/internal/config/size.go +++ b/internal/config/size.go @@ -4,13 +4,20 @@ import ( "errors" "fmt" "math" + "reflect" + "slices" + "strconv" + "strings" "github.com/dustin/go-humanize" + "gopkg.in/yaml.v3" ) var ( errSizeType = errors.New("size must be a number or string") errSizeTooLarge = errors.New("size exceeds maximum supported value") + errNotASize = errors.New( + "is not a size; use a number of bytes or a unit such as 10MB") ) // Size represents a byte size that can be specified in configuration files. @@ -32,6 +39,15 @@ func (s *Size) UnmarshalYAML(unmarshal func(any) error) error { return nil } + // A number too large for an int64 decodes as a float64. Its text can be + // 1e+20, which is not a size string. + var floatVal float64 + + err = unmarshal(&floatVal) + if err == nil && floatVal > math.MaxInt64 { + return errSizeTooLarge + } + // Try to unmarshal as string var strVal string @@ -40,17 +56,66 @@ func (s *Size) UnmarshalYAML(unmarshal func(any) error) error { return errSizeType } - // Parse the string using go-humanize - bytes, err := humanize.ParseBytes(strVal) + size, err := ParseSize(strVal) if err != nil { - return fmt.Errorf("invalid size format: %w", err) + return err } - if bytes > math.MaxInt64 { - return fmt.Errorf("%w: %s", errSizeTooLarge, strVal) + *s = size + + return nil +} + +// checkSizes returns an error naming the first size setting in node whose +// value is not a size. node is the config, or the part of it at the dotted +// keys, after smartconfig has filled in its ${...} references. +func checkSizes(node *yaml.Node, keys []string) error { + typ, err := SettingType(keys) + if err != nil { + // Unknown keys are refused before this check, so keys is in one of + // two places. The env section holds no settings. A value of the + // wrong shape, such as a map given for hostname or inside exclude, + // is reported later by the YAML decoder. + return nil //nolint:nilerr // not an error here } - *s = Size(bytes) + if typ == reflect.TypeFor[Size]() { + var size Size + + err = node.Decode(&size) + if err != nil { + return fmt.Errorf("%s: %w", strings.Join(keys, "."), err) + } + + return nil + } + + switch node.Kind { + case yaml.DocumentNode: + for _, child := range node.Content { + err = checkSizes(child, keys) + if err != nil { + return err + } + } + case yaml.MappingNode: + for i := 0; i+1 < len(node.Content); i += 2 { + err = checkSizes(node.Content[i+1], + slices.Concat(keys, []string{node.Content[i].Value})) + if err != nil { + return err + } + } + case yaml.SequenceNode: + for i, child := range node.Content { + err = checkSizes(child, slices.Concat(keys, []string{strconv.Itoa(i)})) + if err != nil { + return err + } + } + case yaml.ScalarNode, yaml.AliasNode: + // A scalar holds no settings, and the filled config has no aliases. + } return nil } @@ -70,16 +135,17 @@ func (s Size) String() string { return humanize.Bytes(uint64(s)) } -// ParseSize parses a size string into a Size value +// ParseSize parses a size string, such as "10MB", into a Size value. An +// error quotes s. func ParseSize(s string) (Size, error) { - bytes, err := humanize.ParseBytes(s) + bytes, err := humanize.ParseBigBytes(s) if err != nil { - return 0, fmt.Errorf("invalid size format: %w", err) + return 0, fmt.Errorf("%q %w", s, errNotASize) } - if bytes > math.MaxInt64 { + if !bytes.IsInt64() { return 0, fmt.Errorf("%w: %s", errSizeTooLarge, s) } - return Size(bytes), nil + return Size(bytes.Int64()), nil } diff --git a/internal/config/size_test.go b/internal/config/size_test.go new file mode 100644 index 0000000..5265ed4 --- /dev/null +++ b/internal/config/size_test.go @@ -0,0 +1,120 @@ +package config //nolint:testpackage // checks errNotASize and errSizeTooLarge + +import ( + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +// TestParseRefusesValueThatIsNotASize checks that a size setting whose +// value is not a size fails the parse with an error naming the setting and +// quoting the value, also when a ${...} reference fills the value in. +func TestParseRefusesValueThatIsNotASize(t *testing.T) { + t.Parallel() + + sizePath := filepath.Join(t.TempDir(), "size") + + err := os.WriteFile(sizePath, []byte("lots"), 0o600) + if err != nil { + t.Fatalf("write size: %v", err) + } + + tests := []struct { + name string + yaml string + want string + }{ + {"chunk_size", keysTestConfig + "chunk_size: banana\n", + `chunk_size: "banana" is not a size; ` + + "use a number of bytes or a unit such as 10MB"}, + {"blob_size_limit", keysTestConfig + "blob_size_limit: 10XB\n", + `blob_size_limit: "10XB" is not a size; ` + + "use a number of bytes or a unit such as 10MB"}, + {"s3.part_size", strings.Replace(keysTestConfig, " region:", + " part_size: big\n region:", 1), + `s3.part_size: "big" is not a size; ` + + "use a number of bytes or a unit such as 10MB"}, + {"from a reference", + keysTestConfig + "chunk_size: ${FILE:" + sizePath + "}\n", + `chunk_size: "lots" is not a size; ` + + "use a number of bytes or a unit such as 10MB"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + _, err := Parse([]byte(tt.yaml)) + if !errors.Is(err, errNotASize) { + t.Fatalf("Parse() error = %v, want errNotASize", err) + } + + if err.Error() != tt.want { + t.Errorf("Parse() error = %q, want %q", err.Error(), tt.want) + } + }) + } +} + +// TestParseRefusesSizeTooLarge checks that a size setting whose value is +// too large for a size fails the parse with an error naming the setting. +// YAML reads the second value as a float. +func TestParseRefusesSizeTooLarge(t *testing.T) { + t.Parallel() + + tests := []struct { + value string + want string + }{ + {"100EB", "chunk_size: size exceeds maximum supported value: 100EB"}, + {"99999999999999999999", + "chunk_size: size exceeds maximum supported value"}, + } + + for _, tt := range tests { + t.Run(tt.value, func(t *testing.T) { + t.Parallel() + + _, err := Parse([]byte(keysTestConfig + "chunk_size: " + tt.value + "\n")) + if !errors.Is(err, errSizeTooLarge) { + t.Fatalf("Parse() error = %v, want errSizeTooLarge", err) + } + + if err.Error() != tt.want { + t.Errorf("Parse() error = %q, want %q", err.Error(), tt.want) + } + }) + } +} + +// TestParseAcceptsSizes checks that size settings given as a number of +// bytes, as a unit, and through a ${...} reference still parse. +func TestParseAcceptsSizes(t *testing.T) { + t.Parallel() + + sizePath := filepath.Join(t.TempDir(), "size") + + err := os.WriteFile(sizePath, []byte("20MB"), 0o600) + if err != nil { + t.Fatalf("write size: %v", err) + } + + data := strings.Replace(keysTestConfig, " region:", + " part_size: 6000000\n region:", 1) + + "chunk_size: ${FILE:" + sizePath + "}\n" + + "blob_size_limit: 1GB\n" + + cfg, err := Parse([]byte(data)) + if err != nil { + t.Fatalf("Parse() unexpected error: %v", err) + } + + if cfg.ChunkSize != 20_000_000 || cfg.BlobSizeLimit != 1_000_000_000 || + cfg.S3.PartSize != 6_000_000 { + t.Errorf("chunk_size, blob_size_limit, s3.part_size = %d, %d, %d, "+ + "want 20000000, 1000000000, 6000000", + cfg.ChunkSize, cfg.BlobSizeLimit, cfg.S3.PartSize) + } +}