From 3cb00848a6c19cf92043e38cdf6ecb0081dea479 Mon Sep 17 00:00:00 2001 From: Miguel de la Cruz Date: Tue, 14 Apr 2026 22:46:11 +0200 Subject: [PATCH] Add COALESCE guard for property values before PSAv2 migrations (#36079) Rows created before the CreatedBy/UpdatedBy columns were added have NULL in those fields, causing a scan error when reading them. Wraps those columns with COALESCE(..., '') in the tableSelectQuery and in the Upsert RETURNING clause. Also removes the propertyValueColumns shared variable, inlining the column lists directly in each INSERT statement to match the pattern used in the property field store. Co-authored-by: Miguel de la Cruz --- .../store/sqlstore/property_value_store.go | 12 +- .../store/storetest/property_value_store.go | 105 ++++++++++++++++-- 2 files changed, 101 insertions(+), 16 deletions(-) diff --git a/server/channels/store/sqlstore/property_value_store.go b/server/channels/store/sqlstore/property_value_store.go index b05dc023a9e..89cac63f0b1 100644 --- a/server/channels/store/sqlstore/property_value_store.go +++ b/server/channels/store/sqlstore/property_value_store.go @@ -19,13 +19,11 @@ type SqlPropertyValueStore struct { tableSelectQuery sq.SelectBuilder } -var propertyValueColumns = []string{"ID", "TargetID", "TargetType", "GroupID", "FieldID", "Value", "CreateAt", "UpdateAt", "DeleteAt", "CreatedBy", "UpdatedBy"} - func newPropertyValueStore(sqlStore *SqlStore) store.PropertyValueStore { s := SqlPropertyValueStore{SqlStore: sqlStore} s.tableSelectQuery = s.getQueryBuilder(). - Select(propertyValueColumns...). + Select("ID", "TargetID", "TargetType", "GroupID", "FieldID", "Value", "CreateAt", "UpdateAt", "DeleteAt", "COALESCE(CreatedBy, '') as CreatedBy", "COALESCE(UpdatedBy, '') as UpdatedBy"). From("PropertyValues") return &s @@ -49,7 +47,7 @@ func (s *SqlPropertyValueStore) Create(value *model.PropertyValue) (*model.Prope builder := s.getQueryBuilder(). Insert("PropertyValues"). - Columns(propertyValueColumns...). + Columns("ID", "TargetID", "TargetType", "GroupID", "FieldID", "Value", "CreateAt", "UpdateAt", "DeleteAt", "CreatedBy", "UpdatedBy"). Values(value.ID, value.TargetID, value.TargetType, value.GroupID, value.FieldID, valueJSON, value.CreateAt, value.UpdateAt, value.DeleteAt, value.CreatedBy, value.UpdatedBy) if _, err := s.GetMaster().ExecBuilder(builder); err != nil { return nil, errors.Wrap(err, "property_value_create_insert") @@ -83,7 +81,7 @@ func (s *SqlPropertyValueStore) CreateMany(values []*model.PropertyValue) ([]*mo builder := s.getQueryBuilder(). Insert("PropertyValues"). - Columns(propertyValueColumns...). + Columns("ID", "TargetID", "TargetType", "GroupID", "FieldID", "Value", "CreateAt", "UpdateAt", "DeleteAt", "CreatedBy", "UpdatedBy"). Values(value.ID, value.TargetID, value.TargetType, value.GroupID, value.FieldID, valueJSON, value.CreateAt, value.UpdateAt, value.DeleteAt, value.CreatedBy, value.UpdatedBy) if _, err := transaction.ExecBuilder(builder); err != nil { @@ -285,11 +283,11 @@ func (s *SqlPropertyValueStore) Upsert(values []*model.PropertyValue) (_ []*mode builder := s.getQueryBuilder(). Insert("PropertyValues"). - Columns(propertyValueColumns...). + Columns("ID", "TargetID", "TargetType", "GroupID", "FieldID", "Value", "CreateAt", "UpdateAt", "DeleteAt", "CreatedBy", "UpdatedBy"). Values(value.ID, value.TargetID, value.TargetType, value.GroupID, value.FieldID, valueJSON, value.CreateAt, value.UpdateAt, value.DeleteAt, value.CreatedBy, value.UpdatedBy) builder = builder.SuffixExpr(sq.Expr( - "ON CONFLICT (GroupID, TargetID, FieldID) WHERE DeleteAt = 0 DO UPDATE SET Value = ?, UpdateAt = ?, DeleteAt = ?, UpdatedBy = ? RETURNING *", + "ON CONFLICT (GroupID, TargetID, FieldID) WHERE DeleteAt = 0 DO UPDATE SET Value = ?, UpdateAt = ?, DeleteAt = ?, UpdatedBy = ? RETURNING ID, TargetID, TargetType, GroupID, FieldID, Value, CreateAt, UpdateAt, DeleteAt, COALESCE(CreatedBy, '') as CreatedBy, COALESCE(UpdatedBy, '') as UpdatedBy", valueJSON, value.UpdateAt, 0, diff --git a/server/channels/store/storetest/property_value_store.go b/server/channels/store/storetest/property_value_store.go index 51dadc5422e..a8378293c98 100644 --- a/server/channels/store/storetest/property_value_store.go +++ b/server/channels/store/storetest/property_value_store.go @@ -10,22 +10,24 @@ import ( "testing" "time" + sq "github.com/mattermost/squirrel" + "github.com/stretchr/testify/require" + "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/public/shared/request" "github.com/mattermost/mattermost/server/v8/channels/store" - "github.com/stretchr/testify/require" ) func TestPropertyValueStore(t *testing.T, rctx request.CTX, ss store.Store, s SqlStore) { t.Run("CreatePropertyValue", func(t *testing.T) { testCreatePropertyValue(t, rctx, ss) }) t.Run("CreateManyPropertyValues", func(t *testing.T) { testCreateManyPropertyValues(t, rctx, ss) }) t.Run("CreatePropertyValueWithArray", func(t *testing.T) { testCreatePropertyValueWithArray(t, rctx, ss) }) - t.Run("GetPropertyValue", func(t *testing.T) { testGetPropertyValue(t, rctx, ss) }) - t.Run("GetManyPropertyValues", func(t *testing.T) { testGetManyPropertyValues(t, rctx, ss) }) + t.Run("GetPropertyValue", func(t *testing.T) { testGetPropertyValue(t, rctx, ss, s) }) + t.Run("GetManyPropertyValues", func(t *testing.T) { testGetManyPropertyValues(t, rctx, ss, s) }) t.Run("UpdatePropertyValue", func(t *testing.T) { testUpdatePropertyValue(t, rctx, ss) }) - t.Run("UpsertPropertyValue", func(t *testing.T) { testUpsertPropertyValue(t, rctx, ss) }) + t.Run("UpsertPropertyValue", func(t *testing.T) { testUpsertPropertyValue(t, rctx, ss, s) }) t.Run("DeletePropertyValue", func(t *testing.T) { testDeletePropertyValue(t, rctx, ss) }) - t.Run("SearchPropertyValues", func(t *testing.T) { testSearchPropertyValues(t, rctx, ss) }) + t.Run("SearchPropertyValues", func(t *testing.T) { testSearchPropertyValues(t, rctx, ss, s) }) t.Run("SearchPropertyValuesSince", func(t *testing.T) { testSearchPropertyValuesSince(t, rctx, ss) }) t.Run("DeleteForField", func(t *testing.T) { testDeleteForField(t, rctx, ss) }) t.Run("DeleteForTarget", func(t *testing.T) { testDeleteForTarget(t, rctx, ss) }) @@ -316,7 +318,35 @@ func testCreateManyPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { }) } -func testGetPropertyValue(t *testing.T, _ request.CTX, ss store.Store) { +// insertPropertyValueWithNullColumns inserts a property value row that +// simulates a record created before the migrations that added CreatedBy +// and UpdatedBy columns. Those columns are left NULL so that the store's +// COALESCE logic is exercised. +// Returns groupID, targetID, fieldID, valueID. +func insertPropertyValueWithNullColumns(t *testing.T, ss store.Store, s SqlStore) (string, string, string, string) { + t.Helper() + + valueID := model.NewId() + groupID := model.NewId() + targetID := model.NewId() + fieldID := model.NewId() + db := ss.GetInternalMasterDB() + + builder := sq.StatementBuilder.PlaceholderFormat(s.GetQueryPlaceholder()). + Insert("PropertyValues"). + Columns("ID", "TargetID", "TargetType", "GroupID", "FieldID", "Value", "CreateAt", "UpdateAt", "DeleteAt"). + Values(valueID, targetID, "test_type", groupID, fieldID, `"null-columns-value"`, model.GetMillis(), model.GetMillis(), 0) + + query, args, err := builder.ToSql() + require.NoError(t, err) + + _, err = db.Exec(query, args...) + require.NoError(t, err) + + return groupID, targetID, fieldID, valueID +} + +func testGetPropertyValue(t *testing.T, _ request.CTX, ss store.Store, s SqlStore) { t.Run("should fail on nonexisting value", func(t *testing.T) { value, err := ss.PropertyValue().Get("", model.NewId()) require.Zero(t, value) @@ -390,9 +420,19 @@ func testGetPropertyValue(t *testing.T, _ request.CTX, ss store.Store) { require.Zero(t, value) require.ErrorIs(t, err, sql.ErrNoRows) }) + + t.Run("null columns, before createdBy and updatedBy migrations", func(t *testing.T) { + groupID, _, _, valueID := insertPropertyValueWithNullColumns(t, ss, s) + + value, err := ss.PropertyValue().Get(groupID, valueID) + require.NoError(t, err) + require.Equal(t, valueID, value.ID) + require.Empty(t, value.CreatedBy) + require.Empty(t, value.UpdatedBy) + }) } -func testGetManyPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { +func testGetManyPropertyValues(t *testing.T, _ request.CTX, ss store.Store, s SqlStore) { t.Run("should fail on nonexisting values", func(t *testing.T) { values, err := ss.PropertyValue().GetMany("", []string{model.NewId(), model.NewId()}) require.Empty(t, values) @@ -454,6 +494,17 @@ func testGetManyPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { require.NoError(t, err) require.Len(t, fields, 2) }) + + t.Run("null columns, before createdBy and updatedBy migrations", func(t *testing.T) { + groupID, _, _, valueID := insertPropertyValueWithNullColumns(t, ss, s) + + values, err := ss.PropertyValue().GetMany(groupID, []string{valueID}) + require.NoError(t, err) + require.Len(t, values, 1) + require.Equal(t, valueID, values[0].ID) + require.Empty(t, values[0].CreatedBy) + require.Empty(t, values[0].UpdatedBy) + }) } func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { @@ -794,7 +845,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { }) } -func testUpsertPropertyValue(t *testing.T, _ request.CTX, ss store.Store) { +func testUpsertPropertyValue(t *testing.T, _ request.CTX, ss store.Store, s SqlStore) { t.Run("should fail if the property value is not valid", func(t *testing.T) { value := &model.PropertyValue{ TargetID: "", @@ -1048,6 +1099,28 @@ func testUpsertPropertyValue(t *testing.T, _ request.CTX, ss store.Store) { require.Equal(t, updaterUserID, fetched.UpdatedBy, "UpdatedBy should change on upsert") require.Equal(t, `"updated via upsert"`, string(fetched.Value)) }) + + t.Run("null columns, upsert conflict path before createdBy and updatedBy migrations", func(t *testing.T) { + groupID, targetID, fieldID, _ := insertPropertyValueWithNullColumns(t, ss, s) + + // Upsert with the same (GroupID, TargetID, FieldID) to trigger the conflict (update) path. + // The existing row has NULL CreatedBy/UpdatedBy; the COALESCE in RETURNING must handle them. + updaterID := model.NewId() + value := &model.PropertyValue{ + TargetID: targetID, + TargetType: "test_type", + GroupID: groupID, + FieldID: fieldID, + Value: json.RawMessage(`"upserted value"`), + UpdatedBy: updaterID, + } + + upserted, err := ss.PropertyValue().Upsert([]*model.PropertyValue{value}) + require.NoError(t, err) + require.Len(t, upserted, 1) + require.Empty(t, upserted[0].CreatedBy) + require.Equal(t, updaterID, upserted[0].UpdatedBy) + }) } func testDeletePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { @@ -1141,7 +1214,7 @@ func testDeletePropertyValue(t *testing.T, _ request.CTX, ss store.Store) { }) } -func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { +func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store, s SqlStore) { groupID := model.NewId() targetID := model.NewId() fieldID := model.NewId() @@ -1351,6 +1424,20 @@ func testSearchPropertyValues(t *testing.T, _ request.CTX, ss store.Store) { require.ElementsMatch(t, tc.expectedIDs, ids) }) } + + t.Run("null columns, before createdBy and updatedBy migrations", func(t *testing.T) { + nullGroupID, _, _, nullValueID := insertPropertyValueWithNullColumns(t, ss, s) + + results, err := ss.PropertyValue().SearchPropertyValues(model.PropertyValueSearchOpts{ + GroupID: nullGroupID, + PerPage: 10, + }) + require.NoError(t, err) + require.Len(t, results, 1) + require.Equal(t, nullValueID, results[0].ID) + require.Empty(t, results[0].CreatedBy) + require.Empty(t, results[0].UpdatedBy) + }) } func testSearchPropertyValuesSince(t *testing.T, _ request.CTX, ss store.Store) {