fix(podcast): address code review feedback

- Add 30s timeout to episode download HTTP client
- Add 15s timeout to RSS feed fetch HTTP client
- Fix N+1 query in GetAll(withEpisodes): fetch all episodes in a single
  query using IN clause via GetByChannels
- Add sanitizeMetadata helper to strip null bytes from ffmpeg tag values
- Add TODO comment on background goroutine context for server shutdown

Signed-off-by: ji-ho lee <search5@gmail.com>
This commit is contained in:
Jiho Andrew Lee 2026-04-28 00:17:17 +09:00
parent cff5a2acb0
commit 5279f23bc8
5 changed files with 56 additions and 10 deletions

View File

@ -153,6 +153,8 @@ func (s *podcastService) DownloadEpisode(ctx context.Context, id string) error {
return err
}
// Use context.Background() so the download is not cancelled when the HTTP
// request completes. TODO: tie to server shutdown context for graceful termination.
go s.doDownload(context.Background(), ep, ch)
return nil
}
@ -176,7 +178,8 @@ func (s *podcastService) doDownload(ctx context.Context, ep *model.PodcastEpisod
}
defer f.Close()
resp, err := http.Get(ep.EnclosureURL) //nolint:gosec
httpClient := &http.Client{Timeout: 30 * time.Second}
resp, err := httpClient.Get(ep.EnclosureURL) //nolint:gosec
if err != nil {
s.setEpisodeError(ctx, ep, err)
return
@ -287,6 +290,12 @@ func (s *podcastService) setEpisodeError(ctx context.Context, ep *model.PodcastE
}
}
// sanitizeMetadata removes null bytes and trims whitespace from ffmpeg metadata values.
// Since exec.Command passes args directly (no shell), only null bytes need sanitizing.
func sanitizeMetadata(s string) string {
return strings.ReplaceAll(strings.TrimSpace(s), "\x00", "")
}
func (s *podcastService) writeID3Tags(ctx context.Context, dest, suffix, title, album string) {
if s.ff == nil {
return
@ -298,8 +307,8 @@ func (s *podcastService) writeID3Tags(ctx context.Context, dest, suffix, title,
tmp := dest + ".tmp." + suffix
cmd := exec.CommandContext(ctx, ffmpegPath,
"-i", dest,
"-metadata", "title="+title,
"-metadata", "album="+album,
"-metadata", "title="+sanitizeMetadata(title),
"-metadata", "album="+sanitizeMetadata(album),
"-metadata", "artist=",
"-metadata", "genre=Podcast",
"-c", "copy", "-y", tmp,
@ -386,7 +395,8 @@ func (pw *progressWriter) Write(p []byte) (int, error) {
}
func fetchAndParse(rssURL string) (*rssFeed, error) {
resp, err := http.Get(rssURL) //nolint:gosec
httpClient := &http.Client{Timeout: 15 * time.Second}
resp, err := httpClient.Get(rssURL) //nolint:gosec
if err != nil {
return nil, fmt.Errorf("fetching RSS feed: %w", err)
}

View File

@ -64,6 +64,7 @@ type PodcastEpisodeRepository interface {
Get(id string) (*PodcastEpisode, error)
GetNewest(count int) (PodcastEpisodes, error)
GetByChannel(channelID string) (PodcastEpisodes, error)
GetByChannels(channelIDs []string) (PodcastEpisodes, error)
GetByGUID(channelID, guid string) (*PodcastEpisode, error)
Create(ep *PodcastEpisode) error
Update(ep *PodcastEpisode) error

View File

@ -43,14 +43,22 @@ func (r *podcastChannelRepository) GetAll(withEpisodes bool) (model.PodcastChann
if err := r.queryAll(sel, &channels); err != nil {
return nil, err
}
if withEpisodes {
if withEpisodes && len(channels) > 0 {
ids := make([]string, len(channels))
for i, ch := range channels {
ids[i] = ch.ID
}
epRepo := NewPodcastEpisodeRepository(r.ctx, r.db)
allEps, err := epRepo.GetByChannels(ids)
if err != nil {
return nil, err
}
epsByChannel := make(map[string]model.PodcastEpisodes, len(channels))
for _, ep := range allEps {
epsByChannel[ep.ChannelID] = append(epsByChannel[ep.ChannelID], ep)
}
for i := range channels {
eps, err := epRepo.GetByChannel(channels[i].ID)
if err != nil {
return nil, err
}
channels[i].Episodes = eps
channels[i].Episodes = epsByChannel[channels[i].ID]
}
}
return channels, nil

View File

@ -45,6 +45,16 @@ func (r *podcastEpisodeRepository) GetByChannel(channelID string) (model.Podcast
return eps, err
}
func (r *podcastEpisodeRepository) GetByChannels(channelIDs []string) (model.PodcastEpisodes, error) {
if len(channelIDs) == 0 {
return nil, nil
}
sel := r.newSelect().Columns("*").Where(Eq{"channel_id": channelIDs}).OrderBy("channel_id, publish_date DESC")
var eps model.PodcastEpisodes
err := r.queryAll(sel, &eps)
return eps, err
}
func (r *podcastEpisodeRepository) GetByGUID(channelID, guid string) (*model.PodcastEpisode, error) {
sel := r.newSelect().Columns("*").Where(Eq{"channel_id": channelID, "guid": guid})
res := model.PodcastEpisode{}

View File

@ -45,6 +45,23 @@ func (m *MockPodcastEpisodeRepo) GetNewest(count int) (model.PodcastEpisodes, er
return all, nil
}
func (m *MockPodcastEpisodeRepo) GetByChannels(channelIDs []string) (model.PodcastEpisodes, error) {
if m.Err {
return nil, errors.New("error")
}
ids := make(map[string]bool, len(channelIDs))
for _, id := range channelIDs {
ids[id] = true
}
var result model.PodcastEpisodes
for _, ep := range m.Data {
if ids[ep.ChannelID] {
result = append(result, *ep)
}
}
return result, nil
}
func (m *MockPodcastEpisodeRepo) GetByChannel(channelID string) (model.PodcastEpisodes, error) {
if m.Err {
return nil, errors.New("error")