Conversation
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Code Review
This pull request introduces a bulk clear feature for channel override templates and adds a 'Replace' mode to the template application process. Changes span the frontend UI, GraphQL schema, and backend business logic. Feedback was provided regarding the ClearTemplates backend implementation, noting a race condition caused by querying outside the transaction, a potential bug with duplicate channel IDs in the length check, and issues with slice initialization during transaction retries.
| channels, err := db.Channel.Query(). | ||
| Where(channel.IDIn(channelIDs...)). | ||
| All(ctx) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to query channels: %w", err) | ||
| } | ||
|
|
||
| if len(channels) != len(channelIDs) { | ||
| return nil, fmt.Errorf("some channels not found for provided IDs") | ||
| } | ||
|
|
||
| updated = make([]*ent.Channel, 0, len(channels)) |
There was a problem hiding this comment.
The ClearTemplates function queries channels outside the transaction and then iterates over them to perform updates. This pattern is susceptible to race conditions where channel settings might change between the query and the update, leading to lost updates. Additionally, the updated slice is initialized outside the transaction closure; if the transaction retries (depending on the RunInTransaction implementation), the slice will accumulate duplicate entries from failed attempts. Finally, the length check len(channels) != len(channelIDs) will fail if channelIDs contains duplicate IDs, even if all requested channels exist.
func (svc *ChannelOverrideTemplateService) ClearTemplates(
ctx context.Context,
channelIDs []int,
) (updated []*ent.Channel, err error) {
uniqueIDs := lo.Uniq(channelIDs)
err = svc.RunInTransaction(ctx, func(ctx context.Context) error {
db := svc.entFromContext(ctx)
channels, err := db.Channel.Query().
Where(channel.IDIn(uniqueIDs...)).
ForUpdate().
All(ctx)
if err != nil {
return fmt.Errorf("failed to query channels: %w", err)
}
if len(channels) != len(uniqueIDs) {
return fmt.Errorf("some channels not found for provided IDs")
}
updated = make([]*ent.Channel, 0, len(channels))
for _, ch := range channels {
settings := objects.ChannelSettings{}
if ch.Settings != nil {
settings = *ch.Settings
}
settings.HeaderOverrideOperations = nil
settings.OverrideHeaders = nil
settings.BodyOverrideOperations = nil
settings.OverrideParameters = ""
updatedChannel, err := db.Channel.UpdateOneID(ch.ID).
SetSettings(&settings).
Save(ctx)
if err != nil {
return fmt.Errorf("failed to update channel %s: %w", ch.Name, err)
}
updated = append(updated, updatedChannel)
}
return nil
})
if err != nil {
return nil, err
}
if svc.channelService != nil {
svc.channelService.asyncReloadChannels()
}
return updated, nil
}
…lk apply, close #1404