perf(user): add indexes on plexId, jellyfinUserId, and resetPasswordGuid#2806
perf(user): add indexes on plexId, jellyfinUserId, and resetPasswordGuid#2806dougrathbone wants to merge 1 commit intoseerr-team:developfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDatabase indexes are added to the User entity's Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 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
|
@fallenbagel fair feedback - makes sense re:test scope. I wasn't aware of the typeorm migration:generate workflow. I used Claude to help with the migration because I wanted precision on the migration to save time on round tripping the db during QA, but I wasn't aware of the migration guidance. On resetPasswordGuid - I take your point, but added the index because its used on password recovery as a lookup for the reset link. Re the timing of the PRs - I was working on the branches locally before submitting, so the close submit times are just me pushing a batch of work at once rather than these being rapid-fire generated. I want to be clear that Im investigating the codebase by hand with Claude as an assistant, not the other way around. I'm putting in spare hours working through the code as a relative newcomer, and I appreciate you taking the time to review and point me in the right direction. In my defence, i'm not simply yoloing random PRs at you guys using AI. I'll rework the above based on your feedback. |
dougrathbone
force-pushed
the
dougrathbone/perf/user-auth-indexes
branch
from
7c6e40f to
a69fd91
Compare
|
Thanks for the detailed feedback, all good points.
I wasn't aware of the |
Re-iterating my point about this. Password reset link is not something you hit always so this does not bring much value and instead just adds write overhead. As for the changes, I'll re-review later |
Please follow the exact steps in the contribution guide, including the Postgres migration. Do not modify generated TypeORM migrations, as that can cause issues later. We’ll review the result.
Reading and adhering to the contribution guide in full is expected of every external contributor. It covers everything required for submissions (setup, workflow, contribution standards, etc.), including the migration process and the one-line Docker command for Postgres. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/migration/postgres/1776157803546-AddUserAuthIndexes.ts (1)
6-17:⚠️ Potential issue | 🟡 MinorMake schema resolution consistent between
up()anddown().
up()uses unqualified index creation, whiledown()hard-codes"public". That can make rollback fail whensearch_path/schema differs.Suggested fix (match `down()` to `up()` behavior)
public async down(queryRunner: QueryRunner): Promise<void> { - await queryRunner.query(`DROP INDEX "public"."IDX_user_jellyfinUserId"`); - await queryRunner.query(`DROP INDEX "public"."IDX_user_plexId"`); + await queryRunner.query(`DROP INDEX "IDX_user_jellyfinUserId"`); + await queryRunner.query(`DROP INDEX "IDX_user_plexId"`); }#!/bin/bash # Verify schema-qualification mismatch in this migration file. rg -nP 'CREATE INDEX|DROP INDEX' server/migration/postgres/1776157803546-AddUserAuthIndexes.ts🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/migration/postgres/1776157803546-AddUserAuthIndexes.ts` around lines 6 - 17, The down() method in the migration mismatches up(): up() creates unqualified indexes ("IDX_user_plexId", "IDX_user_jellyfinUserId") but down() drops "public"."IDX_..." which can fail if search_path differs; update the down() implementation in class/method AddUserAuthIndexes.up/down (methods up and down) to drop the same unqualified index names (remove the hard-coded "public" schema) or alternatively qualify both CREATE INDEX calls — keep create/drop behavior consistent so the DROP INDEX statements use the same identifier format as the CREATE INDEX statements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server/migration/postgres/1776157803546-AddUserAuthIndexes.ts`:
- Around line 6-17: The down() method in the migration mismatches up(): up()
creates unqualified indexes ("IDX_user_plexId", "IDX_user_jellyfinUserId") but
down() drops "public"."IDX_..." which can fail if search_path differs; update
the down() implementation in class/method AddUserAuthIndexes.up/down (methods up
and down) to drop the same unqualified index names (remove the hard-coded
"public" schema) or alternatively qualify both CREATE INDEX calls — keep
create/drop behavior consistent so the DROP INDEX statements use the same
identifier format as the CREATE INDEX statements.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8fbc86d5-7c0c-4ebd-b904-aaf0154fd7a0
📒 Files selected for processing (3)
server/entity/User.tsserver/migration/postgres/1776157803546-AddUserAuthIndexes.tsserver/migration/sqlite/1776157803546-AddUserAuthIndexes.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/entity/User.ts
These columns are queried on every authentication lookup. Adding indexes avoids full table scans during Plex and Jellyfin sign-in. Migrations generated with `pnpm migration:generate` for both SQLite and PostgreSQL per CONTRIBUTING.md.
dougrathbone
force-pushed
the
dougrathbone/perf/user-auth-indexes
branch
from
a69fd91 to
2d802ad
Compare
|
@fallenbagel Updated the branch to address all feedback:
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/migration/postgres/1776334553142-AddUserAuthIndexes.ts (1)
6-18:⚠️ Potential issue | 🟡 MinorMake
up()/down()schema resolution consistent.
up()relies onsearch_path, butdown()hardcodes"public"(Line 16-17). This can break rollback outside the default schema.Suggested fix
public async down(queryRunner: QueryRunner): Promise<void> { - await queryRunner.query(`DROP INDEX "public"."IDX_user_jellyfinUserId"`); - await queryRunner.query(`DROP INDEX "public"."IDX_user_plexId"`); + await queryRunner.query(`DROP INDEX "IDX_user_jellyfinUserId"`); + await queryRunner.query(`DROP INDEX "IDX_user_plexId"`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/migration/postgres/1776334553142-AddUserAuthIndexes.ts` around lines 6 - 18, The down() currently hardcodes the "public" schema while up() creates indexes relying on the current search_path; update down() in AddUserAuthIndexes (methods up/down) to mirror up() by dropping the same index names without the "public" schema qualifier (or construct the schema dynamically from the connection) so rollback works when using a non-default schema; change the two queryRunner.query calls in down() that DROP INDEX "public"."IDX_user_jellyfinUserId" and DROP INDEX "public"."IDX_user_plexId" to drop "IDX_user_jellyfinUserId" and "IDX_user_plexId" (or resolve schema consistently).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@server/migration/postgres/1776334553142-AddUserAuthIndexes.ts`:
- Around line 6-18: The down() currently hardcodes the "public" schema while
up() creates indexes relying on the current search_path; update down() in
AddUserAuthIndexes (methods up/down) to mirror up() by dropping the same index
names without the "public" schema qualifier (or construct the schema dynamically
from the connection) so rollback works when using a non-default schema; change
the two queryRunner.query calls in down() that DROP INDEX
"public"."IDX_user_jellyfinUserId" and DROP INDEX "public"."IDX_user_plexId" to
drop "IDX_user_jellyfinUserId" and "IDX_user_plexId" (or resolve schema
consistently).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eec61302-4dae-47c4-93f9-938b638bf64a
📒 Files selected for processing (3)
server/entity/User.tsserver/migration/postgres/1776334553142-AddUserAuthIndexes.tsserver/migration/sqlite/1776230665383-AddUserAuthIndexes.ts
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)Summary by CodeRabbit