Feat: Add configurable discovery - #18
Conversation
|
Requires Note to self: once |
425fe39 to
43946b9
Compare
ClassicMMT
changed the title
2ae01e0 to
d69ef28
Compare
5bb10f2 to
5acaca4
Compare
- Add support for datamasque-python 1.2.1 - Add a status command to rulesets, libraries, discover configs, and discover libraries - Refuse YAML of 60 KiB+ in rulesets/discover configs validate - Rework discover libraries for updated library model - Fix rulesets generate and connections update --password - Drop click dependency - Update changelog - Report server reason when library deletion is rejected
- Add validation to discover configs/libraries - Abort on empty YAML directly - Abort with not-found when a run has no discovery output
67b4915 to
fdb5852
Compare
|
I like this idea of However it may be easy to forget to add it in specific CLI commands - claude has found a few above. Preferably we would use these new Could we create a top-level safety net, does something like the following work? (claude) def main() -> None:
try:
app()
except DataMasqueApiError as exc:
abort_api_error("Request failed", exc)
except DataMasqueTransportError as exc:
abort(str(exc), code=ErrorCode.TRANSPORT_ERROR)Apart from consistently using |
- improve naming - refactor some test functions - replace raise errors with abort_api_error - reuse helpers to create configs and libraries in the integration tests - show a proper error when a run has no file report - name the sync-validation limit and its guard function accurately - divide the discovery integration tests into two files - read/write every file in utf-8 - add exit code 10 when a confirmation prompt is declined - assert a specific exit code in every test - declare pydantic rather than rely on dm-python - bump the version to 1.5.0
- also merge abort_api_errors into one
|
@kw-datamasque Thanks for your review! The suggestions were great and I've implemented all of them and some more. |
- Report the server reason when ruleset validation fails
kw-datamasque
left a comment
kw-datamasque
left a comment
There was a problem hiding this comment.
I roleplayed a tester and had Claude test the modified CLI surface to see if it received reasonable outputs.
It found 6 of the 14 comments below (2 pre-existing), marked with (Claude) followed by Input, Expected, Reason, Suggestion for reproducibility.
I've placed Suggestions to most comments hopefully these are easy fixes. The other comments are mostly tidy ups.
| except DataMasqueApiError as exc: | ||
| abort_api_error(f"Failed to start schema discovery on '{connection}'", exc) |
There was a problem hiding this comment.
Starting a run with an invalid discovery config exits with a generic error instead of invalid_input.
(Claude)
Input:
$ printf 'version: "1.0"\nbanana: true\n' > bad.yaml
$ dm discover configs create --name bad-config --type database -f bad.yaml # exit 0, stored invalid
$ dm discover schema <any-db-connection> --config bad-configExpected: this JSON envelope on stderr, and shell exit status $? = 4:
{"error": {"code": "invalid_input", "message": "Schema discovery run failed to start due to discovery config error: Failed to parse configuration. Unknown property `banana`, `version`."}}
What we got (live at cffa2c1): same envelope shape, but the catch-all code and $? = 1:
{"error": {"code": "error", "message": "Failed to start schema discovery on 'db_db2': Schema discovery run failed to start due to discovery config error: Failed to parse configuration. Unknown property `banana`, `version`."}}
Reason: dm-python raises DiscoveryConfigNotFoundError and InvalidDiscoveryConfigError when a 400 body cites the discovery config; both subclass DataMasqueApiError via FailedToStartError (dm-python client/exceptions.py:44,53), so this generic except catches them and abort_api_error's default map only knows 409. A typo'd config name and invalid config YAML therefore both collapse into the catch-all bucket, and error.code / $? is what scripts branch on. Same at the file command below; the two types also need importing at line 11.
Suggestion:
| except DataMasqueApiError as exc: | |
| abort_api_error(f"Failed to start schema discovery on '{connection}'", exc) | |
| except DiscoveryConfigNotFoundError as exc: | |
| abort(str(exc), code=ErrorCode.NOT_FOUND) | |
| except InvalidDiscoveryConfigError as exc: | |
| abort(str(exc), code=ErrorCode.INVALID_INPUT) | |
| except DataMasqueApiError as exc: | |
| abort_api_error(f"Failed to start schema discovery on '{connection}'", exc) |
| missing_statuses: tuple[HTTPStatus, ...] = (HTTPStatus.NOT_FOUND,), | ||
| ) -> None: | ||
| """Turn the error a run without `output_label` returns into a not-found envelope.""" | ||
| if exc.response is None or exc.response.status_code not in missing_statuses: |
There was a problem hiding this comment.
exc.response can't be None since it's a required keyword on DataMasqueApiError
| if exc.response is None or exc.response.status_code not in missing_statuses: | |
| if exc.response.status_code not in missing_statuses: |
| f"Once finished, list results with: dm discover schema-results {run_id}" | ||
| ) | ||
| if should_emit_json(is_json): | ||
| print_json({"id": int(run_id), "status": "queued"}) |
There was a problem hiding this comment.
Not sure about outputting "status": "queued". The start endpoints return only a run id, so we should do the same. DM CLI should act as a wrapper over DM Python's (and therefore DataMasque's) behaviour. The same for the file command below.
| print_json({"id": int(run_id), "status": "queued"}) | |
| print_json({"id": int(run_id)}) |
(and update the two README examples at README.md:221,225, which document the status key)
| except DataMasqueApiError as exc: | ||
| _abort_if_run_output_missing(exc, run_id, "file discovery report") | ||
| abort_api_error(f"Failed to download file discovery report for run {run_id}", exc) | ||
| serialised_report = [result.model_dump(mode="json") for result in report] |
There was a problem hiding this comment.
(Note: Pre-existing behaviour - so we don't need to deal as part of this PR - in which case we can file an issue later)
dm discover file-report returns empty stderr and an empty-list stdout on failed, running, and non-file runs.
(Claude)
Input: dm discover file-report <run-id> where the run failed, is still running, or is not a file-discovery run:
$ dm discover file <connection> --json # {"id": 2506}
$ dm run status 2506 # "running"
$ dm discover file-report 2506Expected: empty stdout, this envelope on stderr, and $? = 3 - the message the 404 handler above is built to produce:
{"error": {"code": "not_found", "message": "No file discovery report available for run 2506.", "hint": "Discovery output is written once the run reaches a final state. Check status with `dm run status 2506`."}}
What we got (live at cffa2c1): stdout [], stderr empty, $? = 0 - while the run was running (2506), again after it reached failed, and likewise for a database run (1866).
Reason: the server answers 200 with an empty list for unfinished, failed, and wrong-type runs alike, so the 404 handling above may never fire at all, and "run never scanned anything" renders identically to "finished and found nothing". An agent scripting file-report after run start reads that as "nothing sensitive found".
Suggestion: when the report is empty, check the run's status and reuse the "written once the run reaches a final state" hint from _abort_if_run_output_missing for non-finished runs (and consider a distinct message for non-file runs).
| client = get_client(profile) | ||
| match = _collapse_to_one_or_abort(_find_by_name(client, name, config_type), name) | ||
|
|
||
| assert match.id is not None |
There was a problem hiding this comment.
We should not be using assert in CLI code (only in tests) since a failure is a raw traceback plus exit 1 and sidesteps the JSON-error contract every other failure path uses.
assert is used in a few other places, please replace with the relevant error handling abort..() logic.
| assert match.id is not None | |
| if match.id is None: | |
| abort(f"Server returned discovery config '{name}' without an id.", code=ErrorCode.ERROR) |
| # --- status ------------------------------------------------------------------ | ||
|
|
||
|
|
||
| def test_config_status_reports_valid( |
There was a problem hiding this comment.
test_config_status_reports_valid and test_config_status_reports_invalid are the same test twice , and test_library_status just below already shows the parametrized shape.
Suggestion: fold test_config_status_reports_valid and test_config_status_reports_invalid into one parametrized test like test_library_status.
| ) -> None: | ||
| """Create or update a ruleset library from a YAML file.""" | ||
| yaml_content = file.read_text() | ||
| yaml_content = read_text_or_abort(file, FileKind.RULESET_LIBRARY) |
There was a problem hiding this comment.
Not sure about passing FileKind into every read_text_or_abort(). Every time we read a file we must now pass it in, benefiting only the error message:
ruleset library file my_ruleset_library.yaml is not valid UTF-8.
instead of
my_ruleset_library.yaml is not valid UTF-8
Downsides:
- (i) every future function that reads a file needs to pass in the kind of file, more code to read and maintain for not much gain since the caller of the API e.g.
dm libraries createwill clearly know the type of file they are reading in anyway. - (ii) if the wrong
FileKindif supplied will become a bug and confuse a user.
Suggestion: What do you think about removing _format_file_label() from read_text_or_abort()? This is non-blocking and I will leave it up to you, depending on whether you agree with the above. If you do that change, apply to read_json_object_or_abort() as well, and any other functions that have it.
| _abort_if_run_output_missing( | ||
| exc, run_id, "schema discovery results", (HTTPStatus.NOT_FOUND, HTTPStatus.BAD_REQUEST) | ||
| ) | ||
| abort_api_error(f"Failed to list schema discovery results for run {run_id}", exc) |
There was a problem hiding this comment.
dm discover schema-results on non-schema runs drops the server's reason and tells the user to poll for a result that can never arrive.
(Claude)
Input: dm discover schema-results <run-id> where the run is not a schema discovery run. Locally:
$ dm run status 1868 # a *finished* file-discovery run
$ dm discover schema-results 1868Expected: the server's reason in the message and no poll hint (it can never come true here). With the suggestion below, envelope on stderr and $? = 4:
{"error": {"code": "invalid_input", "message": "Failed to list schema discovery results for run 1868: Schema discovery has not been run on this connection."}}
(A genuine 404, a schema run with no results yet, keeps not_found, $? = 3, and the poll hint.)
What we got (live at cffa2c1): $? = 3, the reason dropped, and a hint that polling will fix it:
{"error": {"code": "not_found", "message": "No schema discovery results available for run 1868.", "hint": "Discovery output is written once the run reaches a final state. Check status with `dm run status 1868`."}}
The server actually said Schema discovery has not been run on this connection. - visible only in the stray client log line.
Reason: folding BAD_REQUEST into the not-found path guesses the cause and discards the response body via _abort_if_run_output_missing: a 400 also covers "this is not a schema discovery run", where no amount of polling produces results. Dropping 400 from the tuple lets abort_api_error surface the server's reason, and the status_codes entry keeps it out of the generic exit-1 bucket.
Suggestion:
| _abort_if_run_output_missing( | |
| exc, run_id, "schema discovery results", (HTTPStatus.NOT_FOUND, HTTPStatus.BAD_REQUEST) | |
| ) | |
| abort_api_error(f"Failed to list schema discovery results for run {run_id}", exc) | |
| _abort_if_run_output_missing(exc, run_id, "schema discovery results") | |
| abort_api_error( | |
| f"Failed to list schema discovery results for run {run_id}", | |
| exc, | |
| status_codes={HTTPStatus.BAD_REQUEST: ErrorCode.INVALID_INPUT}, | |
| ) |
| def main() -> None: | ||
| try: | ||
| app() |
There was a problem hiding this comment.
(Note: Pre-existing behaviour - so we don't need to deal as part of this PR - in which case we can file an issue later)
Every failed request prints the raw response body to stderr before our error JSON-envelope.
(Claude)
Input: any command whose request fails with a non-2xx, e.g.
$ dm run status 999999Expected: stderr carries exactly one line, the envelope (exit status unchanged):
{"error": {"code": "error", "message": "Request failed: The requested run was not found."}}
What we got (live at cffa2c1): the client's log line lands on stderr before every envelope:
Error when calling API: {'detail': 'The requested run was not found.'}
{"error": {"code": "error", "message": "Request failed: The requested run was not found."}}
The body here is small JSON, but the line prints whatever comes back: a server 500 dumps its entire error page onto stderr (hundreds of KB).
Reason: dm-python's _raise_for_status logs the full response body at ERROR on every non-2xx, and the CLI configures no logging, so Python's last-resort handler prints it to stderr before our one-line abort. The abort helpers fixed the traceback half of the 500 problem but not the logging half.
Suggestion: (plus import logging with the stdlib imports; and a test asserting a non-2xx body never reaches stderr)
| def main() -> None: | |
| try: | |
| app() | |
| def main() -> None: | |
| logging.getLogger("datamasque").addHandler(logging.NullHandler()) | |
| logging.getLogger("datamasque").propagate = False | |
| try: | |
| app() |
| options: list[OptionEntry | ArgumentEntry] = [] | ||
| for param in cmd.params: | ||
| if isinstance(param, click.Option): | ||
| if param.param_type_name == "option": |
There was a problem hiding this comment.
Nit:
Not sure if this is worth spending much time on (focus on the other comments first). Is there a way to avoid the raw string check? Previously we checked isinstance(param, click.Option) which was well typed.
2 participants
No description provided.