Adds the capability to retrieve a property field by name (#30859)

* Adds the capability to retrieve a property field by name Allows to retrieve a property field by name and groupID. As the name is only unique within the context of a group, and we can have multiple fields with the same name in the store, for this method the groupID is directly included in the query instead of being an optional field. * Adds the targetID parameter to correctly filter fields * Ensure the method only retrieves non-deleted fields --------- Co-authored-by: Miguel de la Cruz <miguel@ctrlz.es> Co-authored-by: Mattermost Build <build@mattermost.com>
2025-05-13 12:45:35 +02:00
--- a/server/channels/app/properties/property_field.go
+++ b/server/channels/app/properties/property_field.go
@@ -21,6 +21,10 @@ func (ps *PropertyService) GetPropertyFields(groupID string, ids []string) ([]*m
 	return ps.fieldStore.GetMany(groupID, ids)
 }

+func (ps *PropertyService) GetPropertyFieldByName(groupID, targetID, name string) (*model.PropertyField, error) {
+	return ps.fieldStore.GetFieldByName(groupID, targetID, name)
+}
+
 func (ps *PropertyService) CountActivePropertyFieldsForGroup(groupID string) (int64, error) {
 	return ps.fieldStore.CountForGroup(groupID, false)
 }
--- a/server/channels/store/retrylayer/retrylayer.go
+++ b/server/channels/store/retrylayer/retrylayer.go
@@ -9190,6 +9190,27 @@ func (s *RetryLayerPropertyFieldStore) Get(groupID string, id string) (*model.Pr

 }

+func (s *RetryLayerPropertyFieldStore) GetFieldByName(groupID string, targetID string, name string) (*model.PropertyField, error) {
+
+	tries := 0
+	for {
+		result, err := s.PropertyFieldStore.GetFieldByName(groupID, targetID, name)
+		if err == nil {
+			return result, nil
+		}
+		if !isRepeatableError(err) {
+			return result, err
+		}
+		tries++
+		if tries >= 3 {
+			err = errors.Wrap(err, "giving up after 3 consecutive repeatable transaction failures")
+			return result, err
+		}
+		timepkg.Sleep(100 * timepkg.Millisecond)
+	}
+
+}
+
 func (s *RetryLayerPropertyFieldStore) GetMany(groupID string, ids []string) ([]*model.PropertyField, error) {

 	tries := 0
--- a/server/channels/store/sqlstore/property_field_store.go
+++ b/server/channels/store/sqlstore/property_field_store.go
@@ -67,6 +67,21 @@ func (s *SqlPropertyFieldStore) Get(groupID, id string) (*model.PropertyField, e
 	return &field, nil
 }

+func (s *SqlPropertyFieldStore) GetFieldByName(groupID, targetID, name string) (*model.PropertyField, error) {
+	builder := s.tableSelectQuery.
+		Where(sq.Eq{"GroupID": groupID}).
+		Where(sq.Eq{"TargetID": targetID}).
+		Where(sq.Eq{"Name": name}).
+		Where(sq.Eq{"DeleteAt": 0})
+
+	var field model.PropertyField
+	if err := s.GetReplica().GetBuilder(&field, builder); err != nil {
+		return nil, errors.Wrap(err, "property_field_get_by_name_select")
+	}
+
+	return &field, nil
+}
+
 func (s *SqlPropertyFieldStore) GetMany(groupID string, ids []string) ([]*model.PropertyField, error) {
 	builder := s.tableSelectQuery.Where(sq.Eq{"id": ids})

--- a/server/channels/store/store.go
+++ b/server/channels/store/store.go
@@ -1092,6 +1092,7 @@ type PropertyFieldStore interface {
 	Create(field *model.PropertyField) (*model.PropertyField, error)
 	Get(groupID, id string) (*model.PropertyField, error)
 	GetMany(groupID string, ids []string) ([]*model.PropertyField, error)
+	GetFieldByName(groupID, targetID, name string) (*model.PropertyField, error)
 	CountForGroup(groupID string, includeDeleted bool) (int64, error)
 	SearchPropertyFields(opts model.PropertyFieldSearchOpts) ([]*model.PropertyField, error)
 	Update(groupID string, fields []*model.PropertyField) ([]*model.PropertyField, error)
--- a/server/channels/store/storetest/mocks/PropertyFieldStore.go
+++ b/server/channels/store/storetest/mocks/PropertyFieldStore.go
@@ -120,6 +120,36 @@ func (_m *PropertyFieldStore) Get(groupID string, id string) (*model.PropertyFie
 	return r0, r1
 }

+// GetFieldByName provides a mock function with given fields: groupID, targetID, name
+func (_m *PropertyFieldStore) GetFieldByName(groupID string, targetID string, name string) (*model.PropertyField, error) {
+	ret := _m.Called(groupID, targetID, name)
+
+	if len(ret) == 0 {
+		panic("no return value specified for GetFieldByName")
+	}
+
+	var r0 *model.PropertyField
+	var r1 error
+	if rf, ok := ret.Get(0).(func(string, string, string) (*model.PropertyField, error)); ok {
+		return rf(groupID, targetID, name)
+	}
+	if rf, ok := ret.Get(0).(func(string, string, string) *model.PropertyField); ok {
+		r0 = rf(groupID, targetID, name)
+	} else {
+		if ret.Get(0) != nil {
+			r0 = ret.Get(0).(*model.PropertyField)
+		}
+	}
+
+	if rf, ok := ret.Get(1).(func(string, string, string) error); ok {
+		r1 = rf(groupID, targetID, name)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
 // GetMany provides a mock function with given fields: groupID, ids
 func (_m *PropertyFieldStore) GetMany(groupID string, ids []string) ([]*model.PropertyField, error) {
 	ret := _m.Called(groupID, ids)
--- a/server/channels/store/storetest/property_field_store.go
+++ b/server/channels/store/storetest/property_field_store.go
@@ -19,6 +19,7 @@ func TestPropertyFieldStore(t *testing.T, rctx request.CTX, ss store.Store, s Sq
 	t.Run("CreatePropertyField", func(t *testing.T) { testCreatePropertyField(t, rctx, ss) })
 	t.Run("GetPropertyField", func(t *testing.T) { testGetPropertyField(t, rctx, ss) })
 	t.Run("GetManyPropertyFields", func(t *testing.T) { testGetManyPropertyFields(t, rctx, ss) })
+	t.Run("GetFieldByName", func(t *testing.T) { testGetFieldByName(t, rctx, ss) })
 	t.Run("UpdatePropertyField", func(t *testing.T) { testUpdatePropertyField(t, rctx, ss) })
 	t.Run("DeletePropertyField", func(t *testing.T) { testDeletePropertyField(t, rctx, ss) })
 	t.Run("SearchPropertyFields", func(t *testing.T) { testSearchPropertyFields(t, rctx, ss) })
@@ -173,6 +174,162 @@ func testGetManyPropertyFields(t *testing.T, _ request.CTX, ss store.Store) {
 	})
 }

+func testGetFieldByName(t *testing.T, _ request.CTX, ss store.Store) {
+	t.Run("should fail on nonexisting field", func(t *testing.T) {
+		field, err := ss.PropertyField().GetFieldByName("", "", "nonexistent-field-name")
+		require.Zero(t, field)
+		require.ErrorIs(t, err, sql.ErrNoRows)
+	})
+
+	groupID := model.NewId()
+	targetID := model.NewId()
+	newField := &model.PropertyField{
+		GroupID:  groupID,
+		TargetID: targetID,
+		Name:     "unique-field-name",
+		Type:     model.PropertyFieldTypeText,
+		Attrs: map[string]any{
+			"locked":  true,
+			"special": "value",
+		},
+	}
+	_, cErr := ss.PropertyField().Create(newField)
+	require.NoError(t, cErr)
+	require.NotZero(t, newField.ID)
+
+	t.Run("should be able to retrieve an existing property field by name", func(t *testing.T) {
+		field, err := ss.PropertyField().GetFieldByName(groupID, targetID, "unique-field-name")
+		require.NoError(t, err)
+		require.Equal(t, newField.ID, field.ID)
+		require.Equal(t, "unique-field-name", field.Name)
+		require.True(t, field.Attrs["locked"].(bool))
+		require.Equal(t, "value", field.Attrs["special"])
+	})
+
+	t.Run("should not be able to retrieve an existing field when specifying a different group ID", func(t *testing.T) {
+		field, err := ss.PropertyField().GetFieldByName(model.NewId(), targetID, "unique-field-name")
+		require.Zero(t, field)
+		require.ErrorIs(t, err, sql.ErrNoRows)
+	})
+
+	t.Run("should not be able to retrieve an existing field when specifying a different target ID", func(t *testing.T) {
+		field, err := ss.PropertyField().GetFieldByName(groupID, model.NewId(), "unique-field-name")
+		require.Zero(t, field)
+		require.ErrorIs(t, err, sql.ErrNoRows)
+	})
+
+	// Test with multiple fields with the same name but different groups
+	anotherGroupID := model.NewId()
+	duplicateNameField := &model.PropertyField{
+		GroupID:  anotherGroupID,
+		TargetID: targetID,
+		Name:     "unique-field-name", // Same name as the first field
+		Type:     model.PropertyFieldTypeSelect,
+		Attrs: map[string]any{
+			"options": []string{"a", "b", "c"},
+		},
+	}
+	_, cErr = ss.PropertyField().Create(duplicateNameField)
+	require.NoError(t, cErr)
+	require.NotZero(t, duplicateNameField.ID)
+
+	t.Run("should retrieve the correct field when multiple fields have the same name but different groups", func(t *testing.T) {
+		// Get the field from the first group
+		field, err := ss.PropertyField().GetFieldByName(groupID, targetID, "unique-field-name")
+		require.NoError(t, err)
+		require.Equal(t, newField.ID, field.ID)
+		require.Equal(t, model.PropertyFieldTypeText, field.Type)
+
+		// Get the field from the second group
+		field, err = ss.PropertyField().GetFieldByName(anotherGroupID, targetID, "unique-field-name")
+		require.NoError(t, err)
+		require.Equal(t, duplicateNameField.ID, field.ID)
+		require.Equal(t, model.PropertyFieldTypeSelect, field.Type)
+	})
+
+	// Test with multiple fields with the same name and same group but different target IDs
+	anotherTargetID := model.NewId()
+	sameGroupDifferentTargetField := &model.PropertyField{
+		GroupID:  groupID,
+		TargetID: anotherTargetID,
+		Name:     "unique-field-name", // Same name as the first field
+		Type:     model.PropertyFieldTypeText,
+		Attrs: map[string]any{
+			"min": 1,
+			"max": 100,
+		},
+	}
+	_, cErr = ss.PropertyField().Create(sameGroupDifferentTargetField)
+	require.NoError(t, cErr)
+	require.NotZero(t, sameGroupDifferentTargetField.ID)
+
+	t.Run("should retrieve the correct field when multiple fields have the same name and group but different target IDs", func(t *testing.T) {
+		// Get the field with the first target ID
+		field, err := ss.PropertyField().GetFieldByName(groupID, targetID, "unique-field-name")
+		require.NoError(t, err)
+		require.Equal(t, newField.ID, field.ID)
+		require.Equal(t, model.PropertyFieldTypeText, field.Type)
+
+		// Get the field with the second target ID
+		field, err = ss.PropertyField().GetFieldByName(groupID, anotherTargetID, "unique-field-name")
+		require.NoError(t, err)
+		require.Equal(t, sameGroupDifferentTargetField.ID, field.ID)
+		require.Equal(t, model.PropertyFieldTypeText, field.Type)
+	})
+
+	// Test with a deleted field
+	t.Run("should not retrieve deleted fields", func(t *testing.T) {
+		// Create another field with a unique name
+		deletedField := &model.PropertyField{
+			GroupID:  groupID,
+			TargetID: targetID,
+			Name:     "to-be-deleted-field",
+			Type:     model.PropertyFieldTypeText,
+		}
+		_, cErr := ss.PropertyField().Create(deletedField)
+		require.NoError(t, cErr)
+		require.NotZero(t, deletedField.ID)
+
+		// Verify it can be retrieved before deletion
+		field, err := ss.PropertyField().GetFieldByName(groupID, targetID, "to-be-deleted-field")
+		require.NoError(t, err)
+		require.Equal(t, deletedField.ID, field.ID)
+
+		// Delete the field
+		err = ss.PropertyField().Delete("", deletedField.ID)
+		require.NoError(t, err)
+
+		// Verify it can't be retrieved after deletion
+		field, err = ss.PropertyField().GetFieldByName(groupID, targetID, "to-be-deleted-field")
+		require.Zero(t, field)
+		require.ErrorIs(t, err, sql.ErrNoRows)
+	})
+
+	t.Run("should not retrieve fields with matching name but different DeleteAt status", func(t *testing.T) {
+		// Create a field with the same name/group/target as the deleted one
+		replacementField := &model.PropertyField{
+			GroupID:  groupID,
+			TargetID: targetID,
+			Name:     "to-be-deleted-field", // Same name as the deleted field
+			Type:     model.PropertyFieldTypeText,
+			Attrs: map[string]any{
+				"min": 0,
+				"max": 10,
+			},
+		}
+		_, cErr := ss.PropertyField().Create(replacementField)
+		require.NoError(t, cErr)
+		require.NotZero(t, replacementField.ID)
+
+		// Verify only the non-deleted field is retrieved
+		field, err := ss.PropertyField().GetFieldByName(groupID, targetID, "to-be-deleted-field")
+		require.NoError(t, err)
+		require.Equal(t, replacementField.ID, field.ID)
+		require.Equal(t, model.PropertyFieldTypeText, field.Type)
+		require.Zero(t, field.DeleteAt)
+	})
+}
+
 func testUpdatePropertyField(t *testing.T, _ request.CTX, ss store.Store) {
 	t.Run("should fail on nonexisting field", func(t *testing.T) {
 		field := &model.PropertyField{
--- a/server/channels/store/timerlayer/timerlayer.go
+++ b/server/channels/store/timerlayer/timerlayer.go
@@ -7291,6 +7291,22 @@ func (s *TimerLayerPropertyFieldStore) Get(groupID string, id string) (*model.Pr
 	return result, err
 }

+func (s *TimerLayerPropertyFieldStore) GetFieldByName(groupID string, targetID string, name string) (*model.PropertyField, error) {
+	start := time.Now()
+
+	result, err := s.PropertyFieldStore.GetFieldByName(groupID, targetID, name)
+
+	elapsed := float64(time.Since(start)) / float64(time.Second)
+	if s.Root.Metrics != nil {
+		success := "false"
+		if err == nil {
+			success = "true"
+		}
+		s.Root.Metrics.ObserveStoreMethodDuration("PropertyFieldStore.GetFieldByName", success, elapsed)
+	}
+	return result, err
+}
+
 func (s *TimerLayerPropertyFieldStore) GetMany(groupID string, ids []string) ([]*model.PropertyField, error) {
 	start := time.Now()