Issue 8148 fixes 6121 after review#8156
Open
CarolineDenis wants to merge 27 commits into
Open
Conversation
Contributor
|
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 | ✅ 2 | ❌ 4❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
|
NOTES:
|
Contributor
Author
|
@CodeRabbit full review |
Contributor
✅ Action performedFull review finished. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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)
273-280:⚠️ Potential issue | 🟠 Major | ⚡ Quick winError handling block appears mismatched.
When app user creation fails (line 270), the else branch (lines 273-280) prints messages about the migrator user lacking access and falls back to master credentials for the migrator. This seems incorrect for an app user creation failure scenario.
The expected behavior for a failed
CREATE USERfor the app user should either abort or provide appropriate fallback messaging, not adjust migrator credentials.🐛 Possible fix: proper error handling for app user creation failure
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 273 - 280, The else branch handling the app user CREATE USER failure is incorrect: it prints migrator-access messages and overwrites MIGRATOR_NAME/MIGRATOR_PASSWORD/MIGRATION_DB_ALIAS; instead, update the else for the app user creation check to not touch migrator variables and either exit with a non-zero status or emit a clear app-user-specific error and remediation hint (e.g., suggest correct GRANT/CREATE USER for the app user), referencing the app user creation block and the variables APP_USER/APP_USER_HOST (or whichever app user vars are used) so you fix the correct branch rather than changing MIGRATOR_NAME, MIGRATOR_PASSWORD, or MIGRATION_DB_ALIAS.
🧹 Nitpick comments (6)
sp7_db_setup_check.sh (4)
132-132: 💤 Low valueUnused variable
SQL_DB_NAME.
SQL_DB_NAMEis defined but never used.SQL_DB_IDENTIFIERis used instead. Consider removing to reduce clutter.🤖 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` at line 132, Remove the unused variable assignment to SQL_DB_NAME (the call to sql_string_literal "$DB_NAME") since the script uses SQL_DB_IDENTIFIER instead; either delete the SQL_DB_NAME line or, if you intended to use the quoted value, replace usages of SQL_DB_IDENTIFIER with SQL_DB_NAME or assign SQL_DB_IDENTIFIER=$(sql_string_literal "$DB_NAME") so only the actually used variable remains.
50-58: 💤 Low valueRedundant validation blocks.
Lines 50-58 re-check variables that are already validated in line 45's combined check. Since the script uses
set -e, if line 45 passes, these blocks can never trigger.♻️ Consider removing the redundant checks
-if [[ -z "$MASTER_USER_NAME" || -z "$MASTER_USER_PASSWORD" ]]; then - echo "Error: MASTER_USER_NAME and MASTER_USER_PASSWORD (or MYSQL_ROOT_PASSWORD) must be set." - exit 1 -fi - -if [[ -z "$MIGRATOR_PASSWORD" || -z "$APP_USER_NAME" || -z "$APP_USER_PASSWORD" ]]; then - echo "Error: One or more required user-related environment variables are missing or empty." - exit 1 -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 50 - 58, The script contains redundant environment-variable validation: after the initial combined check of MASTER_USER_NAME and MASTER_USER_PASSWORD (and root password) the later if-block re-checking MIGRATOR_PASSWORD, APP_USER_NAME, and APP_USER_PASSWORD is unreachable when using set -e; remove the redundant second validation block (the if that echoes "One or more required user-related environment variables are missing or empty.") or consolidate all required-variable checks into a single, clear validation that verifies MASTER_USER_NAME, MASTER_USER_PASSWORD, MIGRATOR_PASSWORD, APP_USER_NAME, and APP_USER_PASSWORD once.
12-12: 💤 Low valueMinor redundancy in host variable defaults.
Lines 12 and 17 unconditionally set
MIGRATOR_USER_HOST="%"andAPP_USER_HOST="%", but lines 20-21 then apply${VAR:-%}defaults that can never take effect since these variables are already set. Consider removing the redundant lines 12 and 17, or removing lines 20-21.Also applies to: 17-17, 19-21
🤖 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` at line 12, Remove the redundant hard assignments MIGRATOR_USER_HOST="%" and APP_USER_HOST="%" (the variables used later with "${MIGRATOR_USER_HOST:-%}" and "${APP_USER_HOST:-%}" defaults) so the parameter-expansion defaults can take effect, or alternatively delete the "${...:-%}" defaulting and keep the explicit assignments; update the script by editing the lines that define MIGRATOR_USER_HOST and APP_USER_HOST to eliminate the duplicate defaulting behavior (refer to the MIGRATOR_USER_HOST and APP_USER_HOST symbols and the parameter expansion usages "${MIGRATOR_USER_HOST:-%}" and "${APP_USER_HOST:-%}").
61-61: 💤 Low valueUnused variable
SAME_MASTER_AND_MIGRATOR.This flag is set but never referenced. If it's not needed, consider removing it to reduce noise.
Also applies to: 64-66
🤖 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` at line 61, The variable SAME_MASTER_AND_MIGRATOR is declared but never referenced; remove this unused flag (and any other nearby unused flag declarations) from the script to reduce noise, or if it was intended to control logic, wire it into the relevant conditional(s) (e.g., the startup/migration checks) by referencing SAME_MASTER_AND_MIGRATOR in those conditionals and documenting its purpose; ensure you update any comments/tests that expect the flag to exist.specifyweb/specify/management/commands/base_specify_migration.py (1)
31-35: 💤 Low valueConsider catching a more specific exception.
Catching bare
Exception(line 33) is flagged by static analysis (BLE001). While this provides resilience, catchingdjango.db.OperationalErrorordjango.db.DatabaseErrorwould be more precise and avoid masking unexpected errors.♻️ Suggested refinement
+from django.db import connections, transaction, OperationalError, DatabaseError + # Validate the alias exists and is usable; fallback to 'master' if not try: connections[alias].ensure_connection() - except Exception: + except (OperationalError, DatabaseError): alias = 'master' logger.warning(f"Falling back to database alias '{alias}'")🤖 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, The try/except around connections[alias].ensure_connection() should catch a more specific DB error instead of a bare Exception; update the except block to catch django.db.OperationalError (or django.db.DatabaseError) and import that exception at the top of the module, then keep the existing fallback behavior (set alias = 'master' and logger.warning(...)) so only expected DB connection failures are handled while other exceptions propagate.specifyweb/backend/permissions/initialize.py (1)
78-80: 💤 Low valueMinor: Duplicate
UserPolicymodel assignment.Line 80 reassigns the same model that was already assigned at line 78. One of these can be removed.
Suggested fix
def assign_users_to_roles(apps=apps) -> None: Role = apps.get_model('permissions', 'Role') UserPolicy = apps.get_model('permissions', 'UserPolicy') Agent = apps.get_model('specify', 'Agent') - UserPolicy = apps.get_model('permissions', 'UserPolicy') UserRole = apps.get_model('permissions', 'UserRole')🤖 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 78 - 80, The file contains a duplicate assignment of the UserPolicy model: remove the redundant "UserPolicy = apps.get_model('permissions', 'UserPolicy')" (the second occurrence) in initialize.py so only one UserPolicy = apps.get_model('permissions', 'UserPolicy') remains; ensure you keep the existing Agent = apps.get_model('specify', 'Agent') line and that no other code depends on the duplicate assignment before committing.
🤖 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 264-271: The USER_EXISTS check and the CREATE USER invocation are
using raw $APP_USER_NAME, $APP_USER_HOST and $APP_USER_PASSWORD; replace those
with the pre-escaped SQL_APP_USER_NAME, SQL_APP_USER_HOST and
SQL_APP_USER_PASSWORD variables so SQL quoting is consistent (use
SQL_APP_USER_NAME/SQL_APP_USER_HOST in the SELECT COUNT(*) query and use
SQL_APP_USER_NAME/SQL_APP_USER_HOST/SQL_APP_USER_PASSWORD in the CREATE USER
mysql -e statement and the echoed command string), and ensure the echo still
hides the plain password while showing the escaped identifiers for safety.
In `@specifyweb/backend/permissions/initialize.py`:
- Around line 185-190: Replace the direct creation call with an idempotent
get_or_create pattern: where UserPolicy.objects.create(...) is used in this
snippet, call UserPolicy.objects.get_or_create(...) with the same identifying
fields (collection_id=colid, specifyuser_id=user.id,
resource=CollectionAccessPT.access.resource(),
action=CollectionAccessPT.access.action()) so the initialization matches
assign_users_to_roles() behavior and avoids IntegrityError on repeated runs.
---
Outside diff comments:
In `@sp7_db_setup_check.sh`:
- Around line 273-280: The else branch handling the app user CREATE USER failure
is incorrect: it prints migrator-access messages and overwrites
MIGRATOR_NAME/MIGRATOR_PASSWORD/MIGRATION_DB_ALIAS; instead, update the else for
the app user creation check to not touch migrator variables and either exit with
a non-zero status or emit a clear app-user-specific error and remediation hint
(e.g., suggest correct GRANT/CREATE USER for the app user), referencing the app
user creation block and the variables APP_USER/APP_USER_HOST (or whichever app
user vars are used) so you fix the correct branch rather than changing
MIGRATOR_NAME, MIGRATOR_PASSWORD, or MIGRATION_DB_ALIAS.
---
Nitpick comments:
In `@sp7_db_setup_check.sh`:
- Line 132: Remove the unused variable assignment to SQL_DB_NAME (the call to
sql_string_literal "$DB_NAME") since the script uses SQL_DB_IDENTIFIER instead;
either delete the SQL_DB_NAME line or, if you intended to use the quoted value,
replace usages of SQL_DB_IDENTIFIER with SQL_DB_NAME or assign
SQL_DB_IDENTIFIER=$(sql_string_literal "$DB_NAME") so only the actually used
variable remains.
- Around line 50-58: The script contains redundant environment-variable
validation: after the initial combined check of MASTER_USER_NAME and
MASTER_USER_PASSWORD (and root password) the later if-block re-checking
MIGRATOR_PASSWORD, APP_USER_NAME, and APP_USER_PASSWORD is unreachable when
using set -e; remove the redundant second validation block (the if that echoes
"One or more required user-related environment variables are missing or empty.")
or consolidate all required-variable checks into a single, clear validation that
verifies MASTER_USER_NAME, MASTER_USER_PASSWORD, MIGRATOR_PASSWORD,
APP_USER_NAME, and APP_USER_PASSWORD once.
- Line 12: Remove the redundant hard assignments MIGRATOR_USER_HOST="%" and
APP_USER_HOST="%" (the variables used later with "${MIGRATOR_USER_HOST:-%}" and
"${APP_USER_HOST:-%}" defaults) so the parameter-expansion defaults can take
effect, or alternatively delete the "${...:-%}" defaulting and keep the explicit
assignments; update the script by editing the lines that define
MIGRATOR_USER_HOST and APP_USER_HOST to eliminate the duplicate defaulting
behavior (refer to the MIGRATOR_USER_HOST and APP_USER_HOST symbols and the
parameter expansion usages "${MIGRATOR_USER_HOST:-%}" and
"${APP_USER_HOST:-%}").
- Line 61: The variable SAME_MASTER_AND_MIGRATOR is declared but never
referenced; remove this unused flag (and any other nearby unused flag
declarations) from the script to reduce noise, or if it was intended to control
logic, wire it into the relevant conditional(s) (e.g., the startup/migration
checks) by referencing SAME_MASTER_AND_MIGRATOR in those conditionals and
documenting its purpose; ensure you update any comments/tests that expect the
flag to exist.
In `@specifyweb/backend/permissions/initialize.py`:
- Around line 78-80: The file contains a duplicate assignment of the UserPolicy
model: remove the redundant "UserPolicy = apps.get_model('permissions',
'UserPolicy')" (the second occurrence) in initialize.py so only one UserPolicy =
apps.get_model('permissions', 'UserPolicy') remains; ensure you keep the
existing Agent = apps.get_model('specify', 'Agent') line and that no other code
depends on the duplicate assignment before committing.
In `@specifyweb/specify/management/commands/base_specify_migration.py`:
- Around line 31-35: The try/except around
connections[alias].ensure_connection() should catch a more specific DB error
instead of a bare Exception; update the except block to catch
django.db.OperationalError (or django.db.DatabaseError) and import that
exception at the top of the module, then keep the existing fallback behavior
(set alias = 'master' and logger.warning(...)) so only expected DB connection
failures are handled while other exceptions 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: 083da658-fd07-4d78-b8af-649843f88285
📒 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 full review |
Contributor
✅ Action performedFull review finished. |
Contributor
Author
|
@CodeRabbit full review |
Contributor
✅ Action performedFull review finished. |
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 (2)
specifyweb/backend/businessrules/uniqueness_rules.py (1)
432-444: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winApply the same iterable materialization fix for consistency.
The
remove_uniqueness_rulefunction has the same iterable consumption issue that was just fixed increate_uniqueness_rule. Line 438 callsrule_is_global(scopes), which consumes the iterable by converting it to a tuple internally, before line 444 materializesscopes = list(scopes). If a caller passes a one-shot generator, the scopes list will be empty and no scope fields will match during deletion.For consistency with the fix in
create_uniqueness_rule, materializefieldsandscopesimmediately after theUniquenessRuleFieldassignment.♻️ Proposed fix to materialize before computing final_discipline
UniquenessRuleField = registry.get_model( 'businessrules', 'UniquenessRuleField') if registry else models.UniquenessRuleField + fields = list(fields) + scopes = list(scopes) + final_discipline = None if rule_is_global(scopes) else discipline candidate_rules = UniquenessRule.objects.filter( modelName=model_name, isDatabaseConstraint=is_database_constraint, discipline=final_discipline) - fields = list(fields) - scopes = list(scopes) - rule_ids = []🤖 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/businessrules/uniqueness_rules.py` around lines 432 - 444, In remove_uniqueness_rule, materialize the incoming iterables immediately after obtaining UniquenessRuleField (i.e., convert fields and scopes to lists before calling rule_is_global) so rule_is_global(scopes) doesn't consume a one-shot generator; move the lines that set fields = list(fields) and scopes = list(scopes) to directly after the UniquenessRuleField assignment, then compute final_discipline using rule_is_global(scopes) and proceed with candidate_rules and deletion.specifyweb/backend/permissions/initialize.py (1)
167-174:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
get_or_createfor role assignment to ensure idempotency.The production path in
assign_users_to_roles()(lines 145-148) usesUserRole.objects.get_or_create(), but this testing path usesuser.roles.create(). If test initialization runs multiple times, this can raiseIntegrityErroror create duplicate role assignments.Proposed fix
for user in Specifyuser.objects.all(): for collection in Collection.objects.all(): if user.usertype == 'Manager': - user.roles.create(role=Role.objects.get(collection=collection, name="Collection Admin")) + role = Role.objects.get(collection=collection, name="Collection Admin") + UserRole.objects.get_or_create(specifyuser=user, role=role) if user.usertype == 'FullAccess': - user.roles.create(role=Role.objects.get(collection=collection, name="Full Access - Legacy")) + role = Role.objects.get(collection=collection, name="Full Access - Legacy") + UserRole.objects.get_or_create(specifyuser=user, role=role) if user.usertype in ('LimitedAccess', 'Guest'): - user.roles.create(role=Role.objects.get(collection=collection, name="Read Only - Legacy")) + role = Role.objects.get(collection=collection, name="Read Only - Legacy") + UserRole.objects.get_or_create(specifyuser=user, role=role)Note: You'll need to add
UserRole = apps.get_model('permissions', 'UserRole')near the top of the function (around line 165).🤖 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 167 - 174, The test initialization loop uses user.roles.create(...) which can create duplicate UserRole rows; change this to use the same idempotent pattern as assign_users_to_roles by resolving UserRole via apps.get_model('permissions', 'UserRole') and calling UserRole.objects.get_or_create(user=user, role=Role.objects.get(collection=collection, name=...)) for each branch (matching the Manager, FullAccess and LimitedAccess/Guest cases) instead of user.roles.create so repeated runs will not raise IntegrityError or duplicate assignments.
🧹 Nitpick comments (2)
sp7_db_setup_check.sh (2)
188-189: 💤 Low valueConsider using the pre-escaped
SQL_DB_NAMEfor consistency.The query interpolates raw
$DB_NAMEwhileSQL_DB_NAMEwas defined (line 132) specifically for safe SQL string literal contexts. For consistency with the rest of the script:-"SELECT COUNT(*) FROM information_schema.schemata WHERE schema_name = '$DB_NAME';") +"SELECT COUNT(*) FROM information_schema.schemata WHERE schema_name = $SQL_DB_NAME;")🤖 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 - 189, Replace the raw $DB_NAME usage in the DB_EXISTS query with the pre-escaped SQL_DB_NAME variable to keep SQL string literal safety and consistency; update the mysql command that sets DB_EXISTS (the line invoking mysql -h "$DB_HOST" ... "SELECT COUNT(*) FROM information_schema.schemata WHERE schema_name = '$DB_NAME';") to use '$SQL_DB_NAME' instead, preserving the single-quoted SQL literal and existing quoting around the command and credentials.
60-66: 💤 Low valueUnused variable:
SAME_MASTER_AND_MIGRATORThis flag is computed but never referenced. Consider removing it to avoid clutter, or add a comment if it's reserved for future use.
🤖 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 60 - 66, The variable SAME_MASTER_AND_MIGRATOR is computed but never used; either remove its declaration and the conditional that sets it (the lines declaring SAME_MASTER_AND_MIGRATOR=false and the if block that compares MIGRATOR_NAME/MIGRATOR_PASSWORD to MASTER_USER_NAME/MASTER_USER_PASSWORD) to eliminate dead code, or keep the variable but add a clear comment next to SAME_MASTER_AND_MIGRATOR explaining it's reserved for future use so reviewers understand it's intentional (leave the other flags SAME_MASTER_AND_APP and SAME_MIGRATOR_AND_APP unchanged).
🤖 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`:
- Line 267: The conditional that gates app user creation uses the wrong
variable: replace the "$MIGRATOR_NAME" check with the app user variable so it
reads [[ "$USER_EXISTS" -eq 0 && "$APP_USER_NAME" != "root" ]], keeping the
numeric check for USER_EXISTS and the string comparison for APP_USER_NAME;
update any surrounding comments to reflect that this branch is for creating the
app user guarded by USER_EXISTS and APP_USER_NAME.
- Around line 311-312: The SHOW GRANTS query is fetching the migrator user (uses
${MIGRATOR_NAME}@${MIGRATOR_USER_HOST}) but the surrounding code and messages
expect the app user; change the command that sets APP_GRANTS_RAW to query the
app user instead (replace ${MIGRATOR_NAME} and ${MIGRATOR_USER_HOST} with the
app user variables used elsewhere in the script, e.g.
${APP_USER_NAME}@${APP_USER_HOST} or ${APP_NAME}@${APP_USER_HOST}) so the
retrieved grants match the error message and verification logic.
In `@specifyweb/specify/management/commands/base_specify_migration.py`:
- Around line 30-35: The fallback logging loses the original alias and omits
exception details; before calling connections[alias].ensure_connection() capture
the current alias (e.g., original_alias = alias), catch the exception as e
(except Exception as e) around the ensure_connection() call, set alias =
'master' as before, and log a message that includes original_alias and the
exception details (use logger.warning or logger.error with f"Falling back from
database alias '{original_alias}' to 'master': {e}" or logger.warning(...,
exc_info=True)) so the failing alias and error are preserved; reference the
existing connections[alias].ensure_connection(), alias variable, and
logger.warning/logger.error in your change.
---
Outside diff comments:
In `@specifyweb/backend/businessrules/uniqueness_rules.py`:
- Around line 432-444: In remove_uniqueness_rule, materialize the incoming
iterables immediately after obtaining UniquenessRuleField (i.e., convert fields
and scopes to lists before calling rule_is_global) so rule_is_global(scopes)
doesn't consume a one-shot generator; move the lines that set fields =
list(fields) and scopes = list(scopes) to directly after the UniquenessRuleField
assignment, then compute final_discipline using rule_is_global(scopes) and
proceed with candidate_rules and deletion.
In `@specifyweb/backend/permissions/initialize.py`:
- Around line 167-174: The test initialization loop uses user.roles.create(...)
which can create duplicate UserRole rows; change this to use the same idempotent
pattern as assign_users_to_roles by resolving UserRole via
apps.get_model('permissions', 'UserRole') and calling
UserRole.objects.get_or_create(user=user,
role=Role.objects.get(collection=collection, name=...)) for each branch
(matching the Manager, FullAccess and LimitedAccess/Guest cases) instead of
user.roles.create so repeated runs will not raise IntegrityError or duplicate
assignments.
---
Nitpick comments:
In `@sp7_db_setup_check.sh`:
- Around line 188-189: Replace the raw $DB_NAME usage in the DB_EXISTS query
with the pre-escaped SQL_DB_NAME variable to keep SQL string literal safety and
consistency; update the mysql command that sets DB_EXISTS (the line invoking
mysql -h "$DB_HOST" ... "SELECT COUNT(*) FROM information_schema.schemata WHERE
schema_name = '$DB_NAME';") to use '$SQL_DB_NAME' instead, preserving the
single-quoted SQL literal and existing quoting around the command and
credentials.
- Around line 60-66: The variable SAME_MASTER_AND_MIGRATOR is computed but never
used; either remove its declaration and the conditional that sets it (the lines
declaring SAME_MASTER_AND_MIGRATOR=false and the if block that compares
MIGRATOR_NAME/MIGRATOR_PASSWORD to MASTER_USER_NAME/MASTER_USER_PASSWORD) to
eliminate dead code, or keep the variable but add a clear comment next to
SAME_MASTER_AND_MIGRATOR explaining it's reserved for future use so reviewers
understand it's intentional (leave the other flags SAME_MASTER_AND_APP and
SAME_MIGRATOR_AND_APP unchanged).
🪄 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: 5ccbdb6e-253f-4428-b52b-e59278f7670f
📒 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
|
Reference for sp7_db_setup script docs |
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
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
1 participant
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
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