Skip to content

Commit 7e75fc5

Browse files
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.
1 parent 34b3a21 commit 7e75fc5

5 files changed

Lines changed: 201 additions & 6 deletions

File tree

GEMINI.md

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
# Proteus
2+
3+
## Project Overview
4+
5+
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.
6+
7+
The project uses `golangci-lint` for linting and has a comprehensive test suite.
8+
9+
## Building and Running
10+
11+
### Dependencies
12+
13+
The project has one dependency: `golang.org/x/exp`.
14+
15+
### Linting
16+
17+
To run the linter, use the following command:
18+
19+
```bash
20+
make check
21+
```
22+
23+
This will run `golangci-lint` on the entire codebase.
24+
25+
### Testing
26+
27+
To run the tests, use the following command:
28+
29+
```bash
30+
make test
31+
```
32+
33+
This will run all the tests with the race detector enabled.
34+
35+
### Test Coverage
36+
37+
To generate a test coverage report, use the following command:
38+
39+
```bash
40+
make cover
41+
```
42+
43+
This will generate a coverage report and open it in your browser.
44+
45+
## Development Conventions
46+
47+
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.

parsed.go

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -330,19 +330,26 @@ func (p *Parsed) validValue(setName, paramName string, value *string) error {
330330
return fmt.Errorf("param %s.%s does not exit", setName, paramName)
331331
}
332332

333-
if value == nil {
333+
checkRequired := func() error {
334334
if !param.optional {
335335
return types.ErrViolations([]types.Violation{{
336336
SetName: setName,
337337
ParamName: paramName,
338-
Message: "parameter is required but was not specified"}})
338+
Message: "parameter is required but was not specified",
339+
}})
339340
}
340-
341341
return nil
342342
}
343343

344+
if value == nil {
345+
return checkRequired()
346+
}
347+
344348
err := param.validFn(*value)
345349
if err != nil {
350+
if errors.Is(err, types.ErrNoValue) {
351+
return checkRequired()
352+
}
346353
return types.ErrViolations([]types.Violation{{
347354
SetName: setName,
348355
ParamName: paramName,

parser_test.go

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,10 @@
44
package proteus_test
55

66
import (
7+
"crypto/rand"
8+
"crypto/rsa"
9+
"crypto/x509"
10+
"encoding/pem"
711
"math"
812
"net/url"
913
"strings"
@@ -317,3 +321,126 @@ func (t testWriter) Write(v []byte) (int, error) {
317321
t.t.Logf("%s", v)
318322
return len(v), nil
319323
}
324+
325+
func TestRSAPrivateKey(t *testing.T) {
326+
_, privateKeyStr := generateTestKey(t)
327+
defaultKey, _ := generateTestKey(t)
328+
329+
tests := []struct {
330+
name string
331+
params types.ParamValues
332+
shouldErr bool
333+
optionalIsNil bool
334+
useDefault bool
335+
}{
336+
{
337+
name: "valid key for optional and required",
338+
params: types.ParamValues{
339+
"": {
340+
"optionalkey": privateKeyStr,
341+
"requiredkey": privateKeyStr,
342+
},
343+
},
344+
shouldErr: false,
345+
optionalIsNil: false,
346+
},
347+
{
348+
name: "empty string for optional key",
349+
params: types.ParamValues{
350+
"": {
351+
"optionalkey": "",
352+
"requiredkey": privateKeyStr,
353+
},
354+
},
355+
shouldErr: false,
356+
optionalIsNil: true,
357+
},
358+
{
359+
name: "empty string for optional key with default",
360+
params: types.ParamValues{
361+
"": {
362+
"optionalkey": "",
363+
"requiredkey": privateKeyStr,
364+
},
365+
},
366+
shouldErr: false,
367+
optionalIsNil: false,
368+
useDefault: true,
369+
},
370+
{
371+
name: "no value for optional key",
372+
params: types.ParamValues{
373+
"": {
374+
"requiredkey": privateKeyStr,
375+
},
376+
},
377+
shouldErr: false,
378+
optionalIsNil: true,
379+
},
380+
{
381+
name: "empty string for required key",
382+
params: types.ParamValues{
383+
"": {
384+
"requiredkey": "",
385+
},
386+
},
387+
shouldErr: true,
388+
},
389+
{
390+
name: "no value for required key",
391+
params: types.ParamValues{"": {}},
392+
shouldErr: true,
393+
},
394+
}
395+
396+
for _, tt := range tests {
397+
t.Run(tt.name, func(t *testing.T) {
398+
cfg := struct {
399+
OptionalKey *xtypes.RSAPrivateKey `param:",optional"`
400+
RequiredKey *xtypes.RSAPrivateKey
401+
}{}
402+
403+
if tt.useDefault {
404+
cfg.OptionalKey = &xtypes.RSAPrivateKey{DefaultValue: defaultKey}
405+
}
406+
407+
testProvider := cfgtest.New(tt.params)
408+
defer testProvider.Stop()
409+
410+
_, err := proteus.MustParse(&cfg,
411+
proteus.WithProviders(testProvider))
412+
413+
if tt.shouldErr {
414+
assert.Error(t, err)
415+
} else {
416+
assert.NoError(t, err)
417+
if tt.useDefault {
418+
assert.Equal(t, defaultKey, cfg.OptionalKey.Value())
419+
} else if tt.optionalIsNil {
420+
assert.Equal(t, nil, cfg.OptionalKey.Value())
421+
} else {
422+
assert.NotNil(t, cfg.OptionalKey.Value())
423+
}
424+
425+
if _, ok := tt.params[""]["requiredkey"]; ok && tt.params[""]["requiredkey"] != "" {
426+
assert.NotNil(t, cfg.RequiredKey.Value())
427+
}
428+
}
429+
})
430+
}
431+
}
432+
433+
func generateTestKey(t *testing.T) (*rsa.PrivateKey, string) {
434+
t.Helper()
435+
privateKey, err := rsa.GenerateKey(rand.Reader, 2048)
436+
if err != nil {
437+
t.Fatalf("failed to generate RSA private key: %v", err)
438+
}
439+
privateKeyPEM := &pem.Block{
440+
Type: "RSA PRIVATE KEY",
441+
Bytes: x509.MarshalPKCS1PrivateKey(privateKey),
442+
}
443+
return privateKey, string(pem.EncodeToMemory(privateKeyPEM))
444+
}
445+
446+

types/types.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,9 @@
11
// Package types defines types used by proteus and by code using it.
22
package types
3+
4+
import "errors"
5+
6+
// ErrNoValue can be returned by xtypes on their ValueValid method to indicate
7+
// that the provided value should be considered as if no value was provided at
8+
// all. This allows, for example, to handle empty strings as "no value"
9+
var ErrNoValue = errors.New("no value provided")

xtypes/rsa_priv.go

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,9 +33,13 @@ func (d *RSAPrivateKey) UnmarshalParam(in *string) error {
3333
var privK *rsa.PrivateKey
3434
if in != nil {
3535
var err error
36-
privK, err = parseRSAPriv(*in, d.Base64Encoder)
37-
if err != nil {
38-
return err
36+
if *in == "" {
37+
// empty string results in a nil key
38+
} else {
39+
privK, err = parseRSAPriv(*in, d.Base64Encoder)
40+
if err != nil {
41+
return err
42+
}
3943
}
4044
}
4145

@@ -67,6 +71,9 @@ func (d *RSAPrivateKey) Value() *rsa.PrivateKey {
6771
// ValueValid test if the provided parameter value is valid. Has no side
6872
// effects.
6973
func (d *RSAPrivateKey) ValueValid(s string) error {
74+
if s == "" {
75+
return types.ErrNoValue
76+
}
7077
_, err := parseRSAPriv(s, d.Base64Encoder)
7178
return err
7279
}

0 commit comments

Comments
 (0)