Currently abrt uses "Strata-Message:" header in hopes to extract more useful error description:

    abrt_post_string(case_state, case_url.c_str(), "application/xml", case_data);
    char *case_location = find_header_in_abrt_post_state(case_state, "Location:");
    switch (case_state->http_resp_code)
    {
    case 301: /* "301 Moved Permanently" */
    case 302: /* "302 Found" (just in case) */
    case 305: /* "305 Use Proxy" */
        ...redirect code here...
    default:
        errmsg = case_state->curl_error_msg;
        if (errmsg)
            retval = xasprintf("error in case creation: %s", errmsg);
        else
        {
            errmsg = find_header_in_abrt_post_state(case_state, "Strata-Message:");
            if ((!errmsg || !errmsg[0]) && case_state->body && case_state->body[0])
                errmsg = case_state->body;
            if (errmsg)
                retval = xasprintf("error in case creation, HTTP code: %d, server says: '%s'",
                        case_state->http_resp_code, errmsg);
            else
                retval = xasprintf("error in case creation, HTTP code: %d",
                        case_state->http_resp_code);
        }
        break;

In the above code, default: branch is entered only when an error has happened. If "Strata-Message:" header is not found, then response body will be used.

IOW, if server stops emitting "Strata-Message:" headers, the code will still work.

It is certainly possible, and easy, to just remove find_header_in_abrt_post_state(case_state, "Strata-Message:") code.

Andrew, are there other header field(s) we should examine instead for human-readable error messages?

if you're reading the response, you should be set. When we've tested RHEL 6.0 bits, they don't seem to be doing that. 

Has the code changed since 6.0? If not, maybe we should turn off the headers somewhere to help you debug?

-Ah

(In reply to comment #3)
> if you're reading the response, you should be set. When we've tested RHEL 6.0
> bits, they don't seem to be doing that.

I believe we do use server's response. In detail, current code looks for the error message as follows:

1. if curl library provided error message (retrieved using check_curl_error() after curl_easy_perform() and saved in case_state->curl_error_msg), use it

2. if not, use MSG from "Strata-Message: MSG" if available

3. if not, use HTTP body of the error reply from server, if available

4. if not, just display numeric HTTP error code (or -1 if there were no reply from server at all (although usually in this case there will be curl error message and we'd be in 1st case))


(I have a bit of concern with (3). HTTP body usually contains useful error diagnostics, but it may contains far too much fancy HTML which clutters the message. But OTOH, it's better that just giving only HTTP error code...)

we should drop #2, since it's not in the spec at all.

re: #3, if we're getting an unhelpful error, it's a bug in the server which we should fix, and can on a much shorter update cycle. I agree that the defaults often aren't very helpful.

The rest of the logic seems good. That said, I still don't think #3 is working, but maybe that's because in our tests we removed the content from the Strata message rather than remove the header entirely. We'll test that...

Created attachment 476794 [details]
Proposed patch

This patch removes code which looks into Strata-Message: header

Applied to git and tested.

(In reply to comment #4)
> I believe we do use server's response. In detail, current code looks for the
> error message as follows:
> 
...
> 3. if not, use HTTP body of the error reply from server, if available
> 
> 4. if not, just display numeric HTTP error code (or -1 if there were no reply
> from server at all (although usually in this case there will be curl error
> message and we'd be in 1st case))
> 
> 
> I have a bit of concern with (3). HTTP body usually contains useful error
> diagnostics, but it may contains far too much fancy HTML which clutters the
> message. But OTOH, it's better that just giving only HTTP error code...

Just a note about how this looks from the user's POV:

For example, in today's testing I gave wrong username, and user sees the following error in abrt gui:

error in case creation, HTTP code: 401, server says: '401 Unauthorized

This request requires authentication..<skip>..'

See the problem? The HTTP body we see here is meant for the browser. Thank God in this case it only contained a few line breaks. But in other cases it can easily look like this:

error in case creation, HTTP code: 401, server says: '<h1>401 Unauthorized</h1>

This request <font color=red>requires</font> authentication....'


Not nice, eh?

Comment 8 is exactly why we should not be relying on the returned text and instead implementing the HTTP protocol which has exactly enough information to give a reasonable, localized response to the user based on the HTTP 401.

Thanks Denys, I'll see if I can help this along after sprint meetings today.

It seems to me that the ideal solution (unless we adopt the policy from Comment 9) is to ensure that clients specify their "Accept" parameter. If ABRT accepts "text/plain", then the API should be fulfilling the contract by sending back HTML free text. If the API isn't, I'll fix that. Is an "Accept" parameter already being sent by ABRT? Can it be sent easily?

some thoughts:

 - if the problem is you're running out of UI space, then I agree with comment #9 that you should just interrupt the response code and display your own message.

 - if you just want a different format, then I agree with comment #11, use Accept: to ask for it and we'll make sure we support it.

s/interrupt/interpret/

(In reply to comment #12)
> some thoughts:
> 
>  - if the problem is you're running out of UI space, then I agree with comment
> #9 that you should just interrupt the response code and display your own
> message.
> 
>  - if you just want a different format, then I agree with comment #11, use
> Accept: to ask for it and we'll make sure we support it.

We need a plain text, preferably one line error messages which are suitable for displaying on dumb terminals, storing in logs/databases, and such.

I will experiment with adding "Accept: text/plain" header. Thanks for the idea.

Created attachment 477026 [details]
Patch to add "Accept: text/plain" header

Patch is in abrt git now.
Run tested against both Bugzilla and RHTS.

Denys,

the server won't guarantee a one-line error message: if that's your constraint, it'd be better to process the response code, not display it directly.

(In reply to comment #17)
> the server won't guarantee a one-line error message: if that's your constraint,
> it'd be better to process the response code, not display it directly.

I didn't mean "one-line" as "it should fit into 80 chars".

I meant that it should be just a string without any fancy formatting: no HTML, no XML, not even line breaks ostensibly to make it look better on the text console.

But it's ok if the message is long. An example of such error message:

"Reporting disabled because the backtrace is unusable. Please try to install debuginfo manually using the command: 'debuginfo-install PACKAGE_NAME', then use the Refresh button to regenerate the backtrace."

OK, cool, thanks. We'll make sure text/plain works on the server.

An advisory has been issued which should help the problem
described in this bug report. This report is therefore being
closed with a resolution of ERRATA. For more information
on therefore solution and/or where to find the updated files,
please follow the link below. You may reopen this bug report
if the solution does not work for you.

http://rhn.redhat.com/errata/RHBA-2011-0619.html