emersion / go-webdav

A Go library for WebDAV, CalDAV and CardDAV
MIT License
326 stars 71 forks source link

carddav: return 404 if contact not found #70

Closed bitfehler closed 2 years ago

bitfehler commented 2 years ago

The backend interface is designed so that it can return nil, even though no error occurred, to indicate that no contact for the given request was found. Handle this case everywhere and return a 404, either directly or as part of a multi-get response.

bitfehler commented 2 years ago

Two things to note here: I am assuming that the interface was designed like I describe in the commit message. Please correct me if I am wrong. Either way, once I know, I am happy to add some documentation to the interface. The other thing: I am having a hard time testing the 404 multi-get response with real clients as I currently don't know how to make a real client request a non-existing contact via multi-get, so consider that part slightly theoretical (but we should definitely avoid passing on the nil pointer there).

emersion commented 2 years ago

GetAddressObject is definitely not supposed to return nil, nil. It should return an error explaining why no address object has been returned.

bitfehler commented 2 years ago

I see. Then, was there any plan how to distinguish a "not found" from an actual error? Returning any error always causes a 500 at the moment. I think the only way to avoid it might be returning an internal.HTTPError, but the backend does not have access to this type. Should we expose a function that returns an error that is actually an internal.HTTPError with code "not found"?

emersion commented 2 years ago

Yeah, we should allow backends to return proper errors. The HTTP status code may not be enough for all use-cases, in some cases the XML error would be necessary too (?).

We could move some types to the webdav package, but then we can't have shared private helpers in internal without causing circular package dependencies.

bitfehler commented 2 years ago

Allright, thanks for clarifying. Will come up with something accordingly (and add some docs to the interface)