Allow setup_credentials to work with selected dbs#206
Allow setup_credentials to work with selected dbs#206pSpitzner wants to merge 3 commits intoCorrelAid:mainfrom
setup_credentials to work with selected dbs#206Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pystatis/config.py`:
- Around line 156-166: Validate all entries in db_names against
get_supported_db() before mutating the global config: iterate over db_names and
collect any invalid names (using get_supported_db() as the whitelist) and raise
a KeyError if any invalids are found, then only after that validation update
config. Update the logic around the db_names handling (the block referencing
db_names and get_supported_db()) so no partial updates to config occur when one
or more entries are invalid.
In `@tests/test_config.py`:
- Around line 86-96: The test test_setup_credentials_with_db_names_subset can
falsely pass if other DB envvars remain set; update the test so before calling
config.setup_credentials(db_names=["genesis"]) you explicitly clear or unset the
other DB environment variables (e.g. PYSTATIS_ZENSUS_* and PYSTATIS_REGIO_*) or
after calling config.setup_credentials assert that config_ entries for the
non-requested DBs (e.g. "zensus", "regio") have no username/password set; target
the test function test_setup_credentials_with_db_names_subset and the call to
config.setup_credentials to ensure only "genesis" is populated and other DB
sections remain empty.
- Around line 99-103: The test test_setup_credentials_invalid_credentials_raises
currently uses db_names=["regio_s"] which triggers the unsupported-name guard in
config.setup_credentials and never calls db.check_credentials_are_valid; update
the test to target the invalid-credentials branch by using a supported DB name
like "regio" so the mocker.patch.object(db, "check_credentials_are_valid",
return_value=False) is exercised, and assert the function raises the appropriate
exception (config.PystatisConfigError) instead of KeyError; keep the existing
unsupported-name scenario as a separate test asserting KeyError for "regio_s".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: afd901ef-7bb3-4158-8d3e-014ed8a10825
📒 Files selected for processing (2)
src/pystatis/config.pytests/test_config.py
| def test_setup_credentials_with_db_names_subset(mocker, config_): | ||
| mocker.patch.object(db, "check_credentials_are_valid", return_value=True) | ||
|
|
||
| os.environ["PYSTATIS_GENESIS_API_USERNAME"] = "test" | ||
| os.environ["PYSTATIS_GENESIS_API_PASSWORD"] = "test123!" | ||
|
|
||
| config.setup_credentials(db_names=["genesis"]) | ||
|
|
||
| # Only the specified DB should be set | ||
| assert config_["genesis"]["username"] == "test" | ||
| assert config_["genesis"]["password"] == "test123!" |
There was a problem hiding this comment.
This subset test can still pass if all DBs are configured.
test_setup_credentials above leaves PYSTATIS_ZENSUS_* and PYSTATIS_REGIO_* in os.environ, and this test never asserts those sections stay empty. If setup_credentials(db_names=["genesis"]) regresses to iterating all supported DBs, this test can still go green.
Proposed fix
def test_setup_credentials_with_db_names_subset(mocker, config_):
mocker.patch.object(db, "check_credentials_are_valid", return_value=True)
os.environ["PYSTATIS_GENESIS_API_USERNAME"] = "test"
os.environ["PYSTATIS_GENESIS_API_PASSWORD"] = "test123!"
+ os.environ.pop("PYSTATIS_ZENSUS_API_USERNAME", None)
+ os.environ.pop("PYSTATIS_ZENSUS_API_PASSWORD", None)
+ os.environ.pop("PYSTATIS_REGIO_API_USERNAME", None)
+ os.environ.pop("PYSTATIS_REGIO_API_PASSWORD", None)
config.setup_credentials(db_names=["genesis"])
- # Only the specified DB should be set
assert config_["genesis"]["username"] == "test"
assert config_["genesis"]["password"] == "test123!"
+ assert config_["zensus"]["username"] == ""
+ assert config_["zensus"]["password"] == ""
+ assert config_["regio"]["username"] == ""
+ assert config_["regio"]["password"] == ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_config.py` around lines 86 - 96, The test
test_setup_credentials_with_db_names_subset can falsely pass if other DB envvars
remain set; update the test so before calling
config.setup_credentials(db_names=["genesis"]) you explicitly clear or unset the
other DB environment variables (e.g. PYSTATIS_ZENSUS_* and PYSTATIS_REGIO_*) or
after calling config.setup_credentials assert that config_ entries for the
non-requested DBs (e.g. "zensus", "regio") have no username/password set; target
the test function test_setup_credentials_with_db_names_subset and the call to
config.setup_credentials to ensure only "genesis" is populated and other DB
sections remain empty.
| def test_setup_credentials_invalid_credentials_raises(mocker, config_): | ||
| mocker.patch.object(db, "check_credentials_are_valid", return_value=False) | ||
|
|
||
| with pytest.raises(KeyError) as _exc_info: | ||
| config.setup_credentials(db_names=["regio_s"]) |
There was a problem hiding this comment.
This test never reaches the invalid-credentials branch.
With db_names=["regio_s"], the new guard in src/pystatis/config.py:162-166 raises KeyError before Line 169 can call db.check_credentials_are_valid, so the mock on Line 100 is unused. This currently tests “unsupported DB name”, not “invalid credentials”.
Suggested direction
-def test_setup_credentials_invalid_credentials_raises(mocker, config_):
- mocker.patch.object(db, "check_credentials_are_valid", return_value=False)
-
- with pytest.raises(KeyError) as _exc_info:
- config.setup_credentials(db_names=["regio_s"])
+def test_setup_credentials_invalid_db_name_raises(config_):
+ with pytest.raises(KeyError):
+ config.setup_credentials(db_names=["regio_s"])Then add a separate test for the actual invalid-credentials path using a supported name such as "regio" and pytest.raises(config.PystatisConfigError).
📝 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.
| def test_setup_credentials_invalid_credentials_raises(mocker, config_): | |
| mocker.patch.object(db, "check_credentials_are_valid", return_value=False) | |
| with pytest.raises(KeyError) as _exc_info: | |
| config.setup_credentials(db_names=["regio_s"]) | |
| def test_setup_credentials_invalid_db_name_raises(config_): | |
| with pytest.raises(KeyError): | |
| config.setup_credentials(db_names=["regio_s"]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_config.py` around lines 99 - 103, The test
test_setup_credentials_invalid_credentials_raises currently uses
db_names=["regio_s"] which triggers the unsupported-name guard in
config.setup_credentials and never calls db.check_credentials_are_valid; update
the test to target the invalid-credentials branch by using a supported DB name
like "regio" so the mocker.patch.object(db, "check_credentials_are_valid",
return_value=False) is exercised, and assert the function raises the appropriate
exception (config.PystatisConfigError) instead of KeyError; keep the existing
unsupported-name scenario as a separate test asserting KeyError for "regio_s".
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/pystatis/config.py (1)
161-171:⚠️ Potential issue | 🟠 MajorValidate the full
db_namesselection before the loop.With the membership check inside the iteration,
setup_credentials(db_names=["genesis", "bogus"])can already mutate thegenesissection before the laterKeyError, leaving partially updated in-memory state. Collect invalid names first and raise before anyconfig.set(...)call.Suggested fix
def setup_credentials(db_names: list[str] | None = None) -> None: - """ - Setup credentials for all supported databases. + """ + Setup credentials for the selected supported databases. Args: - db_names (list[str]): Names of the databases to setup + db_names (list[str] | None): Names of the databases to set up. + If None, all supported databases are configured. """ if db_names is None: db_names = get_supported_db() - for db_name in db_names: - # despite this check, we should consider using literals as the type hint for - # db_names - if db_name not in get_supported_db(): - raise KeyError( - f"Provided db_name '{db_name}' no regnized. " - f"Valid options are {get_supported_db()}" - ) + supported_db = get_supported_db() + invalid_db_names = [db_name for db_name in db_names if db_name not in supported_db] + if invalid_db_names: + raise KeyError( + f"Provided db_name(s) {invalid_db_names} not recognized. " + f"Valid options are {supported_db}" + ) + + for db_name in db_names: config.set(db_name, "username", _get_user_input(db_name, "username")) config.set(db_name, "password", _get_user_input(db_name, "password"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pystatis/config.py` around lines 161 - 171, The loop currently validates each db_name as it iterates which allows partial mutation of state; update setup_credentials to validate the entire db_names list up front by computing invalid = [n for n in db_names if n not in get_supported_db()] and if invalid: raise KeyError listing them before any call to config.set(...) or other mutations; keep using the existing symbols db_names, get_supported_db, setup_credentials, and config.set so the remainder of the function can safely assume all names are valid.
🧹 Nitpick comments (1)
src/pystatis/config.py (1)
156-160: Update the docstring to match the new API.It still describes configuring all databases and documents
db_namesas if it were always provided, but this function now also supports subsets and defaults to all supported databases whenNone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pystatis/config.py` around lines 156 - 160, Update the function docstring for setup_db_credentials in src/pystatis/config.py to reflect the new API: state that the function accepts an optional db_names parameter (db_names: list[str] | None = None), document that when db_names is None the function will configure credentials for all supported databases, and describe that if a subset is provided only those databases are configured; update the Args section to mark db_names as optional and include the default value and behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/pystatis/config.py`:
- Around line 161-171: The loop currently validates each db_name as it iterates
which allows partial mutation of state; update setup_credentials to validate the
entire db_names list up front by computing invalid = [n for n in db_names if n
not in get_supported_db()] and if invalid: raise KeyError listing them before
any call to config.set(...) or other mutations; keep using the existing symbols
db_names, get_supported_db, setup_credentials, and config.set so the remainder
of the function can safely assume all names are valid.
---
Nitpick comments:
In `@src/pystatis/config.py`:
- Around line 156-160: Update the function docstring for setup_db_credentials in
src/pystatis/config.py to reflect the new API: state that the function accepts
an optional db_names parameter (db_names: list[str] | None = None), document
that when db_names is None the function will configure credentials for all
supported databases, and describe that if a subset is provided only those
databases are configured; update the Args section to mark db_names as optional
and include the default value and behavior.
|
Hi @pSpitzner and thanks a lot for the PR, I will review it asap! |
|
I would implement it a little differently. Its fine to pass a list of databases, but as this needs another check against the supported databases, I would simply add a user input in the loop to ask the user to skip the database, or enter the database they want to register for. THis way you do not need to know any supported databases and also do not need to pass in a list of databases. Anyway, its always a little nicer to use *db_names as parameter, this *args allows the user to pass single arguments instead of a list :) |
|
I have implemented the change in dev in this commit: b2ce457 |
|
Hi @pmayd, thanks for looking at this so quickly. One last question: Will the new approach you implemented allow me to skip the prompt, somehow? (Maybe I missed a function that does this already - sorry) In particular, if I have set 4 our of the 6 env vars, how could I proceed without user interaction, if I know that the Tables I want to download are coverd by the two accounts I have in the env vars? Thanks again! |
|
Hi @pSpitzner that is a valid request I might not have fully foreseen. My idea was simple: use I see now why you wanted to pass a list. Let me think about the best way to solve this. We can still support that param of course. Because its also not a good idea to parse env vars and skip the ones not found because maybe you want to register the missing one etc. So in my opinion default should always ask for all databases but you should be able to skip if you do not want any interaction. I see, your original solution did solve this by "accident" as I already programmed the input to be skipped in case of existing env var, now with my additional input, I basically took away that option. I will find a solution ;) |
|
@pSpitzner in the end I think your solution is the best here, I will still keep the dialog so normal users can skip the setup as well but for power users its best to have a db_names param here |
|
That sounds like a good solution! I like the prompt (but its good to have a way to skip it, if you want to wrap some code around it, and know what you are doing :P ) Let me know if i can help anyhow |
|
Nothing needed, I will update a new version along with all outstanding dependency updates later this evening ;) |
|
@pSpitzner I just release v0.5.5 so please test it out! |
2 participants
Hi @pmayd,
thanks for this awesome lib, saves us a bunch of time in a few places :)
This PR adds a kwarg to
setup_credentialsso that we can initialize credentials for a selection of sources.The use case is that user might want to get resources from a single service.
(Im working on a demo notebook, and found it hard to ask users to create three accounts, to download one example table)
Without this PR, if the user wants to use
setup_credentialsthis would require an account on all three services.Summary by CodeRabbit
New Features
Improvements
Tests