fix: improve DB setup script#8156
Open
CarolineDenis wants to merge 41 commits into
Open
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughCentralize env-driven DB credentials, validate DB alias at import, harden sp7_db_setup_check.sh with strict mode and grant parsing/verification, conditionally create/grant users and run migrations, adjust docker/Make wiring, add SP6-permissions detection, and fix iterable consumption in uniqueness rules. ChangesDatabase setup, permissions, and startup validation
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
sp7_db_setup_check.sh (2)
236-250:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing
fifor app user creation block and incorrect error message.The
ifblock starting at line 236 appears to be missing its closingfibefore line 269. Additionally, lines 243-249 contain an error message about "Migrator user" lacking access, but this code path is within the block for app user creation failure, which is confusing.Proposed fix
if mysql -h "$DB_HOST" -P "$DB_PORT" -u "$MASTER_USER_NAME" --password="$MASTER_USER_PASSWORD" \ -e "CREATE USER '${APP_USER_NAME}'@'${APP_USER_HOST}' IDENTIFIED BY '${APP_USER_PASSWORD}';"; then NEW_APP_USER_CREATED=1 else - echo "Notice: Migrator user '${MIGRATOR_NAME}' lacks usable access to '${DB_NAME}'." - echo "Make corrections to the intended MIGRATOR user permissions to resolve." - echo " GRANT ALL PRIVILEGES ON \`${DB_NAME}\`.* TO '${MIGRATOR_NAME}'@'${MIGRATOR_USER_HOST}'; FLUSH PRIVILEGES;" - MIGRATOR_NAME="$MASTER_USER_NAME" - MIGRATOR_PASSWORD="$MASTER_USER_PASSWORD" - MIGRATION_DB_ALIAS="master" + echo "Error: Failed to create app user '${APP_USER_NAME}'." + exit 1 fi fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sp7_db_setup_check.sh` around lines 236 - 250, The if-block that checks USER_EXISTS and APP_USER_NAME (starting at the CREATE USER call) is missing its closing fi and the error branch prints the wrong message about the "Migrator user"; fix by inserting a closing fi after the else branch that handles CREATE USER failure so the app-user block is properly terminated, and update the error text inside that failure branch to reference the app user variables (APP_USER_NAME, APP_USER_HOST, APP_USER_PASSWORD) and appropriate grant guidance (or otherwise move the migrator-related reassignment of MIGRATOR_NAME/MIGRATOR_PASSWORD/MIGRATION_DB_ALIAS out of the app-user block if it belongs to a separate migrator-check) so the message and variable changes align with the CREATE USER context.
256-264:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUndefined variables
SAME_MASTER_AND_APPandSAME_MIGRATOR_AND_APP.These variables are referenced but never defined. Only
SAME_MASTER_AND_MIGRATORis defined at lines 61-64. Withset -uenabled, this will cause the script to abort with "unbound variable" errors.Proposed fix - add variable definitions after line 64
SAME_MASTER_AND_MIGRATOR=false if [[ "$MIGRATOR_NAME" == "$MASTER_USER_NAME" && "$MIGRATOR_PASSWORD" == "$MASTER_USER_PASSWORD" ]]; then SAME_MASTER_AND_MIGRATOR=true fi + +SAME_MASTER_AND_APP=false +if [[ "$APP_USER_NAME" == "$MASTER_USER_NAME" && "$APP_USER_PASSWORD" == "$MASTER_USER_PASSWORD" ]]; then + SAME_MASTER_AND_APP=true +fi + +SAME_MIGRATOR_AND_APP=false +if [[ "$APP_USER_NAME" == "$MIGRATOR_NAME" && "$APP_USER_PASSWORD" == "$MIGRATOR_PASSWORD" ]]; then + SAME_MIGRATOR_AND_APP=true +fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sp7_db_setup_check.sh` around lines 256 - 264, The script references undefined shell variables SAME_MASTER_AND_APP and SAME_MIGRATOR_AND_APP which will trigger "unbound variable" under set -u; define and initialize these variables (e.g., export or local assignments to false by default) in the same section where SAME_MASTER_AND_MIGRATOR is set so they always exist, and set them appropriately when comparing credentials (the SAME_MASTER_AND_APP and SAME_MIGRATOR_AND_APP flags should mirror the boolean logic used for SAME_MASTER_AND_MIGRATOR to indicate when app equals master or migrator).specifyweb/backend/permissions/initialize.py (1)
45-55:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
migrate_sp6_usersis undefined — causesNameErrorat runtime.The parameter was removed from the function signature on line 45, but the condition on line 51 still references it. This matches the pipeline failure. Based on the pattern used in
assign_users_to_roles()(line 99) andassign_users_to_roles_during_testing()(line 177), the condition should use the new helper instead.🐛 Proposed fix
def initialize(wipe: bool=False, apps=apps) -> None: with transaction.atomic(): if wipe: wipe_permissions(apps) create_admins(apps) create_roles(apps) - if migrate_sp6_users: + if has_sp6_permissions_tables(): if 'test' in ''.join(sys.argv): assign_users_to_roles_during_testing(apps) else: assign_users_to_roles(apps)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/backend/permissions/initialize.py` around lines 45 - 55, The initialize function references an undefined variable migrate_sp6_users causing a NameError; replace that conditional with a call to the new helper should_migrate_sp6_users() inside initialize (the block that currently calls wipe_permissions, create_admins, create_roles, and then decides between assign_users_to_roles_during_testing and assign_users_to_roles). Update the if-statement to use should_migrate_sp6_users() so the runtime check matches the pattern used by assign_users_to_roles and assign_users_to_roles_during_testing.
🧹 Nitpick comments (1)
specifyweb/specify/management/commands/base_specify_migration.py (1)
31-35: ⚡ Quick winCatch specific database exceptions instead of bare
Exception.Catching
Exception(Ruff BLE001) is too broad and may mask unexpected errors. Use Django's specific database exceptions for clearer error handling.Proposed fix
+from django.db.utils import OperationalError + ... # Validate the alias exists and is usable; fallback to 'master' if not try: connections[alias].ensure_connection() - except Exception: + except (KeyError, OperationalError) as e: alias = 'master' - logger.warning(f"Falling back to database alias '{alias}'") + logger.warning(f"Falling back to database alias 'master': {e}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/specify/management/commands/base_specify_migration.py` around lines 31 - 35, Replace the broad "except Exception" around connections[alias].ensure_connection() with a specific Django DB exception (e.g. catch django.db.DatabaseError or django.db.utils.OperationalError) and import that exception at top of the module; keep the fallback to alias='master' and logger.warning(...) unchanged so only database-related connection errors are handled while other exceptions still propagate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sp7_db_setup_check.sh`:
- Around line 271-272: The echo log and the actual GRANT differ: the echo at the
mysql invocation shows "ALTER, INDEX" but the mysql -e GRANT command executed in
the if-statement omits those privileges; update one side to match the other —
either add ALTER and INDEX to the executed GRANT in the mysql -e string (the
command inside the if) or remove ALTER and INDEX from the echo message so the
logged command accurately reflects the executed GRANT; ensure you update the
SQL_PRIVILEGE list in the echo and the mysql -e GRANT for consistency with
symbols SQL_APP_USER_NAME, SQL_APP_USER_HOST, SQL_DB_IDENTIFIER,
MASTER_USER_NAME and MASTER_USER_PASSWORD referenced in the surrounding code.
- Around line 184-195: The if-block that checks USER_EXISTS and APP_USER_NAME
(creating the migrator user via mysql) is missing its closing fi; close that
conditional right after the else/exit 1 block so the subsequent if [[
"$NEW_MIGRATOR_USER_CREATED" -eq 1 ]] statement is not nested incorrectly.
Locate the block that contains the CREATE USER command (references: USER_EXISTS,
APP_USER_NAME, MIGRATOR_NAME, MIGRATOR_USER_HOST, MIGRATOR_PASSWORD,
NEW_MIGRATOR_USER_CREATED) and insert a single fi immediately after the existing
else/exit 1 end to properly terminate the first if.
In `@specifyweb/backend/businessrules/uniqueness_rules.py`:
- Around line 392-405: In create_uniqueness_rule, materialize the iterables
before using them: call scopes = list(scopes) (and fields = list(fields) if not
already) before invoking rule_is_global(scopes) and computing final_discipline
so a one-shot iterable isn't consumed; update the order near the start of
create_uniqueness_rule (affecting the rule_is_global and final_discipline logic)
so subsequent use of scopes/fields uses the concrete lists.
---
Outside diff comments:
In `@sp7_db_setup_check.sh`:
- Around line 236-250: The if-block that checks USER_EXISTS and APP_USER_NAME
(starting at the CREATE USER call) is missing its closing fi and the error
branch prints the wrong message about the "Migrator user"; fix by inserting a
closing fi after the else branch that handles CREATE USER failure so the
app-user block is properly terminated, and update the error text inside that
failure branch to reference the app user variables (APP_USER_NAME,
APP_USER_HOST, APP_USER_PASSWORD) and appropriate grant guidance (or otherwise
move the migrator-related reassignment of
MIGRATOR_NAME/MIGRATOR_PASSWORD/MIGRATION_DB_ALIAS out of the app-user block if
it belongs to a separate migrator-check) so the message and variable changes
align with the CREATE USER context.
- Around line 256-264: The script references undefined shell variables
SAME_MASTER_AND_APP and SAME_MIGRATOR_AND_APP which will trigger "unbound
variable" under set -u; define and initialize these variables (e.g., export or
local assignments to false by default) in the same section where
SAME_MASTER_AND_MIGRATOR is set so they always exist, and set them appropriately
when comparing credentials (the SAME_MASTER_AND_APP and SAME_MIGRATOR_AND_APP
flags should mirror the boolean logic used for SAME_MASTER_AND_MIGRATOR to
indicate when app equals master or migrator).
In `@specifyweb/backend/permissions/initialize.py`:
- Around line 45-55: The initialize function references an undefined variable
migrate_sp6_users causing a NameError; replace that conditional with a call to
the new helper should_migrate_sp6_users() inside initialize (the block that
currently calls wipe_permissions, create_admins, create_roles, and then decides
between assign_users_to_roles_during_testing and assign_users_to_roles). Update
the if-statement to use should_migrate_sp6_users() so the runtime check matches
the pattern used by assign_users_to_roles and
assign_users_to_roles_during_testing.
---
Nitpick comments:
In `@specifyweb/specify/management/commands/base_specify_migration.py`:
- Around line 31-35: Replace the broad "except Exception" around
connections[alias].ensure_connection() with a specific Django DB exception (e.g.
catch django.db.DatabaseError or django.db.utils.OperationalError) and import
that exception at top of the module; keep the fallback to alias='master' and
logger.warning(...) unchanged so only database-related connection errors are
handled while other exceptions still propagate.
🪄 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 Plus
Run ID: b15a511d-c8d8-4f42-bdad-001651b4500b
📒 Files selected for processing (25)
Makefiledocker-entrypoint.shsp7_db_setup_check.shspecifyweb/backend/businessrules/migration_utils.pyspecifyweb/backend/businessrules/migrations/0004_catnum_uniquerule.pyspecifyweb/backend/businessrules/migrations/0005_cojo.pyspecifyweb/backend/businessrules/migrations/0006_storage_uniqueIdentifier.pyspecifyweb/backend/businessrules/migrations/0007_more_uniqueness_rules.pyspecifyweb/backend/businessrules/tests/test_component.pyspecifyweb/backend/businessrules/tests/test_uniqueness_rules.pyspecifyweb/backend/businessrules/uniqueness_rules.pyspecifyweb/backend/businessrules/utils.pyspecifyweb/backend/cache/redis/README.mdspecifyweb/backend/cache/redis/rqueue.pyspecifyweb/backend/context/remote_prefs.pyspecifyweb/backend/context/views.pyspecifyweb/backend/permissions/initialize.pyspecifyweb/backend/permissions/permissions.pyspecifyweb/backend/stored_queries/format.pyspecifyweb/backend/workbench/upload/auditlog.pyspecifyweb/settings/__init__.pyspecifyweb/settings/specify_settings.pyspecifyweb/specify/management/commands/base_specify_migration.pyspecifyweb/specify/migration_utils/tectonic_ranks.pyspecifyweb/specify/tests/test_delete_blockers.py
💤 Files with no reviewable changes (1)
- specifyweb/specify/tests/test_delete_blockers.py
github-project-automation
Bot
moved this from 📋Back Log
to Dev Attention Needed
in General Tester Board
Contributor
Author
|
@CodeRabbit full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sp7_db_setup_check.sh (1)
267-281:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApp user creation bypasses prepared escaping while GRANT uses it.
Lines 271-272 create the app user with literal
${APP_USER_NAME},${APP_USER_HOST}, and${APP_USER_PASSWORD}interpolation, but the subsequent GRANT statement at line 303 correctly uses the pre-escapedSQL_APP_USER_*variables. This inconsistency creates an injection risk in the CREATE USER path.🛡️ Proposed fix to use escaped variables consistently
if [[ "$USER_EXISTS" -eq 0 && "$APP_USER_NAME" != "root" ]]; then echo "Creating app user '$APP_USER_NAME'..." echo "Executing: mysql -h \"$DB_HOST\" -P \"$DB_PORT\" -u \"$MASTER_USER_NAME\" --password=\"<hidden>\" -e \"CREATE USER '${APP_USER_NAME}'@'${APP_USER_HOST}' IDENTIFIED BY '<hidden>';\"" if mysql -h "$DB_HOST" -P "$DB_PORT" -u "$MASTER_USER_NAME" --password="$MASTER_USER_PASSWORD" \ - -e "CREATE USER '${APP_USER_NAME}'@'${APP_USER_HOST}' IDENTIFIED BY '${APP_USER_PASSWORD}';"; then + -e "CREATE USER ${SQL_APP_USER_NAME}@${SQL_APP_USER_HOST} IDENTIFIED BY ${SQL_APP_USER_PASSWORD};"; then NEW_APP_USER_CREATED=1Also update line 265 to use escaped variables:
-"SELECT COUNT(*) FROM mysql.user WHERE user = '$APP_USER_NAME' AND host = '$APP_USER_HOST';") +"SELECT COUNT(*) FROM mysql.user WHERE user = $SQL_APP_USER_NAME AND host = $SQL_APP_USER_HOST;")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sp7_db_setup_check.sh` around lines 267 - 281, The CREATE USER invocation and its diagnostic echo use unescaped variables (APP_USER_NAME, APP_USER_HOST, APP_USER_PASSWORD) which is inconsistent with the GRANT path that uses the pre-escaped SQL_APP_USER_* variables; update the CREATE USER command and the preceding “Executing:” echo to use SQL_APP_USER_NAME, SQL_APP_USER_HOST and SQL_APP_USER_PASSWORD instead of the raw APP_* variables so both creation and GRANT paths use the same escaped identifiers and avoid injection risk, leaving the surrounding conditional and flags (NEW_APP_USER_CREATED, MIGRATOR_NAME fallback) unchanged.
🧹 Nitpick comments (2)
specifyweb/backend/permissions/initialize.py (1)
185-190: 💤 Low valueConsider using
get_or_create()for consistency.The non-testing
assign_users_to_roles()(lines 151-156) usesUserPolicy.objects.get_or_create(), while this testing version uses.create(). Usingget_or_create()here would maintain consistency and prevent potentialIntegrityErrorif the function is ever invoked multiple times without a database wipe.Suggested change
- UserPolicy.objects.create( + UserPolicy.objects.get_or_create( collection_id=colid, specifyuser_id=user.id, resource=CollectionAccessPT.access.resource(), action=CollectionAccessPT.access.action(), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/backend/permissions/initialize.py` around lines 185 - 190, The test-only block uses UserPolicy.objects.create(...) which can raise IntegrityError on repeated runs; change it to use UserPolicy.objects.get_or_create(...) to match the non-test assign_users_to_roles() behavior. Replace the create call for UserPolicy with a get_or_create call using the same identifying kwargs (collection_id=colid, specifyuser_id=user.id, resource=CollectionAccessPT.access.resource(), action=CollectionAccessPT.access.action()) so the record is reused if it already exists.sp7_db_setup_check.sh (1)
188-203: 💤 Low valueConsider using pre-escaped
SQL_DB_IDENTIFIERfor consistency.Line 195 uses backticks directly around
$DB_NAMEfor theCREATE DATABASEstatement, while line 133 precomputes the escaped form asSQL_DB_IDENTIFIER. For consistency with the rest of the script's SQL injection prevention pattern, consider using the pre-escaped variable.♻️ Proposed change
- -e "CREATE DATABASE \`$DB_NAME\`;"; then + -e "CREATE DATABASE $SQL_DB_IDENTIFIER;"; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sp7_db_setup_check.sh` around lines 188 - 203, The CREATE DATABASE branch uses raw backticks around $DB_NAME instead of the pre-escaped SQL_DB_IDENTIFIER used elsewhere, so replace the literal backticked `\`$DB_NAME\`` occurrences (the echoed "Executing: ..." string and the mysql -e "CREATE DATABASE ..." command) with the precomputed "$SQL_DB_IDENTIFIER" variable to keep escaping/SQL-injection protection consistent and still set NEW_DATABASE_CREATED=1 on success; update both the echo and the mysql invocation that currently reference $DB_NAME so they use SQL_DB_IDENTIFIER instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sp7_db_setup_check.sh`:
- Around line 206-232: The CREATE USER/GRANT/SELECT commands use raw variables
($MIGRATOR_NAME, $MIGRATOR_USER_HOST, $MIGRATOR_PASSWORD) which risks injection
and is inconsistent with the escaped forms; update the mysql invocations and the
USER_EXISTS query to use the pre-escaped variables (SQL_MIGRATOR_NAME,
SQL_MIGRATOR_USER_HOST, SQL_MIGRATOR_PASSWORD) instead, keeping the same logic
around NEW_MIGRATOR_USER_CREATED and the conditional branches so the CREATE
USER, GRANT ALL PRIVILEGES and the initial SELECT COUNT(*) all substitute the
SQL_MIGRATOR_* variants rather than the unescaped originals.
---
Outside diff comments:
In `@sp7_db_setup_check.sh`:
- Around line 267-281: The CREATE USER invocation and its diagnostic echo use
unescaped variables (APP_USER_NAME, APP_USER_HOST, APP_USER_PASSWORD) which is
inconsistent with the GRANT path that uses the pre-escaped SQL_APP_USER_*
variables; update the CREATE USER command and the preceding “Executing:” echo to
use SQL_APP_USER_NAME, SQL_APP_USER_HOST and SQL_APP_USER_PASSWORD instead of
the raw APP_* variables so both creation and GRANT paths use the same escaped
identifiers and avoid injection risk, leaving the surrounding conditional and
flags (NEW_APP_USER_CREATED, MIGRATOR_NAME fallback) unchanged.
---
Nitpick comments:
In `@sp7_db_setup_check.sh`:
- Around line 188-203: The CREATE DATABASE branch uses raw backticks around
$DB_NAME instead of the pre-escaped SQL_DB_IDENTIFIER used elsewhere, so replace
the literal backticked `\`$DB_NAME\`` occurrences (the echoed "Executing: ..."
string and the mysql -e "CREATE DATABASE ..." command) with the precomputed
"$SQL_DB_IDENTIFIER" variable to keep escaping/SQL-injection protection
consistent and still set NEW_DATABASE_CREATED=1 on success; update both the echo
and the mysql invocation that currently reference $DB_NAME so they use
SQL_DB_IDENTIFIER instead.
In `@specifyweb/backend/permissions/initialize.py`:
- Around line 185-190: The test-only block uses UserPolicy.objects.create(...)
which can raise IntegrityError on repeated runs; change it to use
UserPolicy.objects.get_or_create(...) to match the non-test
assign_users_to_roles() behavior. Replace the create call for UserPolicy with a
get_or_create call using the same identifying kwargs (collection_id=colid,
specifyuser_id=user.id, resource=CollectionAccessPT.access.resource(),
action=CollectionAccessPT.access.action()) so the record is reused if it already
exists.
🪄 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 Plus
Run ID: c2d4e7d0-78f6-4625-9e50-7e02e539bc50
📒 Files selected for processing (8)
Makefiledocker-entrypoint.shsp7_db_setup_check.shspecifyweb/backend/businessrules/uniqueness_rules.pyspecifyweb/backend/permissions/initialize.pyspecifyweb/settings/__init__.pyspecifyweb/settings/specify_settings.pyspecifyweb/specify/management/commands/base_specify_migration.py
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
sp7_db_setup_check.sh (3)
274-280:⚠️ Potential issue | 🟠 Major | ⚡ Quick winError message and fallback logic don't match the failure condition.
This block executes when
CREATE USERfor the app user fails, but:
- The error message references the migrator user lacking access
- The fallback swaps migrator credentials to master, not app credentials
This appears to be misplaced error handling. If app user creation fails, the message should indicate that, and the appropriate fallback would be to use migrator or master as the app user.
Proposed fix
if mysql -h "$DB_HOST" -P "$DB_PORT" -u "$MASTER_USER_NAME" --password="$MASTER_USER_PASSWORD" \ -e "CREATE USER $SQL_APP_USER_NAME@$SQL_APP_USER_HOST IDENTIFIED BY $SQL_APP_USER_PASSWORD;"; then NEW_APP_USER_CREATED=1 else - echo "Notice: Migrator user '${MIGRATOR_NAME}' lacks usable access to '${DB_NAME}'." - echo "Make corrections to the intended MIGRATOR user permissions to resolve." - echo " GRANT ALL PRIVILEGES ON \`${DB_NAME}\`.* TO '${MIGRATOR_NAME}'@'${MIGRATOR_USER_HOST}'; FLUSH PRIVILEGES;" - MIGRATOR_NAME="$MASTER_USER_NAME" - MIGRATOR_PASSWORD="$MASTER_USER_PASSWORD" - MIGRATION_DB_ALIAS="master" + echo "Error: Failed to create app user '${APP_USER_NAME}'@'${APP_USER_HOST}'." + echo "Falling back to migrator credentials for app user." + APP_USER_NAME="$MIGRATOR_NAME" + APP_USER_PASSWORD="$MIGRATOR_PASSWORD" fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sp7_db_setup_check.sh` around lines 274 - 280, The error message and fallback in the branch that handles a failed CREATE USER for the app user are incorrect: update the echo lines and variable assignments so they reference the app user (not MIGRATOR) and fall back to a sensible app credential (e.g., set APP_* variables to MIGRATOR or MASTER credentials as intended); specifically change the message that currently mentions MIGRATOR_NAME and the GRANT example to mention the APP user and database, and change the assignments that overwrite MIGRATOR_NAME/MIGRATOR_PASSWORD/MIGRATION_DB_ALIAS (the lines touching MIGRATOR_NAME, MIGRATOR_PASSWORD, MASTER_USER_NAME, MASTER_USER_PASSWORD, and MIGRATION_DB_ALIAS) so they set the corresponding APP_USER_NAME/APP_USER_PASSWORD/APP_DB_ALIAS fallback values instead.
12-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHost variable initialization order prevents environment override.
Lines 12 and 17 unconditionally assign
MIGRATOR_USER_HOSTandAPP_USER_HOSTto%before lines 20-21 attempt to read them from the environment. Since the variables are already set, the${VAR:-default}expansion on lines 20-21 becomes a no-op, ignoring any user-supplied environment values.Remove lines 12 and 17 to allow the env-var fallback logic on lines 19-21 to work correctly:
Proposed fix
MIGRATOR_NAME="${MIGRATOR_NAME:-}" MIGRATOR_PASSWORD="${MIGRATOR_PASSWORD:-}" -MIGRATOR_USER_HOST="%" DB_ROOT_PASSWORD="${MYSQL_ROOT_PASSWORD:-}" DB_NAME="${DATABASE_NAME}" APP_USER_NAME="${APP_USER_NAME:-}" APP_USER_PASSWORD="${APP_USER_PASSWORD:-}" -APP_USER_HOST="%" MASTER_USER_HOST="${MASTER_USER_HOST:-%}" MIGRATOR_USER_HOST="${MIGRATOR_USER_HOST:-%}" APP_USER_HOST="${APP_USER_HOST:-%}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sp7_db_setup_check.sh` around lines 12 - 21, The early unconditional assignments for MIGRATOR_USER_HOST and APP_USER_HOST (the lines setting MIGRATOR_USER_HOST="%" and APP_USER_HOST="%") prevent environment overrides; remove those two assignments so the later defaulting lines using MIGRATOR_USER_HOST="${MIGRATOR_USER_HOST:-%}" and APP_USER_HOST="${APP_USER_HOST:-%}" (and similar MASTER_USER_HOST handling) can correctly fall back to "%" only when the env var is unset, preserving user-supplied values.
188-195:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse pre-escaped variables for database operations.
For consistency with user operations (which use
$SQL_*variables), the database existence check and creation should use the pre-escaped forms. IfDB_NAMEcontains special characters, the current raw interpolation could break.Proposed fix
# Create database if it doesn't exist DB_EXISTS=$(mysql -h "$DB_HOST" -P "$DB_PORT" -u "$MASTER_USER_NAME" --password="$MASTER_USER_PASSWORD" -sse \ -"SELECT COUNT(*) FROM information_schema.schemata WHERE schema_name = '$DB_NAME';") +"SELECT COUNT(*) FROM information_schema.schemata WHERE schema_name = $SQL_DB_NAME;") if [[ "$DB_EXISTS" -eq 0 ]]; then echo "Creating database '$DB_NAME'..." - echo "Executing: mysql -h \"$DB_HOST\" -P \"$DB_PORT\" -u \"$MASTER_USER_NAME\" --password=\"<hidden>\" -e \"CREATE DATABASE \`$DB_NAME\`;\"" + echo "Executing: mysql -h \"$DB_HOST\" -P \"$DB_PORT\" -u \"$MASTER_USER_NAME\" --password=\"<hidden>\" -e \"CREATE DATABASE ${SQL_DB_IDENTIFIER};\"" if mysql -h "$DB_HOST" -P "$DB_PORT" -u "$MASTER_USER_NAME" --password="$MASTER_USER_PASSWORD" \ - -e "CREATE DATABASE \`$DB_NAME\`;"; then + -e "CREATE DATABASE ${SQL_DB_IDENTIFIER};"; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sp7_db_setup_check.sh` around lines 188 - 195, The DB existence check and creation use raw DB_NAME/MASTER_USER_PASSWORD interpolation which can break with special chars; change the mysql invocations and DB_EXISTS assignment to use the pre-escaped SQL_* variables (e.g., replace DB_NAME with SQL_DB_NAME, MASTER_USER_NAME with SQL_MASTER_USER_NAME and MASTER_USER_PASSWORD with SQL_MASTER_USER_PASSWORD) in the SELECT COUNT(...) and CREATE DATABASE statements and in the mysql command arguments (and keep the echoed command showing --password="<hidden>"); update the DB_EXISTS assignment and the conditional mysql -e "CREATE DATABASE \`...\`;" calls to reference those SQL_* variables so names are properly escaped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@sp7_db_setup_check.sh`:
- Around line 274-280: The error message and fallback in the branch that handles
a failed CREATE USER for the app user are incorrect: update the echo lines and
variable assignments so they reference the app user (not MIGRATOR) and fall back
to a sensible app credential (e.g., set APP_* variables to MIGRATOR or MASTER
credentials as intended); specifically change the message that currently
mentions MIGRATOR_NAME and the GRANT example to mention the APP user and
database, and change the assignments that overwrite
MIGRATOR_NAME/MIGRATOR_PASSWORD/MIGRATION_DB_ALIAS (the lines touching
MIGRATOR_NAME, MIGRATOR_PASSWORD, MASTER_USER_NAME, MASTER_USER_PASSWORD, and
MIGRATION_DB_ALIAS) so they set the corresponding
APP_USER_NAME/APP_USER_PASSWORD/APP_DB_ALIAS fallback values instead.
- Around line 12-21: The early unconditional assignments for MIGRATOR_USER_HOST
and APP_USER_HOST (the lines setting MIGRATOR_USER_HOST="%" and
APP_USER_HOST="%") prevent environment overrides; remove those two assignments
so the later defaulting lines using
MIGRATOR_USER_HOST="${MIGRATOR_USER_HOST:-%}" and
APP_USER_HOST="${APP_USER_HOST:-%}" (and similar MASTER_USER_HOST handling) can
correctly fall back to "%" only when the env var is unset, preserving
user-supplied values.
- Around line 188-195: The DB existence check and creation use raw
DB_NAME/MASTER_USER_PASSWORD interpolation which can break with special chars;
change the mysql invocations and DB_EXISTS assignment to use the pre-escaped
SQL_* variables (e.g., replace DB_NAME with SQL_DB_NAME, MASTER_USER_NAME with
SQL_MASTER_USER_NAME and MASTER_USER_PASSWORD with SQL_MASTER_USER_PASSWORD) in
the SELECT COUNT(...) and CREATE DATABASE statements and in the mysql command
arguments (and keep the echoed command showing --password="<hidden>"); update
the DB_EXISTS assignment and the conditional mysql -e "CREATE DATABASE \`...\`;"
calls to reference those SQL_* variables so names are properly escaped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 477eb1b6-2bc9-46da-9efa-544082746367
📒 Files selected for processing (2)
sp7_db_setup_check.shspecifyweb/specify/management/commands/base_specify_migration.py
Contributor
Author
|
@CodeRabbit review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
specifyweb/specify/management/commands/base_specify_migration.py (1)
31-36: 💤 Low valueConsider narrowing the exception type for clarity.
The fallback logic and logging look good. The broad
Exceptioncatch works correctly for graceful fallback, but narrowing todjango.db.Error(or a tuple ofOperationalError,DatabaseError) would make the intent clearer and avoid accidentally swallowing unrelated exceptions.🔧 Optional refinement
from django.core.management.base import BaseCommand -from django.db import connections, transaction +from django.db import connections, transaction, Error as DatabaseError import logging ... original_alias = alias try: connections[alias].ensure_connection() - except Exception as e: + except DatabaseError as e: alias = 'master' logger.warning(f"Database alias '{original_alias}' unavailable ({e}), falling back to 'master'")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specifyweb/specify/management/commands/base_specify_migration.py` around lines 31 - 36, The current except Exception in the connections[alias].ensure_connection() block is too broad; change it to catch Django DB errors (e.g., from django.db import DatabaseError, OperationalError) or the base django.db.Error so you only handle database connection failures. Update the except clause that currently references original_alias and sets alias='master' to catch DatabaseError or (OperationalError, DatabaseError) and keep the existing alias fallback and logger.warning message unchanged (referencing connections[alias].ensure_connection(), original_alias, alias, and logger.warning).Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@specifyweb/specify/management/commands/base_specify_migration.py`:
- Around line 31-36: The current except Exception in the
connections[alias].ensure_connection() block is too broad; change it to catch
Django DB errors (e.g., from django.db import DatabaseError, OperationalError)
or the base django.db.Error so you only handle database connection failures.
Update the except clause that currently references original_alias and sets
alias='master' to catch DatabaseError or (OperationalError, DatabaseError) and
keep the existing alias fallback and logger.warning message unchanged
(referencing connections[alias].ensure_connection(), original_alias, alias, and
logger.warning).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ac12d1f6-2646-403a-a139-f160bfcc7220
📒 Files selected for processing (2)
sp7_db_setup_check.shspecifyweb/specify/management/commands/base_specify_migration.py
🚧 Files skipped from review as they are similar to previous changes (1)
- sp7_db_setup_check.sh
grantfitzsimmons
approved these changes
Jun 30, 2026
grantfitzsimmons
left a comment
grantfitzsimmons
left a comment
Member
There was a problem hiding this comment.
All sp7_db_setup_check.sh functions present. APP_USER_NAME fallback chain functions correctly.
Databases Tested: (Number of tables, discipline)
aumnh_fish_2026_06_26(254, Ichthyology)alfmuseum_2026_06_26(250, Multi)aumnh_herbarium_2026_06_26(253, Botany)aumnh_inverts_2026_06_26(267, Invertebrate)bohart_entomology_2026_06_26(248, Entomology)calvertmarinemuseum_2026_06_26(249, Multi)sfmuseumofearthscience_2026_06_26(228, EarthScience)sp7demofish_2026_06_26(256, Ichthyology)turtleconservancy_2026_06_26(236, Herpetology)uiowapaleo_2026_06_26(249, Paleontology)unitec_2026_06_26(248, Multi)unsm_vp_2026_06_26(251, VertPaleo)uoregon_2026_06_26(252, Multi)uwfc_2026_06_26(260, Ichthyology)vimsfish_2026_06_26(290, Ichthyology)
All exit 0. Every DB reported "No migrations to apply". Privilege verification was done.
When I restarted the container with a fresh DATABASE_NAME, the full chain (sp7_db_setup_check.sh → base_specify_migration → migrate) completed cleanly with the database created, user privileges verified, and all 70+ migrations applied (in under 2 minutes on MariaDB via Docker).
manage.py test test_delete_blockers all 5 passed in 3.661s.
No issues found!
Remove the 'restart: unless-stopped' line from the development service in docker-compose.yml
Replace the previous tolerant invocation of sp7_db_setup_check.sh (which used set +e / set -e) with an explicit if-check. If the DB setup script succeeds, continue to run key migration functions; if it fails, print an error and exit with status 1 to avoid continuing startup with an incomplete DB setup.
2 tasks
Member
|
Added my changes from #8108. Waiting on @melton-jason's review |
grantfitzsimmons
changed the title
fixes #8107 This resulted in the following workflow: 1. MariaDB would create the database on startup 2. The sp7_db_setup_check.sh would see the existing database and assume it was a Specify 6 DB, inserting the fake specify/0001 migration 3. When running the accounts/0001 migration, the operation would fail because there is no Specifyuser table
Find some way to use a router to always use the right database for migrations. See https://docs.djangoproject.com/en/4.2/topics/db/multi-db/ and #8154 (comment)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #8148
Fixes #6121
Fixes #8107
Checklist
self-explanatory (or properly documented)
specify7/specifyweb/specify/management/commands/run_key_migration_functions.py
Line 50 in ea04665
Testing instructions
Summary by CodeRabbit
Bug Fixes
New Features
Chores