diff --git a/internal/userdb/collections.go b/internal/userdb/collections.go index 91fb8cf3..cb31a8ff 100644 --- a/internal/userdb/collections.go +++ b/internal/userdb/collections.go @@ -114,13 +114,53 @@ func ListCollections(db *sql.DB, profileID string) ([]Collection, error) { if err := rows.Scan(&c.ID, &c.ProfileID, &c.CreatorProfileID, &c.Name, &c.CollectionType, &c.IsShared, &c.QueryDefinition, &c.SortConfig, &c.CreatedAt, &c.UpdatedAt); err != nil { return nil, err } - c.AllowedProfileIDs, err = listCollectionProfiles(db, c.ID) - if err != nil { - return nil, err - } collections = append(collections, c) } - return collections, rows.Err() + if err := rows.Err(); err != nil { + return nil, err + } + if err := attachCollectionProfiles(db, profileID, collections); err != nil { + return nil, err + } + return collections, nil +} + +// attachCollectionProfiles fills AllowedProfileIDs for every collection with a +// single batched query, avoiding the N+1 round trip a per-collection lookup +// would create. +func attachCollectionProfiles(db *sql.DB, profileID string, collections []Collection) error { + if len(collections) == 0 { + return nil + } + rows, err := db.Query( + `SELECT allowed.collection_id, allowed.profile_id + FROM personal_collection_profiles AS visible + JOIN personal_collection_profiles AS allowed + ON allowed.collection_id = visible.collection_id + WHERE visible.profile_id = ? + ORDER BY allowed.profile_id ASC`, + profileID, + ) + if err != nil { + return err + } + defer rows.Close() + + byCollection := make(map[string][]string, len(collections)) + for rows.Next() { + var collectionID, profileID string + if err := rows.Scan(&collectionID, &profileID); err != nil { + return err + } + byCollection[collectionID] = append(byCollection[collectionID], profileID) + } + if err := rows.Err(); err != nil { + return err + } + for i := range collections { + collections[i].AllowedProfileIDs = byCollection[collections[i].ID] + } + return nil } // UpdateCollection renames a collection and updates its updated_at timestamp. diff --git a/internal/userdb/listing_batch_test.go b/internal/userdb/listing_batch_test.go new file mode 100644 index 00000000..2a11ba80 --- /dev/null +++ b/internal/userdb/listing_batch_test.go @@ -0,0 +1,92 @@ +package userdb + +import ( + "database/sql" + "reflect" + "testing" + + "github.com/Silo-Server/silo-server/internal/userstore" +) + +func newListingTestDB(t *testing.T) *sql.DB { + t.Helper() + db, err := sql.Open("sqlite3", ":memory:") + if err != nil { + t.Fatalf("open sqlite: %v", err) + } + db.SetMaxOpenConns(1) + t.Cleanup(func() { _ = db.Close() }) + if err := InitSchema(db); err != nil { + t.Fatalf("InitSchema: %v", err) + } + return db +} + +func TestListProfilesAttachesAllowedLibrariesInOrder(t *testing.T) { + db := newListingTestDB(t) + for _, profile := range []Profile{ + {ID: "p1", Name: "One", AllowedLibraryIDs: []int{5, 2}, CreatedAt: "2026-01-01T00:00:00Z"}, + {ID: "p2", Name: "Two", AllowedLibraryIDs: []int{3}, CreatedAt: "2026-01-02T00:00:00Z"}, + {ID: "p3", Name: "Three", CreatedAt: "2026-01-03T00:00:00Z"}, + } { + if err := CreateProfile(db, profile); err != nil { + t.Fatalf("CreateProfile(%s): %v", profile.ID, err) + } + } + + profiles, err := ListProfiles(db) + if err != nil { + t.Fatalf("ListProfiles: %v", err) + } + if len(profiles) != 3 { + t.Fatalf("ListProfiles returned %d profiles, want 3", len(profiles)) + } + if got, want := profiles[0].AllowedLibraryIDs, []int{2, 5}; !reflect.DeepEqual(got, want) { + t.Fatalf("p1 AllowedLibraryIDs = %v, want %v", got, want) + } + if got, want := profiles[1].AllowedLibraryIDs, []int{3}; !reflect.DeepEqual(got, want) { + t.Fatalf("p2 AllowedLibraryIDs = %v, want %v", got, want) + } + if profiles[2].AllowedLibraryIDs != nil { + t.Fatalf("p3 AllowedLibraryIDs = %v, want nil", profiles[2].AllowedLibraryIDs) + } +} + +func TestListCollectionsAttachesAllowedProfilesInOrder(t *testing.T) { + db := newListingTestDB(t) + collection, err := CreateCollection(db, userstore.CreateCollectionInput{ + CreatorProfileID: "p1", + Name: "Shared", + IsShared: true, + AllowedProfileIDs: []string{"p3", "p2"}, + }) + if err != nil { + t.Fatalf("CreateCollection: %v", err) + } + + collections, err := ListCollections(db, "p2") + if err != nil { + t.Fatalf("ListCollections: %v", err) + } + if len(collections) != 1 || collections[0].ID != collection.ID { + t.Fatalf("ListCollections = %+v, want collection %s", collections, collection.ID) + } + if got, want := collections[0].AllowedProfileIDs, []string{"p1", "p2", "p3"}; !reflect.DeepEqual(got, want) { + t.Fatalf("AllowedProfileIDs = %v, want %v", got, want) + } +} + +func TestBatchAttachDoesNotScaleBindParametersWithRows(t *testing.T) { + db := newListingTestDB(t) + const aboveSQLiteVariableLimit = 32767 + + profiles := make([]Profile, aboveSQLiteVariableLimit) + if err := attachAllowedLibraries(db, profiles); err != nil { + t.Fatalf("attachAllowedLibraries: %v", err) + } + + collections := make([]Collection, aboveSQLiteVariableLimit) + if err := attachCollectionProfiles(db, "viewer", collections); err != nil { + t.Fatalf("attachCollectionProfiles: %v", err) + } +} diff --git a/internal/userdb/profile_libraries.go b/internal/userdb/profile_libraries.go index 6bcd316f..0e897c98 100644 --- a/internal/userdb/profile_libraries.go +++ b/internal/userdb/profile_libraries.go @@ -3,6 +3,8 @@ package userdb import ( "database/sql" "fmt" + + "github.com/Silo-Server/silo-server/internal/userstore" ) func listProfileAllowedLibraries(db *sql.DB, profileID string) ([]int, error) { @@ -29,6 +31,38 @@ func listProfileAllowedLibraries(db *sql.DB, profileID string) ([]int, error) { return libraryIDs, nil } +// attachAllowedLibraries fills AllowedLibraryIDs for every profile with a +// single batched query, avoiding the N+1 round trip a per-profile lookup +// would create. +func attachAllowedLibraries(db *sql.DB, profiles []Profile) error { + if len(profiles) == 0 { + return nil + } + rows, err := db.Query( + `SELECT profile_id, library_id + FROM profile_allowed_libraries + ORDER BY library_id ASC`, + ) + if err != nil { + return fmt.Errorf("listing allowed libraries: %w", err) + } + defer rows.Close() + + var allowedLibraries []userstore.ProfileAllowedLibrary + for rows.Next() { + var allowedLibrary userstore.ProfileAllowedLibrary + if err := rows.Scan(&allowedLibrary.ProfileID, &allowedLibrary.LibraryID); err != nil { + return fmt.Errorf("scanning allowed library: %w", err) + } + allowedLibraries = append(allowedLibraries, allowedLibrary) + } + if err := rows.Err(); err != nil { + return fmt.Errorf("iterating allowed libraries: %w", err) + } + userstore.AttachAllowedLibraries(profiles, allowedLibraries) + return nil +} + func replaceProfileAllowedLibrariesTx(tx *sql.Tx, profileID string, libraryIDs []int) error { if _, err := tx.Exec("DELETE FROM profile_allowed_libraries WHERE profile_id = ?", profileID); err != nil { return fmt.Errorf("clearing allowed libraries for profile %s: %w", profileID, err) diff --git a/internal/userdb/profiles.go b/internal/userdb/profiles.go index 09cf606e..54f80b0f 100644 --- a/internal/userdb/profiles.go +++ b/internal/userdb/profiles.go @@ -128,15 +128,14 @@ func ListProfiles(db *sql.DB) ([]Profile, error) { ); err != nil { return nil, fmt.Errorf("scanning profile row: %w", err) } - p.AllowedLibraryIDs, err = listProfileAllowedLibraries(db, p.ID) - if err != nil { - return nil, err - } profiles = append(profiles, p) } if err := rows.Err(); err != nil { return nil, fmt.Errorf("iterating profile rows: %w", err) } + if err := attachAllowedLibraries(db, profiles); err != nil { + return nil, err + } return profiles, nil } diff --git a/internal/userstore/pgstore/profile_libraries.go b/internal/userstore/pgstore/profile_libraries.go index 534a5b6a..f15d668c 100644 --- a/internal/userstore/pgstore/profile_libraries.go +++ b/internal/userstore/pgstore/profile_libraries.go @@ -6,6 +6,8 @@ import ( "github.com/jackc/pgx/v5" "github.com/jackc/pgx/v5/pgconn" + + "github.com/Silo-Server/silo-server/internal/userstore" ) type profileLibraryQuerier interface { @@ -41,6 +43,45 @@ func listProfileAllowedLibraries(ctx context.Context, q profileLibraryQuerier, u return libraryIDs, nil } +// attachAllowedLibraries fills AllowedLibraryIDs for every profile with a +// single batched query, avoiding the N+1 round trip a per-profile lookup +// would create. +func (s *PostgresUserStore) attachAllowedLibraries(ctx context.Context, profiles []userstore.Profile) error { + if len(profiles) == 0 { + return nil + } + ids := make([]string, len(profiles)) + for i := range profiles { + ids[i] = profiles[i].ID + } + rows, err := s.pool.Query(ctx, + `SELECT profile_id, library_id + FROM user_profile_allowed_libraries + WHERE user_id = $1 AND profile_id = ANY($2) + ORDER BY library_id ASC`, + s.userID, + ids, + ) + if err != nil { + return fmt.Errorf("listing allowed libraries for user %d: %w", s.userID, err) + } + defer rows.Close() + + var allowedLibraries []userstore.ProfileAllowedLibrary + for rows.Next() { + var allowedLibrary userstore.ProfileAllowedLibrary + if err := rows.Scan(&allowedLibrary.ProfileID, &allowedLibrary.LibraryID); err != nil { + return fmt.Errorf("scanning allowed library for user %d: %w", s.userID, err) + } + allowedLibraries = append(allowedLibraries, allowedLibrary) + } + if err := rows.Err(); err != nil { + return fmt.Errorf("iterating allowed libraries for user %d: %w", s.userID, err) + } + userstore.AttachAllowedLibraries(profiles, allowedLibraries) + return nil +} + func replaceProfileAllowedLibraries(ctx context.Context, q profileLibraryQuerier, userID int, profileID string, libraryIDs []int) error { if _, err := q.Exec(ctx, "DELETE FROM user_profile_allowed_libraries WHERE user_id = $1 AND profile_id = $2", diff --git a/internal/userstore/pgstore/profiles.go b/internal/userstore/pgstore/profiles.go index 4131a23a..44e89fe2 100644 --- a/internal/userstore/pgstore/profiles.go +++ b/internal/userstore/pgstore/profiles.go @@ -126,15 +126,14 @@ func (s *PostgresUserStore) ListProfiles(ctx context.Context) ([]userstore.Profi if err != nil { return nil, fmt.Errorf("scanning profile row: %w", err) } - p.AllowedLibraryIDs, err = listProfileAllowedLibraries(ctx, s.pool, s.userID, p.ID) - if err != nil { - return nil, err - } profiles = append(profiles, *p) } if err := rows.Err(); err != nil { return nil, fmt.Errorf("iterating profile rows: %w", err) } + if err := s.attachAllowedLibraries(ctx, profiles); err != nil { + return nil, err + } return profiles, nil } diff --git a/internal/userstore/profile_libraries.go b/internal/userstore/profile_libraries.go new file mode 100644 index 00000000..6e2e4235 --- /dev/null +++ b/internal/userstore/profile_libraries.go @@ -0,0 +1,22 @@ +package userstore + +// ProfileAllowedLibrary associates one profile with one allowed library. +type ProfileAllowedLibrary struct { + ProfileID string + LibraryID int +} + +// AttachAllowedLibraries replaces each profile's AllowedLibraryIDs with the +// matching associations, preserving their input order. +func AttachAllowedLibraries(profiles []Profile, allowedLibraries []ProfileAllowedLibrary) { + byProfile := make(map[string][]int, len(profiles)) + for _, allowedLibrary := range allowedLibraries { + byProfile[allowedLibrary.ProfileID] = append( + byProfile[allowedLibrary.ProfileID], + allowedLibrary.LibraryID, + ) + } + for i := range profiles { + profiles[i].AllowedLibraryIDs = byProfile[profiles[i].ID] + } +} diff --git a/internal/userstore/profile_libraries_test.go b/internal/userstore/profile_libraries_test.go new file mode 100644 index 00000000..3dcce786 --- /dev/null +++ b/internal/userstore/profile_libraries_test.go @@ -0,0 +1,31 @@ +package userstore + +import ( + "reflect" + "testing" +) + +func TestAttachAllowedLibraries(t *testing.T) { + profiles := []Profile{ + {ID: "p1", AllowedLibraryIDs: []int{99}}, + {ID: "p2", AllowedLibraryIDs: []int{99}}, + {ID: "p3", AllowedLibraryIDs: []int{99}}, + } + allowedLibraries := []ProfileAllowedLibrary{ + {ProfileID: "p2", LibraryID: 1}, + {ProfileID: "p1", LibraryID: 2}, + {ProfileID: "p1", LibraryID: 5}, + } + + AttachAllowedLibraries(profiles, allowedLibraries) + + if got, want := profiles[0].AllowedLibraryIDs, []int{2, 5}; !reflect.DeepEqual(got, want) { + t.Fatalf("p1 AllowedLibraryIDs = %v, want %v", got, want) + } + if got, want := profiles[1].AllowedLibraryIDs, []int{1}; !reflect.DeepEqual(got, want) { + t.Fatalf("p2 AllowedLibraryIDs = %v, want %v", got, want) + } + if profiles[2].AllowedLibraryIDs != nil { + t.Fatalf("p3 AllowedLibraryIDs = %v, want nil", profiles[2].AllowedLibraryIDs) + } +}