Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong HTTP status code when API request fails #5095

Closed
bobapple opened this issue Mar 27, 2017 · 2 comments · Fixed by #6199
Closed

Wrong HTTP status code when API request fails #5095

bobapple opened this issue Mar 27, 2017 · 2 comments · Fixed by #6199
Assignees
Labels
area/api REST API bug Something isn't working
Milestone

Comments

@bobapple
Copy link
Member

Creating a comment via the API requires two parameters in the request body: author and comment.

Creating a comment works just fine, this is an example how I print also the HTTP response code:

bob@bobs-mbp $ curl -w "\nHTTP Status: %{http_code}" -k -s -u icinga:icinga -H 'Accept: application/json' -X POST 'https://demo:5665/v1/actions/add-comment?service=demo%21disk' -d '{ "author": "icingaadmin", "comment": "This is a comment" }'
{"results":[{"code":200.0,"legacy_id":10.0,"name":"demo!disk!demo-1490607359-8","status":"Successfully added comment 'demo!disk!demo-1490607359-8' for object 'demo!disk'."}]}
HTTP Status: 200

When I omit one of the required parameters, lets say I don't set the author, the response I get tells me that something is wrong. However, the HTTP response code is still 200.

bob@bobs-mbp $ curl -w "\nHTTP Status: %{http_code}" -k -s -u icinga:icinga -H 'Accept: application/json' -X POST 'https://demo:5665/v1/actions/add-comment?service=demo%21disk' -d '{ "comment": "This is a comment" }'
{"results":[{"code":403.0,"status":"Comments require author and comment."}]}
HTTP Status: 200
@gunnarbeutner gunnarbeutner added area/api REST API bug Something isn't working labels Mar 27, 2017
@dnsmichi dnsmichi self-assigned this Mar 27, 2017
@dnsmichi
Copy link
Contributor

Each result set might contain a different return status other than 200.

        for (const ConfigObject::Ptr& obj : objs) {
                try {
                        results->Add(action->Invoke(obj, params));
                } catch (const std::exception& ex) {
                        Dictionary::Ptr fail = new Dictionary();
                        fail->Set("code", 500);
                        fail->Set("status", "Action execution failed: '" + DiagnosticInformation(ex, false) + "'.");
                        if (HttpUtility::GetLastParameter(params, "verboseErrors"))
                                fail->Set("diagnostic information", DiagnosticInformation(ex));
                        results->Add(fail);
                }
        }

        Dictionary::Ptr result = new Dictionary();
        result->Set("results", results);

        response.SetStatus(200, "OK");
        HttpUtility::SendJsonBody(response, result);

An attempt to fix would be to always return the highest error number detected from returned results here.

@bobapple
Copy link
Member Author

We had one more discussion with @gunnarbeutner about this and came to this points:

  • If all results are bad, the HTTP status should also be something not 200
  • If any of the results is successfull, the HTTP status should be 200

This makes it easier to catch errors when working with the API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api REST API bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants