From 8a1b1e70caecbedd5fd84b0157e59b46fcfd679c Mon Sep 17 00:00:00 2001 From: Francisco Delmar Kurpiel Date: Sat, 20 Sep 2025 16:33:12 +0200 Subject: [PATCH] feat(core): Introduce ErrNoValue for nuanced validation This commit introduces a new mechanism to allow xtypes to signal that a provided value (such as an empty string) should be treated as if no value was provided at all. This solves a design limitation where the validation logic could not distinguish between a valid empty value and a "not provided" marker for certain types. Problem: Previously, an xtype like `xtypes.RSAPrivateKey` would fail validation when an optional parameter was backed by an environment variable set to an empty string. - The validation function (`ValueValid`) is context-agnostic and does not know if a parameter is optional. - If `ValueValid` returned an error for an empty string, it would incorrectly fail optional parameters. - If `ValueValid` did *not* return an error, it would incorrectly allow a required parameter to be considered valid while producing a `nil` value, without `MustParse` returning an error. Solution: This commit implements a sentinel error-based solution: 1. **`types.ErrNoValue`**: A new exported error, `ErrNoValue`, has been added to the `types` package. 2. **xtype Adoption**: xtypes that need this behavior (like `RSAPrivateKey`) can now return `types.ErrNoValue` from their `ValueValid` function when they encounter a value (like an empty string) that should be treated as "not provided". 3. **Core Logic Update**: The central validation function `validValue` in `parsed.go` has been updated to recognize `ErrNoValue`. When it receives this error, it now correctly applies the `optional`/`required` logic as if the parameter had not been supplied at all. Consequences: - **For Users:** This change fixes the bug where optional `RSAPrivateKey` parameters would fail with empty environment variables. The behavior is now consistent and correct. The default value of an optional parameter is now correctly used when an empty string is provided. - **For xtype Developers:** Developers of new xtypes now have a clear, idiomatic pattern to handle cases where certain string representations should be considered "empty" or "not provided". They can return `types.ErrNoValue` from `ValueValid` to trigger this behavior. - **Testing:** A comprehensive test case (`TestRSAPrivateKey`) has been added to `parser_test.go` to verify this new behavior and prevent future regressions. This change makes the validation system more robust and flexible while maintaining safety and correctness. --- GEMINI.md | 47 ++++++++++++++++ parsed.go | 13 +++-- parser_test.go | 125 +++++++++++++++++++++++++++++++++++++++++++ types/dynamic.go | 1 + types/param_value.go | 1 + types/types.go | 9 ++++ types/violation.go | 1 + xtypes/rsa_priv.go | 5 +- 8 files changed, 198 insertions(+), 4 deletions(-) create mode 100644 GEMINI.md diff --git a/GEMINI.md b/GEMINI.md new file mode 100644 index 0000000..bcfcbf7 --- /dev/null +++ b/GEMINI.md @@ -0,0 +1,47 @@ +# Proteus + +## Project Overview + +Proteus is a Go library for managing application configuration. It allows developers to define the configuration of an application in a struct and load it from different sources like environment variables and command-line flags. The library also supports configuration updates. + +The project uses `golangci-lint` for linting and has a comprehensive test suite. + +## Building and Running + +### Dependencies + +The project has one dependency: `golang.org/x/exp`. + +### Linting + +To run the linter, use the following command: + +```bash +make check +``` + +This will run `golangci-lint` on the entire codebase. + +### Testing + +To run the tests, use the following command: + +```bash +make test +``` + +This will run all the tests with the race detector enabled. + +### Test Coverage + +To generate a test coverage report, use the following command: + +```bash +make cover +``` + +This will generate a coverage report and open it in your browser. + +## Development Conventions + +The project follows standard Go conventions. All code is formatted using `gofmt`. The project uses a `Makefile` to automate common tasks like linting, testing, and generating coverage reports. diff --git a/parsed.go b/parsed.go index 699a895..564e474 100644 --- a/parsed.go +++ b/parsed.go @@ -330,19 +330,26 @@ func (p *Parsed) validValue(setName, paramName string, value *string) error { return fmt.Errorf("param %s.%s does not exit", setName, paramName) } - if value == nil { + checkRequired := func() error { if !param.optional { return types.ErrViolations([]types.Violation{{ SetName: setName, ParamName: paramName, - Message: "parameter is required but was not specified"}}) + Message: "parameter is required but was not specified", + }}) } - return nil } + if value == nil { + return checkRequired() + } + err := param.validFn(*value) if err != nil { + if errors.Is(err, types.ErrNoValue) { + return checkRequired() + } return types.ErrViolations([]types.Violation{{ SetName: setName, ParamName: paramName, diff --git a/parser_test.go b/parser_test.go index ffa5b48..29e7d8e 100644 --- a/parser_test.go +++ b/parser_test.go @@ -4,6 +4,10 @@ package proteus_test import ( + "crypto/rand" + "crypto/rsa" + "crypto/x509" + "encoding/pem" "math" "net/url" "strings" @@ -317,3 +321,124 @@ func (t testWriter) Write(v []byte) (int, error) { t.t.Logf("%s", v) return len(v), nil } + +func TestRSAPrivateKey(t *testing.T) { + _, privateKeyStr := generateTestKey(t) + defaultKey, _ := generateTestKey(t) + + tests := []struct { + name string + params types.ParamValues + shouldErr bool + optionalIsNil bool + useDefault bool + }{ + { + name: "valid key for optional and required", + params: types.ParamValues{ + "": { + "optionalkey": privateKeyStr, + "requiredkey": privateKeyStr, + }, + }, + shouldErr: false, + optionalIsNil: false, + }, + { + name: "empty string for optional key", + params: types.ParamValues{ + "": { + "optionalkey": "", + "requiredkey": privateKeyStr, + }, + }, + shouldErr: false, + optionalIsNil: true, + }, + { + name: "empty string for optional key with default", + params: types.ParamValues{ + "": { + "optionalkey": "", + "requiredkey": privateKeyStr, + }, + }, + shouldErr: false, + optionalIsNil: false, + useDefault: true, + }, + { + name: "no value for optional key", + params: types.ParamValues{ + "": { + "requiredkey": privateKeyStr, + }, + }, + shouldErr: false, + optionalIsNil: true, + }, + { + name: "empty string for required key", + params: types.ParamValues{ + "": { + "requiredkey": "", + }, + }, + shouldErr: true, + }, + { + name: "no value for required key", + params: types.ParamValues{"": {}}, + shouldErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := struct { + OptionalKey *xtypes.RSAPrivateKey `param:",optional"` + RequiredKey *xtypes.RSAPrivateKey + }{} + + if tt.useDefault { + cfg.OptionalKey = &xtypes.RSAPrivateKey{DefaultValue: defaultKey} + } + + testProvider := cfgtest.New(tt.params) + defer testProvider.Stop() + + _, err := proteus.MustParse(&cfg, + proteus.WithProviders(testProvider)) + + if tt.shouldErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + if tt.useDefault { + assert.Equal(t, defaultKey, cfg.OptionalKey.Value()) + } else if tt.optionalIsNil { + assert.Equal(t, nil, cfg.OptionalKey.Value()) + } else { + assert.NotNil(t, cfg.OptionalKey.Value()) + } + + if _, ok := tt.params[""]["requiredkey"]; ok && tt.params[""]["requiredkey"] != "" { + assert.NotNil(t, cfg.RequiredKey.Value()) + } + } + }) + } +} + +func generateTestKey(t *testing.T) (*rsa.PrivateKey, string) { + t.Helper() + privateKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("failed to generate RSA private key: %v", err) + } + privateKeyPEM := &pem.Block{ + Type: "RSA PRIVATE KEY", + Bytes: x509.MarshalPKCS1PrivateKey(privateKey), + } + return privateKey, string(pem.EncodeToMemory(privateKeyPEM)) +} diff --git a/types/dynamic.go b/types/dynamic.go index 7b466c5..a814d92 100644 --- a/types/dynamic.go +++ b/types/dynamic.go @@ -1,3 +1,4 @@ +//nolint:revive package types // XType is a configuration parameter parameter that supports being updated diff --git a/types/param_value.go b/types/param_value.go index 6f263eb..5e38aef 100644 --- a/types/param_value.go +++ b/types/param_value.go @@ -1,3 +1,4 @@ +//nolint:revive package types // ParamValues holds the values of the configuration parameters, as read by diff --git a/types/types.go b/types/types.go index aa683ac..47451d2 100644 --- a/types/types.go +++ b/types/types.go @@ -1,2 +1,11 @@ // Package types defines types used by proteus and by code using it. +// +//nolint:revive package types + +import "errors" + +// ErrNoValue can be returned by xtypes on their ValueValid method to indicate +// that the provided value should be considered as if no value was provided at +// all. This allows, for example, to handle empty strings as "no value" +var ErrNoValue = errors.New("no value provided") diff --git a/types/violation.go b/types/violation.go index 9dc5c3f..38fd0d6 100644 --- a/types/violation.go +++ b/types/violation.go @@ -1,3 +1,4 @@ +//nolint:revive package types import ( diff --git a/xtypes/rsa_priv.go b/xtypes/rsa_priv.go index 4818137..3ec57bc 100644 --- a/xtypes/rsa_priv.go +++ b/xtypes/rsa_priv.go @@ -31,7 +31,7 @@ var _ types.Redactor = &RSAPrivateKey{} // UnmarshalParam parses the input as a string. func (d *RSAPrivateKey) UnmarshalParam(in *string) error { var privK *rsa.PrivateKey - if in != nil { + if in != nil && *in != "" { var err error privK, err = parseRSAPriv(*in, d.Base64Encoder) if err != nil { @@ -67,6 +67,9 @@ func (d *RSAPrivateKey) Value() *rsa.PrivateKey { // ValueValid test if the provided parameter value is valid. Has no side // effects. func (d *RSAPrivateKey) ValueValid(s string) error { + if s == "" { + return types.ErrNoValue + } _, err := parseRSAPriv(s, d.Base64Encoder) return err }