From 318bb4944fce293043bb8ab16df87e9cfb0ef747 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Deluan=20Quint=C3=A3o?= Date: Wed, 1 Jul 2026 13:12:29 -0400 Subject: [PATCH] perf(db): skip library filter when a non-admin sees all libraries (#5696) * perf(db): skip library filter when a non-admin sees all libraries applyLibraryFilter already short-circuits for admins and headless contexts, but a non-admin who has been granted every library still paid for the correlated user_library subquery, which filters out nothing yet is the slow non-admin list/count path. Reuse the visibility check search3 already had: skip the subquery when the user's granted libraries cover the whole library table. The two helpers (userSeesAllLibraries/visibleLibraryIDs) are promoted from artist_repository to the base sqlRepository so all ~13 call sites benefit and search3 shares the single implementation. The skip is strictly gated on granted count >= total library count, never on an empty/unknown library set, so access control is unchanged for restricted users. * refactor(db): make all-libraries skip fail closed; test cleanups - userSeesAllLibraries: require len(visible) == total (not >=) so the filter skip can never over-grant if the visible set is ever inflated. - tests: assert ToSql() returns no error; restore r.db in AfterEach to avoid leaking mutated state to other specs. Signed-off-by: Deluan --------- Signed-off-by: Deluan --- persistence/artist_repository.go | 26 --------- persistence/sql_base_repository.go | 32 +++++++++++ persistence/sql_base_repository_test.go | 70 ++++++++++++++++++++++--- 3 files changed, 95 insertions(+), 33 deletions(-) diff --git a/persistence/artist_repository.go b/persistence/artist_repository.go index 2c6b77054..5152e774f 100644 --- a/persistence/artist_repository.go +++ b/persistence/artist_repository.go @@ -668,32 +668,6 @@ func isLibraryIDFilter(filter Sqlizer) bool { return ok } -// userSeesAllLibraries reports whether the visible set already covers every library, so a search -// needs no library filter at all. -func (r *artistRepository) userSeesAllLibraries(visible []int) bool { - user := loggedUser(r.ctx) - if user.IsAdmin || user.ID == invalidUserId { - return true // visible is the whole library table - } - total, err := NewLibraryRepository(r.ctx, r.db).CountAll() - if err != nil || total == 0 { - return false - } - return int64(len(visible)) >= total -} - -// visibleLibraryIDs returns the libraries the current user can see: all libraries for admin and -// headless processes, otherwise the user's granted libraries. -func (r *artistRepository) visibleLibraryIDs() ([]int, error) { - user := loggedUser(r.ctx) - if user.IsAdmin || user.ID == invalidUserId { - var ids []int - err := r.queryAllSlice(Select("id").From("library"), &ids) - return ids, err - } - return slice.Map(user.Libraries, func(lib model.Library) int { return lib.ID }), nil -} - func (r *artistRepository) Count(options ...rest.QueryOptions) (int64, error) { return r.CountAll(r.parseRestOptions(r.ctx, options...)) } diff --git a/persistence/sql_base_repository.go b/persistence/sql_base_repository.go index 321e790db..ce5221d19 100644 --- a/persistence/sql_base_repository.go +++ b/persistence/sql_base_repository.go @@ -229,6 +229,12 @@ func (r sqlRepository) applyLibraryFilter(sq SelectBuilder, tableName ...string) return sq } + // A non-admin granted every library sees everything the subquery would return, so applying it is + // pure overhead. Skip it in that case (same fast path admins get). + if visible, err := r.visibleLibraryIDs(); err == nil && r.userSeesAllLibraries(visible) { + return sq + } + table := r.tableName if len(tableName) > 0 { table = tableName[0] @@ -240,6 +246,32 @@ func (r sqlRepository) applyLibraryFilter(sq SelectBuilder, tableName ...string) "SELECT ul.library_id FROM user_library ul WHERE ul.user_id = ?)", user.ID)) } +// userSeesAllLibraries reports whether the visible set already covers every library, so a +// library filter would exclude nothing. +func (r sqlRepository) userSeesAllLibraries(visible []int) bool { + user := loggedUser(r.ctx) + if user.IsAdmin || user.ID == invalidUserId { + return true // visible is the whole library table + } + total, err := NewLibraryRepository(r.ctx, r.db).CountAll() + if err != nil || total == 0 { + return false + } + return int64(len(visible)) == total +} + +// visibleLibraryIDs returns the libraries the current user can see: all libraries for admin and +// headless processes, otherwise the user's granted libraries. +func (r sqlRepository) visibleLibraryIDs() ([]int, error) { + user := loggedUser(r.ctx) + if user.IsAdmin || user.ID == invalidUserId { + var ids []int + err := r.queryAllSlice(Select("id").From("library"), &ids) + return ids, err + } + return slice.Map(user.Libraries, func(lib model.Library) int { return lib.ID }), nil +} + func (r sqlRepository) seedKey() string { // Seed keys must be all lowercase, or else SQLite3 will encode it, making it not match the seed // used in the query. Hashing the user ID and converting it to a hex string will do the trick diff --git a/persistence/sql_base_repository_test.go b/persistence/sql_base_repository_test.go index b46e2066b..2cd6ab8a3 100644 --- a/persistence/sql_base_repository_test.go +++ b/persistence/sql_base_repository_test.go @@ -226,9 +226,21 @@ var _ = Describe("sqlRepository", func() { Describe("applyLibraryFilter", func() { var sq squirrel.SelectBuilder + var savedDB = r.db BeforeEach(func() { sq = squirrel.Select("*").From("test_table") + // Add library 2 so a user granted only library 1 is a genuine strict subset. + savedDB = r.db + r.db = GetDBXBuilder() + _, err := r.db.NewQuery("INSERT OR IGNORE INTO library (id, name, path) VALUES (2, 'Lib 2', '/lib2')").Execute() + Expect(err).ToNot(HaveOccurred()) + }) + + AfterEach(func() { + _, err := r.db.NewQuery("DELETE FROM library WHERE id = 2").Execute() + Expect(err).ToNot(HaveOccurred()) + r.db = savedDB }) Context("Admin User", func() { @@ -238,31 +250,73 @@ var _ = Describe("sqlRepository", func() { It("should not apply library filter for admin users", func() { result := r.applyLibraryFilter(sq) - sql, _, _ := result.ToSql() + sql, _, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) Expect(sql).To(Equal("SELECT * FROM test_table")) }) }) - Context("Regular User", func() { + Context("Regular User with a subset of libraries", func() { BeforeEach(func() { - r.ctx = request.WithUser(context.Background(), model.User{ID: "user123", IsAdmin: false}) + // Strict subset: granted lib 1, DB has libs 1 and 2, so the filter must apply. + r.ctx = request.WithUser(context.Background(), model.User{ + ID: "user123", IsAdmin: false, Libraries: model.Libraries{{ID: 1}}, + }) }) It("should apply library filter for regular users", func() { result := r.applyLibraryFilter(sq) - sql, args, _ := result.ToSql() + sql, args, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) Expect(sql).To(ContainSubstring("IN (SELECT ul.library_id FROM user_library ul WHERE ul.user_id = ?)")) Expect(args).To(ContainElement("user123")) }) It("should use custom table name when provided", func() { result := r.applyLibraryFilter(sq, "custom_table") - sql, args, _ := result.ToSql() + sql, args, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) Expect(sql).To(ContainSubstring("custom_table.library_id IN")) Expect(args).To(ContainElement("user123")) }) }) + Context("Regular User with no libraries", func() { + BeforeEach(func() { + r.ctx = request.WithUser(context.Background(), model.User{ID: "empty", IsAdmin: false}) + }) + + It("should apply the library filter (never skip on empty)", func() { + result := r.applyLibraryFilter(sq) + sql, _, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(ContainSubstring("IN (SELECT ul.library_id FROM user_library ul WHERE ul.user_id = ?)")) + }) + }) + + Context("Regular User who can see all libraries", func() { + BeforeEach(func() { + // Granted every library in the DB, so the filter would exclude nothing. + r.ctx = request.WithUser(context.Background(), model.User{ + ID: "alllibs", IsAdmin: false, Libraries: model.Libraries{{ID: 1}, {ID: 2}}, + }) + }) + + It("should not apply the library filter (subquery would filter nothing)", func() { + result := r.applyLibraryFilter(sq) + sql, _, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(Equal("SELECT * FROM test_table")) + }) + + It("should not apply the filter even with a custom table name", func() { + result := r.applyLibraryFilter(sq, "custom_table") + sql, _, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) + Expect(sql).To(Equal("SELECT * FROM test_table")) + }) + }) + Context("Headless Process (No User Context)", func() { BeforeEach(func() { r.ctx = context.Background() // No user context @@ -270,13 +324,15 @@ var _ = Describe("sqlRepository", func() { It("should not apply library filter for headless processes", func() { result := r.applyLibraryFilter(sq) - sql, _, _ := result.ToSql() + sql, _, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) Expect(sql).To(Equal("SELECT * FROM test_table")) }) It("should not apply library filter even with custom table name", func() { result := r.applyLibraryFilter(sq, "custom_table") - sql, _, _ := result.ToSql() + sql, _, err := result.ToSql() + Expect(err).ToNot(HaveOccurred()) Expect(sql).To(Equal("SELECT * FROM test_table")) }) })