From 5ba80d51ae2300a3f937e417739aa5a8ed2c113f Mon Sep 17 00:00:00 2001 From: Miguel de la Cruz Date: Thu, 13 Feb 2025 12:12:51 +0100 Subject: [PATCH] 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) Co-authored-by: Julien Tant <785518+JulienTant@users.noreply.github.com> --- .../store/sqlstore/property_field_store.go | 80 ++++++++++++------- .../store/sqlstore/property_value_store.go | 63 ++++++++------- .../store/storetest/property_field_store.go | 43 +++++++++- .../store/storetest/property_value_store.go | 44 +++++++++- 4 files changed, 169 insertions(+), 61 deletions(-) diff --git a/server/channels/store/sqlstore/property_field_store.go b/server/channels/store/sqlstore/property_field_store.go index c434f1702b5..1f10ec84757 100644 --- 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 diff --git a/server/channels/store/sqlstore/property_value_store.go b/server/channels/store/sqlstore/property_value_store.go index 80c686aa0db..1e39cb0c79c 100644 --- 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 diff --git a/server/channels/store/storetest/property_field_store.go b/server/channels/store/storetest/property_field_store.go index a2880c21424..19fe3d68012 100644 --- 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) { diff --git a/server/channels/store/storetest/property_value_store.go b/server/channels/store/storetest/property_value_store.go index 271f1500d04..41a4ba3cf0f 100644 --- 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) {