From 40d29f2cb14a454650938adbe3da45590d6eb51d Mon Sep 17 00:00:00 2001 From: Kendall Garner <17521368+kgarner7@users.noreply.github.com> Date: Sat, 9 Dec 2023 21:58:50 -0800 Subject: [PATCH] add migration, more tests --- .../20231209211223_alter_lyric_column.go | 77 +++++++++++++++++++ model/lyrics.go | 35 +++++---- model/lyrics_test.go | 38 +++++++++ ...yricsBySongId with data should match .JSON | 37 +++++++++ ...LyricsBySongId with data should match .XML | 20 +++++ ...csBySongId without data should match .JSON | 8 ++ ...icsBySongId without data should match .XML | 3 + server/subsonic/responses/responses_test.go | 59 ++++++++++++++ 8 files changed, 261 insertions(+), 16 deletions(-) create mode 100644 db/migration/20231209211223_alter_lyric_column.go create mode 100644 model/lyrics_test.go create mode 100644 server/subsonic/responses/.snapshots/Responses getLyricsBySongId with data should match .JSON create mode 100644 server/subsonic/responses/.snapshots/Responses getLyricsBySongId with data should match .XML create mode 100644 server/subsonic/responses/.snapshots/Responses getLyricsBySongId without data should match .JSON create mode 100644 server/subsonic/responses/.snapshots/Responses getLyricsBySongId without data should match .XML diff --git a/db/migration/20231209211223_alter_lyric_column.go b/db/migration/20231209211223_alter_lyric_column.go new file mode 100644 index 000000000..c1d70012e --- /dev/null +++ b/db/migration/20231209211223_alter_lyric_column.go @@ -0,0 +1,77 @@ +package migrations + +import ( + "context" + "database/sql" + "encoding/json" + + "github.com/navidrome/navidrome/model" + "github.com/pressly/goose/v3" +) + +func init() { + goose.AddMigrationContext(upAlterLyricColumn, downAlterLyricColumn) +} + +func upAlterLyricColumn(ctx context.Context, tx *sql.Tx) error { + _, err := tx.ExecContext(ctx, `alter table media_file rename COLUMN lyrics TO lyrics_old`) + if err != nil { + return err + } + + _, err = tx.ExecContext(ctx, `alter table media_file add lyrics JSONB default '[]';`) + if err != nil { + return err + } + + stmt, err := tx.Prepare(`update media_file SET lyrics = ? where id = ?`) + if err != nil { + return err + } + + rows, err := tx.Query(`select id, lyrics_old FROM media_file WHERE lyrics_old <> '';`) + if err != nil { + return err + } + + var id, lyrics string + for rows.Next() { + err = rows.Scan(&id, &lyrics) + if err != nil { + return err + } + + lyrics, err := model.ToLyrics("xxx", lyrics) + if err != nil { + return err + } + + text, err := json.Marshal(model.Lyrics{*lyrics}) + if err != nil { + return err + } + + _, err = stmt.Exec(string(text[:]), id) + if err != nil { + return err + } + } + + err = rows.Err() + if err != nil { + return err + } + + _, err = tx.ExecContext(ctx, `ALTER TABLE media_file DROP COLUMN lyrics_old;`) + if err != nil { + return err + } + + notice(tx, "A full rescan will be performed to pick up additional lyrics (existing lyrics have been preserved)") + return forceFullRescan(tx) +} + +func downAlterLyricColumn(ctx context.Context, tx *sql.Tx) error { + // This code is executed when the migration is rolled back. + return nil +} diff --git a/model/lyrics.go b/model/lyrics.go index d4379d6ff..c95892682 100644 --- a/model/lyrics.go +++ b/model/lyrics.go @@ -5,6 +5,7 @@ import ( "strconv" "strings" + "github.com/navidrome/navidrome/log" "github.com/navidrome/navidrome/utils" ) @@ -26,7 +27,7 @@ type Lyric struct { const timeRegexString = `(\[(([0-9]{1,2}):)?([0-9]{1,2}):([0-9]{1,2})(\.([0-9]{1,3}))?\])` var ( - lineRegex = regexp.MustCompile(timeRegexString + "([^\n]+)") + lineRegex = regexp.MustCompile(timeRegexString + "([^\n]+)?") lrcIdRegex = regexp.MustCompile(`\[(ar|ti|offset):([^\]]+)\]`) ) @@ -54,18 +55,18 @@ func ToLyrics(language, text string) (*Lyric, error) { if idTag != nil { switch idTag[1] { case "ar": - artist = idTag[2] + artist = utils.SanitizeText(strings.TrimSpace(idTag[2])) case "offset": { - off, err := strconv.ParseInt(idTag[2], 10, 64) + off, err := strconv.ParseInt(strings.TrimSpace(idTag[2]), 10, 64) if err != nil { - return nil, err + log.Warn("Error parsing offset", "offset", idTag[2], "error", err) + } else { + offset = &off } - - offset = &off } case "ti": - title = idTag[2] + title = utils.SanitizeText(strings.TrimSpace(idTag[2])) } continue @@ -74,9 +75,9 @@ func ToLyrics(language, text string) (*Lyric, error) { syncedMatch := lineRegex.FindStringSubmatch(line) if syncedMatch == nil { synced = false - text = line + text = utils.SanitizeText(line) } else { - var hours int64 + var hours, millis int64 var err error if syncedMatch[3] != "" { @@ -96,18 +97,20 @@ func ToLyrics(language, text string) (*Lyric, error) { return nil, err } - millis, err := strconv.ParseInt(syncedMatch[7], 10, 64) - if err != nil { - return nil, err - } + if syncedMatch[7] != "" { + millis, err = strconv.ParseInt(syncedMatch[7], 10, 64) + if err != nil { + return nil, err + } - if len(syncedMatch[7]) == 2 { - millis *= 10 + if len(syncedMatch[7]) == 2 { + millis *= 10 + } } timeInMillis := (((((hours * 60) + min) * 60) + sec) * 1000) + millis time = &timeInMillis - text = syncedMatch[8] + text = utils.SanitizeText(syncedMatch[8]) } } else { text = line diff --git a/model/lyrics_test.go b/model/lyrics_test.go new file mode 100644 index 000000000..93adb1a13 --- /dev/null +++ b/model/lyrics_test.go @@ -0,0 +1,38 @@ +package model_test + +import ( + . "github.com/navidrome/navidrome/model" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("ToLyrics", func() { + num := int64(1551) + + It("should parse tags with spaces", func() { + lyrics, err := ToLyrics("xxx", "[offset: 1551 ]\n[ti: A title ]\n[ar: An artist ]\n[00:00.00]Hi there") + Expect(err).ToNot(HaveOccurred()) + Expect(lyrics.DisplayArtist).To(Equal("An artist")) + Expect(lyrics.DisplayTitle).To(Equal("A title")) + Expect(lyrics.Offset).To(Equal(&num)) + }) + + It("Should ignore bad offset", func() { + lyrics, err := ToLyrics("xxx", "[offset: NotANumber ]\n[00:00.00]Hi there") + Expect(err).ToNot(HaveOccurred()) + Expect(lyrics.Offset).To(BeNil()) + }) + + It("should accept lines with no text and weird times", func() { + var a, b, c, d = int64(0), int64(10040), int64(40000), int64(1000 * 60 * 60) + lyrics, err := ToLyrics("xxx", "[00:00.00]Hi there\n\n\n[00:10.040] \n[00:40]Test\n[01:00:00]late") + Expect(err).ToNot(HaveOccurred()) + Expect(lyrics.Synced).To(BeTrue()) + Expect(lyrics.Line).To(Equal([]Line{ + {Start: &a, Value: "Hi there"}, + {Start: &b, Value: ""}, + {Start: &c, Value: "Test"}, + {Start: &d, Value: "late"}, + })) + }) +}) diff --git a/server/subsonic/responses/.snapshots/Responses getLyricsBySongId with data should match .JSON b/server/subsonic/responses/.snapshots/Responses getLyricsBySongId with data should match .JSON new file mode 100644 index 000000000..a176285a0 --- /dev/null +++ b/server/subsonic/responses/.snapshots/Responses getLyricsBySongId with data should match .JSON @@ -0,0 +1,37 @@ +{ + "status": "ok", + "version": "1.8.0", + "type": "navidrome", + "serverVersion": "v0.0.0", + "openSubsonic": true, + "lyricsList": { + "structuredLyrics": [ + { + "lang": "eng", + "line": [ + { + "start": 18800, + "value": "We're no strangers to love" + }, + { + "start": 22801, + "value": "You know the rules and so do I" + } + ], + "synced": true + }, + { + "lang": "xxx", + "line": [ + { + "value": "We're no strangers to love" + }, + { + "value": "You know the rules and so do I" + } + ], + "synced": false + } + ] + } +} diff --git a/server/subsonic/responses/.snapshots/Responses getLyricsBySongId with data should match .XML b/server/subsonic/responses/.snapshots/Responses getLyricsBySongId with data should match .XML new file mode 100644 index 000000000..76e9b7dce --- /dev/null +++ b/server/subsonic/responses/.snapshots/Responses getLyricsBySongId with data should match .XML @@ -0,0 +1,20 @@ + + + + + We're no strangers to love + + + You know the rules and so do I + + + + + We're no strangers to love + + + You know the rules and so do I + + + + diff --git a/server/subsonic/responses/.snapshots/Responses getLyricsBySongId without data should match .JSON b/server/subsonic/responses/.snapshots/Responses getLyricsBySongId without data should match .JSON new file mode 100644 index 000000000..876cc71ce --- /dev/null +++ b/server/subsonic/responses/.snapshots/Responses getLyricsBySongId without data should match .JSON @@ -0,0 +1,8 @@ +{ + "status": "ok", + "version": "1.8.0", + "type": "navidrome", + "serverVersion": "v0.0.0", + "openSubsonic": true, + "lyricsList": {} +} diff --git a/server/subsonic/responses/.snapshots/Responses getLyricsBySongId without data should match .XML b/server/subsonic/responses/.snapshots/Responses getLyricsBySongId without data should match .XML new file mode 100644 index 000000000..040cf6b9e --- /dev/null +++ b/server/subsonic/responses/.snapshots/Responses getLyricsBySongId without data should match .XML @@ -0,0 +1,3 @@ + + + diff --git a/server/subsonic/responses/responses_test.go b/server/subsonic/responses/responses_test.go index 17ea9ec3d..54e93e38e 100644 --- a/server/subsonic/responses/responses_test.go +++ b/server/subsonic/responses/responses_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/navidrome/navidrome/consts" + "github.com/navidrome/navidrome/model" . "github.com/navidrome/navidrome/server/subsonic/responses" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -795,4 +796,62 @@ var _ = Describe("Responses", func() { }) }) }) + + Describe("getLyricsBySongId", func() { + BeforeEach(func() { + response.LyricsList = &LyricsList{} + }) + + Describe("without data", func() { + It("should match .XML", func() { + Expect(xml.MarshalIndent(response, "", " ")).To(MatchSnapshot()) + }) + It("should match .JSON", func() { + Expect(json.MarshalIndent(response, "", " ")).To(MatchSnapshot()) + }) + }) + + Describe("with data", func() { + BeforeEach(func() { + times := []int64{int64(18800), int64(22801)} + + response.LyricsList.StructuredLyrics = model.Lyrics{ + { + Lang: "eng", + Synced: true, + Line: []model.Line{ + { + Start: ×[0], + Value: "We're no strangers to love", + }, + { + Start: ×[1], + Value: "You know the rules and so do I", + }, + }, + }, + { + Lang: "xxx", + Synced: false, + Line: []model.Line{ + { + Value: "We're no strangers to love", + }, + { + Value: "You know the rules and so do I", + }, + }, + }, + } + }) + + It("should match .XML", func() { + Expect(xml.MarshalIndent(response, "", " ")).To(MatchSnapshot()) + }) + It("should match .JSON", func() { + Expect(json.MarshalIndent(response, "", " ")).To(MatchSnapshot()) + }) + }) + }) + })