Move OCS error handlers to pkg #593
Conversation
| // granted to it by virtue of its status as an Intergovernmental Organization | ||
| // or submit itself to any jurisdiction. | ||
|
|
||
| package errhandler |
There was a problem hiding this comment.
Nice abstraction! however, I think the chose of name is incorrect, and the base principles. While an errhandler package would allow for abstracting error handling, this package deals more with OCS messages.
On my mind working with interfaces is more appropriate as a generalized idea of error handling, then, every package that deals with the ocs api would implement the errhandler interface. Otherwise if you want to keep this generic, more functions will be added for different packages.
There was a problem hiding this comment.
Hi @refs, thanks for the review! The main idea behind moving this to pkg was to prevent implementing it again. We're currently implementing the OCM HTTP service and it would have the same error codes as these, and I imagine all HTTP services would be having these, and the fact that we could pass messages to add details makes these quite generic. But we could definitely change the package name to signify that it handles HTTP error codes.
Please let me know if that would be fine. And any suggestions that you have for the package name 😄
There was a problem hiding this comment.
@ishank011 are really the OCS error responses the same for OCM? The ones that I've found in OCM only have a "MESSAGE" field:
{
"message": "RESOURCE_NOT_FOUND"
}
There was a problem hiding this comment.
@labkode OCM defines it's own error codes and a few associated methods in https://github.com/cernbox/ocmd/blob/master/api/api.go#L14 which match with the OCS erros (https://github.com/cs3org/reva/blob/master/internal/http/services/owncloud/ocs/reqres.go#L54). We can add more if required. So instead of having separate implementations in both the services, this way we can just use a common package.
So what was previously done as https://github.com/cernbox/ocmd/blob/master/handlers/handlers.go#L308
ae := api.NewAPIError(api.APIErrorInvalidParameter).WithMessage("body is not json")
w.Write(ae.JSON())
can now be done in a similar way as OCS does.
errhandler.WriteError(w, r, MetaBadRequest.StatusCode, "body is not json"", nil)
|
The OCS errors need a better implementation, because there is a difference between the OCS v1 Vs V2 http status code handling. We need to respond with different codes, depending on the endpoint. Tracked in owncloud/ocis-reva#26 |
|
The TODO is right above the mapping code: https://github.com/cs3org/reva/blob/master/internal/http/services/owncloud/ocs/reqres.go#L117-L138 It is derived from owncloud/core@bacf160#diff-c1d6f3585e52926be0ec6169c0f58c38R442-R466 However, it is only necessary for the v2 endpoint. This ties the mapping to the endpoint. 🤷♂️ We can pass a boolean isV2 flag (shudder) or a mapper function as a new parameter (meh) which clutters up the handler signatures. In this case the context might be the right place. Then the ocs handler can store the error mapper function in the context and the error handler can check if the context has a mapper function. If you feel this does not make functionality obvious enough we could also store the mapper function or a preconfigure error handler in the handler when setting up the v1 and v2 handlers. |
My two cents: Agree with the implementation details, but my point on an untied, abstract, cold interface definition that's not tied to the OCS API still stands. Do the OCM messages care about OCS meta statuses? Because this implementation requires just that. Or at least is not reflecting a more general approach |
|
@ishank011 for time being we leave the OCS on one side and the OCM on other. These two services will evolve independently. OCS and OCDAV at some point will be moved to the OCIS repo while OCM will continue to live in Reva. |
|
@labkode Okay, will close this and add a similar implementation for OCM. Thanks all for the reviews! |
No description provided.