fix(cli): prevent adb config from crashing or creating empty configs#679
fix(cli): prevent adb config from crashing or creating empty configs#679ad-claw000 wants to merge 32 commits into
Conversation
Release 0.4.56
Release 0.4.57
Release 0.4.58
Closes #370 - Fixed `adb config create` crashing on NoneType when no name is provided in non-interactive mode. - Fixed `adb config activate` writing an empty dictionary if the config file does not exist, which previously led to an empty config file that corrupted future executions with KeyError. - Updated `get_configurations` to safely retrieve 'active' key.
fdac0e2 to
53883e2
Compare
ad-claw000
left a comment
There was a problem hiding this comment.
Applied autopep8 formatting to the new log messages to fix the pre-commit CI failure.
There was a problem hiding this comment.
Pull request overview
This PR aims to harden the adb config CLI workflow to avoid crashes and config-file corruption (as described in #370), primarily by making config reading/writing more defensive when config files are missing or malformed. It also includes a number of formatting-only changes, updates the test container setup to use ephemeral host ports, and adds a new unit test module for aperturedb.Images.
Changes:
- Make
adb confighandling more resilient to missing/corrupted config files (e.g., avoidKeyErroron missing"active", prevent writing empty configs on activation failures, and improve non-interactivecreatevalidation). - Update test docker-compose + runner script to use random host ports and discover them via
docker compose port. - Add
test/test_Images.pyand apply formatting changes across tests/examples/library code.
Reviewed changes
Copilot reviewed 17 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
aperturedb/cli/configure.py |
Implements the CLI robustness fixes for create/activate/get_key and makes config parsing more defensive. |
test/run_test_container.sh |
Discovers runtime ports from docker-compose and returns GATEWAY:PORT for the started instance. |
test/docker-compose.yml |
Switches to ephemeral host port mappings (0) for lenz/nginx services. |
test/test_Images.py |
Adds unit tests for rotate/resolve and basic Images behaviors using mocks. |
test/test_Stats.py |
Formatting-only change to assertion style. |
test/test_Server.py |
Formatting-only change to function call and assertion formatting. |
test/test_Datawizard.py |
Formatting-only f-string spacing change. |
test/conftest.py |
Formatting-only change to long error message construction. |
aperturedb/Utils.py |
Refactors long HTML string building for readability; minor formatting change in percentage calculation. |
aperturedb/Query.py |
Adjusts time formatting logic and formatting style. |
aperturedb/ParallelQuerySet.py |
Splits long log messages for readability. |
aperturedb/ParallelQuery.py |
Splits long log message for readability. |
aperturedb/Descriptors.py |
Docstring clarifications and parameter documentation updates. |
aperturedb/CSVWriter.py |
Improves assertion message formatting for readability. |
aperturedb/CommonLibrary.py |
Improves assertion formatting/readability for required configuration fields. |
examples/loading_with_models/get_tl_embeddings.py |
Formatting-only f-string spacing change. |
examples/CelebADataKaggle.py |
Splits a long f-string assignment for readability. |
.github/workflows/pr.yaml |
Extends cleanup step to remove prior test logs as well as db artifacts. |
.github/workflows/checks.yml |
Bumps pre-commit workflow Python version to 3.12. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Addressed the review comments:
|
|
I will address the 4 unresolved review comments:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
aperturedb/cli/configure.py:132
get_active_configcan still crash with aKeyErrorwhenactiveis set but the referenced config name is missing from bothall_configs["local"]andall_configs["global"], or when"global"is not present. Consider explicitly checking for membership in both scopes and exiting with a helpful message (similar to thenot activecase) instead of indexing directly.
if active.startswith("env:"):
return all_configs["environment"][active[4:]]
else:
return all_configs["local"][active] if "local" in all_configs and active in all_configs["local"] \
else all_configs["global"][active]
aperturedb/cli/configure.py:447
- In the global-config fallback, this mutates
gcby unconditionally settingtarget_configs["active"] = gaand may write it back as JSONnullif the global config file was missing anactivekey. Consider avoiding writing theactivefield here unless you’re intentionally changing it (e.g., don’t set it whengais falsy, or work on a copy) soadb config get-keydoesn’t accidentally alter activation state while just adding a user key.
target_configs = configs
target_path = config_path
if name not in configs and name in gc:
target_configs = gc
target_configs["active"] = ga
target_path = global_config_path
if user is None:
key_user = target_configs[name].username
else:
key_user = user
if target_configs[name].has_user_keys():
user_key = target_configs[name].get_user_key(key_user)
if user_key is None:
conn = __create_connector(target_configs[name])
user_key = keys.generate_user_key(conn, key_user)
target_configs[name].add_user_key(key_user, user_key)
_write_config(target_path, target_configs)
- Raised typer.Exit(code=2) in get_key exception handlers (FileNotFoundError, JSONDecodeError) to ensure a non-zero exit code when config files are corrupted or missing. - Reverted CLIP time range formatting in Query.get_specific to use f-strings instead of .format(), as requested by reviewer to move this refactor to a separate PR.
|
Addressed the remaining actionable review comments:
See commit d0eb7f9. |
…-create-empty-file
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
test/test_SPARQL.py:66
- After removing the
pytest_collection_modifyitemshook, this module no longer applies theexternal_networkmarker to the SPARQL tests/fixtures. That means CI can’t reliably filter these network-dependent tests out (and they may run unexpectedly). Add an explicit marker (e.g.,pytestmark = pytest.mark.external_networkat module scope or@pytest.mark.external_networkontest_sparql/ theload_cookbookfixture) so the marker is always present.
# Test functions that depends on the setup
@pytest.fixture
def sparql(db):
sparql = SPARQL(db)
Closes #370.
This PR fixes multiple issues reported in #370 that caused
adb configto break:adb config createcrashed onNoneTypewhen no name was provided in non-interactive mode.adb config activatewrote an empty dictionary{}if the config file didn't exist, which resulted in a broken JSON structure that corrupted future executions with aKeyError: 'active'.get_configurationsto use.get('active')to fail gracefully if the config file was already corrupted by the above bug.Additionally:
external_networkmarker to the SPARQL test to replace a problematic pytest collection hook, ensuring cloud tests can be properly filtered out in CI environments lacking credentials.RangeType.TIMEformatting logic to correctly use modulo 60 for minutes, preventing cumulative minute totals.