diff --git a/doc/changelog.rst b/doc/changelog.rst index b78c026..44593a6 100644 --- a/doc/changelog.rst +++ b/doc/changelog.rst @@ -16,6 +16,9 @@ Added Changed ^^^^^^^ +- The commands fail with the exit code 1 when the server answers an error, or a bulk response + with failed operations. The response is still displayed on stdout, and a summary of the error + is displayed on stderr. :issue:`2` - The errors of the server discovery give the URL of the server. Fixed diff --git a/doc/tutorial.rst b/doc/tutorial.rst index f1cca45..f1fb870 100644 --- a/doc/tutorial.rst +++ b/doc/tutorial.rst @@ -256,6 +256,29 @@ See the :doc:`scim2-tester documentation ` for more details SUCCESS check_access_invalid_resource_typ ... +Errors +------ + +When the server answers an error, the command displays it on stdout and fails with the exit code 1. +A summary of the error is displayed on stderr, so the output can still be read by another program. + +.. code-block:: console + :caption: Querying an user that does not exist. + + $ scim query user unknown-id + { + "schemas": [ + "urn:ietf:params:scim:api:messages:2.0:Error" + ], + "status": "404", + "detail": "Resource unknown-id not found" + } + Error: 404 Resource unknown-id not found + $ echo $? + 1 + +The :ref:`reference:bulk` command fails the same way when some operations of the bulk response failed. + JSON input ---------- diff --git a/scim2_cli/bulk.py b/scim2_cli/bulk.py index 82cc8c8..483b586 100644 --- a/scim2_cli/bulk.py +++ b/scim2_cli/bulk.py @@ -3,8 +3,8 @@ from scim2_cli.utils import DOC_URL from scim2_cli.utils import SCIM_EXCEPTIONS +from scim2_cli.utils import echo_response from scim2_cli.utils import exception_to_click_error -from scim2_cli.utils import formatted_payload from scim2_cli.utils import indent_option @@ -33,4 +33,4 @@ def bulk_cli(ctx: click.Context, indent: bool) -> None: except SCIM_EXCEPTIONS as scim_exc: raise exception_to_click_error(scim_exc) from scim_exc - click.echo(formatted_payload(response.model_dump(), indent)) + echo_response(response, indent) diff --git a/scim2_cli/create.py b/scim2_cli/create.py index a8cae36..34c39d3 100644 --- a/scim2_cli/create.py +++ b/scim2_cli/create.py @@ -15,9 +15,9 @@ from scim2_cli.utils import SCIM_EXCEPTIONS from scim2_cli.utils import ModelCommand from scim2_cli.utils import command_name +from scim2_cli.utils import echo_response from scim2_cli.utils import escape_options_help from scim2_cli.utils import exception_to_click_error -from scim2_cli.utils import formatted_payload from scim2_cli.utils import indent_option from scim2_cli.utils import inherited from scim2_cli.utils import me_option @@ -44,7 +44,7 @@ def create_payload( except SCIM_EXCEPTIONS as scim_exc: raise exception_to_click_error(scim_exc) from scim_exc - click.echo(formatted_payload(response.model_dump(), indent)) + echo_response(response, indent) def create_factory( diff --git a/scim2_cli/delete.py b/scim2_cli/delete.py index 0ab0648..44b2dcf 100644 --- a/scim2_cli/delete.py +++ b/scim2_cli/delete.py @@ -1,8 +1,6 @@ import click from click import ClickException from scim2_client import Me -from scim2_models import Message -from scim2_models import Resource from sphinx_click.rst_to_ansi_formatter import make_rst_to_ansi_formatter from scim2_cli.utils import exception_to_click_error @@ -11,7 +9,7 @@ from .utils import DOC_URL from .utils import SCIM_EXCEPTIONS -from .utils import formatted_payload +from .utils import echo_response @click.command(cls=make_rst_to_ansi_formatter(DOC_URL), name="delete") @@ -58,11 +56,4 @@ def delete_cli( except SCIM_EXCEPTIONS as scim_exc: raise exception_to_click_error(scim_exc) from scim_exc - if response: - payload = ( - response.model_dump() - if isinstance(response, Resource | Message) - else response - ) - payload = formatted_payload(payload, indent) - click.echo(payload) + echo_response(response, indent) diff --git a/scim2_cli/modify.py b/scim2_cli/modify.py index 1f1222e..802a542 100644 --- a/scim2_cli/modify.py +++ b/scim2_cli/modify.py @@ -18,10 +18,10 @@ from scim2_cli.utils import DOC_URL from scim2_cli.utils import SCIM_EXCEPTIONS +from scim2_cli.utils import echo_response from scim2_cli.utils import escape_control_characters from scim2_cli.utils import exception_to_click_error from scim2_cli.utils import find_target -from scim2_cli.utils import formatted_payload from scim2_cli.utils import indent_option from scim2_cli.utils import me_option @@ -206,5 +206,4 @@ def modify_cli( except SCIM_EXCEPTIONS as scim_exc: raise exception_to_click_error(scim_exc) from scim_exc - if response is not None: - click.echo(formatted_payload(response.model_dump(), indent)) + echo_response(response, indent) diff --git a/scim2_cli/query.py b/scim2_cli/query.py index ae718f4..70b35bc 100644 --- a/scim2_cli/query.py +++ b/scim2_cli/query.py @@ -17,7 +17,7 @@ from .utils import DOC_URL from .utils import SCIM_EXCEPTIONS -from .utils import formatted_payload +from .utils import echo_response DISCOVERY_MODELS = (Schema, ResourceType, ServiceProviderConfig) @@ -169,5 +169,4 @@ def query_cli( except SCIM_EXCEPTIONS as scim_exc: raise exception_to_click_error(scim_exc) from scim_exc - payload = formatted_payload(response.model_dump(), indent) - click.echo(payload) + echo_response(response, indent) diff --git a/scim2_cli/replace.py b/scim2_cli/replace.py index 0a991dd..755239d 100644 --- a/scim2_cli/replace.py +++ b/scim2_cli/replace.py @@ -21,7 +21,7 @@ from .utils import DOC_URL from .utils import SCIM_EXCEPTIONS from .utils import ModelCommand -from .utils import formatted_payload +from .utils import echo_response from .utils import renamed_fields from .utils import unacceptable_fields @@ -45,7 +45,7 @@ def replace_payload( except SCIM_EXCEPTIONS as scim_exc: raise exception_to_click_error(scim_exc) from scim_exc - click.echo(formatted_payload(response.model_dump(), indent)) + echo_response(response, indent) def replace_factory( diff --git a/scim2_cli/search.py b/scim2_cli/search.py index 063c1c4..a8e86f1 100644 --- a/scim2_cli/search.py +++ b/scim2_cli/search.py @@ -7,7 +7,7 @@ from .utils import DOC_URL from .utils import SCIM_EXCEPTIONS -from .utils import formatted_payload +from .utils import echo_response @click.command(cls=make_rst_to_ansi_formatter(DOC_URL), name="search") @@ -113,5 +113,4 @@ def search_cli( except SCIM_EXCEPTIONS as scim_exc: raise exception_to_click_error(scim_exc) from scim_exc - payload = formatted_payload(response.model_dump(), indent) - click.echo(payload) + echo_response(response, indent) diff --git a/scim2_cli/utils.py b/scim2_cli/utils.py index 770c8e0..7fcde82 100644 --- a/scim2_cli/utils.py +++ b/scim2_cli/utils.py @@ -12,7 +12,9 @@ from click.core import ParameterSource from scim2_client import SCIMClientException from scim2_models import BaseModel +from scim2_models import BulkResponse from scim2_models import Context +from scim2_models import Error from scim2_models import Mutability from scim2_models import Resource from scim2_models import ResourceType @@ -129,6 +131,30 @@ def formatted_payload(obj: Any, indent: bool) -> str: return json.dumps(obj, indent=INDENTATION_SIZE if indent else None) +def echo_response(response: Any, indent: bool) -> None: + """Display the response of the server, and exit with an error code if it reports a failure.""" + if response is None: + return + + click.echo(formatted_payload(response.model_dump(), indent)) + if isinstance(response, Error): + summary = " ".join( + str(part) for part in (response.status, response.detail) if part + ) + click.echo(f"Error: {escape_control_characters(summary)}", err=True) + click.get_current_context().exit(1) + + if isinstance(response, BulkResponse): + operations = response.operations or [] + failed = [op for op in operations if op.status and op.status >= 400] + if failed: + click.echo( + f"Error: {len(failed)} of {len(operations)} bulk operations failed", + err=True, + ) + click.get_current_context().exit(1) + + def split_headers(headers: list[str]) -> dict[str, str]: """Make a dict from header strings. diff --git a/tests/test_bulk.py b/tests/test_bulk.py index 38f36ec..85241da 100644 --- a/tests/test_bulk.py +++ b/tests/test_bulk.py @@ -109,7 +109,7 @@ def test_too_many_operations(invoke): def test_error_response(invoke, httpserver): - """The error the server answers is displayed.""" + """The error the server answers is displayed, and the command fails.""" httpserver.expect_oneshot_request("/Bulk", method="POST").respond_with_json( { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], @@ -122,4 +122,36 @@ def test_error_response(invoke, httpserver): result = invoke("bulk", input=json.dumps(BULK_REQUEST)) - assert json.loads(result.output)["status"] == "413" + assert result.exit_code == 1, result.output + assert json.loads(result.stdout)["status"] == "413" + assert result.stderr == ( + "Error: 413 The size of the bulk operation exceeds the maxPayloadSize\n" + ) + + +def test_failed_operations(invoke, httpserver): + """A bulk response with failed operations is displayed, and the command fails.""" + operations = [ + BULK_RESPONSE["Operations"][0], + { + "method": "POST", + "bulkId": "ytrewq", + "status": "409", + "response": { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "scimType": "uniqueness", + "status": "409", + "detail": "userName is already taken", + }, + }, + ] + httpserver.expect_oneshot_request("/Bulk", method="POST").respond_with_json( + {**BULK_RESPONSE, "Operations": operations}, + content_type="application/scim+json", + ) + + result = invoke("bulk", input=json.dumps(BULK_REQUEST)) + + assert result.exit_code == 1, result.output + assert len(json.loads(result.stdout)["Operations"]) == 2 + assert result.stderr == "Error: 1 of 2 bulk operations failed\n" diff --git a/tests/test_delete.py b/tests/test_delete.py index 011f1d3..4f2070f 100644 --- a/tests/test_delete.py +++ b/tests/test_delete.py @@ -72,7 +72,7 @@ def test_bad_resource_type(runner, httpserver): def test_not_found(runner, httpserver): - """Test pydantic errors handling.""" + """An error answered by the server is displayed, and the command fails.""" httpserver.expect_request( "/Users/unknown-id", method="DELETE", @@ -97,8 +97,12 @@ def test_not_found(runner, httpserver): ], catch_exceptions=False, ) - assert result.exit_code == 0, result.output - json_output = json.loads(result.output) + assert result.exit_code == 1, result.output + assert ( + result.stderr + == "Error: 404 Resource 2819c223-7f76-453a-919d-413861904646 not found\n" + ) + json_output = json.loads(result.stdout) assert json_output == { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], "detail": "Resource 2819c223-7f76-453a-919d-413861904646 not found", diff --git a/tests/test_errors.py b/tests/test_errors.py new file mode 100644 index 0000000..97eb103 --- /dev/null +++ b/tests/test_errors.py @@ -0,0 +1,73 @@ +import json + +import pytest + +from scim2_cli import cli + +ERROR = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status": "404", + "detail": "Resource 1 not found", +} + + +@pytest.mark.parametrize( + ("arguments", "method", "path"), + [ + (["query", "user", "1"], "GET", "/Users/1"), + (["query", "user"], "GET", "/Users"), + (["search", "user"], "POST", "/Users/.search"), + (["create", "user", "--user-name", "bjensen"], "POST", "/Users"), + ( + ["replace", "user", "--id", "1", "--user-name", "bjensen"], + "PUT", + "/Users/1", + ), + (["modify", "user", "1", "remove", "nickName"], "PATCH", "/Users/1"), + (["delete", "user", "1"], "DELETE", "/Users/1"), + (["query", "--me"], "GET", "/Me"), + ], +) +def test_error_responses_fail_the_command(runner, httpserver, arguments, method, path): + """An error answered by the server is displayed on stdout, summed up on stderr, and fails the command.""" + httpserver.expect_oneshot_request(path, method=method).respond_with_json( + ERROR, status=404, content_type="application/scim+json" + ) + + result = runner.invoke(cli, ["--url", httpserver.url_for("/"), *arguments]) + + assert result.exit_code == 1, result.output + assert json.loads(result.stdout) == ERROR + assert result.stderr == "Error: 404 Resource 1 not found\n" + + +def test_error_without_detail(runner, httpserver): + """An error without detail is summed up with its status.""" + error = { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status": "500", + } + httpserver.expect_oneshot_request("/Users/1").respond_with_json( + error, status=500, content_type="application/scim+json" + ) + + result = runner.invoke( + cli, ["--url", httpserver.url_for("/"), "query", "user", "1"] + ) + + assert result.exit_code == 1, result.output + assert result.stderr == "Error: 500\n" + + +def test_success_does_not_fail_the_command(runner, httpserver, simple_user_payload): + """A successful response leaves the command successful, with nothing on stderr.""" + httpserver.expect_oneshot_request("/Users/1").respond_with_json( + simple_user_payload("1"), content_type="application/scim+json" + ) + + result = runner.invoke( + cli, ["--url", httpserver.url_for("/"), "query", "user", "1"] + ) + + assert result.exit_code == 0, result.output + assert result.stderr == "" diff --git a/tests/test_escape.py b/tests/test_escape.py index 6bfb15f..6e00747 100644 --- a/tests/test_escape.py +++ b/tests/test_escape.py @@ -145,3 +145,25 @@ def test_line_breaks_and_tabulations_are_kept(runner, httpserver): ).output assert "first\n\tsecond" in output + + +@pytest.mark.parametrize(("sequence", "escaped"), HOSTILE_SEQUENCES) +def test_error_summary_is_escaped(runner, httpserver, sequence, escaped): + """The detail of an error is escaped in the summary on stderr.""" + httpserver.expect_request("/Users/1").respond_with_json( + { + "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], + "status": "404", + "detail": f"not found{sequence}", + }, + status=404, + content_type="application/scim+json", + ) + + result = runner.invoke( + cli, ["--url", httpserver.url_for("/"), "query", "user", "1"], color=True + ) + + assert result.exit_code == 1 + assert f"not found{escaped}" in result.stderr + assert sequence not in result.stderr diff --git a/tests/test_me.py b/tests/test_me.py index bbed38f..1459cd9 100644 --- a/tests/test_me.py +++ b/tests/test_me.py @@ -63,7 +63,9 @@ def test_query_error(invoke, httpserver): result = invoke("query", "--me") - assert json.loads(result.output)["status"] == "501" + assert result.exit_code == 1, result.output + assert json.loads(result.stdout)["status"] == "501" + assert result.stderr == "Error: 501 /Me is not supported\n" @pytest.mark.parametrize("arguments", [["user"], ["user", "1"]]) diff --git a/tests/test_modify.py b/tests/test_modify.py index 73b1e56..f0595bf 100644 --- a/tests/test_modify.py +++ b/tests/test_modify.py @@ -305,7 +305,7 @@ def test_stdin_and_operations(invoke): def test_error_response(invoke, httpserver): - """The error the server answers is displayed.""" + """The error the server answers is displayed, and the command fails.""" httpserver.expect_oneshot_request("/Users/1", method="PATCH").respond_with_json( { "schemas": ["urn:ietf:params:scim:api:messages:2.0:Error"], @@ -318,7 +318,9 @@ def test_error_response(invoke, httpserver): result = invoke("modify", "user", "1", "remove", "nickName") - assert json.loads(result.output)["detail"] == "Resource 1 not found" + assert result.exit_code == 1, result.output + assert json.loads(result.stdout)["detail"] == "Resource 1 not found" + assert result.stderr == "Error: 404 Resource 1 not found\n" def test_unexpected_response(invoke, httpserver):