mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-18 07:30:29 +00:00
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:
parent
d2629094d0
commit
3a0eec08b9
@ -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)
|
||||
}
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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
|
||||
|
||||
@ -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{}
|
||||
|
||||
@ -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")
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user