Improves the Property System Architecture groups (#35395)

* Improves the Property System Architecture groups

The group creation for builtin property groups is moved from behaving
like a singleton in the app layer (first call creates the group) to
register groups and making sure they're present at server startup
time.

At the same time, it adds a groups cache as a sync map in the property
service, to avoid having individual caches per feature as package
variables, making the group caching part of the system.

* Fix i18n

* Fix test and calls after updating the branch

* Avoid panics by controlling the errors

* Adjust translations after merge

---------

Co-authored-by: Miguel de la Cruz <miguel@ctrlz.es>
Co-authored-by: Mattermost Build <build@mattermost.com>

server/channels/api4/content_flagging.go

@@ -239,9 +239,9 @@ func getContentFlaggingFields(c *Context, w http.ResponseWriter, r *http.Request
 		return
 	}
 
-	groupId, appErr := c.App.ContentFlaggingGroupId()
-	if appErr != nil {
-		c.Err = appErr
+	groupId, err := c.App.ContentFlaggingGroupId()
+	if err != nil {
+		c.Err = model.NewAppError("getContentFlaggingGroupId", "app.data_spillage.get_group.error", nil, "", http.StatusInternalServerError).Wrap(err)
 		return
 	}
 

server/channels/app/content_flagging.go

@@ -89,9 +89,9 @@ func (a *App) FlagPost(rctx request.CTX, post *model.Post, teamId, reportingUser
 		return appErr
 	}
 
-	groupId, appErr := a.ContentFlaggingGroupId()
-	if appErr != nil {
-		return appErr
+	groupId, err := a.ContentFlaggingGroupId()
+	if err != nil {
+		return model.NewAppError("FlagPost", "app.data_spillage.get_group.error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
 
 	reportingUser, appErr := a.GetUser(reportingUserId)
@@ -232,18 +232,19 @@ func (a *App) setContentFlaggingPropertiesForThreadReplies(post *model.Post, con
 	return nil
 }
 
-func (a *App) ContentFlaggingGroupId() (string, *model.AppError) {
-	group, err := a.Srv().propertyAccessService.GetPropertyGroup(model.ContentFlaggingGroupName)
+func (a *App) ContentFlaggingGroupId() (string, error) {
+	group, err := a.Srv().propertyAccessService.Group(model.ContentFlaggingGroupName)
 	if err != nil {
-		return "", model.NewAppError("getContentFlaggingGroupId", "app.data_spillage.get_group.error", nil, "", http.StatusInternalServerError)
+		return "", fmt.Errorf("cannot retrieve content flagging property group ID: %w", err)
 	}
+
 	return group.ID, nil
 }
 
 func (a *App) GetPostContentFlaggingPropertyValue(postId, propertyFieldName string) (*model.PropertyValue, *model.AppError) {
-	groupId, appErr := a.ContentFlaggingGroupId()
-	if appErr != nil {
-		return nil, appErr
+	groupId, err := a.ContentFlaggingGroupId()
+	if err != nil {
+		return nil, model.NewAppError("GetPostContentFlaggingPropertyValue", "app.data_spillage.get_group.error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
 
 	statusPropertyField, err := a.Srv().propertyAccessService.GetPropertyFieldByName(anonymousCallerId, groupId, "", propertyFieldName)
@@ -538,9 +539,9 @@ func (a *App) IsUserTeamContentReviewer(userId, teamId string) (bool, *model.App
 }
 
 func (a *App) GetPostContentFlaggingPropertyValues(postId string) ([]*model.PropertyValue, *model.AppError) {
-	groupId, appErr := a.ContentFlaggingGroupId()
-	if appErr != nil {
-		return nil, appErr
+	groupId, err := a.ContentFlaggingGroupId()
+	if err != nil {
+		return nil, model.NewAppError("GetPostContentFlaggingPropertyValues", "app.data_spillage.get_group.error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
 
 	propertyValues, err := a.PropertyAccessService().SearchPropertyValues(anonymousCallerId, groupId, model.PropertyValueSearchOpts{TargetIDs: []string{postId}, PerPage: CONTENT_FLAGGING_MAX_PROPERTY_VALUES})
@@ -585,9 +586,9 @@ func (a *App) PermanentDeleteFlaggedPost(rctx request.CTX, actionRequest *model.
 		return appErr
 	}
 
-	groupId, appErr := a.ContentFlaggingGroupId()
-	if appErr != nil {
-		return appErr
+	groupId, err := a.ContentFlaggingGroupId()
+	if err != nil {
+		return model.NewAppError("PermanentDeleteFlaggedPost", "app.data_spillage.get_group.error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
 
 	mappedFields, appErr := a.GetContentFlaggingMappedFields(groupId)
@@ -619,7 +620,7 @@ func (a *App) PermanentDeleteFlaggedPost(rctx request.CTX, actionRequest *model.
 		},
 	}
 
-	_, err := a.Srv().propertyAccessService.CreatePropertyValues(anonymousCallerId, propertyValues)
+	_, err = a.Srv().propertyAccessService.CreatePropertyValues(anonymousCallerId, propertyValues)
 	if err != nil {
 		return model.NewAppError("PermanentlyRemoveFlaggedPost", "app.data_spillage.create_property_values.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
@@ -727,9 +728,9 @@ func (a *App) KeepFlaggedPost(rctx request.CTX, actionRequest *model.FlagContent
 		return model.NewAppError("KeepFlaggedPost", "api.data_spillage.error.post_not_in_progress", nil, "", http.StatusBadRequest)
 	}
 
-	groupId, appErr := a.ContentFlaggingGroupId()
-	if appErr != nil {
-		return appErr
+	groupId, err := a.ContentFlaggingGroupId()
+	if err != nil {
+		return model.NewAppError("KeepFlaggedPost", "app.data_spillage.get_group.error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
 
 	mappedFields, appErr := a.GetContentFlaggingMappedFields(groupId)
@@ -756,8 +757,8 @@ func (a *App) KeepFlaggedPost(rctx request.CTX, actionRequest *model.FlagContent
 		}
 
 		// Restore the post, its replies, and all associated files
-		if err := a.Srv().Store().Post().RestoreContentFlaggedPost(flaggedPost, statusField.ID, contentFlaggingManagedField.ID); err != nil {
-			return model.NewAppError("KeepFlaggedPost", "app.data_spillage.keep_post.undelete.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
+		if rErr := a.Srv().Store().Post().RestoreContentFlaggedPost(flaggedPost, statusField.ID, contentFlaggingManagedField.ID); rErr != nil {
+			return model.NewAppError("KeepFlaggedPost", "app.data_spillage.keep_post.undelete.app_error", nil, "", http.StatusInternalServerError).Wrap(rErr)
 		}
 	}
 
@@ -989,9 +990,9 @@ func (a *App) AssignFlaggedPostReviewer(rctx request.CTX, flaggedPostId, flagged
 
 	status := strings.Trim(string(statusPropertyValue.Value), `"`)
 
-	groupId, appErr := a.ContentFlaggingGroupId()
-	if appErr != nil {
-		return appErr
+	groupId, err := a.ContentFlaggingGroupId()
+	if err != nil {
+		return model.NewAppError("AssignFlaggedPostReviewer", "app.data_spillage.get_group.error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}
 
 	mappedFields, appErr := a.GetContentFlaggingMappedFields(groupId)
@@ -1011,7 +1012,7 @@ func (a *App) AssignFlaggedPostReviewer(rctx request.CTX, flaggedPostId, flagged
 		Value:      json.RawMessage(fmt.Sprintf(`"%s"`, reviewerId)),
 	}
 
-	assigneePropertyValue, err := a.Srv().propertyAccessService.UpsertPropertyValue(anonymousCallerId, assigneePropertyValue)
+	assigneePropertyValue, err = a.Srv().propertyAccessService.UpsertPropertyValue(anonymousCallerId, assigneePropertyValue)
 	if err != nil {
 		return model.NewAppError("AssignFlaggedPostReviewer", "app.data_spillage.assign_reviewer.upsert_property_value.app_error", nil, "", http.StatusInternalServerError).Wrap(err)
 	}

server/channels/app/content_flagging_test.go

@@ -38,8 +38,8 @@ func setBaseConfig(th *TestHelper) *model.AppError {
 
 func searchPropertyValue(t *testing.T, th *TestHelper, postId, fieldName string) []*model.PropertyValue {
 	t.Helper()
-	groupId, appErr := th.App.ContentFlaggingGroupId()
-	require.Nil(t, appErr)
+	groupId, err := th.App.ContentFlaggingGroupId()
+	require.NoError(t, err)
 
 	mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId)
 	require.Nil(t, appErr)
@@ -197,8 +197,8 @@ func TestAssignFlaggedPostReviewer(t *testing.T) {
 		require.Equal(t, `"`+model.ContentFlaggingStatusAssigned+`"`, string(statusValue.Value))
 
 		// Verify reviewer property was created
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId)
 		require.Nil(t, appErr)
@@ -232,8 +232,8 @@ func TestAssignFlaggedPostReviewer(t *testing.T) {
 		require.Equal(t, `"`+model.ContentFlaggingStatusAssigned+`"`, string(statusValue.Value))
 
 		// Verify reviewer property was updated
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId)
 		require.Nil(t, appErr)
@@ -276,8 +276,8 @@ func TestAssignFlaggedPostReviewer(t *testing.T) {
 		require.Equal(t, `"`+model.ContentFlaggingStatusAssigned+`"`, string(statusValue.Value))
 
 		// Verify reviewer property still exists with correct value
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId)
 		require.Nil(t, appErr)
@@ -306,8 +306,8 @@ func TestAssignFlaggedPostReviewer(t *testing.T) {
 		require.Equal(t, `"`+model.ContentFlaggingStatusAssigned+`"`, string(statusValue.Value))
 
 		// Verify reviewer property was created with empty value
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId)
 		require.Nil(t, appErr)
@@ -334,15 +334,15 @@ func TestAssignFlaggedPostReviewer(t *testing.T) {
 
 		post := setupFlaggedPost(t, th)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		statusValue, appErr := th.App.GetPostContentFlaggingPropertyValue(post.Id, ContentFlaggingPropertyNameStatus)
 		require.Nil(t, appErr)
 
 		// Set the status to Assigned
 		statusValue.Value = json.RawMessage(fmt.Sprintf(`"%s"`, model.ContentFlaggingStatusAssigned))
-		_, err := th.App.PropertyAccessService().UpdatePropertyValue(anonymousCallerId, groupId, statusValue)
+		_, err = th.App.PropertyAccessService().UpdatePropertyValue(anonymousCallerId, groupId, statusValue)
 		require.NoError(t, err)
 
 		appErr = th.App.AssignFlaggedPostReviewer(th.Context, post.Id, th.BasicChannel.TeamId, th.BasicUser.Id, th.SystemAdminUser.Id)
@@ -992,18 +992,18 @@ func TestCanFlagPost(t *testing.T) {
 	t.Run("should be able to flag post which has not already been flagged", func(t *testing.T) {
 		post := th.CreatePost(t, th.BasicChannel)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
-		appErr = th.App.canFlagPost(groupId, post.Id, "en")
+		appErr := th.App.canFlagPost(groupId, post.Id, "en")
 		require.Nil(t, appErr)
 	})
 
 	t.Run("should not be able to flag post which has already been flagged", func(t *testing.T) {
 		post := th.CreatePost(t, th.BasicChannel)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		statusField, err := th.Server.propertyAccessService.GetPropertyFieldByName(anonymousCallerId, groupId, "", ContentFlaggingPropertyNameStatus)
 		require.NoError(t, err)
@@ -1018,7 +1018,7 @@ func TestCanFlagPost(t *testing.T) {
 		require.NoError(t, err)
 
 		// Can't fleg when post already flagged in pending status
-		appErr = th.App.canFlagPost(groupId, post.Id, "en")
+		appErr := th.App.canFlagPost(groupId, post.Id, "en")
 		require.NotNil(t, appErr)
 		require.Equal(t, "Cannot quarantine this post as it is already quarantined for review.", appErr.Id)
 
@@ -1079,8 +1079,8 @@ func TestFlagPost(t *testing.T) {
 		require.Nil(t, appErr)
 
 		// Verify property values were created
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId)
 		require.Nil(t, appErr)
@@ -1264,8 +1264,8 @@ func TestFlagPost(t *testing.T) {
 		require.Nil(t, appErr)
 
 		// Verify property values were created with empty comment
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId)
 		require.Nil(t, appErr)
@@ -1298,8 +1298,8 @@ func TestFlagPost(t *testing.T) {
 		require.Nil(t, appErr)
 
 		// Verify reporting time property was set
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId)
 		require.Nil(t, appErr)
@@ -1630,8 +1630,8 @@ func TestGetReviewerPostsForFlaggedPost(t *testing.T) {
 
 		post := setupFlaggedPost(t, th)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId)
 		require.Nil(t, appErr)
@@ -1655,8 +1655,8 @@ func TestGetReviewerPostsForFlaggedPost(t *testing.T) {
 		require.Nil(t, setBaseConfig(th))
 		post := th.CreatePost(t, th.BasicChannel)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId)
 		require.Nil(t, appErr)
@@ -1689,8 +1689,8 @@ func TestGetReviewerPostsForFlaggedPost(t *testing.T) {
 		// Wait for async reviewer post creation to complete
 		time.Sleep(2 * time.Second)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId)
 		require.Nil(t, appErr)
@@ -1714,8 +1714,8 @@ func TestGetReviewerPostsForFlaggedPost(t *testing.T) {
 
 	t.Run("should handle invalid flagged post ID", func(t *testing.T) {
 		require.Nil(t, setBaseConfig(th))
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId)
 		require.Nil(t, appErr)
@@ -1740,11 +1740,11 @@ func TestPostReviewerMessage(t *testing.T) {
 
 		post := setupFlaggedPost(t, th)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		testMessage := "Test reviewer message"
-		_, appErr = th.App.postReviewerMessage(th.Context, testMessage, groupId, post.Id)
+		_, appErr := th.App.postReviewerMessage(th.Context, testMessage, groupId, post.Id)
 		require.Nil(t, appErr)
 
 		// Verify message was posted to the reviewer thread
@@ -1797,8 +1797,8 @@ func TestPostReviewerMessage(t *testing.T) {
 		// Wait for async reviewer post creation to complete
 		time.Sleep(2 * time.Second)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		testMessage := "Test message for multiple reviewers"
 		_, appErr = th.App.postReviewerMessage(th.Context, testMessage, groupId, post.Id)
@@ -1854,11 +1854,11 @@ func TestPostReviewerMessage(t *testing.T) {
 		require.Nil(t, setBaseConfig(th))
 		post := th.CreatePost(t, th.BasicChannel)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		testMessage := "Test message for non-flagged post"
-		_, appErr = th.App.postReviewerMessage(th.Context, testMessage, groupId, post.Id)
+		_, appErr := th.App.postReviewerMessage(th.Context, testMessage, groupId, post.Id)
 		require.Nil(t, appErr)
 	})
 
@@ -1867,11 +1867,11 @@ func TestPostReviewerMessage(t *testing.T) {
 
 		post := setupFlaggedPost(t, th)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		testMessage := "Test message with special chars: @user #channel ~team & <script>alert('xss')</script>"
-		_, appErr = th.App.postReviewerMessage(th.Context, testMessage, groupId, post.Id)
+		_, appErr := th.App.postReviewerMessage(th.Context, testMessage, groupId, post.Id)
 		require.Nil(t, appErr)
 
 		// Verify message was posted correctly with special characters preserved
@@ -1935,8 +1935,8 @@ func TestSendFlaggedPostRemovalNotification(t *testing.T) {
 
 		post := setupFlaggedPost(t, th)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		actorComment := "This post violates community guidelines"
 		createdPosts := th.App.sendFlaggedPostRemovalNotification(th.Context, post, th.SystemAdminUser.Id, actorComment, groupId)
@@ -2000,8 +2000,8 @@ func TestSendFlaggedPostRemovalNotification(t *testing.T) {
 		})
 		require.Nil(t, appErr)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		createdPosts := th.App.sendFlaggedPostRemovalNotification(th.Context, post, th.SystemAdminUser.Id, "Test comment", groupId)
 
@@ -2023,8 +2023,8 @@ func TestSendFlaggedPostRemovalNotification(t *testing.T) {
 
 		post := setupFlaggedPost(t, th)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		createdPosts := th.App.sendFlaggedPostRemovalNotification(th.Context, post, th.SystemAdminUser.Id, "", groupId)
 
@@ -2042,8 +2042,8 @@ func TestSendFlaggedPostRemovalNotification(t *testing.T) {
 
 		post := setupFlaggedPost(t, th)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		specialComment := "Comment with @mentions #channels ~teams & <script>alert('xss')</script>"
 		createdPosts := th.App.sendFlaggedPostRemovalNotification(th.Context, post, th.SystemAdminUser.Id, specialComment, groupId)
@@ -2070,8 +2070,8 @@ func TestSendKeepFlaggedPostNotification(t *testing.T) {
 
 		post := setupFlaggedPost(t, th)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		actorComment := "This post is acceptable after review"
 		createdPosts := th.App.sendKeepFlaggedPostNotification(th.Context, post, th.SystemAdminUser.Id, actorComment, groupId)
@@ -2129,8 +2129,8 @@ func TestSendKeepFlaggedPostNotification(t *testing.T) {
 
 		post := setupFlaggedPost(t, th)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		comment := "Test comment"
 		createdPosts := th.App.sendKeepFlaggedPostNotification(th.Context, post, th.SystemAdminUser.Id, comment, groupId)
@@ -2153,8 +2153,8 @@ func TestSendKeepFlaggedPostNotification(t *testing.T) {
 
 		post := setupFlaggedPost(t, th)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		createdPosts := th.App.sendKeepFlaggedPostNotification(th.Context, post, th.SystemAdminUser.Id, "", groupId)
 
@@ -2172,8 +2172,8 @@ func TestSendKeepFlaggedPostNotification(t *testing.T) {
 
 		post := setupFlaggedPost(t, th)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		specialComment := "Comment with @mentions #channels ~teams & <script>alert('xss')</script>"
 		createdPosts := th.App.sendKeepFlaggedPostNotification(th.Context, post, th.SystemAdminUser.Id, specialComment, groupId)
@@ -2192,8 +2192,8 @@ func TestSendKeepFlaggedPostNotification(t *testing.T) {
 
 		post := setupFlaggedPost(t, th)
 
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		// Use BasicUser as actor instead of SystemAdminUser
 		createdPosts := th.App.sendKeepFlaggedPostNotification(th.Context, post, th.BasicUser.Id, "Reviewed by different user", groupId)
@@ -2233,8 +2233,8 @@ func TestPermanentDeleteFlaggedPost(t *testing.T) {
 		require.Equal(t, `"`+model.ContentFlaggingStatusRemoved+`"`, string(statusValue.Value))
 
 		// Verify actor properties were created
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId)
 		require.Nil(t, appErr)
@@ -2319,14 +2319,14 @@ func TestPermanentDeleteFlaggedPost(t *testing.T) {
 		post := setupFlaggedPost(t, th)
 
 		// Set status to removed
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		statusValue, appErr := th.App.GetPostContentFlaggingPropertyValue(post.Id, ContentFlaggingPropertyNameStatus)
 		require.Nil(t, appErr)
 
 		statusValue.Value = json.RawMessage(fmt.Sprintf(`"%s"`, model.ContentFlaggingStatusRemoved))
-		_, err := th.App.PropertyAccessService().UpdatePropertyValue(anonymousCallerId, groupId, statusValue)
+		_, err = th.App.PropertyAccessService().UpdatePropertyValue(anonymousCallerId, groupId, statusValue)
 		require.NoError(t, err)
 
 		actionRequest := &model.FlagContentActionRequest{
@@ -2343,14 +2343,14 @@ func TestPermanentDeleteFlaggedPost(t *testing.T) {
 		post := setupFlaggedPost(t, th)
 
 		// Set status to retained
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		statusValue, appErr := th.App.GetPostContentFlaggingPropertyValue(post.Id, ContentFlaggingPropertyNameStatus)
 		require.Nil(t, appErr)
 
 		statusValue.Value = json.RawMessage(fmt.Sprintf(`"%s"`, model.ContentFlaggingStatusRetained))
-		_, err := th.App.PropertyAccessService().UpdatePropertyValue(anonymousCallerId, groupId, statusValue)
+		_, err = th.App.PropertyAccessService().UpdatePropertyValue(anonymousCallerId, groupId, statusValue)
 		require.NoError(t, err)
 
 		actionRequest := &model.FlagContentActionRequest{
@@ -2386,8 +2386,8 @@ func TestPermanentDeleteFlaggedPost(t *testing.T) {
 		require.Nil(t, appErr)
 
 		// Verify empty comment was stored
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId)
 		require.Nil(t, appErr)
@@ -2414,8 +2414,8 @@ func TestPermanentDeleteFlaggedPost(t *testing.T) {
 		require.Nil(t, appErr)
 
 		// Verify special characters were properly escaped and stored
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		mappedFields, appErr := th.App.GetContentFlaggingMappedFields(groupId)
 		require.Nil(t, appErr)
@@ -2704,14 +2704,14 @@ func TestKeepFlaggedPost(t *testing.T) {
 		post := setupFlaggedPost(t, th)
 
 		// Set status to removed
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		statusValue, appErr := th.App.GetPostContentFlaggingPropertyValue(post.Id, ContentFlaggingPropertyNameStatus)
 		require.Nil(t, appErr)
 
 		statusValue.Value = json.RawMessage(fmt.Sprintf(`"%s"`, model.ContentFlaggingStatusRemoved))
-		_, err := th.App.Srv().propertyAccessService.UpdatePropertyValue("", groupId, statusValue)
+		_, err = th.Server.propertyAccessService.UpdatePropertyValue("", groupId, statusValue)
 		require.NoError(t, err)
 
 		actionRequest := &model.FlagContentActionRequest{
@@ -2728,14 +2728,14 @@ func TestKeepFlaggedPost(t *testing.T) {
 		post := setupFlaggedPost(t, th)
 
 		// Set status to retained
-		groupId, appErr := th.App.ContentFlaggingGroupId()
-		require.Nil(t, appErr)
+		groupId, err := th.App.ContentFlaggingGroupId()
+		require.NoError(t, err)
 
 		statusValue, appErr := th.App.GetPostContentFlaggingPropertyValue(post.Id, ContentFlaggingPropertyNameStatus)
 		require.Nil(t, appErr)
 
 		statusValue.Value = json.RawMessage(fmt.Sprintf(`"%s"`, model.ContentFlaggingStatusRetained))
-		_, err := th.App.Srv().propertyAccessService.UpdatePropertyValue("", groupId, statusValue)
+		_, err = th.Server.propertyAccessService.UpdatePropertyValue("", groupId, statusValue)
 		require.NoError(t, err)
 
 		actionRequest := &model.FlagContentActionRequest{

server/channels/app/custom_profile_attributes.go

@@ -6,6 +6,7 @@ package app
 import (
 	"database/sql"
 	"encoding/json"
+	"fmt"
 	"net/http"
 	"sort"
 
@@ -19,26 +20,13 @@ const (
 	CustomProfileAttributesFieldLimit = 20
 )
 
-var cpaGroupID string
-
 func (a *App) CpaGroupID() (string, error) {
-	return getCpaGroupID(a.Srv().propertyAccessService)
-}
-
-// ToDo: we should explore moving this to the database cache layer
-// instead of maintaining the ID cached at the application level
-func getCpaGroupID(service *PropertyAccessService) (string, error) {
-	if cpaGroupID != "" {
-		return cpaGroupID, nil
-	}
-
-	cpaGroup, err := service.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName)
+	group, err := a.Srv().propertyAccessService.Group(model.CustomProfileAttributesPropertyGroupName)
 	if err != nil {
-		return "", errors.Wrap(err, "cannot register Custom Profile Attributes property group")
+		return "", fmt.Errorf("cannot retrieve custom profile attributes property group ID: %w", err)
 	}
-	cpaGroupID = cpaGroup.ID
 
-	return cpaGroupID, nil
+	return group.ID, nil
 }
 
 func (a *App) GetCPAField(callerId, fieldID string) (*model.CPAField, *model.AppError) {

server/channels/app/properties/property_group.go

@@ -4,11 +4,48 @@
 package properties
 
 import (
+	"fmt"
+
 	"github.com/mattermost/mattermost/server/public/model"
 )
 
+// RegisterBuiltinGroups registers a set of property groups at startup
+// and populates the cache so that subsequent Group calls are free of
+// database round-trips.
+func (ps *PropertyService) RegisterBuiltinGroups(groups []*model.PropertyGroup) error {
+	for _, group := range groups {
+		if _, err := ps.RegisterPropertyGroup(group.Name); err != nil {
+			return fmt.Errorf("failed to register builtin property group %q: %w", group.Name, err)
+		}
+	}
+	return nil
+}
+
+// Group returns the cached PropertyGroup for a given name. If the
+// group is not in the cache (e.g. a plugin-registered group), it
+// falls back to a database lookup and caches the result.
+func (ps *PropertyService) Group(name string) (*model.PropertyGroup, error) {
+	if cached, ok := ps.groupCache.Load(name); ok {
+		return cached.(*model.PropertyGroup), nil
+	}
+
+	group, err := ps.groupStore.Get(name)
+	if err != nil {
+		return nil, fmt.Errorf("property group %q not found: %w", name, err)
+	}
+
+	ps.groupCache.Store(name, group)
+	return group, nil
+}
+
 func (ps *PropertyService) RegisterPropertyGroup(name string) (*model.PropertyGroup, error) {
-	return ps.groupStore.Register(name)
+	group, err := ps.groupStore.Register(name)
+	if err != nil {
+		return nil, err
+	}
+
+	ps.groupCache.Store(name, group)
+	return group, nil
 }
 
 func (ps *PropertyService) GetPropertyGroup(name string) (*model.PropertyGroup, error) {

server/channels/app/properties/service.go

@@ -5,6 +5,7 @@ package properties
 
 import (
 	"errors"
+	"sync"
 
 	"github.com/mattermost/mattermost/server/v8/channels/store"
 )
@@ -13,6 +14,7 @@ type PropertyService struct {
 	groupStore store.PropertyGroupStore
 	fieldStore store.PropertyFieldStore
 	valueStore store.PropertyValueStore
+	groupCache sync.Map // name -> *model.PropertyGroup
 }
 
 type ServiceConfig struct {

server/channels/app/property_access.go

@@ -75,6 +75,12 @@ func (pas *PropertyAccessService) GetPropertyGroup(name string) (*model.Property
 	return pas.propertyService.GetPropertyGroup(name)
 }
 
+// Group returns the cached PropertyGroup for a given name. If the
+// group is not in the cache, it falls back to a database lookup.
+func (pas *PropertyAccessService) Group(name string) (*model.PropertyGroup, error) {
+	return pas.propertyService.Group(name)
+}
+
 // Property Field Methods
 
 // CreatePropertyField creates a new property field.
@@ -1300,9 +1306,9 @@ func (pas *PropertyAccessService) applyValueReadAccessControl(values []*model.Pr
 }
 
 func (pas *PropertyAccessService) groupHasAccessRestrictions(groupId string) (bool, error) {
-	cpaID, err := getCpaGroupID(pas)
+	cpaGroup, err := pas.Group(model.CustomProfileAttributesPropertyGroupName)
 	if err != nil {
 		return false, err
 	}
-	return groupId == cpaID, nil
+	return groupId == cpaGroup.ID, nil
 }

server/channels/app/property_access_test.go

@@ -18,9 +18,8 @@ func TestGetPropertyFieldReadAccess(t *testing.T) {
 	pas := th.App.PropertyAccessService()
 
 	// Register the CPA group
-	group, err := pas.RegisterPropertyGroup("cpa")
+	group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName)
 	require.NoError(t, err)
-	cpaGroupID = group.ID
 
 	pluginID1 := "plugin-1"
 	pluginID2 := "plugin-2"
@@ -371,9 +370,8 @@ func TestGetPropertyFieldsReadAccess(t *testing.T) {
 	pas := th.App.PropertyAccessService()
 
 	// Register the CPA group
-	group, err := pas.RegisterPropertyGroup("cpa")
+	group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName)
 	require.NoError(t, err)
-	cpaGroupID = group.ID
 
 	pluginID := "plugin-1"
 	userID := model.NewId()
@@ -474,9 +472,8 @@ func TestSearchPropertyFieldsReadAccess(t *testing.T) {
 	pas := th.App.PropertyAccessService()
 
 	// Register the CPA group
-	group, err := pas.RegisterPropertyGroup("cpa")
+	group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName)
 	require.NoError(t, err)
-	cpaGroupID = group.ID
 
 	pluginID := "plugin-1"
 	userID := model.NewId()
@@ -581,9 +578,8 @@ func TestGetPropertyFieldByNameReadAccess(t *testing.T) {
 	pas := th.App.PropertyAccessService()
 
 	// Register the CPA group
-	group, err := pas.RegisterPropertyGroup("cpa")
+	group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName)
 	require.NoError(t, err)
-	cpaGroupID = group.ID
 
 	pluginID := "plugin-1"
 	userID := model.NewId()
@@ -626,11 +622,9 @@ func TestCreatePropertyField_SourcePluginIDValidation(t *testing.T) {
 	pas := th.App.PropertyAccessService()
 
 	// Register the CPA group
-	group, err := pas.RegisterPropertyGroup("cpa")
+	group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName)
 	require.NoError(t, err)
-	cpaGroupID = group.ID
-
-	groupID := cpaGroupID
+	groupID := group.ID
 
 	t.Run("allows field creation without source_plugin_id", func(t *testing.T) {
 		field := &model.PropertyField{
@@ -778,11 +772,9 @@ func TestCreatePropertyFieldForPlugin(t *testing.T) {
 	pas := th.App.PropertyAccessService()
 
 	// Register the CPA group
-	group, err := pas.RegisterPropertyGroup("cpa")
+	group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName)
 	require.NoError(t, err)
-	cpaGroupID = group.ID
-
-	groupID := cpaGroupID
+	groupID := group.ID
 
 	t.Run("automatically sets source_plugin_id", func(t *testing.T) {
 		field := &model.PropertyField{
@@ -858,11 +850,9 @@ func TestUpdatePropertyField_WriteAccessControl(t *testing.T) {
 	pas := th.App.PropertyAccessService()
 
 	// Register the CPA group
-	group, err := pas.RegisterPropertyGroup("cpa")
+	group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName)
 	require.NoError(t, err)
-	cpaGroupID = group.ID
-
-	groupID := cpaGroupID
+	groupID := group.ID
 
 	t.Run("allows update of unprotected field", func(t *testing.T) {
 		field := &model.PropertyField{
@@ -1039,11 +1029,9 @@ func TestUpdatePropertyFields_BulkWriteAccessControl(t *testing.T) {
 	pas := th.App.PropertyAccessService()
 
 	// Register the CPA group
-	group, err := pas.RegisterPropertyGroup("cpa")
+	group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName)
 	require.NoError(t, err)
-	cpaGroupID = group.ID
-
-	groupID := cpaGroupID
+	groupID := group.ID
 
 	t.Run("allows bulk update of unprotected fields", func(t *testing.T) {
 		field1 := &model.PropertyField{GroupID: groupID, Name: "Field1", Type: model.PropertyFieldTypeText}
@@ -1137,11 +1125,9 @@ func TestDeletePropertyField_WriteAccessControl(t *testing.T) {
 	pas := th.App.PropertyAccessService()
 
 	// Register the CPA group
-	group, err := pas.RegisterPropertyGroup("cpa")
+	group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName)
 	require.NoError(t, err)
-	cpaGroupID = group.ID
-
-	groupID := cpaGroupID
+	groupID := group.ID
 
 	t.Run("allows deletion of unprotected field", func(t *testing.T) {
 		field := &model.PropertyField{GroupID: groupID, Name: "Unprotected", Type: model.PropertyFieldTypeText}
@@ -1219,11 +1205,9 @@ func TestCreatePropertyValue_WriteAccessControl(t *testing.T) {
 	pas := th.App.PropertyAccessService()
 
 	// Register the CPA group
-	group, err := pas.RegisterPropertyGroup("cpa")
+	group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName)
 	require.NoError(t, err)
-	cpaGroupID = group.ID
-
-	groupID := cpaGroupID
+	groupID := group.ID
 
 	t.Run("allows creating value for public field", func(t *testing.T) {
 		field := &model.PropertyField{GroupID: groupID, Name: "Public", Type: model.PropertyFieldTypeText}
@@ -1334,11 +1318,9 @@ func TestDeletePropertyValue_WriteAccessControl(t *testing.T) {
 	pas := th.App.PropertyAccessService()
 
 	// Register the CPA group
-	group, err := pas.RegisterPropertyGroup("cpa")
+	group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName)
 	require.NoError(t, err)
-	cpaGroupID = group.ID
-
-	groupID := cpaGroupID
+	groupID := group.ID
 
 	t.Run("allows deleting value for public field", func(t *testing.T) {
 		field := &model.PropertyField{GroupID: groupID, Name: "Public", Type: model.PropertyFieldTypeText}
@@ -1394,11 +1376,9 @@ func TestDeletePropertyValuesForTarget_WriteAccessControl(t *testing.T) {
 	pas := th.App.PropertyAccessService()
 
 	// Register the CPA group
-	group, err := pas.RegisterPropertyGroup("cpa")
+	group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName)
 	require.NoError(t, err)
-	cpaGroupID = group.ID
-
-	groupID := cpaGroupID
+	groupID := group.ID
 
 	t.Run("allows deleting all values when caller has write access to all fields", func(t *testing.T) {
 		field1 := &model.PropertyField{GroupID: groupID, Name: "Field1", Type: model.PropertyFieldTypeText}
@@ -1470,9 +1450,8 @@ func TestGetPropertyValueReadAccess(t *testing.T) {
 	pas := th.App.PropertyAccessService()
 
 	// Register the CPA group
-	group, rerr := pas.RegisterPropertyGroup("cpa")
+	group, rerr := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName)
 	require.NoError(t, rerr)
-	cpaGroupID = group.ID
 
 	pluginID1 := "plugin-1"
 	pluginID2 := "plugin-2"
@@ -1857,9 +1836,8 @@ func TestGetPropertyValuesReadAccess(t *testing.T) {
 	pas := th.App.PropertyAccessService()
 
 	// Register the CPA group
-	group, rerr := pas.RegisterPropertyGroup("cpa")
+	group, rerr := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName)
 	require.NoError(t, rerr)
-	cpaGroupID = group.ID
 
 	pluginID1 := "plugin-1"
 	pluginID2 := "plugin-2"
@@ -1943,9 +1921,8 @@ func TestSearchPropertyValuesReadAccess(t *testing.T) {
 	pas := th.App.PropertyAccessService()
 
 	// Register the CPA group
-	group, rerr := pas.RegisterPropertyGroup("cpa")
+	group, rerr := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName)
 	require.NoError(t, rerr)
-	cpaGroupID = group.ID
 
 	pluginID1 := "plugin-1"
 	pluginID2 := "plugin-2"
@@ -2089,9 +2066,8 @@ func TestCreatePropertyValues_WriteAccessControl(t *testing.T) {
 	pas := th.App.PropertyAccessService()
 
 	// Register the CPA group
-	group, err := pas.RegisterPropertyGroup("cpa")
+	group, err := pas.RegisterPropertyGroup(model.CustomProfileAttributesPropertyGroupName)
 	require.NoError(t, err)
-	cpaGroupID = group.ID
 
 	pluginID1 := "plugin-1"
 	pluginID2 := "plugin-2"

server/channels/app/server.go

@@ -243,6 +243,13 @@ func NewServer(options ...Option) (*Server, error) {
 		return nil, errors.Wrapf(err, "unable to create properties service")
 	}
 
+	if err = propertyService.RegisterBuiltinGroups([]*model.PropertyGroup{
+		{Name: model.CustomProfileAttributesPropertyGroupName},
+		{Name: model.ContentFlaggingGroupName},
+	}); err != nil {
+		return nil, errors.Wrap(err, "failed to register builtin property groups")
+	}
+
 	// Wrap PropertyService with access control layer to enforce caller-based permissions
 	s.propertyAccessService = NewPropertyAccessService(propertyService, func(pluginID string) bool {
 		_, err := s.ch.GetPluginStatus(pluginID)

server/i18n/en.json

@@ -5528,7 +5528,7 @@
   },
   {
     "id": "app.custom_profile_attributes.cpa_group_id.app_error",
-    "translation": "Cannot register Custom Profile Attributes property group"
+    "translation": "Unable to retrieve the custom profile attributes property group."
   },
   {
     "id": "app.custom_profile_attributes.create_property_field.app_error",
@@ -5640,7 +5640,7 @@
   },
   {
     "id": "app.data_spillage.get_group.error",
-    "translation": "Failed to get Content Flagging bot."
+    "translation": "Unable to retrieve the content flagging property group."
   },
   {
     "id": "app.data_spillage.get_reviewer_settings.app_error",