Updates the property field and value update methods to use a single query for multiple entities (#30198)

* Updates the property field and value update methods to use a single query for multiple entities * Update server/channels/store/sqlstore/property_field_store.go Co-authored-by: Julien Tant <785518+JulienTant@users.noreply.github.com> --------- Co-authored-by: Miguel de la Cruz (aider) <miguel@ctrlz.es> Co-authored-by: Julien Tant <785518+JulienTant@users.noreply.github.com>
2025-02-25 18:55:24 -06:00 · 2025-02-13 12:12:51 +01:00 · 2025-02-13 12:12:51 +01:00 · 5ba80d51ae
commit 5ba80d51ae
parent 83ec1d9f7d
4 changed files with 169 additions and 61 deletions
--- a/server/channels/store/sqlstore/property_field_store.go
+++ b/server/channels/store/sqlstore/property_field_store.go
@ -128,44 +128,66 @@ func (s *SqlPropertyFieldStore) Update(fields []*model.PropertyField) (_ []*mode
 	defer finalizeTransactionX(transaction, &err)

 	updateTime := model.GetMillis()
-	for _, field := range fields {
-		field.UpdateAt = updateTime
+	isPostgres := s.DriverName() == model.DatabaseDriverPostgres
+	nameCase := sq.Case("id")
+	typeCase := sq.Case("id")
+	attrsCase := sq.Case("id")
+	targetIDCase := sq.Case("id")
+	targetTypeCase := sq.Case("id")
+	deleteAtCase := sq.Case("id")
+	ids := make([]string, len(fields))

+	for i, field := range fields {
+		field.UpdateAt = updateTime
 		if vErr := field.IsValid(); vErr != nil {
 			return nil, errors.Wrap(vErr, "property_field_update_isvalid")
 		}

-		queryString, args, err := s.getQueryBuilder().
-			Update("PropertyFields").
-			Set("Name", field.Name).
-			Set("Type", field.Type).
-			Set("Attrs", field.Attrs).
-			Set("TargetID", field.TargetID).
-			Set("TargetType", field.TargetType).
-			Set("UpdateAt", field.UpdateAt).
-			Set("DeleteAt", field.DeleteAt).
-			Where(sq.Eq{"id": field.ID}).
-			ToSql()
-		if err != nil {
-			return nil, errors.Wrap(err, "property_field_update_tosql")
-		}
-
-		result, err := transaction.Exec(queryString, args...)
-		if err != nil {
-			return nil, errors.Wrapf(err, "failed to update property field with id: %s", field.ID)
-		}
-
-		count, err := result.RowsAffected()
-		if err != nil {
-			return nil, errors.Wrap(err, "property_field_update_rowsaffected")
-		}
-		if count == 0 {
-			return nil, store.NewErrNotFound("PropertyField", field.ID)
+		ids[i] = field.ID
+		whenID := sq.Expr("?", field.ID)
+		if isPostgres {
+			nameCase = nameCase.When(whenID, sq.Expr("?::text", field.Name))
+			typeCase = typeCase.When(whenID, sq.Expr("?::property_field_type", field.Type))
+			attrsCase = attrsCase.When(whenID, sq.Expr("?::jsonb", field.Attrs))
+			targetIDCase = targetIDCase.When(whenID, sq.Expr("?::text", field.TargetID))
+			targetTypeCase = targetTypeCase.When(whenID, sq.Expr("?::text", field.TargetType))
+			deleteAtCase = deleteAtCase.When(whenID, sq.Expr("?::bigint", field.DeleteAt))
+		} else {
+			nameCase = nameCase.When(whenID, sq.Expr("?", field.Name))
+			typeCase = typeCase.When(whenID, sq.Expr("?", field.Type))
+			attrsCase = attrsCase.When(whenID, sq.Expr("?", field.Attrs))
+			targetIDCase = targetIDCase.When(whenID, sq.Expr("?", field.TargetID))
+			targetTypeCase = targetTypeCase.When(whenID, sq.Expr("?", field.TargetType))
+			deleteAtCase = deleteAtCase.When(whenID, sq.Expr("?", field.DeleteAt))
 		}
 	}

+	builder := s.getQueryBuilder().
+		Update("PropertyFields").
+		Set("Name", nameCase).
+		Set("Type", typeCase).
+		Set("Attrs", attrsCase).
+		Set("TargetID", targetIDCase).
+		Set("TargetType", targetTypeCase).
+		Set("UpdateAt", updateTime).
+		Set("DeleteAt", deleteAtCase).
+		Where(sq.Eq{"id": ids})
+
+	result, err := transaction.ExecBuilder(builder)
+	if err != nil {
+		return nil, errors.Wrap(err, "property_field_update_exec")
+	}
+
+	count, err := result.RowsAffected()
+	if err != nil {
+		return nil, errors.Wrap(err, "property_field_update_rowsaffected")
+	}
+	if count != int64(len(fields)) {
+		return nil, errors.Errorf("failed to update, some property fields were not found, got %d of %d", count, len(fields))
+	}
+
 	if err := transaction.Commit(); err != nil {
-		return nil, errors.Wrap(err, "property_field_update_commit")
+		return nil, errors.Wrap(err, "property_field_update_commit_transaction")
 	}

 	return fields, nil
--- a/server/channels/store/sqlstore/property_value_store.go
+++ b/server/channels/store/sqlstore/property_value_store.go
@ -136,45 +136,54 @@ func (s *SqlPropertyValueStore) Update(values []*model.PropertyValue) (_ []*mode
 	defer finalizeTransactionX(transaction, &err)

 	updateTime := model.GetMillis()
-	for _, value := range values {
-		value.UpdateAt = updateTime
+	isPostgres := s.DriverName() == model.DatabaseDriverPostgres
+	valueCase := sq.Case("id")
+	deleteAtCase := sq.Case("id")
+	ids := make([]string, len(values))

-		if err := value.IsValid(); err != nil {
-			return nil, errors.Wrap(err, "property_value_update_isvalid")
+	for i, value := range values {
+		value.UpdateAt = updateTime
+		if vErr := value.IsValid(); vErr != nil {
+			return nil, errors.Wrap(vErr, "property_value_update_isvalid")
 		}

+		ids[i] = value.ID
 		valueJSON := value.Value
 		if s.IsBinaryParamEnabled() {
 			valueJSON = AppendBinaryFlag(valueJSON)
 		}

-		queryString, args, err := s.getQueryBuilder().
-			Update("PropertyValues").
-			Set("Value", valueJSON).
-			Set("UpdateAt", value.UpdateAt).
-			Set("DeleteAt", value.DeleteAt).
-			Where(sq.Eq{"id": value.ID}).
-			ToSql()
-		if err != nil {
-			return nil, errors.Wrap(err, "property_value_update_tosql")
-		}
-
-		result, err := transaction.Exec(queryString, args...)
-		if err != nil {
-			return nil, errors.Wrapf(err, "failed to update property value with id: %s", value.ID)
-		}
-
-		count, err := result.RowsAffected()
-		if err != nil {
-			return nil, errors.Wrap(err, "property_value_update_rowsaffected")
-		}
-		if count == 0 {
-			return nil, store.NewErrNotFound("PropertyValue", value.ID)
+		if isPostgres {
+			valueCase = valueCase.When(sq.Expr("?", value.ID), sq.Expr("?::jsonb", valueJSON))
+			deleteAtCase = deleteAtCase.When(sq.Expr("?", value.ID), sq.Expr("?::bigint", value.DeleteAt))
+		} else {
+			valueCase = valueCase.When(sq.Expr("?", value.ID), sq.Expr("?", valueJSON))
+			deleteAtCase = deleteAtCase.When(sq.Expr("?", value.ID), sq.Expr("?", value.DeleteAt))
 		}
 	}

+	builder := s.getQueryBuilder().
+		Update("PropertyValues").
+		Set("Value", valueCase).
+		Set("DeleteAt", deleteAtCase).
+		Set("UpdateAt", updateTime).
+		Where(sq.Eq{"id": ids})
+
+	result, err := transaction.ExecBuilder(builder)
+	if err != nil {
+		return nil, errors.Wrap(err, "property_value_update_exec")
+	}
+
+	count, err := result.RowsAffected()
+	if err != nil {
+		return nil, errors.Wrap(err, "property_value_update_rowsaffected")
+	}
+	if count != int64(len(values)) {
+		return nil, errors.Errorf("failed to update, some property values were not found, got %d of %d", count, len(values))
+	}
+
 	if err := transaction.Commit(); err != nil {
-		return nil, errors.Wrap(err, "property_value_update_commit")
+		return nil, errors.Wrap(err, "property_value_update_commit_transaction")
 	}

 	return values, nil
--- a/server/channels/store/storetest/property_field_store.go
+++ b/server/channels/store/storetest/property_field_store.go
@ -146,8 +146,7 @@ func testUpdatePropertyField(t *testing.T, _ request.CTX, ss store.Store) {
 		}
 		updatedField, err := ss.PropertyField().Update([]*model.PropertyField{field})
 		require.Zero(t, updatedField)
-		var enf *store.ErrNotFound
-		require.ErrorAs(t, err, &enf)
+		require.ErrorContains(t, err, "failed to update, some property fields were not found, got 0 of 1")
 	})

 	t.Run("should fail if the property field is not valid", func(t *testing.T) {
@ -280,6 +279,46 @@ func testUpdatePropertyField(t *testing.T, _ request.CTX, ss store.Store) {
 		require.Equal(t, groupID, updated2.GroupID)
 		require.Equal(t, originalUpdateAt2, updated2.UpdateAt)
 	})
+
+	t.Run("should not update any fields if one update points to a nonexisting one", func(t *testing.T) {
+		// Create a valid field
+		field1 := &model.PropertyField{
+			GroupID: model.NewId(),
+			Name:    "First field",
+			Type:    model.PropertyFieldTypeText,
+		}
+
+		_, err := ss.PropertyField().Create(field1)
+		require.NoError(t, err)
+
+		originalUpdateAt := field1.UpdateAt
+
+		// Try to update both the valid field and a nonexistent one
+		field2 := &model.PropertyField{
+			ID:         model.NewId(),
+			GroupID:    model.NewId(),
+			Name:       "Second field",
+			Type:       model.PropertyFieldTypeText,
+			TargetID:   model.NewId(),
+			TargetType: "test_type",
+			CreateAt:   1,
+			Attrs: map[string]any{
+				"key": "value",
+			},
+		}
+
+		field1.Name = "Updated First"
+
+		_, err = ss.PropertyField().Update([]*model.PropertyField{field1, field2})
+		require.Error(t, err)
+		require.ErrorContains(t, err, "failed to update, some property fields were not found")
+
+		// Check that the valid field was not updated
+		updated1, err := ss.PropertyField().Get(field1.ID)
+		require.NoError(t, err)
+		require.Equal(t, "First field", updated1.Name)
+		require.Equal(t, originalUpdateAt, updated1.UpdateAt)
+	})
 }

 func testDeletePropertyField(t *testing.T, _ request.CTX, ss store.Store) {
--- a/server/channels/store/storetest/property_value_store.go
+++ b/server/channels/store/storetest/property_value_store.go
@ -149,8 +149,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) {
 		}
 		updatedValue, err := ss.PropertyValue().Update([]*model.PropertyValue{value})
 		require.Zero(t, updatedValue)
-		var enf *store.ErrNotFound
-		require.ErrorAs(t, err, &enf)
+		require.ErrorContains(t, err, "failed to update, some property values were not found, got 0 of 1")
 	})

 	t.Run("should fail if the property value is not valid", func(t *testing.T) {
@ -220,7 +219,7 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) {
 		require.Greater(t, updated2.UpdateAt, updated2.CreateAt)
 	})

-	t.Run("should not update any fields if one update is invalid", func(t *testing.T) {
+	t.Run("should not update any values if one update is invalid", func(t *testing.T) {
 		// Create two valid values
 		groupID := model.NewId()
 		value1 := &model.PropertyValue{
@ -266,6 +265,45 @@ func testUpdatePropertyValue(t *testing.T, _ request.CTX, ss store.Store) {
 		require.Equal(t, groupID, updated2.GroupID)
 		require.Equal(t, originalUpdateAt2, updated2.UpdateAt)
 	})
+
+	t.Run("should not update any values if one update points to a nonexisting one", func(t *testing.T) {
+		// Create a valid value
+		value1 := &model.PropertyValue{
+			TargetID:   model.NewId(),
+			TargetType: "test_type",
+			GroupID:    model.NewId(),
+			FieldID:    model.NewId(),
+			Value:      json.RawMessage(`"Value 1"`),
+		}
+
+		_, err := ss.PropertyValue().Create(value1)
+		require.NoError(t, err)
+
+		originalUpdateAt := value1.UpdateAt
+
+		// Try to update both the valid value and a nonexistent one
+		value2 := &model.PropertyValue{
+			ID:         model.NewId(),
+			TargetID:   model.NewId(),
+			CreateAt:   1,
+			TargetType: "test_type",
+			GroupID:    model.NewId(),
+			FieldID:    model.NewId(),
+			Value:      json.RawMessage(`"Value 2"`),
+		}
+
+		value1.Value = json.RawMessage(`"Updated Value 1"`)
+
+		_, err = ss.PropertyValue().Update([]*model.PropertyValue{value1, value2})
+		require.Error(t, err)
+		require.ErrorContains(t, err, "failed to update, some property values were not found")
+
+		// Check that the valid value was not updated
+		updated1, err := ss.PropertyValue().Get(value1.ID)
+		require.NoError(t, err)
+		require.Equal(t, json.RawMessage(`"Value 1"`), updated1.Value)
+		require.Equal(t, originalUpdateAt, updated1.UpdateAt)
+	})
 }

 func testDeletePropertyValue(t *testing.T, _ request.CTX, ss store.Store) {