IntenseWebs/mattermost
3
0
Fork 0
mirror of https://github.com/mattermost/mattermost.git synced 2026-08-27 05:37:15 -05:00
Code Issues Packages Projects Releases Wiki Activity

Add COALESCE guard for property values before PSAv2 migrations (#36079)

Browse Source 
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 <miguel@ctrlz.es>
This commit is contained in:
Miguel de la Cruz
2026-04-14 20:46:11 +00:00
committed by GitHub
co-authored by Miguel de la Cruz
parent 0f2c16754f
commit 3cb00848a6
2 changed files with 101 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files
server/channels/store/sqlstore/property_value_store.go
+5 -7
View File
@@ -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,
server/channels/store/storetest/property_value_store.go
+96 -9
View File
@@ -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) {