feat: add accounts indexes (email + password)#3897
feat: add accounts indexes (email + password)#3897slawkens wants to merge 5 commits intoopentibiabr:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a migration (v56) and schema updates: makes Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip Migrating from UI to YAML configuration.Use the |
There was a problem hiding this comment.
Pull request overview
Adds database indexes intended to speed up account lookups by email (and password) in the accounts table.
Changes:
- Adds
accounts_emailandaccounts_passwordindexes toaccountsinschema.sql. - Introduces migration
56.luato create the same indexes on existing databases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| schema.sql | Adds new indexes to the accounts table definition. |
| data-otservbr-global/migrations/56.lua | Adds a migration that creates the new indexes for existing DBs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger.info("Updating database to version 56 (feat: accounts email + password indexes)") | ||
|
|
||
| db.query("CREATE INDEX `accounts_email` ON `accounts` (`email`);") | ||
| db.query("CREATE INDEX `accounts_password` ON `accounts` (`password`);") |
| db.query("CREATE INDEX `accounts_email` ON `accounts` (`email`);") | ||
| db.query("CREATE INDEX `accounts_password` ON `accounts` (`password`);") |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data-otservbr-global/migrations/56.lua`:
- Around line 4-5: The migration currently ignores return values from
db.query("CREATE INDEX `accounts_email`...") and db.query("CREATE INDEX
`accounts_password`..."); update the migration (in the code that runs these
statements) to check each db.query call's boolean result and fail fast by
logging an error and returning false (or otherwise signalling failure from
onUpdateDatabase) when a query returns false so the migration system does not
advance db_version on failed DDL; ensure both db.query calls are wrapped with
this check and a clear error message referencing the failed SQL and preserve
existing logging conventions.
In `@schema.sql`:
- Around line 29-31: The seed in schema.sql sets server_config.db_version to 54
but the schema already includes migration-56 changes (indexes `accounts_email`
and `accounts_password`), causing fresh installs to re-run migration 56 and
error; fix by updating the seed Insert into `server_config` to set `db_version`
to '56' (i.e., update the values inserted for `db_version`) so schema and seed
match, or alternatively modify migration 56 to be idempotent by checking for the
existence of indexes `accounts_email` and `accounts_password` on the `accounts`
table before attempting to create them.
- Line 31: The index declaration INDEX `accounts_password` on the TEXT column
`password` is invalid; update the schema so the indexed column is indexable by
either changing `password` from TEXT to a bounded type (e.g., VARCHAR with an
appropriate length) or by adding a prefix length to the index (e.g., INDEX
`accounts_password` (`password`(N))); locate the `password` column definition
(currently TEXT) and the INDEX `accounts_password` entry and apply one of these
fixes, ensuring the chosen prefix length N or VARCHAR length is sufficient for
your use case and MySQL-compatible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: efd1adfc-1f97-4be7-a392-ae6497779f75
📒 Files selected for processing (2)
data-otservbr-global/migrations/56.luaschema.sql
| db.query("CREATE INDEX `accounts_email` ON `accounts` (`email`);") | ||
| db.query("CREATE INDEX `accounts_password` ON `accounts` (`password`);") |
There was a problem hiding this comment.
Fail fast on DDL errors to avoid false-success migration state.
db.query(...) results are ignored on Line [4]-Line [5]. Per src/database/database.cpp Line [224]-Line [242], failures return false (not exceptions), and src/database/databasemanager.cpp Line [135] still advances db_version after onUpdateDatabase() returns. A failed index creation can therefore still mark version 56 as applied.
Suggested fix
function onUpdateDatabase()
logger.info("Updating database to version 56 (feat: accounts email + password indexes)")
- db.query("CREATE INDEX `accounts_email` ON `accounts` (`email`);")
- db.query("CREATE INDEX `accounts_password` ON `accounts` (`password`);")
+ local ok = db.query("CREATE INDEX `accounts_email` ON `accounts` (`email`);")
+ if not ok then
+ error("Migration 56 failed creating index `accounts_email`.")
+ end
+
+ ok = db.query("CREATE INDEX `accounts_password` ON `accounts` (`password`(191));")
+ if not ok then
+ error("Migration 56 failed creating index `accounts_password`.")
+ end
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@data-otservbr-global/migrations/56.lua` around lines 4 - 5, The migration
currently ignores return values from db.query("CREATE INDEX
`accounts_email`...") and db.query("CREATE INDEX `accounts_password`...");
update the migration (in the code that runs these statements) to check each
db.query call's boolean result and fail fast by logging an error and returning
false (or otherwise signalling failure from onUpdateDatabase) when a query
returns false so the migration system does not advance db_version on failed DDL;
ensure both db.query calls are wrapped with this check and a clear error message
referencing the failed SQL and preserve existing logging conventions.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data-otservbr-global/migrations/56.lua`:
- Around line 12-14: The logger.error call in the migration rollback block is
missing a closing parenthesis which breaks parsing; update the logger.error
invocation (the call that follows db.query("CREATE INDEX `accounts_password` ON
`accounts` (`password`);")) to include the missing closing parenthesis so the
call is syntactically correct and the migration file parses cleanly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f8d10ad-a9aa-4a37-904f-133117cb3569
📒 Files selected for processing (2)
data-otservbr-global/migrations/56.luaschema.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- schema.sql
majestyotbr
changed the title
Description
Added indexes into accounts table - for email and password. This gives 10x performance boost when searching for those columns. I tested on db with 3k accounts.
Type of change
Please delete options that are not relevant.
How Has This Been Tested
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist
Summary by CodeRabbit