From 4213990d38ab9ebca811e86d5c7551156987ed90 Mon Sep 17 00:00:00 2001 From: Jocelyn Collado-Kuri Date: Tue, 11 Aug 2026 13:59:05 -0700 Subject: [PATCH 1/8] add lenient backend types --- pkg/promlib/middleware/custom_query_params.go | 8 +- pkg/promlib/models/lenient.go | 110 +++++++++ pkg/promlib/models/lenient_test.go | 223 ++++++++++++++++++ pkg/promlib/models/settings.go | 58 +++-- pkg/promlib/models/settings_test.go | 8 +- 5 files changed, 373 insertions(+), 34 deletions(-) create mode 100644 pkg/promlib/models/lenient.go create mode 100644 pkg/promlib/models/lenient_test.go 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..7a07ab58 --- /dev/null +++ b/pkg/promlib/models/lenient.go @@ -0,0 +1,110 @@ +package models + +import ( + "encoding/json" + "strconv" + "strings" +) + +// 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) + } + return nil + } + + var number float64 + if err := json.Unmarshal(data, &number); err == nil { + *b = LenientBool(number != 0) + } + + 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 scalar any + if err := json.Unmarshal(data, &scalar); err == nil { + switch scalar.(type) { + case float64, bool: + *s = LenientString(strings.TrimSpace(string(data))) + } + } + + return nil +} + +// LenientFloat64 also accepts a quoted number. +type LenientFloat64 float64 + +func (f *LenientFloat64) UnmarshalJSON(data []byte) error { + var number float64 + if err := json.Unmarshal(data, &number); err == nil { + *f = LenientFloat64(number) + return nil + } + + var str string + if err := json.Unmarshal(data, &str); err == nil { + if parsed, err := strconv.ParseFloat(strings.TrimSpace(str), 64); err == nil { + *f = LenientFloat64(parsed) + } + } + + return nil +} + +// LenientInt64 also accepts quoted and fractional numbers, truncating them. dsconfig has no +// integer valueType, so a stored 1000.0 is schema-valid but encoding/json rejects it. +type LenientInt64 int64 + +func (i *LenientInt64) UnmarshalJSON(data []byte) error { + var number float64 + if err := json.Unmarshal(data, &number); err == nil { + *i = LenientInt64(number) + return nil + } + + var str string + if err := json.Unmarshal(data, &str); err == nil { + if parsed, err := strconv.ParseFloat(strings.TrimSpace(str), 64); err == nil { + *i = LenientInt64(parsed) + } + } + + return nil +} + +// LenientExemplarTraceIDDestinations ignores a value it cannot read rather than failing the +// unmarshal. It deliberately does not salvage a partial value: the backend never reads this +// property, so a guessed one 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 +} diff --git a/pkg/promlib/models/lenient_test.go b/pkg/promlib/models/lenient_test.go new file mode 100644 index 00000000..e0f0744e --- /dev/null +++ b/pkg/promlib/models/lenient_test.go @@ -0,0 +1,223 @@ +package models_test + +import ( + "encoding/json" + "fmt" + "maps" + "net/http" + "slices" + "testing" + + "github.com/grafana/grafana-plugin-sdk-go/backend" + "github.com/stretchr/testify/require" + + "github.com/grafana/grafana-prometheus-datasource/pkg/promlib/models" +) + +// Provisioning, Terraform and operators all store values whose JSON type does not match +// the struct. 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, int64(1000), int64(*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, int64(1000), int64(*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, int64(5), int64(*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) + }) + } +} + +// These two were already strict before #220, so they stay strict — this shim only absorbs +// the discrepancies #220 introduced. +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 +// rather than a fixed list on purpose: the original gap was a property nobody remembered to +// account for, and a property added without a lenient type fails here instead of shipping. +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. A property only +// belongs here if it was strict before #220 or is validated separately — if a new property +// shows up in the test above, give it a lenient type from lenient.go rather than listing it. +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 the keys promoted from embedded structs, so this stays in step with +// how jsonData is actually decoded. No field uses omitempty, so every key is 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)) +} diff --git a/pkg/promlib/models/settings.go b/pkg/promlib/models/settings.go index b7c1b4a1..ee07216e 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 *LenientInt64 `json:"seriesLimit"` + ExemplarTraceIDDestinations LenientExemplarTraceIDDestinations `json:"exemplarTraceIdDestinations"` } // ExemplarTraceIDDestination mirrors the frontend ExemplarTraceIdDestination type. diff --git a/pkg/promlib/models/settings_test.go b/pkg/promlib/models/settings_test.go index 346b718b..f4a172e2 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.LenientInt64(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, int64(42), int64(*opts.SeriesLimit)) } func TestPromOptions_Validate(t *testing.T) { From 656837296b440493cce8b10c0fbc684626923a3a Mon Sep 17 00:00:00 2001 From: Jocelyn Collado-Kuri Date: Tue, 11 Aug 2026 14:49:58 -0700 Subject: [PATCH 2/8] add logs to keep track of when leniency (either coerced or dropped) was applied --- pkg/promlib/models/lenient.go | 58 ++++++++++++- pkg/promlib/models/lenient_test.go | 129 +++++++++++++++++++++++++++++ 2 files changed, 186 insertions(+), 1 deletion(-) diff --git a/pkg/promlib/models/lenient.go b/pkg/promlib/models/lenient.go index 7a07ab58..b7dfec51 100644 --- a/pkg/promlib/models/lenient.go +++ b/pkg/promlib/models/lenient.go @@ -4,8 +4,30 @@ 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 @@ -20,15 +42,21 @@ func (b *LenientBool) UnmarshalJSON(data []byte) error { 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 } @@ -46,11 +74,18 @@ func (s *LenientString) UnmarshalJSON(data []byte) error { var scalar any if err := json.Unmarshal(data, &scalar); err == nil { switch scalar.(type) { - case float64, bool: + case float64: + *s = LenientString(strings.TrimSpace(string(data))) + coerced("string", "float64", data) + return nil + case bool: *s = LenientString(strings.TrimSpace(string(data))) + coerced("string", "bool", data) + return nil } } + dropped("string", "unknown", data) return nil } @@ -68,9 +103,14 @@ func (f *LenientFloat64) UnmarshalJSON(data []byte) error { if err := json.Unmarshal(data, &str); err == nil { if parsed, err := strconv.ParseFloat(strings.TrimSpace(str), 64); err == nil { *f = LenientFloat64(parsed) + coerced("float64", "string", data) + return nil } + dropped("float64", "string", data) + return nil } + dropped("float64", "unknown", data) return nil } @@ -79,9 +119,18 @@ func (f *LenientFloat64) UnmarshalJSON(data []byte) error { type LenientInt64 int64 func (i *LenientInt64) UnmarshalJSON(data []byte) error { + var whole int64 + if err := json.Unmarshal(data, &whole); err == nil { + *i = LenientInt64(whole) + return nil + } + + // A fractional literal is rejected by encoding/json even when the value is whole, so + // reaching here means 1000.0 or 1000.5 rather than 1000: leniency either way. var number float64 if err := json.Unmarshal(data, &number); err == nil { *i = LenientInt64(number) + coerced("int64", "float64", data) return nil } @@ -89,9 +138,14 @@ func (i *LenientInt64) UnmarshalJSON(data []byte) error { if err := json.Unmarshal(data, &str); err == nil { if parsed, err := strconv.ParseFloat(strings.TrimSpace(str), 64); err == nil { *i = LenientInt64(parsed) + coerced("int64", "string", data) + return nil } + dropped("int64", "string", data) + return nil } + dropped("int64", "unknown", data) return nil } @@ -104,7 +158,9 @@ 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 } diff --git a/pkg/promlib/models/lenient_test.go b/pkg/promlib/models/lenient_test.go index e0f0744e..9acf2ca8 100644 --- a/pkg/promlib/models/lenient_test.go +++ b/pkg/promlib/models/lenient_test.go @@ -9,6 +9,7 @@ import ( "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" @@ -221,3 +222,131 @@ func jsonDataKeys(t *testing.T) []string { return slices.Sorted(maps.Keys(fields)) } + +// The warning is the record a strictness migration is decided on: aggregate by type in Loki, +// and retire a lenient type once it stops appearing. A correctly typed value is not leniency +// and 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->int64 coerced "10"`, `float64->string coerced 2.4`}, + }, + { + // An integer literal needs no leniency, but a fractional one does even when the + // value is whole, because encoding/json rejects it for an integer field. + name: "a whole number is silent while a fractional one is coerced", + jsonData: `{"seriesLimit":1000}`, + }, + { + name: "a fractional literal is coerced for an integer property", + jsonData: `{"seriesLimit":1000.0}`, + want: []string{`float64->int64 coerced 1000.0`}, + }, + { + // 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`}, + }, + { + // float64 and bool are separate labels rather than one combined value, so each + // can be aggregated on its own. + name: "a boolean read as a string names bool as the source", + jsonData: `{"prometheusVersion":true}`, + want: []string{`bool->string coerced true`}, + }, + { + // A value that is the right JSON type but unreadable is a different problem from + // one that is structurally wrong, 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->int64 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 parses jsonData with the package logger swapped for a recorder, and +// returns "from->to outcome value" for each warning emitted. Swapping a package-level logger +// means 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 +} + +// lenientLogRecorder captures every warning, which is every warning the lenient types emit: +// they are the only thing in this package 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"])) +} From 6fc1278a6dd586b30da7eebb9e946e50f683f3ee Mon Sep 17 00:00:00 2001 From: Jocelyn Collado-Kuri Date: Tue, 11 Aug 2026 14:53:14 -0700 Subject: [PATCH 3/8] changeset --- .changeset/lucky-pandas-wander.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 .changeset/lucky-pandas-wander.md diff --git a/.changeset/lucky-pandas-wander.md b/.changeset/lucky-pandas-wander.md new file mode 100644 index 00000000..b86563d5 --- /dev/null +++ b/.changeset/lucky-pandas-wander.md @@ -0,0 +1,8 @@ +--- +'promlib': patch +'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"` or `1000.0` for a number — 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. + +**Breaking (Go API):** the affected `models.PromOptions` fields change from `string`/`bool`/`float64`/`*int64` to named lenient types (`LenientBool`, `LenientString`, `LenientFloat64`, `LenientInt64`, `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)`. From 64d81f940dd6cacda8fbbd0bb08ce3496600104e Mon Sep 17 00:00:00 2001 From: Jocelyn Collado-Kuri Date: Tue, 11 Aug 2026 15:52:57 -0700 Subject: [PATCH 4/8] update grafana prometheus --- .changeset/lucky-pandas-wander.md | 1 - .changeset/thin-weeks-play.md | 7 +++++++ 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 .changeset/thin-weeks-play.md diff --git a/.changeset/lucky-pandas-wander.md b/.changeset/lucky-pandas-wander.md index b86563d5..ce498c27 100644 --- a/.changeset/lucky-pandas-wander.md +++ b/.changeset/lucky-pandas-wander.md @@ -1,5 +1,4 @@ --- -'promlib': patch 'grafana-prometheus-datasource': patch --- diff --git a/.changeset/thin-weeks-play.md b/.changeset/thin-weeks-play.md new file mode 100644 index 00000000..7837093c --- /dev/null +++ b/.changeset/thin-weeks-play.md @@ -0,0 +1,7 @@ +--- +'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"` or `1000.0` for a number — 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. + +**Breaking (Go API):** the affected `models.PromOptions` fields change from `string`/`bool`/`float64`/`*int64` to named lenient types (`LenientBool`, `LenientString`, `LenientFloat64`, `LenientInt64`, `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)`. From 969fa0e74fffa034d095ce46af104941b2e85606 Mon Sep 17 00:00:00 2001 From: Jocelyn Collado-Kuri Date: Tue, 11 Aug 2026 15:53:46 -0700 Subject: [PATCH 5/8] match other unmarshal functions structure for --- pkg/promlib/models/lenient.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/pkg/promlib/models/lenient.go b/pkg/promlib/models/lenient.go index b7dfec51..37feefbe 100644 --- a/pkg/promlib/models/lenient.go +++ b/pkg/promlib/models/lenient.go @@ -71,18 +71,18 @@ func (s *LenientString) UnmarshalJSON(data []byte) error { return nil } - var scalar any - if err := json.Unmarshal(data, &scalar); err == nil { - switch scalar.(type) { - case float64: - *s = LenientString(strings.TrimSpace(string(data))) - coerced("string", "float64", data) - return nil - case bool: - *s = LenientString(strings.TrimSpace(string(data))) - coerced("string", "bool", data) - 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) From d06008af6f2895b630038ffd8fd190f64efb93fc Mon Sep 17 00:00:00 2001 From: Jocelyn Collado-Kuri Date: Wed, 12 Aug 2026 13:43:14 -0700 Subject: [PATCH 6/8] drop lenient int and only use lenient float instead --- pkg/promlib/models/lenient.go | 35 ----------------------------- pkg/promlib/models/lenient_test.go | 24 +++++++++++--------- pkg/promlib/models/settings.go | 2 +- pkg/promlib/models/settings_test.go | 4 ++-- 4 files changed, 17 insertions(+), 48 deletions(-) diff --git a/pkg/promlib/models/lenient.go b/pkg/promlib/models/lenient.go index 37feefbe..ea4110cf 100644 --- a/pkg/promlib/models/lenient.go +++ b/pkg/promlib/models/lenient.go @@ -114,41 +114,6 @@ func (f *LenientFloat64) UnmarshalJSON(data []byte) error { return nil } -// LenientInt64 also accepts quoted and fractional numbers, truncating them. dsconfig has no -// integer valueType, so a stored 1000.0 is schema-valid but encoding/json rejects it. -type LenientInt64 int64 - -func (i *LenientInt64) UnmarshalJSON(data []byte) error { - var whole int64 - if err := json.Unmarshal(data, &whole); err == nil { - *i = LenientInt64(whole) - return nil - } - - // A fractional literal is rejected by encoding/json even when the value is whole, so - // reaching here means 1000.0 or 1000.5 rather than 1000: leniency either way. - var number float64 - if err := json.Unmarshal(data, &number); err == nil { - *i = LenientInt64(number) - coerced("int64", "float64", data) - return nil - } - - var str string - if err := json.Unmarshal(data, &str); err == nil { - if parsed, err := strconv.ParseFloat(strings.TrimSpace(str), 64); err == nil { - *i = LenientInt64(parsed) - coerced("int64", "string", data) - return nil - } - dropped("int64", "string", data) - return nil - } - - dropped("int64", "unknown", data) - return nil -} - // LenientExemplarTraceIDDestinations ignores a value it cannot read rather than failing the // unmarshal. It deliberately does not salvage a partial value: the backend never reads this // property, so a guessed one would only disagree with what the frontend reads from jsonData. diff --git a/pkg/promlib/models/lenient_test.go b/pkg/promlib/models/lenient_test.go index 9acf2ca8..6abf0199 100644 --- a/pkg/promlib/models/lenient_test.go +++ b/pkg/promlib/models/lenient_test.go @@ -90,7 +90,7 @@ func TestParsePromOptions_LooselyTypedJSONData(t *testing.T) { jsonData: `{"seriesLimit":"1000"}`, assert: func(t *testing.T, opts *models.PromOptions) { require.NotNil(t, opts.SeriesLimit) - require.Equal(t, int64(1000), int64(*opts.SeriesLimit)) + require.Equal(t, 1000.0, float64(*opts.SeriesLimit)) }, }, { @@ -99,7 +99,7 @@ func TestParsePromOptions_LooselyTypedJSONData(t *testing.T) { jsonData: `{"seriesLimit":1000.0}`, assert: func(t *testing.T, opts *models.PromOptions) { require.NotNil(t, opts.SeriesLimit) - require.Equal(t, int64(1000), int64(*opts.SeriesLimit)) + require.Equal(t, 1000.0, float64(*opts.SeriesLimit)) }, }, { @@ -137,7 +137,7 @@ func TestParsePromOptions_LooselyTypedJSONData(t *testing.T) { require.Equal(t, http.MethodGet, opts.HTTPMethod) require.Equal(t, "30s", opts.TimeInterval) require.NotNil(t, opts.SeriesLimit) - require.Equal(t, int64(5), int64(*opts.SeriesLimit)) + require.Equal(t, 5.0, float64(*opts.SeriesLimit)) require.True(t, bool(opts.QueryStatsEnabled)) }, }, @@ -257,18 +257,22 @@ func TestLenientTypes_LogOnlyWhenLenient(t *testing.T) { { 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->int64 coerced "10"`, `float64->string coerced 2.4`}, + want: []string{`string->bool coerced "true"`, `string->float64 coerced "10"`, `float64->string coerced 2.4`}, }, { - // An integer literal needs no leniency, but a fractional one does even when the - // value is whole, because encoding/json rejects it for an integer field. - name: "a whole number is silent while a fractional one is coerced", + // Every JSON number shape decodes into a float64 target, so none of these needs + // leniency. An integer field would have rejected 1000.0 and 1e3, which is why + // seriesLimit is a float: it mirrors the frontend's `number` and stays quiet. + name: "any number shape is accepted without coercion", jsonData: `{"seriesLimit":1000}`, }, { - name: "a fractional literal is coerced for an integer property", + name: "a fractional literal needs no coercion either", jsonData: `{"seriesLimit":1000.0}`, - want: []string{`float64->int64 coerced 1000.0`}, + }, + { + name: "nor does exponent notation", + jsonData: `{"seriesLimit":1e3}`, }, { // Same target and outcome as the string case above; only "from" tells them apart. @@ -288,7 +292,7 @@ func TestLenientTypes_LogOnlyWhenLenient(t *testing.T) { // one that is structurally wrong, 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->int64 dropped "ten"`, `string->float64 dropped "lots"`}, + want: []string{`string->float64 dropped "ten"`, `string->float64 dropped "lots"`}, }, { name: "an unusable shape is dropped", diff --git a/pkg/promlib/models/settings.go b/pkg/promlib/models/settings.go index ee07216e..7d63fc12 100644 --- a/pkg/promlib/models/settings.go +++ b/pkg/promlib/models/settings.go @@ -53,7 +53,7 @@ type PromOptions struct { DisableRecordingRules LenientBool `json:"disableRecordingRules"` OauthPassThru LenientBool `json:"oauthPassThru"` SeriesEndpoint LenientBool `json:"seriesEndpoint"` - SeriesLimit *LenientInt64 `json:"seriesLimit"` + SeriesLimit *LenientFloat64 `json:"seriesLimit"` ExemplarTraceIDDestinations LenientExemplarTraceIDDestinations `json:"exemplarTraceIdDestinations"` } diff --git a/pkg/promlib/models/settings_test.go b/pkg/promlib/models/settings_test.go index f4a172e2..22518afa 100644 --- a/pkg/promlib/models/settings_test.go +++ b/pkg/promlib/models/settings_test.go @@ -175,7 +175,7 @@ func TestPromOptions_ApplyDefaults(t *testing.T) { } func TestPromOptions_ApplyDefaults_DoesNotMutateUnrelatedFields(t *testing.T) { - seriesLimit := models.LenientInt64(42) + seriesLimit := models.LenientFloat64(42) opts := models.PromOptions{ TimeInterval: "30s", QueryTimeout: "60s", @@ -188,7 +188,7 @@ func TestPromOptions_ApplyDefaults_DoesNotMutateUnrelatedFields(t *testing.T) { require.Equal(t, "60s", opts.QueryTimeout) require.Equal(t, "Prometheus", string(opts.PrometheusType)) require.NotNil(t, opts.SeriesLimit) - require.Equal(t, int64(42), int64(*opts.SeriesLimit)) + require.Equal(t, 42.0, float64(*opts.SeriesLimit)) } func TestPromOptions_Validate(t *testing.T) { From 44c7baee6918f8f5963160506f59f57d3d6592bb Mon Sep 17 00:00:00 2001 From: Jocelyn Collado-Kuri Date: Wed, 12 Aug 2026 18:10:25 -0700 Subject: [PATCH 7/8] support distinguising between empty limits and an actual 0 limit. --- .changeset/lucky-pandas-wander.md | 6 +- pkg/promlib/models/lenient.go | 59 ++++++++++++--- pkg/promlib/models/lenient_test.go | 115 ++++++++++++++++++++++------- pkg/promlib/models/settings.go | 1 + 4 files changed, 139 insertions(+), 42 deletions(-) diff --git a/.changeset/lucky-pandas-wander.md b/.changeset/lucky-pandas-wander.md index ce498c27..116afbbe 100644 --- a/.changeset/lucky-pandas-wander.md +++ b/.changeset/lucky-pandas-wander.md @@ -2,6 +2,8 @@ '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"` or `1000.0` for a number — 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. +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. -**Breaking (Go API):** the affected `models.PromOptions` fields change from `string`/`bool`/`float64`/`*int64` to named lenient types (`LenientBool`, `LenientString`, `LenientFloat64`, `LenientInt64`, `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)`. +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/models/lenient.go b/pkg/promlib/models/lenient.go index ea4110cf..a5cfc23a 100644 --- a/pkg/promlib/models/lenient.go +++ b/pkg/promlib/models/lenient.go @@ -93,30 +93,46 @@ func (s *LenientString) UnmarshalJSON(data []byte) error { 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 { - *f = LenientFloat64(number) - return 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 { - *f = LenientFloat64(parsed) - coerced("float64", "string", data) - return nil + // value was a number string + return parsed, "string", true } - dropped("float64", "string", data) - return nil + // value was a non number string (i.e. "ten") + return 0, "string", false } - dropped("float64", "unknown", data) - return nil + // value was an unsupported value to coerce from. + return 0, "unknown", false } -// LenientExemplarTraceIDDestinations ignores a value it cannot read rather than failing the -// unmarshal. It deliberately does not salvage a partial value: the backend never reads this -// property, so a guessed one would only disagree with what the frontend reads from jsonData. +// 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 { @@ -129,3 +145,22 @@ func (d *LenientExemplarTraceIDDestinations) UnmarshalJSON(data []byte) error { 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 index 6abf0199..2dc99a08 100644 --- a/pkg/promlib/models/lenient_test.go +++ b/pkg/promlib/models/lenient_test.go @@ -5,7 +5,9 @@ import ( "fmt" "maps" "net/http" + "reflect" "slices" + "strings" "testing" "github.com/grafana/grafana-plugin-sdk-go/backend" @@ -15,9 +17,8 @@ import ( "github.com/grafana/grafana-prometheus-datasource/pkg/promlib/models" ) -// Provisioning, Terraform and operators all store values whose JSON type does not match -// the struct. Mistyping any property #220 declared was harmless before it, so it must not -// fail the datasource now. +// 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 @@ -154,8 +155,7 @@ func TestParsePromOptions_LooselyTypedJSONData(t *testing.T) { } } -// These two were already strict before #220, so they stay strict — this shim only absorbs -// the discrepancies #220 introduced. +// Already strict before #220, so they stay strict. func TestParsePromOptions_PreexistingStrictFieldsStayStrict(t *testing.T) { for _, jsonData := range []string{ `{"httpMethod":30}`, @@ -171,9 +171,8 @@ func TestParsePromOptions_PreexistingStrictFieldsStayStrict(t *testing.T) { } } -// Every lenient property must tolerate any stored type. The key list comes from the struct -// rather than a fixed list on purpose: the original gap was a property nobody remembered to -// account for, and a property added without a lenient type fails here instead of shipping. +// 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`, @@ -198,9 +197,8 @@ func TestParsePromOptions_LenientFieldsCannotFailTheDatasource(t *testing.T) { } } -// strictJSONDataFields fail the datasource on a type mismatch, by design. A property only -// belongs here if it was strict before #220 or is validated separately — if a new property -// shows up in the test above, give it a lenient type from lenient.go rather than listing it. +// 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, @@ -208,8 +206,7 @@ var strictJSONDataFields = map[string]bool{ } // jsonDataKeys returns every json key PromOptions declares. Marshalling a zero value lets -// encoding/json resolve the keys promoted from embedded structs, so this stays in step with -// how jsonData is actually decoded. No field uses omitempty, so every key is present. +// encoding/json resolve promoted keys; no field uses omitempty, so all of them are present. func jsonDataKeys(t *testing.T) []string { t.Helper() @@ -223,9 +220,8 @@ func jsonDataKeys(t *testing.T) []string { return slices.Sorted(maps.Keys(fields)) } -// The warning is the record a strictness migration is decided on: aggregate by type in Loki, -// and retire a lenient type once it stops appearing. A correctly typed value is not leniency -// and must stay silent, or the signal never goes quiet. +// 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 @@ -260,9 +256,8 @@ func TestLenientTypes_LogOnlyWhenLenient(t *testing.T) { want: []string{`string->bool coerced "true"`, `string->float64 coerced "10"`, `float64->string coerced 2.4`}, }, { - // Every JSON number shape decodes into a float64 target, so none of these needs - // leniency. An integer field would have rejected 1000.0 and 1e3, which is why - // seriesLimit is a float: it mirrors the frontend's `number` and stays quiet. + // 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}`, }, @@ -281,15 +276,14 @@ func TestLenientTypes_LogOnlyWhenLenient(t *testing.T) { want: []string{`float64->bool coerced 1`}, }, { - // float64 and bool are separate labels rather than one combined value, so each - // can be aggregated on its own. + // 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`}, }, { - // A value that is the right JSON type but unreadable is a different problem from - // one that is structurally wrong, so the string source is reported either way. + // 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"`}, @@ -318,9 +312,8 @@ func TestLenientTypes_LogOnlyWhenLenient(t *testing.T) { } } -// captureLenientLogs parses jsonData with the package logger swapped for a recorder, and -// returns "from->to outcome value" for each warning emitted. Swapping a package-level logger -// means these cases cannot run in parallel. +// 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() @@ -337,8 +330,7 @@ func captureLenientLogs(t *testing.T, jsonData string) []string { return recorder.lenient } -// lenientLogRecorder captures every warning, which is every warning the lenient types emit: -// they are the only thing in this package that logs. +// Captures every warning, which is all of them: the lenient types are the only thing that logs. type lenientLogRecorder struct { log.Logger lenient []string @@ -354,3 +346,70 @@ func (r *lenientLogRecorder) Warn(_ string, args ...any) { 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 7d63fc12..8fa8e7e6 100644 --- a/pkg/promlib/models/settings.go +++ b/pkg/promlib/models/settings.go @@ -76,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 From bf7adfbbad44a7eef54a0406b0bfc2e83045b308 Mon Sep 17 00:00:00 2001 From: Jocelyn Collado-Kuri Date: Wed, 12 Aug 2026 18:16:00 -0700 Subject: [PATCH 8/8] update changeset --- .changeset/thin-weeks-play.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/.changeset/thin-weeks-play.md b/.changeset/thin-weeks-play.md index 7837093c..3cfccc8c 100644 --- a/.changeset/thin-weeks-play.md +++ b/.changeset/thin-weeks-play.md @@ -2,6 +2,8 @@ '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"` or `1000.0` for a number — 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. +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. -**Breaking (Go API):** the affected `models.PromOptions` fields change from `string`/`bool`/`float64`/`*int64` to named lenient types (`LenientBool`, `LenientString`, `LenientFloat64`, `LenientInt64`, `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)`. +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.