[XrdHttp] A GET with 'Want-Digest' header request issued on a non-existing file will return a 404 error instead of 500

[ccaffy]

Fixes #2018.

[abh3]

Like I said, I'm OK with this though I do have a question that may or may not lead to another change. So, if @amadio is OK with it so am I.

[abh3]

I'm OK with the way it is but you could be a bit more discerning here. For instance, if the error is kXR_Unsupported then the status code should really be 501 with the text as returned by the server. Which leads to the question why override the server supplied error text with a generic one? Is the original error message lost somewhere? One would expect that the server's error message will better explain how to correct the problem.

[amadio]

Overriding is what used to be done, so I understand @ccaffy's choice. However, I agree that your suggestion is an improvement. I suppose we could have something like

switch(xrderrcode) {
case kXR_NotFound:
    break; // reuse error code and message already set
case kXR_Unsupported:
    httpStatusCode = 501;
    break;
default:
    httpStatusCode = 500;
    httpStatusText = "Underlying filesystem failed to calculate checksum.";
}
prot->SendSimpleResp(httpStatusCode, NULL, NULL, httpStatusText.c_str(), httpStatusText.length(), false);

[ccaffy]

I'm fine with this improvement. Will apply it now ;)

[ffurano]

uhmmmmm wait please, IMO there may be another corner case, let's talk

[ccaffy]

After the chat with @ffurano , we are good to go! @amadio you may merge this fix whenever you want :)

Cheers,
Cedric

[xrootd-dev]

Well, yes, but the question remains why isn't the server's error text promoted to the repsonse. Should that not be the actual explanation as opposed to substituting a generic response?

…

On Tue, 30 May 2023, Guilherme Amadio wrote: @amadio commented on this pull request. > @@ -1798,7 +1798,16 @@ XrdHttpReq::PostProcessChecksum(std::string &digest_header) { if (convert_to_base64) {free(digest_value);} return 0; } else { - prot->SendSimpleResp(500, NULL, NULL, "Underlying filesystem failed to calculate checksum.", 0, false); + // This is a result of the "Ugly" hack put in place to prevent a failing stat() on a + // non-manager server to fail a transfer. + // In the case a user issues a GET with a 'Want-Digest' header on a non-existing file, the non-failing + // stat() will result in a failed checksum query giving a 500 error back to the user. + // Here we just return a 404 instead. httpStatusCode and httpStatusText contain the right reason why the request failed. + if(xrderrcode != kXR_NotFound) { Overriding is what used to be done, so I understand @ccaffy's choice. However, I agree that your suggestion is an improvement. I suppose we could have something like ```cpp switch(xrderrcode) { case kXR_NotFound: break; // reuse error code and message already set case kXR_Unsupported: httpStatusCode = 501; break; default: httpStatusCode = 500; httpStatusText = "Underlying filesystem failed to calculate checksum."; } prot->SendSimpleResp(httpStatusCode, NULL, NULL, httpStatusText.c_str(), httpStatusText.length(), false); ``` -- Reply to this email directly or view it on GitHub: #2019 (comment) You are receiving this because you are subscribed to this thread. Message ID: ***@***.***> ######################################################################## Use REPLY-ALL to reply to list To unsubscribe from the XROOTD-DEV list, click the following link: https://listserv.slac.stanford.edu/cgi-bin/wa?SUBED1=XROOTD-DEV&A=1

[ccaffy]

It was just that it was done like that. But I agree, let's then just use the server error text that is set by the XRootD layer, it will simplify the code!

[abh3]

Excellent!

…

On Wed, 31 May 2023, ccaffy wrote: It was just that it was done like that. But I agree, let's then just use the server error text that is set by the XRootD layer, it will simplify the code! -- Reply to this email directly or view it on GitHub: #2019 (comment) You are receiving this because your review was requested. Message ID: ***@***.***>

[amadio]

@ccaffy Very nice, but could you squash the two commits into a single one? Thanks!

[ccaffy]

@ccaffy Very nice, but could you squash the two commits into a single one? Thanks!

Done :)