perf(user): add indexes on plexId, jellyfinUserId, and resetPasswordGuid#2806
perf(user): add indexes on plexId, jellyfinUserId, and resetPasswordGuid#2806dougrathbone wants to merge 2 commits intoseerr-team:developfrom
Conversation
📝 WalkthroughWalkthroughAdds database indexes for three User lookup columns via entity decorators, PostgreSQL and SQLite migrations, and a test that verifies index creation and a plexId query against the test SQLite DB. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
server/migration/postgres/1775000000000-AddUserLookupIndexes.ts (1)
6-16: ConsiderIF NOT EXISTSfor idempotency (same as SQLite migration).PostgreSQL also supports
IF NOT EXISTSforCREATE INDEX, which would make the migration idempotent in development environments wheresynchronizemay have already created these indexes.♻️ Proposed fix for idempotency
public async up(queryRunner: QueryRunner): Promise<void> { await queryRunner.query( - `CREATE INDEX "IDX_user_plexId" ON "user" ("plexId")` + `CREATE INDEX IF NOT EXISTS "IDX_user_plexId" ON "user" ("plexId")` ); await queryRunner.query( - `CREATE INDEX "IDX_user_jellyfinUserId" ON "user" ("jellyfinUserId")` + `CREATE INDEX IF NOT EXISTS "IDX_user_jellyfinUserId" ON "user" ("jellyfinUserId")` ); await queryRunner.query( - `CREATE INDEX "IDX_user_resetPasswordGuid" ON "user" ("resetPasswordGuid")` + `CREATE INDEX IF NOT EXISTS "IDX_user_resetPasswordGuid" ON "user" ("resetPasswordGuid")` ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/migration/postgres/1775000000000-AddUserLookupIndexes.ts` around lines 6 - 16, The CREATE INDEX statements in the migration's up(queryRunner: QueryRunner) are not idempotent; change each queryRunner.query call that creates "IDX_user_plexId", "IDX_user_jellyfinUserId", and "IDX_user_resetPasswordGuid" to use CREATE INDEX IF NOT EXISTS so the migration can be safely re-run in environments where synchronize already created those indexes; keep the same index names and SQL calls but add IF NOT EXISTS to each CREATE INDEX string.server/migration/sqlite/1775000000000-AddUserLookupIndexes.ts (2)
6-16: ConsiderIF NOT EXISTSfor idempotency in development environments.The migration may fail in development if
synchronize: truehas already created these indexes from the entity decorators. AddingIF NOT EXISTSwould make the migration idempotent.♻️ Proposed fix for idempotency
public async up(queryRunner: QueryRunner): Promise<void> { await queryRunner.query( - `CREATE INDEX "IDX_user_plexId" ON "user" ("plexId")` + `CREATE INDEX IF NOT EXISTS "IDX_user_plexId" ON "user" ("plexId")` ); await queryRunner.query( - `CREATE INDEX "IDX_user_jellyfinUserId" ON "user" ("jellyfinUserId")` + `CREATE INDEX IF NOT EXISTS "IDX_user_jellyfinUserId" ON "user" ("jellyfinUserId")` ); await queryRunner.query( - `CREATE INDEX "IDX_user_resetPasswordGuid" ON "user" ("resetPasswordGuid")` + `CREATE INDEX IF NOT EXISTS "IDX_user_resetPasswordGuid" ON "user" ("resetPasswordGuid")` ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/migration/sqlite/1775000000000-AddUserLookupIndexes.ts` around lines 6 - 16, The CREATE INDEX statements in the migration's up method (inside the migration class implementing up(queryRunner: QueryRunner)) should be made idempotent by adding IF NOT EXISTS to each SQL statement; update the three queries for IDX_user_plexId, IDX_user_jellyfinUserId, and IDX_user_resetPasswordGuid to use `CREATE INDEX IF NOT EXISTS ...` so the migration won't fail when indexes already exist (e.g., when synchronize: true created them).
18-22: Optional: AddIF EXISTStodown()for defensive rollback.This is less critical than
up(), but addingIF EXISTSprevents errors if the migration is rolled back after a partial failure.♻️ Optional defensive fix
public async down(queryRunner: QueryRunner): Promise<void> { - await queryRunner.query(`DROP INDEX "IDX_user_resetPasswordGuid"`); - await queryRunner.query(`DROP INDEX "IDX_user_jellyfinUserId"`); - await queryRunner.query(`DROP INDEX "IDX_user_plexId"`); + await queryRunner.query(`DROP INDEX IF EXISTS "IDX_user_resetPasswordGuid"`); + await queryRunner.query(`DROP INDEX IF EXISTS "IDX_user_jellyfinUserId"`); + await queryRunner.query(`DROP INDEX IF EXISTS "IDX_user_plexId"`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/migration/sqlite/1775000000000-AddUserLookupIndexes.ts` around lines 18 - 22, Update the down(queryRunner: QueryRunner) method in the AddUserLookupIndexes migration to use defensive DROP INDEX statements with IF EXISTS; replace the three DROP INDEX "IDX_user_resetPasswordGuid", "IDX_user_jellyfinUserId", and "IDX_user_plexId" calls so they execute DROP INDEX IF EXISTS "IDX_user_resetPasswordGuid", DROP INDEX IF EXISTS "IDX_user_jellyfinUserId", and DROP INDEX IF EXISTS "IDX_user_plexId" respectively to avoid errors on partial rollbacks.server/entity/User.index.test.ts (1)
49-52: Consider usingassert.notStrictEqualfor cleaner type narrowing.The current pattern works but the
!assertion afterassert.ok(found !== null)is slightly redundant. Usingassert.notStrictEqualor assigning the assertion inline would be cleaner.♻️ Optional refinement
const found = await repo.findOne({ where: { plexId: 99999 } }); - assert.ok(found !== null); - assert.strictEqual(found!.email, 'plextest@seerr.dev'); - assert.strictEqual(found!.plexId, 99999); + assert.notStrictEqual(found, null); + assert.strictEqual(found?.email, 'plextest@seerr.dev'); + assert.strictEqual(found?.plexId, 99999);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/entity/User.index.test.ts` around lines 49 - 52, Replace the null-check pattern on the retrieved user to use assert.notStrictEqual for better type narrowing: where the test calls repo.findOne(...) and then uses assert.ok(found !== null) followed by non-null assertions on found (variable found), change the assertion to assert.notStrictEqual(found, null) (or assert.notStrictEqual(found, null, 'expected user to exist')) and then remove the unnecessary non-null (!) operator when accessing found.email and found.plexId so the TypeScript compiler correctly infers found is non-null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/migration/postgres/1775000000000-AddUserLookupIndexes.ts`:
- Around line 6-22: The up() migration creates indexes without schema
qualification while down() drops them using "public" qualification, which can
fail on non-default schemas; update either up() or down() to be
consistent—preferably remove the "public" prefix from the DROP INDEX calls in
down() (referencing down() and the DROP INDEX statements for
"IDX_user_resetPasswordGuid", "IDX_user_jellyfinUserId", and "IDX_user_plexId")
or alternatively add "public" to the CREATE INDEX calls in up() so both up() and
down() use the same schema resolution when invoking queryRunner.query().
---
Nitpick comments:
In `@server/entity/User.index.test.ts`:
- Around line 49-52: Replace the null-check pattern on the retrieved user to use
assert.notStrictEqual for better type narrowing: where the test calls
repo.findOne(...) and then uses assert.ok(found !== null) followed by non-null
assertions on found (variable found), change the assertion to
assert.notStrictEqual(found, null) (or assert.notStrictEqual(found, null,
'expected user to exist')) and then remove the unnecessary non-null (!) operator
when accessing found.email and found.plexId so the TypeScript compiler correctly
infers found is non-null.
In `@server/migration/postgres/1775000000000-AddUserLookupIndexes.ts`:
- Around line 6-16: The CREATE INDEX statements in the migration's
up(queryRunner: QueryRunner) are not idempotent; change each queryRunner.query
call that creates "IDX_user_plexId", "IDX_user_jellyfinUserId", and
"IDX_user_resetPasswordGuid" to use CREATE INDEX IF NOT EXISTS so the migration
can be safely re-run in environments where synchronize already created those
indexes; keep the same index names and SQL calls but add IF NOT EXISTS to each
CREATE INDEX string.
In `@server/migration/sqlite/1775000000000-AddUserLookupIndexes.ts`:
- Around line 6-16: The CREATE INDEX statements in the migration's up method
(inside the migration class implementing up(queryRunner: QueryRunner)) should be
made idempotent by adding IF NOT EXISTS to each SQL statement; update the three
queries for IDX_user_plexId, IDX_user_jellyfinUserId, and
IDX_user_resetPasswordGuid to use `CREATE INDEX IF NOT EXISTS ...` so the
migration won't fail when indexes already exist (e.g., when synchronize: true
created them).
- Around line 18-22: Update the down(queryRunner: QueryRunner) method in the
AddUserLookupIndexes migration to use defensive DROP INDEX statements with IF
EXISTS; replace the three DROP INDEX "IDX_user_resetPasswordGuid",
"IDX_user_jellyfinUserId", and "IDX_user_plexId" calls so they execute DROP
INDEX IF EXISTS "IDX_user_resetPasswordGuid", DROP INDEX IF EXISTS
"IDX_user_jellyfinUserId", and DROP INDEX IF EXISTS "IDX_user_plexId"
respectively to avoid errors on partial rollbacks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4fa613c7-e9e9-4003-8072-f315583a276c
📒 Files selected for processing (4)
server/entity/User.index.test.tsserver/entity/User.tsserver/migration/postgres/1775000000000-AddUserLookupIndexes.tsserver/migration/sqlite/1775000000000-AddUserLookupIndexes.ts
There was a problem hiding this comment.
Per our contributing guide, we expect AI-assisted development, not AI-driven development. This PR (along with #2804 and #2805) was submitted in rapid succession, and the quality issues across them suggest heavy AI generation with insufficient review.
Things I noticed in this PR:
-
Migrations appear to be AI-written and not generated. The sqlite and postgres files are byte-for-byte identical. This never happens with
typeorm migration:generate. Please regenerate following the instructions inCONTRIBUTING.md. -
Irrelevant test files written. You're testing that TypeORM applied an
@Indexdecorator, which is TypeORM's job, not ours/applications. The tests are also sqlite-only and would break on postgres. This is also another red flag that led us to flag this PR. A human reviewer would have caught that these serve no purpose. -
Why is an index being added on
resetPasswordGuid? This is only hit during password resets (which doesn't happen always) and adds write overhead for negligible benefit, unlikeplexId/jellyfinUserIdwhich are used on every auth lookup.
CC: @seerr-team/seerr-core
2 participants
Description
User.plexId,User.jellyfinUserId, andUser.resetPasswordGuidare looked up by value on every authentication request but have no database index. Every lookup is a full table scan.While not a huge win on small instances, this seemed like a no-brainer quick win that might also help in situations where attackers try to brown out the host through brute force logins.
Query sites affected:
plexIdauth.ts), Tautulli watch stats, user import syncjellyfinUserIdauth.ts), avatar proxy (avatarproxy.ts), user syncresetPasswordGuidauth.ts)emailis intentionally excluded -- itsunique: trueconstraint already creates an implicit index in both SQLite and PostgreSQL.Changes:
@Index()decorators to the three columns inserver/entity/User.ts1775000000000-AddUserLookupIndexes1775000000000-AddUserLookupIndexesPerformance improvement:
AI Disclosure: I used Claude Code to help write the tests and validate the SQLite db changes. I reviewed and understood all generated code before submitting.
How Has This Been Tested?
New test suite
server/entity/User.index.test.tsusing the real SQLite test database viasetupTestDb().Tests:
user table has an index on plexId- queriessqlite_masterand assertsIDX_user_plexIdexistsuser table has an index on jellyfinUserId- same forIDX_user_jellyfinUserIduser table has an index on resetPasswordGuid- same forIDX_user_resetPasswordGuidplexId lookup returns correct user- creates a user with a knownplexId, queries by that value, asserts the correct record is returnedTo run:
To verify manually on a running instance:
SQLite:
PostgreSQL:
Both should show
IDX_user_plexId,IDX_user_jellyfinUserId, andIDX_user_resetPasswordGuidafter migration.Screenshots / Logs (if applicable)
N/A -- backend-only change with no UI impact.
Checklist:
pnpm buildpnpm i18n:extract(no new UI strings added)