Arvados

16774-keep-web-errors @ 34f5aff9a166c0e03564b607981284b6d4af9548

developer-run-tests: #2191

WebDAV errors are currently plain text but maybe they should follow this https://www.ietf.org/proceedings/50/I-D/webdav-status-00.txt

S3 errors are now XML as expected by S3 clients.

Might be worth using XML encoder here, in case error messages/paths have "<" chars etc.

16774-keep-web-errors @ aa1c0f3049f7b78e7590dde868b915bef9a7ebbe

Fix tests

Use xml.EncodeElement for error response

developer-run-tests: #2194

review at aa1c0f3049f7b78e7590dde868b915bef9a7ebbe

services/keep-web/handler.go:

• Maybe I'm too picky here, but any reason to have "\n" in the errors? Would this cause issues on non-Unix systems that use "\r\n" like Windows? also reading RFC 2046 https://www.ietf.org/rfc/rfc2046.txt section 4.1.1 says: "The canonical form of any MIME "text" subtype MUST always represent a line break as a CRLF sequence. Similarly, any occurrence of CRLF in MIME "text" MUST represent a line break. Use of CR and LF outside of line break sequences is also forbidden."
  This is for Content-Type: text/plain, but I don't know what Content-Type we are using in the errors.

• as a separate note, I also notice an extra "\n" on the check... as in c.Check(body, check.Equals, notFoundMessage+"\n") any reason why is there?

services/keep-web/s3_test.go:

I noticed the checks are comparing the body only. It will be good to add a comparison of the statusCode too.. since clients could be only checking that.

Nico César wrote:

review at aa1c0f3049f7b78e7590dde868b915bef9a7ebbe

services/keep-web/handler.go:

• Maybe I'm too picky here, but any reason to have "\n" in the errors? Would this cause issues on non-Unix systems that use "\r\n" like Windows? also reading RFC 2046 https://www.ietf.org/rfc/rfc2046.txt section 4.1.1 says: "The canonical form of any MIME "text" subtype MUST always represent a line break as a CRLF sequence. Similarly, any occurrence of CRLF in MIME "text" MUST represent a line break. Use of CR and LF outside of line break sequences is also forbidden."
  This is for Content-Type: text/plain, but I don't know what Content-Type we are using in the errors.

Sure. I don't think this is a problem in practice but I fixed it anyway.

• as a separate note, I also notice an extra "\n" on the check... as in c.Check(body, check.Equals, notFoundMessage+"\n") any reason why is there?

The responses use http.Error() which calls Fprintln() which adds a newline to the end of the message:

https://golang.org/src/net/http/server.go?s=63178:63230#L2041

So the string comparison has to add the newline added by Fprintln().

services/keep-web/s3_test.go:

I noticed the checks are comparing the body only. It will be good to add a comparison of the statusCode too.. since clients could be only checking that.

I added checks for the codes in addition to the error messages.

16774-keep-web-errors @ 7d94b1ed55350c01689ad048aee961b261263dd9

developer-run-tests: #2196

review @ 7d94b1ed55350c01689ad048aee961b261263dd9

I think that \n or \r\n should be avoided from the message at all leaving as constants since 401 status code will come with them message:

const notFoundMessage = "The requested path was not found, or you do not have permission to access it." 
const unauthorizedMessage = "A valid Arvados token must be provided to access this resource." 

But either way is good to merge.