Fix issue with Username being required for Shared server import config#9971
Fix issue with Username being required for Shared server import config#9971KijongHan wants to merge 3 commits into
Conversation
validate_json_data required Username for all non-Service connections, rejecting legitimate JSON that only carried SharedUsername. Accept either attribute when Shared is true; keep Username required for non-shared servers. Extract is_shared as a local while we're here — it's now referenced twice in the loop body. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…d servers When loading database servers from an import file, fall back to SharedUsername for the underlying server.username when the server is shared and no explicit Username is provided. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughValidation and loader functions in web/pgadmin/utils/init.py now derive an is_shared flag from input and compute an effective username (using SharedUsername when appropriate). Validation enforces username requirements conditionally for shared vs non-shared servers; the loader applies computed shared and username values to the server objects. ChangesShared Server Configuration and Validation
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
|
||
| is_shared = obj.get("Shared", None) | ||
| username = obj.get("Username", None) | ||
| if is_shared and not username: |
There was a problem hiding this comment.
Hi @KijongHan , In case where username is a empty string in json data, it will fall back to SharedUsername. If SharedUsername is also missing, the server imports silently with username = None. We can use username is None to only trigger the fallback when the key is genuinely absent.
I was able to reproduce this with following json
{
"Servers": {
"1": {
"Group": "Servers",
"Name": "Test",
"Shared": true,
"Username": "",
"Host": "127.0.0.1",
"Port": 5432,
"MaintenanceDB": "postgres"
}
}
}
There was a problem hiding this comment.
Hey @hiteshjambhale thanks alot for checking this use case! I share your concern around an empty username string being considered a valid import, but I'm thinking that we should short-circuit the import process further up the validation chain (during JSON validation) here that way we handle the validation in the right place
From what I can see, an empty string username being considered valid has been there for some time, so I want to check with you @asheshv or @akshay-joshi and get your opinion on whether we should tighten the validation logic on the username property and not allow empty strings
There was a problem hiding this comment.
Pull request overview
This PR updates pgAdmin’s servers.json import path to correctly handle shared servers by loosening validation so shared server definitions can provide SharedUsername instead of always requiring Username, and by applying a fallback so imported shared servers have an effective username consistent with the UI flow.
Changes:
- Adjusted
servers.jsonvalidation to accept eitherUsernameorSharedUsernamefor shared servers. - Updated server import to populate
Server.usernamefromSharedUsernamewhenUsernameis not provided for shared servers. - Minor refactor to reuse computed
is_sharedduring validation/import.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| is_shared = obj.get("Shared", None) | ||
| username = obj.get("Username", None) | ||
| if is_shared and not username: | ||
| username = obj.get("SharedUsername", None) | ||
|
|
There was a problem hiding this comment.
the import currently prefers Username over SharedUsername when both are present
I think this Username override makes sense, so that SharedUsername is only the default value (this is how it is currently treated in the UI creation stage as well at the moment)
and it does not populate shared_username when only legacy Username is provided
I dont think we should be treating shared_username property value like a fallback. It should be "pure". This feels aligned to how username is used as a network login (so fallback seems reasonable) but shared_username is treated as the "default username for these shared servers" so it doesn't make sense to me to introduce a fallback here. Keen for your thoughts @asheshv
Resolves #9226
What does this PR do?
This PR resolves
Username attribute not found for server 'shared_server'errors where server json import validation logic had a hard constraint onUsernamebeing required even whenSharedwas set to true. This PR introduces more coherent constraint validation logic for shared servers so that you are required to either provideSharedUsernameorUsername(for backwards compatibility). Furthermore, the underlying data model 'Username' property is set to 'SharedUsername' (for shared servers only) to align with UI behaviour for setting this fallback.The UX also feels better as when after registering shared servers with only the
SharedUsernameset, when trying to connect to the server for the first time it prompts for the password (instead of showing an infinite loading spinner because the underlyingUsernameis not set)Summary of changes
add separate
Usernameconstraint validation logic for shared servers when importingservers.jsonadd fallback to set
UsernametoSharedUsernamefor shared servers so that the UX flow matches the manual UI creation flow for registering new shared serversSummary by CodeRabbit