diff --git a/.changeset/lucky-pandas-wander.md b/.changeset/lucky-pandas-wander.md new file mode 100644 index 00000000..116afbbe --- /dev/null +++ b/.changeset/lucky-pandas-wander.md @@ -0,0 +1,9 @@ +--- +'grafana-prometheus-datasource': patch +--- + +Fix: Stop rejecting loosely-typed jsonData. Since promlib v0.0.13 a datasource provisioned through the API, Terraform or an operator with an off-spec value — `"true"` for a boolean, `"1000"` for a number, `2.4` for a version string — failed to load, and every query, health check and metric lookup against it errored. Such values are now coerced where the type can be read and ignored where it cannot, each logging a warning. `timeInterval`, `queryTimeout` and `httpMethod` are unchanged and still reject a wrong type. + +An ignored value leaves `seriesLimit` unset rather than 0, so it stays distinguishable from a configured limit of zero and readers still apply their own default. + +**Breaking (Go API):** the affected `models.PromOptions` fields change from `string`/`bool`/`float64`/`*int64` to named lenient types (`LenientBool`, `LenientString`, `LenientFloat64`, `LenientExemplarTraceIDDestinations`) with the same underlying types and JSON encoding. Literals still assign and compare as before, but passing one to a `string`, `bool` or `float64` parameter now needs an explicit conversion, e.g. `string(opts.CustomQueryParameters)`. `SeriesLimit` becomes `*LenientFloat64`, matching the frontend's `number`. `HTTPMethod`, `TimeInterval` and `QueryTimeout` keep their existing types. diff --git a/.changeset/thin-weeks-play.md b/.changeset/thin-weeks-play.md new file mode 100644 index 00000000..3cfccc8c --- /dev/null +++ b/.changeset/thin-weeks-play.md @@ -0,0 +1,9 @@ +--- +'promlib': patch +--- + +Fix: Stop rejecting loosely-typed jsonData. Since promlib v0.0.13 a datasource provisioned through the API, Terraform or an operator with an off-spec value — `"true"` for a boolean, `"1000"` for a number, `2.4` for a version string — failed to load, and every query, health check and metric lookup against it errored. Such values are now coerced where the type can be read and ignored where it cannot, each logging a warning. `timeInterval`, `queryTimeout` and `httpMethod` are unchanged and still reject a wrong type. + +An ignored value leaves `seriesLimit` unset rather than 0, so it stays distinguishable from a configured limit of zero and readers still apply their own default. + +**Breaking (Go API):** the affected `models.PromOptions` fields change from `string`/`bool`/`float64`/`*int64` to named lenient types (`LenientBool`, `LenientString`, `LenientFloat64`, `LenientExemplarTraceIDDestinations`) with the same underlying types and JSON encoding. Literals still assign and compare as before, but passing one to a `string`, `bool` or `float64` parameter now needs an explicit conversion, e.g. `string(opts.CustomQueryParameters)`. `SeriesLimit` becomes `*LenientFloat64`, matching the frontend's `number`. `HTTPMethod`, `TimeInterval` and `QueryTimeout` keep their existing types. diff --git a/pkg/promlib/middleware/custom_query_params.go b/pkg/promlib/middleware/custom_query_params.go index fee92597..066c3c70 100644 --- a/pkg/promlib/middleware/custom_query_params.go +++ b/pkg/promlib/middleware/custom_query_params.go @@ -31,10 +31,10 @@ func CustomQueryParameters(logger log.Logger, jsonData *models.PromOptions) sdkh return next } - customQueryParams := jsonData.CustomQueryParameters - warnVal := jsonData.MaxSamplesProcessedWarningThreshold - errVal := jsonData.MaxSamplesProcessedErrorThreshold - queryStatsEnabled := jsonData.QueryStatsEnabled + customQueryParams := string(jsonData.CustomQueryParameters) + warnVal := float64(jsonData.MaxSamplesProcessedWarningThreshold) + errVal := float64(jsonData.MaxSamplesProcessedErrorThreshold) + queryStatsEnabled := bool(jsonData.QueryStatsEnabled) if customQueryParams == "" && warnVal == 0 && errVal == 0 && !queryStatsEnabled { return next diff --git a/pkg/promlib/models/lenient.go b/pkg/promlib/models/lenient.go new file mode 100644 index 00000000..a5cfc23a --- /dev/null +++ b/pkg/promlib/models/lenient.go @@ -0,0 +1,166 @@ +package models + +import ( + "encoding/json" + "strconv" + "strings" + + "github.com/grafana/grafana-plugin-sdk-go/backend/log" +) + +// maxLoggedValueLen keeps a stored object or array from filling a log line. +const maxLoggedValueLen = 64 + +func coerced(toValueType, fromValueType string, data []byte) { + logLenient(fromValueType, toValueType, "coerced", data) +} + +func dropped(toValueType, fromValueType string, data []byte) { + logLenient(fromValueType, toValueType, "dropped", data) +} + +func logLenient(fromValueType, toValueType, outcome string, data []byte) { + value := string(data) + if len(value) > maxLoggedValueLen { + value = value[:maxLoggedValueLen] + "…" + } + log.DefaultLogger.Warn("datasource jsonData value does not match its declared type", + "from", fromValueType, "to", toValueType, "outcome", outcome, "value", value) +} + +// LenientBool also accepts the string and numeric spellings of a boolean. +type LenientBool bool + +func (b *LenientBool) UnmarshalJSON(data []byte) error { + var value bool + if err := json.Unmarshal(data, &value); err == nil { + *b = LenientBool(value) + return nil + } + + var str string + if err := json.Unmarshal(data, &str); err == nil { + if parsed, err := strconv.ParseBool(strings.TrimSpace(str)); err == nil { + *b = LenientBool(parsed) + coerced("bool", "string", data) + return nil + } + dropped("bool", "string", data) + return nil + } + + var number float64 + if err := json.Unmarshal(data, &number); err == nil { + *b = LenientBool(number != 0) + coerced("bool", "float64", data) + return nil + } + + dropped("bool", "unknown", data) + return nil +} + +// LenientString also accepts a scalar, keeping its JSON text, so an identifier or version +// that YAML turned into a number (prometheusVersion: 2.4) is not blanked. +type LenientString string + +func (s *LenientString) UnmarshalJSON(data []byte) error { + var str string + if err := json.Unmarshal(data, &str); err == nil { + *s = LenientString(str) + return nil + } + + var number float64 + if err := json.Unmarshal(data, &number); err == nil { + *s = LenientString(strings.TrimSpace(string(data))) + coerced("string", "float64", data) + return nil + } + + var boolean bool + if err := json.Unmarshal(data, &boolean); err == nil { + *s = LenientString(strings.TrimSpace(string(data))) + coerced("string", "bool", data) + return nil + } + + dropped("string", "unknown", data) + return nil +} + +// LenientFloat64 also accepts a quoted number. +type LenientFloat64 float64 + +func (f *LenientFloat64) UnmarshalJSON(data []byte) error { + value, from, ok := readFloat64(data) + if !ok { + dropped("float64", from, data) + return nil + } + + *f = LenientFloat64(value) + if from != "" { + coerced("float64", from, data) + } + + return nil +} + +// readFloat64 reports what LenientFloat64 reads, which JSON type it came from ("" meaning the +// declared type, so no leniency), and whether it could be read at all. Split out so +// clearDroppedPointers can ask the same question without logging and skewing the counts. +func readFloat64(data []byte) (value float64, from string, ok bool) { + var number float64 + if err := json.Unmarshal(data, &number); err == nil { + // value was expected float64 + return number, "", true + } + + var str string + if err := json.Unmarshal(data, &str); err == nil { + if parsed, err := strconv.ParseFloat(strings.TrimSpace(str), 64); err == nil { + // value was a number string + return parsed, "string", true + } + // value was a non number string (i.e. "ten") + return 0, "string", false + } + + // value was an unsupported value to coerce from. + return 0, "unknown", false +} + +// LenientExemplarTraceIDDestinations ignores a value it cannot read. It does not salvage a +// partial one: a guess would only disagree with what the frontend reads from jsonData. +type LenientExemplarTraceIDDestinations []ExemplarTraceIDDestination + +func (d *LenientExemplarTraceIDDestinations) UnmarshalJSON(data []byte) error { + var destinations []ExemplarTraceIDDestination + if err := json.Unmarshal(data, &destinations); err == nil { + *d = destinations + return nil + } + + dropped("exemplarDestinations", "unknown", data) + return nil +} + +// encoding/json allocates a pointer field before the lenient type sees the value, so a dropped +// value leaves it non-nil at zero — indistinguishable from a stored 0, which for seriesLimit is +// the difference between "apply your own default" and "limit is zero". A lenient type is handed +// a pointer to the allocated value, never to the field, so only the parser can restore nil. +func (o *PromOptions) clearDroppedPointers(data []byte) { + var raw map[string]json.RawMessage + if err := json.Unmarshal(data, &raw); err != nil { + return + } + + if value, ok := raw["seriesLimit"]; ok { + if _, _, readable := readFloat64(value); !readable { + // value was an unsupported value to coerce from + // set to nil so ensure it is not confused with a stored 0 + o.SeriesLimit = nil + } + } +} diff --git a/pkg/promlib/models/lenient_test.go b/pkg/promlib/models/lenient_test.go new file mode 100644 index 00000000..2dc99a08 --- /dev/null +++ b/pkg/promlib/models/lenient_test.go @@ -0,0 +1,415 @@ +package models_test + +import ( + "encoding/json" + "fmt" + "maps" + "net/http" + "reflect" + "slices" + "strings" + "testing" + + "github.com/grafana/grafana-plugin-sdk-go/backend" + "github.com/grafana/grafana-plugin-sdk-go/backend/log" + "github.com/stretchr/testify/require" + + "github.com/grafana/grafana-prometheus-datasource/pkg/promlib/models" +) + +// Mistyping any property #220 declared was harmless before it, so it must not fail the +// datasource now. +func TestParsePromOptions_LooselyTypedJSONData(t *testing.T) { + cases := []struct { + name string + jsonData string + assert func(t *testing.T, opts *models.PromOptions) + }{ + { + name: "booleans stored as quoted strings", + jsonData: `{"seriesEndpoint":"true","disableRecordingRules":"false","oauthPassThru":"1"}`, + assert: func(t *testing.T, opts *models.PromOptions) { + require.True(t, bool(opts.SeriesEndpoint)) + require.False(t, bool(opts.DisableRecordingRules)) + require.True(t, bool(opts.OauthPassThru)) + }, + }, + { + name: "capitalised booleans are not silently inverted", + jsonData: `{"seriesEndpoint":"True","disableMetricsLookup":"TRUE","incrementalQuerying":"False"}`, + assert: func(t *testing.T, opts *models.PromOptions) { + require.True(t, bool(opts.SeriesEndpoint)) + require.True(t, bool(opts.DisableMetricsLookup)) + require.False(t, bool(opts.IncrementalQuerying)) + }, + }, + { + name: "an unrecognised boolean spelling falls back to false", + jsonData: `{"seriesEndpoint":"yes","oauthPassThru":"maybe"}`, + assert: func(t *testing.T, opts *models.PromOptions) { + require.False(t, bool(opts.SeriesEndpoint)) + require.False(t, bool(opts.OauthPassThru)) + }, + }, + { + name: "booleans stored as 0/1", + jsonData: `{"queryStatsEnabled":1,"disableMetricsLookup":0}`, + assert: func(t *testing.T, opts *models.PromOptions) { + require.True(t, bool(opts.QueryStatsEnabled)) + require.False(t, bool(opts.DisableMetricsLookup)) + }, + }, + { + // Promoted fields live on the embedded struct, easy to miss. + name: "promoted fields on the embedded struct are lenient too", + jsonData: `{"manageAlerts":"true","allowAsRecordingRulesTarget":1,"alertmanagerUid":42}`, + assert: func(t *testing.T, opts *models.PromOptions) { + require.True(t, bool(opts.ManageAlerts)) + require.True(t, bool(opts.AllowAsRecordingRulesTarget)) + require.Equal(t, "42", string(opts.AlertmanagerUID)) + }, + }, + { + name: "strings stored as bare numbers keep their text", + jsonData: `{"incrementalQueryOverlapWindow":10,"prometheusVersion":2.4,"customQueryParameters":123}`, + assert: func(t *testing.T, opts *models.PromOptions) { + require.Equal(t, "10", string(opts.IncrementalQueryOverlapWindow)) + require.Equal(t, "2.4", string(opts.PrometheusVersion)) + require.Equal(t, "123", string(opts.CustomQueryParameters)) + }, + }, + { + name: "thresholds stored as quoted numbers", + jsonData: `{"maxSamplesProcessedWarningThreshold":"100000","maxSamplesProcessedErrorThreshold":"200000"}`, + assert: func(t *testing.T, opts *models.PromOptions) { + require.Equal(t, 100000.0, float64(opts.MaxSamplesProcessedWarningThreshold)) + require.Equal(t, 200000.0, float64(opts.MaxSamplesProcessedErrorThreshold)) + }, + }, + { + name: "seriesLimit stored as a quoted number", + jsonData: `{"seriesLimit":"1000"}`, + assert: func(t *testing.T, opts *models.PromOptions) { + require.NotNil(t, opts.SeriesLimit) + require.Equal(t, 1000.0, float64(*opts.SeriesLimit)) + }, + }, + { + // 1000.0 is schema-valid but encoding/json rejects it for an integer field. + name: "seriesLimit stored as a fractional literal", + jsonData: `{"seriesLimit":1000.0}`, + assert: func(t *testing.T, opts *models.PromOptions) { + require.NotNil(t, opts.SeriesLimit) + require.Equal(t, 1000.0, float64(*opts.SeriesLimit)) + }, + }, + { + // Not salvaged into a one-element list: the backend never reads this, so a + // guessed value would only disagree with what the frontend reads. + name: "an exemplar value that is not a list is ignored", + jsonData: `{"exemplarTraceIdDestinations":{"name":"traceID"},"queryStatsEnabled":"true"}`, + assert: func(t *testing.T, opts *models.PromOptions) { + require.Empty(t, opts.ExemplarTraceIDDestinations) + require.True(t, bool(opts.QueryStatsEnabled)) + }, + }, + { + name: "a well-formed exemplar list still decodes", + jsonData: `{"exemplarTraceIdDestinations":[{"name":"traceID","datasourceUid":"abc"}]}`, + assert: func(t *testing.T, opts *models.PromOptions) { + require.Len(t, opts.ExemplarTraceIDDestinations, 1) + require.Equal(t, "traceID", opts.ExemplarTraceIDDestinations[0].Name) + require.Equal(t, "abc", opts.ExemplarTraceIDDestinations[0].DatasourceUID) + }, + }, + { + name: "a value that cannot be read falls back to the zero value", + jsonData: `{"seriesEndpoint":{"a":1},"prometheusVersion":["x"],"queryStatsEnabled":"true"}`, + assert: func(t *testing.T, opts *models.PromOptions) { + require.False(t, bool(opts.SeriesEndpoint)) + require.Empty(t, opts.PrometheusVersion) + require.True(t, bool(opts.QueryStatsEnabled)) + }, + }, + { + name: "a mistyped field does not discard the fields around it", + jsonData: `{"httpMethod":"GET","timeInterval":"30s","seriesLimit":"5","queryStatsEnabled":"true"}`, + assert: func(t *testing.T, opts *models.PromOptions) { + require.Equal(t, http.MethodGet, opts.HTTPMethod) + require.Equal(t, "30s", opts.TimeInterval) + require.NotNil(t, opts.SeriesLimit) + require.Equal(t, 5.0, float64(*opts.SeriesLimit)) + require.True(t, bool(opts.QueryStatsEnabled)) + }, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + opts, err := models.ParsePromOptions(backend.DataSourceInstanceSettings{ + JSONData: []byte(tc.jsonData), + }) + require.NoError(t, err) + tc.assert(t, opts) + }) + } +} + +// Already strict before #220, so they stay strict. +func TestParsePromOptions_PreexistingStrictFieldsStayStrict(t *testing.T) { + for _, jsonData := range []string{ + `{"httpMethod":30}`, + `{"timeInterval":30}`, + `{"queryTimeout":60}`, + } { + t.Run(jsonData, func(t *testing.T) { + _, err := models.ParsePromOptions(backend.DataSourceInstanceSettings{ + JSONData: []byte(jsonData), + }) + require.ErrorContains(t, err, "error unmarshalling JSONData") + }) + } +} + +// Every lenient property must tolerate any stored type. The key list comes from the struct on +// purpose: the original gap was a property nobody remembered to account for. +func TestParsePromOptions_LenientFieldsCannotFailTheDatasource(t *testing.T) { + values := []string{ + `"true"`, `"false"`, `"True"`, `"nonsense"`, `true`, `false`, `1`, `0`, + `30.5`, `30.0`, `60`, `"60"`, `"30s"`, `"abc"`, `""`, `null`, + `[]`, `["a"]`, `[1]`, `{}`, `{"a":1}`, + } + + for _, key := range jsonDataKeys(t) { + if strictJSONDataFields[key] { + continue + } + for _, value := range values { + jsonData := fmt.Sprintf(`{%q:%s}`, key, value) + t.Run(key+"="+value, func(t *testing.T) { + opts, err := models.ParsePromOptions(backend.DataSourceInstanceSettings{ + JSONData: []byte(jsonData), + }) + require.NoError(t, err, "jsonData %s must not fail the datasource", jsonData) + require.NotNil(t, opts) + }) + } + } +} + +// strictJSONDataFields fail the datasource on a type mismatch, by design. Only add a property +// here if it predates #220 or is validated separately; otherwise give it a lenient type. +var strictJSONDataFields = map[string]bool{ + "httpMethod": true, + "timeInterval": true, + "queryTimeout": true, +} + +// jsonDataKeys returns every json key PromOptions declares. Marshalling a zero value lets +// encoding/json resolve promoted keys; no field uses omitempty, so all of them are present. +func jsonDataKeys(t *testing.T) []string { + t.Helper() + + data, err := json.Marshal(models.PromOptions{}) + require.NoError(t, err) + + var fields map[string]json.RawMessage + require.NoError(t, json.Unmarshal(data, &fields)) + require.NotEmpty(t, fields) + + return slices.Sorted(maps.Keys(fields)) +} + +// These warnings are what a strictness migration is decided on, so a correctly typed value must +// stay silent or the signal never goes quiet. +func TestLenientTypes_LogOnlyWhenLenient(t *testing.T) { + cases := []struct { + name string + jsonData string + want []string + }{ + { + name: "correctly typed values log nothing", + jsonData: `{"seriesEndpoint":true,"seriesLimit":10,"prometheusVersion":"2.50.1"}`, + }, + { + name: "null is absence, not a type mismatch", + jsonData: `{"seriesEndpoint":null,"seriesLimit":null,"prometheusVersion":null}`, + }, + { + name: "undeclared properties log nothing", + jsonData: `{"sigV4Auth":123,"someLegacyField":{"a":1}}`, + }, + { + name: "a salvaged boolean is coerced", + jsonData: `{"seriesEndpoint":"True"}`, + want: []string{`string->bool coerced "True"`}, + }, + { + name: "an unreadable boolean is dropped", + jsonData: `{"seriesEndpoint":"banana"}`, + want: []string{`string->bool dropped "banana"`}, + }, + { + name: "every lenient value is reported, not just the first", + jsonData: `{"seriesEndpoint":"true","seriesLimit":"10","prometheusVersion":2.4}`, + want: []string{`string->bool coerced "true"`, `string->float64 coerced "10"`, `float64->string coerced 2.4`}, + }, + { + // Every number shape decodes into a float64 target, so none of these needs leniency. + // An integer field would have rejected 1000.0 and 1e3. + name: "any number shape is accepted without coercion", + jsonData: `{"seriesLimit":1000}`, + }, + { + name: "a fractional literal needs no coercion either", + jsonData: `{"seriesLimit":1000.0}`, + }, + { + name: "nor does exponent notation", + jsonData: `{"seriesLimit":1e3}`, + }, + { + // Same target and outcome as the string case above; only "from" tells them apart. + name: "a number read as a boolean is distinguishable from a string", + jsonData: `{"seriesEndpoint":1}`, + want: []string{`float64->bool coerced 1`}, + }, + { + // Separate labels rather than one combined value, so each aggregates on its own. + name: "a boolean read as a string names bool as the source", + jsonData: `{"prometheusVersion":true}`, + want: []string{`bool->string coerced true`}, + }, + { + // An unreadable string is a different problem from a structurally wrong value, so the + // string source is reported either way. + name: "an unparseable number string is reported as a string, not unknown", + jsonData: `{"seriesLimit":"ten","maxSamplesProcessedWarningThreshold":"lots"}`, + want: []string{`string->float64 dropped "ten"`, `string->float64 dropped "lots"`}, + }, + { + name: "an unusable shape is dropped", + jsonData: `{"seriesEndpoint":["true"],"oauthPassThru":{"a":1}}`, + want: []string{`unknown->bool dropped ["true"]`, `unknown->bool dropped {"a":1}`}, + }, + { + name: "a non-list exemplar value is dropped", + jsonData: `{"exemplarTraceIdDestinations":{"name":"x"}}`, + want: []string{`unknown->exemplarDestinations dropped {"name":"x"}`}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + logged := captureLenientLogs(t, tc.jsonData) + if tc.want == nil { + require.Empty(t, logged) + return + } + require.ElementsMatch(t, tc.want, logged) + }) + } +} + +// captureLenientLogs returns "from->to outcome value" for each warning emitted. It swaps a +// package-level logger, so these cases cannot run in parallel. +func captureLenientLogs(t *testing.T, jsonData string) []string { + t.Helper() + + restore := log.DefaultLogger + // Embed the real logger so anything other than Warn passes through instead of panicking + // on a nil interface. + recorder := &lenientLogRecorder{Logger: restore} + log.DefaultLogger = recorder + defer func() { log.DefaultLogger = restore }() + + _, err := models.ParsePromOptions(backend.DataSourceInstanceSettings{JSONData: []byte(jsonData)}) + require.NoError(t, err) + + return recorder.lenient +} + +// Captures every warning, which is all of them: the lenient types are the only thing that logs. +type lenientLogRecorder struct { + log.Logger + lenient []string +} + +func (r *lenientLogRecorder) Warn(_ string, args ...any) { + fields := map[string]any{} + for i := 0; i+1 < len(args); i += 2 { + if key, ok := args[i].(string); ok { + fields[key] = args[i+1] + } + } + r.lenient = append(r.lenient, + fmt.Sprintf("%v->%v %v %v", fields["from"], fields["to"], fields["outcome"], fields["value"])) +} + +// seriesLimit is a pointer because unset means "apply your own default" where 0 means "limit is +// zero", so an ignored value must leave it unset rather than assert a limit nobody chose. +func TestParsePromOptions_DroppedPointerIsLeftUnset(t *testing.T) { + cases := []struct { + name string + jsonData string + want *float64 + }{ + {name: "absent stays unset", jsonData: `{}`}, + {name: "null stays unset", jsonData: `{"seriesLimit":null}`}, + {name: "ignored string is left unset", jsonData: `{"seriesLimit":"ten"}`}, + {name: "ignored object is left unset", jsonData: `{"seriesLimit":{}}`}, + {name: "ignored array is left unset", jsonData: `{"seriesLimit":[]}`}, + {name: "ignored boolean is left unset", jsonData: `{"seriesLimit":true}`}, + + {name: "a stored number is kept", jsonData: `{"seriesLimit":1000}`, want: ptr(1000)}, + {name: "an explicit zero is kept, not mistaken for unset", jsonData: `{"seriesLimit":0}`, want: ptr(0)}, + {name: "a quoted number is coerced and kept", jsonData: `{"seriesLimit":"1000"}`, want: ptr(1000)}, + {name: "a quoted zero is coerced and kept", jsonData: `{"seriesLimit":"0"}`, want: ptr(0)}, + {name: "a fractional number is kept", jsonData: `{"seriesLimit":1000.5}`, want: ptr(1000.5)}, + {name: "exponent notation is kept", jsonData: `{"seriesLimit":1e3}`, want: ptr(1000)}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + opts, err := models.ParsePromOptions(backend.DataSourceInstanceSettings{ + JSONData: []byte(tc.jsonData), + }) + require.NoError(t, err) + + if tc.want == nil { + require.Nil(t, opts.SeriesLimit) + return + } + require.NotNil(t, opts.SeriesLimit) + require.Equal(t, *tc.want, float64(*opts.SeriesLimit)) + }) + } +} + +func ptr(v float64) *float64 { return &v } + +// clearDroppedPointers names seriesLimit explicitly, so a pointer property added later would +// silently keep its allocated zero. This fails when that happens. +func TestPointerPropertiesAreAccountedFor(t *testing.T) { + corrected := map[string]bool{"seriesLimit": true} + + var walk func(reflect.Type) + walk = func(structType reflect.Type) { + for i := range structType.NumField() { + field := structType.Field(i) + name, _, _ := strings.Cut(field.Tag.Get("json"), ",") + if field.Anonymous && name == "" && field.Type.Kind() == reflect.Struct { + walk(field.Type) + continue + } + if name == "" || name == "-" || field.Type.Kind() != reflect.Pointer { + continue + } + require.True(t, corrected[name], + "%q is a pointer property: a dropped value would leave it non-nil at zero. "+ + "Handle it in clearDroppedPointers and add it here.", name) + } + } + walk(reflect.TypeOf(models.PromOptions{})) +} diff --git a/pkg/promlib/models/settings.go b/pkg/promlib/models/settings.go index b7c1b4a1..8fa8e7e6 100644 --- a/pkg/promlib/models/settings.go +++ b/pkg/promlib/models/settings.go @@ -11,14 +11,16 @@ import ( // DataSourceJsonData mirrors the base @grafana/data DataSourceJsonData interface // that all Grafana datasource jsonData types extend. +// +// All unknown fields before #220, so all lenient. See lenient.go. type DataSourceJsonData struct { - AuthType string `json:"authType"` - DefaultRegion string `json:"defaultRegion"` - Profile string `json:"profile"` - ManageAlerts bool `json:"manageAlerts"` - AllowAsRecordingRulesTarget bool `json:"allowAsRecordingRulesTarget"` - AlertmanagerUID string `json:"alertmanagerUid"` - DisableGrafanaCache bool `json:"disableGrafanaCache"` + AuthType LenientString `json:"authType"` + DefaultRegion LenientString `json:"defaultRegion"` + Profile LenientString `json:"profile"` + ManageAlerts LenientBool `json:"manageAlerts"` + AllowAsRecordingRulesTarget LenientBool `json:"allowAsRecordingRulesTarget"` + AlertmanagerUID LenientString `json:"alertmanagerUid"` + DisableGrafanaCache LenientBool `json:"disableGrafanaCache"` } // PromOptions holds the typed datasource configuration stored in jsonData. @@ -28,27 +30,31 @@ type PromOptions struct { // PromOptions extends DataSourceJsonData. // Even though it is not directly consumed by the prom datasource, it is consumed via plugin-sdk. DataSourceJsonData - HTTPMethod string `json:"httpMethod"` - TimeInterval string `json:"timeInterval"` - QueryTimeout string `json:"queryTimeout"` - CustomQueryParameters string `json:"customQueryParameters"` - MaxSamplesProcessedWarningThreshold float64 `json:"maxSamplesProcessedWarningThreshold"` - MaxSamplesProcessedErrorThreshold float64 `json:"maxSamplesProcessedErrorThreshold"` - QueryStatsEnabled bool `json:"queryStatsEnabled"` + + // Strict: httpMethod is validated below, and timeInterval/queryTimeout were already + // strict before #220. See lenient.go. + HTTPMethod string `json:"httpMethod"` + TimeInterval string `json:"timeInterval"` + QueryTimeout string `json:"queryTimeout"` + + CustomQueryParameters LenientString `json:"customQueryParameters"` + MaxSamplesProcessedWarningThreshold LenientFloat64 `json:"maxSamplesProcessedWarningThreshold"` + MaxSamplesProcessedErrorThreshold LenientFloat64 `json:"maxSamplesProcessedErrorThreshold"` + QueryStatsEnabled LenientBool `json:"queryStatsEnabled"` // Frontend only types - PrometheusType string `json:"prometheusType"` - PrometheusVersion string `json:"prometheusVersion"` - DisableMetricsLookup bool `json:"disableMetricsLookup"` - CacheLevel string `json:"cacheLevel"` - DefaultEditor string `json:"defaultEditor"` - IncrementalQuerying bool `json:"incrementalQuerying"` - IncrementalQueryOverlapWindow string `json:"incrementalQueryOverlapWindow"` - DisableRecordingRules bool `json:"disableRecordingRules"` - OauthPassThru bool `json:"oauthPassThru"` - SeriesEndpoint bool `json:"seriesEndpoint"` - SeriesLimit *int64 `json:"seriesLimit"` - ExemplarTraceIDDestinations []ExemplarTraceIDDestination `json:"exemplarTraceIdDestinations"` + PrometheusType LenientString `json:"prometheusType"` + PrometheusVersion LenientString `json:"prometheusVersion"` + DisableMetricsLookup LenientBool `json:"disableMetricsLookup"` + CacheLevel LenientString `json:"cacheLevel"` + DefaultEditor LenientString `json:"defaultEditor"` + IncrementalQuerying LenientBool `json:"incrementalQuerying"` + IncrementalQueryOverlapWindow LenientString `json:"incrementalQueryOverlapWindow"` + DisableRecordingRules LenientBool `json:"disableRecordingRules"` + OauthPassThru LenientBool `json:"oauthPassThru"` + SeriesEndpoint LenientBool `json:"seriesEndpoint"` + SeriesLimit *LenientFloat64 `json:"seriesLimit"` + ExemplarTraceIDDestinations LenientExemplarTraceIDDestinations `json:"exemplarTraceIdDestinations"` } // ExemplarTraceIDDestination mirrors the frontend ExemplarTraceIdDestination type. @@ -70,6 +76,7 @@ func ParsePromOptions(settings backend.DataSourceInstanceSettings) (*PromOptions if err := json.Unmarshal(data, &opts); err != nil { return nil, fmt.Errorf("error unmarshalling JSONData: %w", err) } + opts.clearDroppedPointers(data) opts.ApplyDefaults() if err := opts.Validate(); err != nil { return nil, err diff --git a/pkg/promlib/models/settings_test.go b/pkg/promlib/models/settings_test.go index 346b718b..22518afa 100644 --- a/pkg/promlib/models/settings_test.go +++ b/pkg/promlib/models/settings_test.go @@ -123,7 +123,7 @@ func TestParsePromOptions_QueryStatsEnabled(t *testing.T) { t.Run(tc.name, func(t *testing.T) { opts, err := models.ParsePromOptions(settingsWithJSON(t, tc.json)) require.NoError(t, err) - require.Equal(t, tc.want, opts.QueryStatsEnabled) + require.Equal(t, tc.want, bool(opts.QueryStatsEnabled)) }) } } @@ -175,7 +175,7 @@ func TestPromOptions_ApplyDefaults(t *testing.T) { } func TestPromOptions_ApplyDefaults_DoesNotMutateUnrelatedFields(t *testing.T) { - seriesLimit := int64(42) + seriesLimit := models.LenientFloat64(42) opts := models.PromOptions{ TimeInterval: "30s", QueryTimeout: "60s", @@ -186,9 +186,9 @@ func TestPromOptions_ApplyDefaults_DoesNotMutateUnrelatedFields(t *testing.T) { require.Equal(t, "30s", opts.TimeInterval) require.Equal(t, "60s", opts.QueryTimeout) - require.Equal(t, "Prometheus", opts.PrometheusType) + require.Equal(t, "Prometheus", string(opts.PrometheusType)) require.NotNil(t, opts.SeriesLimit) - require.Equal(t, int64(42), *opts.SeriesLimit) + require.Equal(t, 42.0, float64(*opts.SeriesLimit)) } func TestPromOptions_Validate(t *testing.T) {