-
Notifications
You must be signed in to change notification settings - Fork 1.8k
[configoptional] Validate nested types #13611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0df1271
99dc11c
e1529cf
8350d6b
de89d75
5a98134
c8682c3
13410c6
d5e9a5b
1c6bf94
1619f7c
68ad97d
f1c3137
0641ebc
0c19972
8ee4bfe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| # Use this changelog template to create an entry for release notes. | ||
|
|
||
| # One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
| change_type: bug_fix | ||
|
|
||
| # The name of the component, or a single word describing the area of concern, (e.g. otlpreceiver) | ||
| component: configoptional | ||
|
|
||
| # A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
| note: Allow validating nested types | ||
|
|
||
| # One or more tracking issues or pull requests related to the change | ||
| issues: [13579] | ||
|
|
||
| # (Optional) One or more lines of additional information to render under the primary note. | ||
| # These lines will be padded with 2 spaces and then inserted directly into the document. | ||
| # Use pipe (|) for multiline entries. | ||
| subtext: '`configoptional.Optional` now implements `xconfmap.Validator`' | ||
|
|
||
| # Optional: The change log or logs in which this entry should be included. | ||
| # e.g. '[user]' or '[user, api]' | ||
| # Include 'user' if the change is relevant to end users. | ||
| # Include 'api' if there is a change to a library API. | ||
| # Default: '[user]' | ||
| change_logs: [user, api] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,13 +4,16 @@ | |
| package configoptional | ||
|
|
||
| import ( | ||
| "errors" | ||
| "fmt" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "go.opentelemetry.io/collector/confmap" | ||
| "go.opentelemetry.io/collector/confmap/confmaptest" | ||
| "go.opentelemetry.io/collector/confmap/xconfmap" | ||
| ) | ||
|
|
||
| type Config[T any] struct { | ||
|
|
@@ -460,3 +463,123 @@ func TestComparePointerMarshal(t *testing.T) { | |
| }) | ||
| } | ||
| } | ||
|
|
||
| type invalid struct{} | ||
|
|
||
| func (invalid) Validate() error { | ||
| return errors.New("invalid") | ||
| } | ||
|
|
||
| var _ xconfmap.Validator = invalid{} | ||
|
|
||
| type hasNested struct { | ||
| CouldBe Optional[invalid] | ||
| } | ||
|
|
||
| func TestOptionalValidate(t *testing.T) { | ||
| require.NoError(t, xconfmap.Validate(hasNested{ | ||
| CouldBe: None[invalid](), | ||
| })) | ||
| require.NoError(t, xconfmap.Validate(hasNested{ | ||
| CouldBe: Default(invalid{}), | ||
| })) | ||
| require.Error(t, xconfmap.Validate(hasNested{ | ||
| CouldBe: Some(invalid{}), | ||
| })) | ||
| } | ||
|
|
||
| type validatedConfig struct { | ||
| Default Optional[optionalConfig] `mapstructure:"default"` | ||
| Some Optional[someConfig] `mapstructure:"some"` | ||
| } | ||
|
|
||
| var _ xconfmap.Validator = (*optionalConfig)(nil) | ||
|
|
||
| type optionalConfig struct { | ||
| StringVal string `mapstructure:"string_val"` | ||
| } | ||
|
|
||
| func (n optionalConfig) Validate() error { | ||
| if n.StringVal == "invalid" { | ||
| return errors.New("field `string_val` cannot be set to `invalid`") | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| type someConfig struct { | ||
| Nested Optional[optionalConfig] `mapstructure:"nested"` | ||
| } | ||
|
|
||
| func newDefaultValidatedConfig() validatedConfig { | ||
| return validatedConfig{ | ||
| Default: Default(optionalConfig{StringVal: "valid"}), | ||
| } | ||
| } | ||
|
|
||
| func newInvalidDefaultConfig() validatedConfig { | ||
| return validatedConfig{ | ||
| Default: Default(optionalConfig{StringVal: "invalid"}), | ||
| } | ||
| } | ||
|
|
||
| func TestOptionalFileValidate(t *testing.T) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find this test a bit confusing. The test struct is pretty convoluted and tests multiple things at once, so it's not easy to convince myself that the test is correct and covers all the cases we care about.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's fair. I've been erring on the side of testing too many things for this PR, but if it weakens the signal that we've covered the cases we want, I'll pare it down some or at least explain why we're doing what we are.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've reduced the struct size and made the test names more descriptive. The only excess now is the nested optional struct, which I left in there just to make sure we're properly calling
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a lot simpler to understand, thank you. Maybe it would be worth it to add a test case for "invalid default + explicit", just in case?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense to me, done. |
||
| cases := []struct { | ||
| name string | ||
| variant string | ||
| cfg func() validatedConfig | ||
| err error | ||
| }{ | ||
| { | ||
| name: "valid default with just key set and no subfields", | ||
| variant: "implicit", | ||
| cfg: newDefaultValidatedConfig, | ||
| }, | ||
| { | ||
| name: "valid default with keys set in default", | ||
| variant: "explicit", | ||
| cfg: newDefaultValidatedConfig, | ||
| }, | ||
| { | ||
| name: "invalid config", | ||
| variant: "invalid", | ||
| cfg: newDefaultValidatedConfig, | ||
| err: errors.New("default: field `string_val` cannot be set to `invalid`\nsome: nested: field `string_val` cannot be set to `invalid`"), | ||
| }, | ||
| { | ||
| name: "invalid default throws an error", | ||
| variant: "implicit", | ||
| cfg: newInvalidDefaultConfig, | ||
| err: errors.New("default: field `string_val` cannot be set to `invalid`"), | ||
| }, | ||
| { | ||
| name: "invalid default does not throw an error when key is not set", | ||
| variant: "no_default", | ||
| cfg: newInvalidDefaultConfig, | ||
| }, | ||
| { | ||
| name: "invalid default invalid default does not throw an error when the value is overridden", | ||
| variant: "explicit", | ||
| cfg: newInvalidDefaultConfig, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range cases { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| conf, err := confmaptest.LoadConf(fmt.Sprintf("testdata/validate_%s.yaml", tt.variant)) | ||
| require.NoError(t, err) | ||
|
|
||
| cfg := tt.cfg() | ||
|
|
||
| err = conf.Unmarshal(&cfg) | ||
| require.NoError(t, err) | ||
|
|
||
| err = xconfmap.Validate(cfg) | ||
| if tt.err == nil { | ||
| require.NoError(t, err) | ||
| } else { | ||
| require.EqualError(t, err, tt.err.Error()) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| default: | ||
| string_val: valid | ||
| some: | ||
| nested: | ||
| string_val: valid |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| default: | ||
| some: | ||
| nested: | ||
| string_val: value1 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| default: | ||
| string_val: invalid | ||
| some: | ||
| nested: | ||
| string_val: invalid |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| some: | ||
| nested: | ||
| string_val: value1 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,67 +11,76 @@ import ( | |
| "github.com/stretchr/testify/require" | ||
|
|
||
| "go.opentelemetry.io/collector/component" | ||
| "go.opentelemetry.io/collector/confmap/xconfmap" | ||
| "go.opentelemetry.io/collector/exporter/exporterhelper/internal/request" | ||
| ) | ||
|
|
||
| func TestConfig_Validate(t *testing.T) { | ||
| cfg := newTestConfig() | ||
| require.NoError(t, cfg.Validate()) | ||
| require.NoError(t, xconfmap.Validate(cfg)) | ||
|
|
||
| cfg.NumConsumers = 0 | ||
| require.EqualError(t, cfg.Validate(), "`num_consumers` must be positive") | ||
| require.EqualError(t, xconfmap.Validate(cfg), "`num_consumers` must be positive") | ||
|
|
||
| cfg = newTestConfig() | ||
| cfg.QueueSize = 0 | ||
| require.EqualError(t, cfg.Validate(), "`queue_size` must be positive") | ||
| require.EqualError(t, xconfmap.Validate(cfg), "`queue_size` must be positive") | ||
|
|
||
| cfg = newTestConfig() | ||
| cfg.QueueSize = 0 | ||
| require.EqualError(t, cfg.Validate(), "`queue_size` must be positive") | ||
| require.EqualError(t, xconfmap.Validate(cfg), "`queue_size` must be positive") | ||
|
|
||
| storageID := component.MustNewID("test") | ||
| cfg = newTestConfig() | ||
| cfg.WaitForResult = true | ||
| cfg.StorageID = &storageID | ||
| require.EqualError(t, cfg.Validate(), "`wait_for_result` is not supported with a persistent queue configured with `storage`") | ||
| require.EqualError(t, xconfmap.Validate(cfg), "`wait_for_result` is not supported with a persistent queue configured with `storage`") | ||
|
|
||
| cfg = newTestConfig() | ||
| cfg.QueueSize = cfg.Batch.Get().MinSize - 1 | ||
| require.EqualError(t, cfg.Validate(), "`min_size` must be less than or equal to `queue_size`") | ||
| require.EqualError(t, xconfmap.Validate(cfg), "`min_size` must be less than or equal to `queue_size`") | ||
|
|
||
| cfg = newTestConfig() | ||
| cfg.Batch.Get().Sizer = request.SizerType{} | ||
| require.EqualError(t, xconfmap.Validate(cfg), "batch: `batch` supports only `items` or `bytes` sizer") | ||
|
|
||
| cfg = newTestConfig() | ||
| cfg.Sizer = request.SizerTypeBytes | ||
| require.NoError(t, cfg.Validate()) | ||
| require.NoError(t, xconfmap.Validate(cfg)) | ||
|
|
||
| // Confirm Validate doesn't return error with invalid config when feature is disabled | ||
| cfg.Enabled = false | ||
| assert.NoError(t, cfg.Validate()) | ||
| assert.NoError(t, xconfmap.Validate(cfg)) | ||
| } | ||
|
|
||
| func TestBatchConfig_Validate(t *testing.T) { | ||
| cfg := newTestBatchConfig() | ||
| require.NoError(t, cfg.Validate()) | ||
| require.NoError(t, xconfmap.Validate(cfg)) | ||
|
|
||
| cfg = newTestBatchConfig() | ||
| cfg.FlushTimeout = 0 | ||
| require.EqualError(t, cfg.Validate(), "`flush_timeout` must be positive") | ||
| require.EqualError(t, xconfmap.Validate(cfg), "`flush_timeout` must be positive") | ||
|
|
||
| cfg = newTestBatchConfig() | ||
| cfg.MinSize = -1 | ||
| require.EqualError(t, cfg.Validate(), "`min_size` must be non-negative") | ||
| require.EqualError(t, xconfmap.Validate(cfg), "`min_size` must be non-negative") | ||
|
|
||
| cfg = newTestBatchConfig() | ||
| cfg.MaxSize = -1 | ||
| require.EqualError(t, cfg.Validate(), "`max_size` must be non-negative") | ||
| require.EqualError(t, xconfmap.Validate(cfg), "`max_size` must be non-negative") | ||
|
|
||
| cfg = newTestBatchConfig() | ||
| cfg.Sizer = request.SizerTypeRequests | ||
| require.EqualError(t, cfg.Validate(), "`batch` supports only `items` or `bytes` sizer") | ||
| require.EqualError(t, xconfmap.Validate(cfg), "`batch` supports only `items` or `bytes` sizer") | ||
|
|
||
| cfg = newTestBatchConfig() | ||
| cfg.Sizer = request.SizerType{} | ||
| require.EqualError(t, xconfmap.Validate(cfg), "`batch` supports only `items` or `bytes` sizer") | ||
|
Comment on lines
+77
to
+78
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚀 |
||
|
|
||
| cfg = newTestBatchConfig() | ||
| cfg.MinSize = 2048 | ||
| cfg.MaxSize = 1024 | ||
| require.EqualError(t, cfg.Validate(), "`max_size` must be greater or equal to `min_size`") | ||
| require.EqualError(t, xconfmap.Validate(cfg), "`max_size` must be greater or equal to `min_size`") | ||
| } | ||
|
|
||
| func newTestBatchConfig() BatchConfig { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.