Load predefined users from a JSON file through command line. #9229#9652
Load predefined users from a JSON file through command line. #9229#9652khushboovashi wants to merge 2 commits intopgadmin-org:masterfrom
Conversation
WalkthroughAdds documentation and implementation for a bulk user import feature. The documentation describes the JSON file format and usage of a new Changes
Sequence DiagramsequenceDiagram
actor CLI as User / CLI
participant JSON as JSON Parser
participant Validator as Validator
participant MU as ManageUsers
participant MR as ManageRoles
participant DB as Database
CLI->>JSON: Read & Parse JSON file
JSON-->>Validator: User entries list
loop For each user entry
Validator->>Validator: Validate required fields
Validator->>MU: Check if user exists
MU->>DB: Query user by username/email
DB-->>MU: User exists?
alt User not found
Validator->>Validator: Validate password length
Validator->>MR: Map role names to IDs
MR->>DB: Get role by name
DB-->>MR: Role ID
MU->>DB: Create user entry
DB-->>MU: User created
else User exists
Validator-->>Validator: Mark as skipped
end
end
Validator-->>CLI: Report: created count, skipped count, errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
web/setup.py (3)
246-257: No validation ofauth_sourcevalues from user-supplied JSON.The
auth_sourcefield is taken verbatim from the JSON (line 247). If a user provides an unrecognized value (e.g.,"Internal"capitalized, or a typo like"interanl"), it silently passes through andcreate_userwill attempt to create the user with an invalid auth source. Consider validating against the known constants (INTERNAL,LDAP,OAUTH2,KERBEROS,WEBSERVER) and reporting an error for unrecognized values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/setup.py` around lines 246 - 257, The code assigns auth_source directly from user_entry into user_data without validating it; update the block that reads auth_source (the user_entry.get('auth_source', INTERNAL) and the user_data dict) to validate the value against the allowed constants (INTERNAL, LDAP, OAUTH2, KERBEROS, WEBSERVER) and normalize casing if desired, and if the provided value is not one of these, raise or return a clear error before calling create_user so invalid auth_source values (e.g., "Internal" or typos) are rejected and only recognized constants are used.
209-211:open()without explicit encoding.
open(file_path)uses the platform default encoding, which may not be UTF-8 on all systems (notably Windows). Since JSON is typically UTF-8:- with open(file_path) as f: + with open(file_path, encoding='utf-8') as f:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/setup.py` around lines 209 - 211, The file opens JSON files using open(file_path) without an explicit encoding; update the code that reads into jsonlib.load to open the file with an explicit UTF-8 encoding (i.e., pass encoding="utf-8" to open) so reads are deterministic across platforms—locate the try block that uses file_path and jsonlib.load and change the open call accordingly.
289-295: Useconfig.PASSWORD_LENGTH_MINinstead of hardcoded6for consistency with the rest of the codebase.Password validation elsewhere in the codebase (e.g.,
web/pgadmin/setup/user_info.py,web/pgadmin/browser/__init__.py) consistently usesconfig.PASSWORD_LENGTH_MIN. Hardcoding6here means this validation will drift if the configured minimum changes.Since
configis already imported, update the check and error message:♻️ Proposed fix
if auth_source == INTERNAL: - if len(user_data['newPassword']) < 6: + if len(user_data['newPassword']) < \ + config.PASSWORD_LENGTH_MIN: print(f"Skipping user '{user_data['username']}': " - f"password must be at least 6 characters") + f"password must be at least " + f"{config.PASSWORD_LENGTH_MIN} characters")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/setup.py` around lines 289 - 295, Replace the hardcoded minimum password length 6 with the configured constant by using config.PASSWORD_LENGTH_MIN in the validation and the error message: in the block that checks auth_source == INTERNAL and inspects user_data['newPassword'] (and increments error_count/continues), change the numeric literal to config.PASSWORD_LENGTH_MIN and update the printed string to interpolate that constant so the message reflects the configured minimum.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/setup.py`:
- Around line 166-170: The load_users command currently accepts a json parameter
but never uses it; update the load_users function to honor the json flag (or
remove the parameter) — specifically, in the load_users function implement
conditional output: after loading users, if json is True serialize the resulting
users list to JSON (matching the format used by the get-users command) and print
to stdout (or write to the same destination get-users uses for JSON), otherwise
keep the existing human-readable output; keep the `@update_sqlite_path` decorator
as-is and use the existing user-loading logic in load_users to produce the
object you will JSON-serialize.
- Around line 202-219: Remove the redundant print of the exception in the
unquote error path: when unquote(input_file) raises, stop calling print(str(e))
and instead directly return _handle_error(str(e), True) (same behavior as the
JSON error branches). Update the try/except around unquote to catch Exception as
e, assign file_path = unquote(input_file) in the try, and in the except simply
return _handle_error(str(e), True) (references: unquote, input_file, file_path,
_handle_error).
- Around line 240-244: The print statement in the user import block (where
user_entry is validated) uses an f-string with no placeholders and doesn't
identify which entry failed; update the message to include context (e.g., the
user_entry contents or its index) so failures are debuggable and to satisfy Ruff
F541. Locate the validation around the user_entry variable in web/setup.py (the
block that checks 'username' and 'email') and change the print to include either
the current index from an enumerate or the user_entry dict (or both) in the
output, and keep incrementing error_count and continue as before.
- Around line 270-285: Replace calls to ManageUsers.get_user and
ManageRoles.get_role inside load_users with direct ORM queries against the User
and Role models using the existing app context (avoid calling
create_app()/test_request_context inside ManageUsers/ManageRoles to eliminate
per-user app creation); also change the f-string "f\"Skipping user:
missing...\"" to a plain string, replace the hardcoded password min length 6
with config.PASSWORD_LENGTH_MIN, open files with explicit encoding (e.g.
open(file_path, encoding="utf-8")), remove the redundant explicit print of
errors so only _handle_error prints/logs them, and remove the unused json
parameter from the function signature and any callers. Ensure you reference
ManageUsers.get_user and ManageRoles.get_role to locate the affected calls and
update load_users and the function definition that has the json parameter.
---
Nitpick comments:
In `@web/setup.py`:
- Around line 246-257: The code assigns auth_source directly from user_entry
into user_data without validating it; update the block that reads auth_source
(the user_entry.get('auth_source', INTERNAL) and the user_data dict) to validate
the value against the allowed constants (INTERNAL, LDAP, OAUTH2, KERBEROS,
WEBSERVER) and normalize casing if desired, and if the provided value is not one
of these, raise or return a clear error before calling create_user so invalid
auth_source values (e.g., "Internal" or typos) are rejected and only recognized
constants are used.
- Around line 209-211: The file opens JSON files using open(file_path) without
an explicit encoding; update the code that reads into jsonlib.load to open the
file with an explicit UTF-8 encoding (i.e., pass encoding="utf-8" to open) so
reads are deterministic across platforms—locate the try block that uses
file_path and jsonlib.load and change the open call accordingly.
- Around line 289-295: Replace the hardcoded minimum password length 6 with the
configured constant by using config.PASSWORD_LENGTH_MIN in the validation and
the error message: in the block that checks auth_source == INTERNAL and inspects
user_data['newPassword'] (and increments error_count/continues), change the
numeric literal to config.PASSWORD_LENGTH_MIN and update the printed string to
interpolate that constant so the message reflects the configured minimum.
| @app.command() | ||
| @update_sqlite_path | ||
| def load_users(input_file: str, | ||
| sqlite_path: Optional[str] = None, | ||
| json: Optional[bool] = False): |
There was a problem hiding this comment.
json parameter is declared but never used — output format is unaffected.
The json parameter (line 170) is accepted but never referenced in the method body, so passing --json has no effect. Either implement JSON output (consistent with other commands like get-users) or remove the parameter. The sqlite_path parameter is consumed by the @update_sqlite_path decorator, so that one is fine.
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 169-169: Unused method argument: sqlite_path
(ARG002)
[warning] 170-170: Unused method argument: json
(ARG002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/setup.py` around lines 166 - 170, The load_users command currently
accepts a json parameter but never uses it; update the load_users function to
honor the json flag (or remove the parameter) — specifically, in the load_users
function implement conditional output: after loading users, if json is True
serialize the resulting users list to JSON (matching the format used by the
get-users command) and print to stdout (or write to the same destination
get-users uses for JSON), otherwise keep the existing human-readable output;
keep the `@update_sqlite_path` decorator as-is and use the existing user-loading
logic in load_users to produce the object you will JSON-serialize.
| try: | ||
| file_path = unquote(input_file) | ||
| except Exception as e: | ||
| print(str(e)) | ||
| return _handle_error(str(e), True) | ||
|
|
||
| # Read and parse JSON file | ||
| try: | ||
| with open(file_path) as f: | ||
| data = jsonlib.load(f) | ||
| except jsonlib.decoder.JSONDecodeError as e: | ||
| return _handle_error( | ||
| gettext("Error parsing input file %s: %s" % (file_path, e)), | ||
| True) | ||
| except Exception as e: | ||
| return _handle_error( | ||
| gettext("Error reading input file %s: [%d] %s" % | ||
| (file_path, e.errno, e.strerror)), True) |
There was a problem hiding this comment.
Double error output on unquote failure (lines 204-206).
When the unquote call fails, str(e) is printed on line 205, then _handle_error(str(e), True) (line 206) prints the same message again before calling sys.exit(1). The JSON-parsing error paths (lines 212-219) correctly delegate to _handle_error only. Remove the extra print on line 205.
Proposed fix
try:
file_path = unquote(input_file)
except Exception as e:
- print(str(e))
return _handle_error(str(e), True)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| file_path = unquote(input_file) | |
| except Exception as e: | |
| print(str(e)) | |
| return _handle_error(str(e), True) | |
| # Read and parse JSON file | |
| try: | |
| with open(file_path) as f: | |
| data = jsonlib.load(f) | |
| except jsonlib.decoder.JSONDecodeError as e: | |
| return _handle_error( | |
| gettext("Error parsing input file %s: %s" % (file_path, e)), | |
| True) | |
| except Exception as e: | |
| return _handle_error( | |
| gettext("Error reading input file %s: [%d] %s" % | |
| (file_path, e.errno, e.strerror)), True) | |
| try: | |
| file_path = unquote(input_file) | |
| except Exception as e: | |
| return _handle_error(str(e), True) | |
| # Read and parse JSON file | |
| try: | |
| with open(file_path) as f: | |
| data = jsonlib.load(f) | |
| except jsonlib.decoder.JSONDecodeError as e: | |
| return _handle_error( | |
| gettext("Error parsing input file %s: %s" % (file_path, e)), | |
| True) | |
| except Exception as e: | |
| return _handle_error( | |
| gettext("Error reading input file %s: [%d] %s" % | |
| (file_path, e.errno, e.strerror)), True) |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 204-204: Do not catch blind exception: Exception
(BLE001)
[warning] 216-216: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/setup.py` around lines 202 - 219, Remove the redundant print of the
exception in the unquote error path: when unquote(input_file) raises, stop
calling print(str(e)) and instead directly return _handle_error(str(e), True)
(same behavior as the JSON error branches). Update the try/except around unquote
to catch Exception as e, assign file_path = unquote(input_file) in the try, and
in the except simply return _handle_error(str(e), True) (references: unquote,
input_file, file_path, _handle_error).
| if 'username' not in user_entry and\ | ||
| 'email' not in user_entry: | ||
| print(f"Skipping user: missing 'username' or 'email'") | ||
| error_count += 1 | ||
| continue |
There was a problem hiding this comment.
f-string on line 242 has no placeholders — include the entry context for debuggability.
Static analysis (Ruff F541) flags this. More importantly, when a user entry lacks both username and email, the current message doesn't identify which entry in the array caused the issue, making debugging hard for large imports.
Proposed fix
- if 'username' not in user_entry and\
- 'email' not in user_entry:
- print(f"Skipping user: missing 'username' or 'email'")
+ if 'username' not in user_entry and\
+ 'email' not in user_entry:
+ print("Skipping user entry: missing 'username' "
+ "or 'email'")🧰 Tools
🪛 Ruff (0.15.1)
[error] 242-242: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/setup.py` around lines 240 - 244, The print statement in the user import
block (where user_entry is validated) uses an f-string with no placeholders and
doesn't identify which entry failed; update the message to include context
(e.g., the user_entry contents or its index) so failures are debuggable and to
satisfy Ruff F541. Locate the validation around the user_entry variable in
web/setup.py (the block that checks 'username' and 'email') and change the print
to include either the current index from an enumerate or the user_entry dict (or
both) in the output, and keep incrementing error_count and continue as before.
| uid = ManageUsers.get_user( | ||
| username=user_data['username'], | ||
| auth_source=auth_source) | ||
| if uid: | ||
| print(f"Skipping user '{user_data['username']}': " | ||
| f"already exists") | ||
| skipped_count += 1 | ||
| continue | ||
|
|
||
| # Get role ID | ||
| rid = ManageRoles.get_role(user_data['role']) | ||
| if rid is None: | ||
| print(f"Skipping user '{user_data['username']}': " | ||
| f"role '{user_data['role']}' does not exist") | ||
| error_count += 1 | ||
| continue |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.py" | xargs grep -l "def get_user" | head -5Repository: pgadmin-org/pgadmin4
Length of output: 452
🏁 Script executed:
find . -type f -name "*.py" | xargs grep -l "def get_role" | head -5Repository: pgadmin-org/pgadmin4
Length of output: 143
🏁 Script executed:
cat -n web/setup.py | sed -n '200,315p'Repository: pgadmin-org/pgadmin4
Length of output: 5715
🏁 Script executed:
cat -n web/setup.py | grep -A 20 "def get_user"Repository: pgadmin-org/pgadmin4
Length of output: 3337
🏁 Script executed:
cat -n web/setup.py | grep -A 20 "def get_role"Repository: pgadmin-org/pgadmin4
Length of output: 763
🏁 Script executed:
# Check the json parameter usage
cat -n web/setup.py | sed -n '166,200p'Repository: pgadmin-org/pgadmin4
Length of output: 1395
🏁 Script executed:
cat -n web/setup.py | sed -n '166,320p' | grep -E "(json|_handle_error)"Repository: pgadmin-org/pgadmin4
Length of output: 444
🏁 Script executed:
# Find _handle_error definition
cat -n web/setup.py | grep -A 10 "def _handle_error"Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
# Search for _handle_error definition more broadly
rg "def _handle_error" web/setup.py -A 5Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
# Check if json parameter is actually used anywhere in load_users
rg "json" web/setup.py | grep -i "load_users\|def load_users" -A 30Repository: pgadmin-org/pgadmin4
Length of output: 46
🏁 Script executed:
# Search for _handle_error in the entire file
grep -n "_handle_error" web/setup.py | head -10Repository: pgadmin-org/pgadmin4
Length of output: 490
🏁 Script executed:
# Look for the definition of _handle_error
grep -B 2 -A 8 "_handle_error" web/setup.py | head -40Repository: pgadmin-org/pgadmin4
Length of output: 1620
🏁 Script executed:
# Check how json parameter is used in the entire ManageUsers class
sed -n '166,320p' web/setup.py | cat -nRepository: pgadmin-org/pgadmin4
Length of output: 7212
🏁 Script executed:
# Find the INTERNAL, LDAP etc. constants
rg "INTERNAL\s*=" web/pgadmin/utils/constants.pyRepository: pgadmin-org/pgadmin4
Length of output: 85
🏁 Script executed:
# Also check what _handle_error does
rg "def _handle_error" web/pgadmin -A 8Repository: pgadmin-org/pgadmin4
Length of output: 551
Creating a new Flask app per user inside the loop is very expensive.
ManageUsers.get_user() (line 270) and ManageRoles.get_role() (line 280) each internally call create_app() and open their own test_request_context. Since load_users already has an app context at line 236, these calls create 2×N redundant Flask application instances for N users. This will make bulk import extremely slow.
Query the User and Role models directly within the existing app context instead.
Additionally:
- Line 242: f-string without placeholders (
f"Skipping user: missing...") should be a regular string - Line 291: hardcoded
6should useconfig.PASSWORD_LENGTH_MIN - Line 210:
open(file_path)should specify encoding - Lines 205–206: error is printed twice (once explicitly, then by
_handle_error) jsonparameter (line 170) is unused
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/setup.py` around lines 270 - 285, Replace calls to ManageUsers.get_user
and ManageRoles.get_role inside load_users with direct ORM queries against the
User and Role models using the existing app context (avoid calling
create_app()/test_request_context inside ManageUsers/ManageRoles to eliminate
per-user app creation); also change the f-string "f\"Skipping user:
missing...\"" to a plain string, replace the hardcoded password min length 6
with config.PASSWORD_LENGTH_MIN, open files with explicit encoding (e.g.
open(file_path, encoding="utf-8")), remove the redundant explicit print of
errors so only _handle_error prints/logs them, and remove the unused json
parameter from the function signature and any callers. Ensure you reference
ManageUsers.get_user and ManageRoles.get_role to locate the affected calls and
update load_users and the function definition that has the json parameter.
1 participant
Summary by CodeRabbit
New Features
Documentation