fix(saved-views): skip saved views whose data no longer decodes

saved_view.data was scanned straight into a typed SavedViewData, so bun
decoded the spec during the scan itself. The spec decode is strict --
QueryEnvelope rejects unknown fields and unknown query types, and RequestType
rejects values outside its enum -- so a single row written by an older build
failed the whole scan, and List wrapped it as an internal error, hiding every
other view in the org.

List now scans into RawStorableSavedView, which keeps the data as text, and the
module decodes per row, logging and skipping a view that no longer decodes.
Create, get, update and delete keep using StorableSavedView unchanged.

Assisted-by: Claude Opus 5

pkg/modules/savedview/implsavedview/module.go

@@ -2,8 +2,10 @@ package implsavedview
 
 import (
 	"context"
+	"log/slog"
 
 	"github.com/SigNoz/signoz/pkg/errors"
+	"github.com/SigNoz/signoz/pkg/factory"
 	"github.com/SigNoz/signoz/pkg/modules/savedview"
 	"github.com/SigNoz/signoz/pkg/types/authtypes"
 	"github.com/SigNoz/signoz/pkg/types/savedviewtypes"
@@ -11,11 +13,15 @@ import (
 )
 
 type module struct {
-	store savedviewtypes.Store
+	store    savedviewtypes.Store
+	settings factory.ScopedProviderSettings
 }
 
-func NewModule(store savedviewtypes.Store) savedview.Module {
-	return &module{store: store}
+func NewModule(store savedviewtypes.Store, settings factory.ProviderSettings) savedview.Module {
+	return &module{
+		store:    store,
+		settings: factory.NewScopedProviderSettings(settings, "github.com/SigNoz/signoz/pkg/modules/savedview/implsavedview"),
+	}
 }
 
 func (module *module) GetViewsForFilters(ctx context.Context, orgID string, source savedviewtypes.Source, name string) ([]*savedviewtypes.SavedView, error) {
@@ -23,7 +29,18 @@ func (module *module) GetViewsForFilters(ctx context.Context, orgID string, sour
 	if err != nil {
 		return nil, err
 	}
-	return savedviewtypes.NewSavedViewsFromStorableSavedViews(storables), nil
+
+	views := make([]*savedviewtypes.SavedView, 0, len(storables))
+	for _, storable := range storables {
+		view, err := storable.ToSavedView()
+		if err != nil {
+			module.settings.Logger().WarnContext(ctx, "saved view data did not decode", slog.String("saved_view_id", storable.ID.StringValue()), slog.Any("error", err))
+			continue
+		}
+		views = append(views, view)
+	}
+
+	return views, nil
 }
 
 func (module *module) CreateView(ctx context.Context, orgID string, view savedviewtypes.PostableSavedView) (valuer.UUID, error) {

pkg/modules/savedview/implsavedview/module_test.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/DATA-DOG/go-sqlmock"
 	"github.com/SigNoz/signoz/pkg/errors"
+	"github.com/SigNoz/signoz/pkg/instrumentation/instrumentationtest"
 	"github.com/SigNoz/signoz/pkg/modules/savedview"
 	"github.com/SigNoz/signoz/pkg/modules/savedview/implsavedview"
 	"github.com/SigNoz/signoz/pkg/sqlstore"
@@ -23,7 +24,7 @@ import (
 func newTestStore() (savedview.Module, *savedviewtypestest.StoreTest) {
 	sqlStore := sqlstoretest.New(sqlstore.Config{Provider: "sqlite"}, sqlmock.QueryMatcherRegexp)
 	store := implsavedview.NewStore(sqlStore)
-	return implsavedview.NewModule(store), savedviewtypestest.New(store, sqlStore.Mock())
+	return implsavedview.NewModule(store, instrumentationtest.New().ToProviderSettings()), savedviewtypestest.New(store, sqlStore.Mock())
 }
 
 func testPostableSavedView(name string, source savedviewtypes.Source) savedviewtypes.PostableSavedView {
@@ -294,6 +295,50 @@ func TestModule_GetViewsForFilters(t *testing.T) {
 	require.NoError(t, st.AssertExpectations())
 }
 
+// A view whose stored data no longer decodes must not take the rest of the list
+// down with it.
+func TestModule_UndecodableViewIsSkippedFromList(t *testing.T) {
+	orgID := valuer.GenerateUUID().StringValue()
+	ctx := contextWithClaims(orgID, "creator@signoz.io")
+
+	const undecodableData = `{"schemaVersion":"v2","spec":{"displayName":"Broken View","panelType":"list","requestType":"raw",` +
+		`"queries":[{"type":"builder_query","spec":{"signal":"logs","sinceRemovedField":1}}]}}`
+
+	broken := testSavedView(orgID, valuer.GenerateUUID(), "creator@signoz.io", testPostableSavedView("broken-view", savedviewtypes.SourceLogs))
+	healthy := testSavedView(orgID, valuer.GenerateUUID(), "creator@signoz.io", testPostableSavedView("healthy-view", savedviewtypes.SourceLogs))
+
+	t.Run("list serves every view that still decodes", func(t *testing.T) {
+		m, st := newTestStore()
+
+		st.ExpectListRows(orgID,
+			savedviewtypestest.Row{View: broken, Data: undecodableData},
+			savedviewtypestest.Row{View: healthy},
+		)
+
+		views, err := m.GetViewsForFilters(ctx, orgID, savedviewtypes.SourceLogs, "")
+		require.NoError(t, err)
+		require.Len(t, views, 1)
+		assert.Equal(t, "healthy-view", views[0].Name)
+		assert.Len(t, views[0].Spec.Queries, 1)
+
+		require.NoError(t, st.AssertExpectations())
+	})
+
+	// Get still decodes during the scan, so it fails for the view itself. That is
+	// as far as the damage goes: nothing else in the org is affected.
+	t.Run("get fails for the view itself", func(t *testing.T) {
+		m, st := newTestStore()
+
+		st.ExpectGetRows(orgID, broken.ID, savedviewtypestest.Row{View: broken, Data: undecodableData})
+
+		_, err := m.GetView(ctx, orgID, broken.ID)
+		require.Error(t, err)
+		assert.True(t, errors.Ast(err, errors.TypeNotFound), "expected a not-found error, got %v", err)
+
+		require.NoError(t, st.AssertExpectations())
+	})
+}
+
 func TestModule_Collect(t *testing.T) {
 	m, st := newTestStore()
 

pkg/modules/savedview/implsavedview/store.go

@@ -79,8 +79,8 @@ func (store *store) Delete(ctx context.Context, orgID string, id valuer.UUID) er
 	return nil
 }
 
-func (store *store) List(ctx context.Context, orgID string, source savedviewtypes.Source, name string) ([]*savedviewtypes.StorableSavedView, error) {
-	var storables []*savedviewtypes.StorableSavedView
+func (store *store) List(ctx context.Context, orgID string, source savedviewtypes.Source, name string) ([]*savedviewtypes.RawStorableSavedView, error) {
+	var storables []*savedviewtypes.RawStorableSavedView
 	q := store.sqlstore.BunDB().NewSelect().Model(&storables).
 		Where("org_id = ?", orgID).
 		Where("name LIKE ?", "%"+name+"%")

pkg/signoz/module.go

@@ -139,7 +139,7 @@ func NewModules(
 		OrgGetter:           orgGetter,
 		OrgSetter:           orgSetter,
 		Preference:          implpreference.NewModule(implpreference.NewStore(sqlstore), preferencetypes.NewAvailablePreference()),
-		SavedView:           implsavedview.NewModule(implsavedview.NewStore(sqlstore)),
+		SavedView:           implsavedview.NewModule(implsavedview.NewStore(sqlstore), providerSettings),
 		Apdex:               implapdex.NewModule(sqlstore),
 		Dashboard:           dashboard,
 		UserSetter:          userSetter,

pkg/types/savedviewtypes/savedview.go

@@ -2,6 +2,7 @@ package savedviewtypes
 
 import (
 	"crypto/rand"
+	"encoding/json"
 	"strings"
 	"time"
 
@@ -53,7 +54,39 @@ type StorableSavedView struct {
 }
 
 func (s *StorableSavedView) ToSavedView() *SavedView {
-	spec := s.Data.Spec
+	return newSavedView(s.Identifiable, s.TimeAuditable, s.UserAuditable, s.OrgID, s.Name, s.Source, s.Data)
+}
+
+// RawStorableSavedView is a saved view row with its data left as text. Listing
+// scans into this instead of StorableSavedView because decoding a spec is strict
+// enough to reject data written by older builds, which would fail the whole
+// query over a single row.
+type RawStorableSavedView struct {
+	bun.BaseModel `bun:"table:saved_view"`
+
+	types.Identifiable
+	types.TimeAuditable
+	types.UserAuditable
+	OrgID  string `bun:"org_id,notnull"`
+	Name   string `bun:"name,type:text,notnull"`
+	Source Source `bun:"source,type:text,notnull"`
+	Data   string `bun:"data,type:text,notnull"`
+}
+
+// ToSavedView decodes the stored data. It fails for a view whose data is no
+// longer a valid spec, leaving it to the caller to skip that view rather than
+// give up on the rest.
+func (s *RawStorableSavedView) ToSavedView() (*SavedView, error) {
+	var data SavedViewData
+	if err := json.Unmarshal([]byte(s.Data), &data); err != nil {
+		return nil, errors.WrapInvalidInputf(err, ErrCodeSavedViewInvalidInput, "error in unmarshalling saved view data")
+	}
+
+	return newSavedView(s.Identifiable, s.TimeAuditable, s.UserAuditable, s.OrgID, s.Name, s.Source, data), nil
+}
+
+func newSavedView(identifiable types.Identifiable, timeAuditable types.TimeAuditable, userAuditable types.UserAuditable, orgID string, name string, source Source, data SavedViewData) *SavedView {
+	spec := data.Spec
 	if spec.Queries == nil {
 		spec.Queries = []qbtypes.QueryEnvelope{}
 	}
@@ -62,13 +95,13 @@ func (s *StorableSavedView) ToSavedView() *SavedView {
 	}
 
 	return &SavedView{
-		Identifiable:  s.Identifiable,
-		TimeAuditable: s.TimeAuditable,
-		UserAuditable: s.UserAuditable,
-		OrgID:         s.OrgID,
-		Name:          s.Name,
-		Source:        s.Source,
-		SchemaVersion: SchemaVersion{valuer.NewString(s.Data.SchemaVersion)},
+		Identifiable:  identifiable,
+		TimeAuditable: timeAuditable,
+		UserAuditable: userAuditable,
+		OrgID:         orgID,
+		Name:          name,
+		Source:        source,
+		SchemaVersion: SchemaVersion{valuer.NewString(data.SchemaVersion)},
 		Spec:          spec,
 	}
 }
@@ -206,17 +239,7 @@ func (p *ListSavedViewsParams) Validate() error {
 	return p.Source.Validate()
 }
 
-// NewSavedViewsFromStorableSavedViews converts scanned rows to their domain shape.
-func NewSavedViewsFromStorableSavedViews(storableSavedViews []*StorableSavedView) []*SavedView {
-	savedViews := make([]*SavedView, len(storableSavedViews))
-	for idx, storableSavedView := range storableSavedViews {
-		savedViews[idx] = storableSavedView.ToSavedView()
-	}
-
-	return savedViews
-}
-
-func NewStatsFromStorableSavedViews(savedViews []*StorableSavedView) map[string]any {
+func NewStatsFromStorableSavedViews(savedViews []*RawStorableSavedView) map[string]any {
 	stats := make(map[string]any)
 	for _, savedView := range savedViews {
 		key := "savedview.source." + strings.ToLower(savedView.Source.StringValue()) + ".count"

pkg/types/savedviewtypes/savedview_test.go

@@ -250,17 +250,13 @@ func TestStorableSavedView_ToSavedView(t *testing.T) {
 	})
 
 	t.Run("nil selectedFields normalizes to an empty slice, not nil", func(t *testing.T) {
-		storable := &StorableSavedView{
-			Data: SavedViewData{
-				SchemaVersion: SavedViewSchemaVersion.StringValue(),
-				Spec: SavedViewSpec{
-					DisplayName:    "My View",
-					PanelType:      PanelTypeGraph,
-					Queries:        validQueries(),
-					SelectedFields: nil,
-				},
-			},
-		}
+		storable := storableWithSpec(SavedViewSpec{
+			DisplayName:    "My View",
+			PanelType:      PanelTypeGraph,
+			RequestType:    qbtypes.RequestTypeTimeSeries,
+			Queries:        validQueries(),
+			SelectedFields: nil,
+		})
 
 		view := storable.ToSavedView()
 
@@ -269,16 +265,12 @@ func TestStorableSavedView_ToSavedView(t *testing.T) {
 	})
 
 	t.Run("nil queries normalizes to an empty slice, not nil", func(t *testing.T) {
-		storable := &StorableSavedView{
-			Data: SavedViewData{
-				SchemaVersion: SavedViewSchemaVersion.StringValue(),
-				Spec: SavedViewSpec{
-					DisplayName: "My View",
-					PanelType:   PanelTypeGraph,
-					Queries:     nil,
-				},
-			},
-		}
+		storable := storableWithSpec(SavedViewSpec{
+			DisplayName: "My View",
+			PanelType:   PanelTypeGraph,
+			RequestType: qbtypes.RequestTypeTimeSeries,
+			Queries:     nil,
+		})
 
 		view := storable.ToSavedView()
 
@@ -287,8 +279,42 @@ func TestStorableSavedView_ToSavedView(t *testing.T) {
 	})
 }
 
+func storableWithSpec(spec SavedViewSpec) *StorableSavedView {
+	return NewStorableSavedView(&SavedView{SchemaVersion: SavedViewSchemaVersion, Spec: spec})
+}
+
+// Stored data written by older builds can fail the strict spec decode. Such a
+// view must fail on its own rather than being served with a silently wrong spec.
+func TestRawStorableSavedView_ToSavedView_UndecodableData(t *testing.T) {
+	specWithQueries := func(queries string) string {
+		return `{"schemaVersion":"v2","spec":{"displayName":"My View","panelType":"list","requestType":"raw","queries":` + queries + `}}`
+	}
+
+	cases := []struct {
+		name string
+		data string
+	}{
+		{name: "unknown field in a builder spec", data: specWithQueries(`[{"type":"builder_query","spec":{"signal":"logs","sinceRemovedField":1}}]`)},
+		{name: "unknown query type", data: specWithQueries(`[{"type":"builder_query_v1","spec":{}}]`)},
+		{name: "v1-era compositeQuery shape", data: specWithQueries(`[{"queryName":"A","dataSource":"logs"}]`)},
+		{name: "requestType no longer accepted", data: `{"schemaVersion":"v2","spec":{"displayName":"My View","panelType":"list","requestType":""}}`},
+		{name: "data that is not a view", data: `not json at all`},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			storable := &RawStorableSavedView{Name: "my-view", Source: SourceLogs, Data: c.data}
+
+			view, err := storable.ToSavedView()
+
+			require.Error(t, err)
+			assert.Nil(t, view)
+		})
+	}
+}
+
 func TestNewStatsFromStorableSavedViews(t *testing.T) {
-	storables := []*StorableSavedView{
+	storables := []*RawStorableSavedView{
 		{Source: SourceLogs},
 		{Source: SourceLogs},
 		{Source: SourceTraces},
@@ -302,17 +328,3 @@ func TestNewStatsFromStorableSavedViews(t *testing.T) {
 	assert.NotContains(t, stats, "savedview.source.metrics.count")
 }
 
-func TestNewSavedViewsFromStorableSavedViews(t *testing.T) {
-	storables := []*StorableSavedView{
-		{Name: "a", Source: SourceLogs, Data: SavedViewData{SchemaVersion: SavedViewSchemaVersion.StringValue(), Spec: SavedViewSpec{DisplayName: "a", PanelType: PanelTypeGraph, Queries: validQueries()}}},
-		{Name: "b", Source: SourceTraces, Data: SavedViewData{SchemaVersion: SavedViewSchemaVersion.StringValue(), Spec: SavedViewSpec{DisplayName: "b", PanelType: PanelTypeTable, Queries: validQueries()}}},
-	}
-
-	views := NewSavedViewsFromStorableSavedViews(storables)
-
-	require.Len(t, views, 2)
-	assert.Equal(t, "a", views[0].Name)
-	assert.Equal(t, SourceLogs, views[0].Source)
-	assert.Equal(t, "b", views[1].Name)
-	assert.Equal(t, SourceTraces, views[1].Source)
-}

pkg/types/savedviewtypes/savedviewtypestest/store.go

@@ -27,8 +27,21 @@ func (t *StoreTest) Store() savedviewtypes.Store { return t.store }
 // Mock returns the sqlmock handle for setting query expectations.
 func (t *StoreTest) Mock() sqlmock.Sqlmock { return t.mock }
 
-func savedViewRow(view *savedviewtypes.SavedView) []driver.Value {
-	data, _ := json.Marshal(savedviewtypes.NewStorableSavedView(view).Data)
+// Row is a saved view as stored. Data overrides the data column derived from
+// View, so tests can inject stored data that no longer decodes.
+type Row struct {
+	View *savedviewtypes.SavedView
+	Data string
+}
+
+func savedViewRow(row Row) []driver.Value {
+	view := row.View
+	data := row.Data
+	if data == "" {
+		marshalled, _ := json.Marshal(savedviewtypes.NewStorableSavedView(view).Data)
+		data = string(marshalled)
+	}
+
 	return []driver.Value{
 		view.ID.StringValue(),
 		view.CreatedAt,
@@ -38,7 +51,7 @@ func savedViewRow(view *savedviewtypes.SavedView) []driver.Value {
 		view.OrgID,
 		view.Name,
 		view.Source.StringValue(),
-		string(data),
+		data,
 	}
 }
 
@@ -56,9 +69,19 @@ func (t *StoreTest) ExpectCreateError(err error) {
 // ExpectGet sets up the SQL expectation for a Get call. Pass view = nil to
 // simulate a not-found row.
 func (t *StoreTest) ExpectGet(orgID string, id valuer.UUID, view *savedviewtypes.SavedView) {
+	if view == nil {
+		t.ExpectGetRows(orgID, id)
+		return
+	}
+
+	t.ExpectGetRows(orgID, id, Row{View: view})
+}
+
+// ExpectGetRows is ExpectGet with control over the stored data column.
+func (t *StoreTest) ExpectGetRows(orgID string, id valuer.UUID, returned ...Row) {
 	rows := sqlmock.NewRows(savedViewColumns)
-	if view != nil {
-		rows.AddRow(savedViewRow(view)...)
+	for _, row := range returned {
+		rows.AddRow(savedViewRow(row)...)
 	}
 
 	t.mock.ExpectQuery(`SELECT (.+) FROM "saved_view".+WHERE \(org_id = '` + regexp.QuoteMeta(orgID) + `' AND id = '` + regexp.QuoteMeta(id.StringValue()) + `'\)`).
@@ -81,9 +104,19 @@ func (t *StoreTest) ExpectDelete(orgID string, id valuer.UUID, rowsAffected int6
 
 // ExpectList sets up the SQL expectation for a List call scoped to orgID.
 func (t *StoreTest) ExpectList(orgID string, views []*savedviewtypes.SavedView) {
+	returned := make([]Row, len(views))
+	for idx, view := range views {
+		returned[idx] = Row{View: view}
+	}
+
+	t.ExpectListRows(orgID, returned...)
+}
+
+// ExpectListRows is ExpectList with control over each row's stored data column.
+func (t *StoreTest) ExpectListRows(orgID string, returned ...Row) {
 	rows := sqlmock.NewRows(savedViewColumns)
-	for _, view := range views {
-		rows.AddRow(savedViewRow(view)...)
+	for _, row := range returned {
+		rows.AddRow(savedViewRow(row)...)
 	}
 
 	t.mock.ExpectQuery(`SELECT (.+) FROM "saved_view".+WHERE \(org_id = '` + regexp.QuoteMeta(orgID) + `'\)`).WillReturnRows(rows)

pkg/types/savedviewtypes/store.go

@@ -11,5 +11,5 @@ type Store interface {
 	Get(ctx context.Context, orgID string, id valuer.UUID) (*StorableSavedView, error)
 	Update(ctx context.Context, view *StorableSavedView) error
 	Delete(ctx context.Context, orgID string, id valuer.UUID) error
-	List(ctx context.Context, orgID string, source Source, name string) ([]*StorableSavedView, error)
+	List(ctx context.Context, orgID string, source Source, name string) ([]*RawStorableSavedView, error)
 }