From a6e019863edbc518ae552ea6782762ec42fcbca2 Mon Sep 17 00:00:00 2001 From: David Krauser Date: Wed, 27 May 2026 14:27:16 -0400 Subject: [PATCH] [MM-68999] Add SchemaVersion to PropertyGroup for group-specific field schema versioning (#36747) --- server/channels/app/server.go | 2 +- ...dd_property_groups_schema_version.down.sql | 1 + ..._add_property_groups_schema_version.up.sql | 1 + .../store/sqlstore/property_group_store.go | 6 +-- .../store/storetest/property_group_store.go | 52 +++++++++++++++++++ server/public/model/property_group.go | 17 ++++-- server/public/model/property_group_test.go | 18 +++++++ 7 files changed, 90 insertions(+), 7 deletions(-) create mode 100644 server/channels/db/migrations/postgres/000193_add_property_groups_schema_version.down.sql create mode 100644 server/channels/db/migrations/postgres/000193_add_property_groups_schema_version.up.sql diff --git a/server/channels/app/server.go b/server/channels/app/server.go index 4bdd3e5e173..8a80bbbc813 100644 --- a/server/channels/app/server.go +++ b/server/channels/app/server.go @@ -272,7 +272,7 @@ func NewServer(options ...Option) (*Server, error) { // Register builtin property groups before creating hooks that reference them if err = s.propertyService.RegisterBuiltinGroups([]*model.PropertyGroup{ - {Name: model.AccessControlPropertyGroupName, Version: model.PropertyGroupVersionV2}, + {Name: model.AccessControlPropertyGroupName, Version: model.PropertyGroupVersionV2, SchemaVersion: model.AccessControlPropertyGroupSchemaVersion}, {Name: model.ContentFlaggingGroupName, Version: model.PropertyGroupVersionV1}, }); err != nil { return nil, errors.Wrap(err, "failed to register builtin property groups") diff --git a/server/channels/db/migrations/postgres/000193_add_property_groups_schema_version.down.sql b/server/channels/db/migrations/postgres/000193_add_property_groups_schema_version.down.sql new file mode 100644 index 00000000000..530f336153f --- /dev/null +++ b/server/channels/db/migrations/postgres/000193_add_property_groups_schema_version.down.sql @@ -0,0 +1 @@ +ALTER TABLE PropertyGroups DROP COLUMN IF EXISTS SchemaVersion; diff --git a/server/channels/db/migrations/postgres/000193_add_property_groups_schema_version.up.sql b/server/channels/db/migrations/postgres/000193_add_property_groups_schema_version.up.sql new file mode 100644 index 00000000000..33f1d544b28 --- /dev/null +++ b/server/channels/db/migrations/postgres/000193_add_property_groups_schema_version.up.sql @@ -0,0 +1 @@ +ALTER TABLE PropertyGroups ADD COLUMN IF NOT EXISTS SchemaVersion integer DEFAULT 1 NOT NULL; diff --git a/server/channels/store/sqlstore/property_group_store.go b/server/channels/store/sqlstore/property_group_store.go index 52ed9ce8fc4..a38907b1c0f 100644 --- a/server/channels/store/sqlstore/property_group_store.go +++ b/server/channels/store/sqlstore/property_group_store.go @@ -11,7 +11,7 @@ import ( "github.com/mattermost/mattermost/server/v8/channels/store" ) -var propertyGroupColumns = []string{"ID", "Name", "Version"} +var propertyGroupColumns = []string{"ID", "Name", "Version", "SchemaVersion"} type SqlPropertyGroupStore struct { *SqlStore @@ -34,8 +34,8 @@ func (s *SqlPropertyGroupStore) Register(group *model.PropertyGroup) (*model.Pro builder := s.getQueryBuilder(). Insert("PropertyGroups"). - Columns("ID", "Name", "Version"). - Values(group.ID, group.Name, group.Version) + Columns("ID", "Name", "Version", "SchemaVersion"). + Values(group.ID, group.Name, group.Version, group.SchemaVersion) builder = builder.SuffixExpr(sq.Expr("ON CONFLICT (Name) DO NOTHING")) diff --git a/server/channels/store/storetest/property_group_store.go b/server/channels/store/storetest/property_group_store.go index 0af6df9393c..4692feb5bcd 100644 --- a/server/channels/store/storetest/property_group_store.go +++ b/server/channels/store/storetest/property_group_store.go @@ -15,6 +15,7 @@ import ( func TestPropertyGroupStore(t *testing.T, rctx request.CTX, ss store.Store, s SqlStore) { t.Run("RegisterAndGetPropertyGroup", func(t *testing.T) { testRegisterAndGetPropertyGroup(t, rctx, ss) }) t.Run("IncrementVersion", func(t *testing.T) { testIncrementVersion(t, rctx, ss) }) + t.Run("SchemaVersionPersistence", func(t *testing.T) { testSchemaVersionPersistence(t, rctx, ss) }) } func testRegisterAndGetPropertyGroup(t *testing.T, _ request.CTX, ss store.Store) { @@ -121,6 +122,57 @@ func testRegisterAndGetPropertyGroup(t *testing.T, _ request.CTX, ss store.Store }) } +func testSchemaVersionPersistence(t *testing.T, _ request.CTX, ss store.Store) { + t.Run("explicit SchemaVersion is persisted and returned", func(t *testing.T) { + group, err := ss.PropertyGroup().Register(&model.PropertyGroup{ + Name: "schema_version_explicit_test", + Version: model.PropertyGroupVersionV1, + SchemaVersion: 5, + }) + require.NoError(t, err) + require.Equal(t, 5, group.SchemaVersion) + + fetched, err := ss.PropertyGroup().Get("schema_version_explicit_test") + require.NoError(t, err) + require.Equal(t, 5, fetched.SchemaVersion) + }) + + t.Run("SchemaVersion defaults to 1 when not set", func(t *testing.T) { + group, err := ss.PropertyGroup().Register(&model.PropertyGroup{ + Name: "schema_version_default_test", + Version: model.PropertyGroupVersionV1, + }) + require.NoError(t, err) + require.Equal(t, 1, group.SchemaVersion) + + fetched, err := ss.PropertyGroup().Get("schema_version_default_test") + require.NoError(t, err) + require.Equal(t, 1, fetched.SchemaVersion) + }) + + t.Run("SchemaVersion is not updated on re-registration", func(t *testing.T) { + original, err := ss.PropertyGroup().Register(&model.PropertyGroup{ + Name: "schema_version_immutable_test", + Version: model.PropertyGroupVersionV1, + SchemaVersion: 2, + }) + require.NoError(t, err) + require.Equal(t, 2, original.SchemaVersion) + + reregistered, err := ss.PropertyGroup().Register(&model.PropertyGroup{ + Name: "schema_version_immutable_test", + Version: model.PropertyGroupVersionV1, + SchemaVersion: 99, + }) + require.NoError(t, err) + require.Equal(t, 2, reregistered.SchemaVersion) + + fetched, err := ss.PropertyGroup().Get("schema_version_immutable_test") + require.NoError(t, err) + require.Equal(t, 2, fetched.SchemaVersion) + }) +} + func testIncrementVersion(t *testing.T, _ request.CTX, ss store.Store) { t.Run("should increment version of an existing group", func(t *testing.T) { registered, err := ss.PropertyGroup().Register(&model.PropertyGroup{ diff --git a/server/public/model/property_group.go b/server/public/model/property_group.go index 9ed5cf0ed9e..9fc0e88d804 100644 --- a/server/public/model/property_group.go +++ b/server/public/model/property_group.go @@ -32,10 +32,17 @@ const ( PropertyGroupVersionV2 = 2 ) +// AccessControlPropertyGroupSchemaVersion is the current schema version for +// the access_control group's field definitions. Increment this constant +// whenever the shape of access_control fields (attrs, types, options) changes +// in a way that consumers need to detect. +const AccessControlPropertyGroupSchemaVersion = 1 + type PropertyGroup struct { - ID string `json:"id"` - Name string `json:"name"` - Version int `json:"version"` + ID string `json:"id"` + Name string `json:"name"` + Version int `json:"version"` + SchemaVersion int `json:"schema_version"` } func (pg *PropertyGroup) IsPSAv1() bool { @@ -54,6 +61,10 @@ func (pg *PropertyGroup) PreSave() { if pg.Version == 0 { pg.Version = PropertyGroupVersionV1 } + + if pg.SchemaVersion <= 0 { + pg.SchemaVersion = 1 + } } func (pg *PropertyGroup) IsValid() *AppError { diff --git a/server/public/model/property_group_test.go b/server/public/model/property_group_test.go index 253c6914b36..6ef26cf5c55 100644 --- a/server/public/model/property_group_test.go +++ b/server/public/model/property_group_test.go @@ -129,4 +129,22 @@ func TestPropertyGroupPreSave(t *testing.T) { pg.PreSave() assert.Equal(t, PropertyGroupVersionV2, pg.Version) }) + + t.Run("defaults schema_version to 1 when zero", func(t *testing.T) { + pg := &PropertyGroup{Name: "test_group"} + pg.PreSave() + assert.Equal(t, 1, pg.SchemaVersion) + }) + + t.Run("defaults schema_version to 1 when negative", func(t *testing.T) { + pg := &PropertyGroup{Name: "test_group", SchemaVersion: -5} + pg.PreSave() + assert.Equal(t, 1, pg.SchemaVersion) + }) + + t.Run("does not overwrite existing schema_version", func(t *testing.T) { + pg := &PropertyGroup{Name: "test_group", SchemaVersion: 3} + pg.PreSave() + assert.Equal(t, 3, pg.SchemaVersion) + }) }