feat: add a more precise error handling to the SDK #36
Conversation
sdk/client.go
Outdated
| if err := json.Unmarshal(resp, &msg); err != nil { | ||
| // if the response is not a MetaMessage, it's probably a success message | ||
| // Therefore we just return the original error from the request | ||
| return resErr |
There was a problem hiding this comment.
Why is the success message in the response error?
There was a problem hiding this comment.
The main reason is sometimes the response message is not in the MetaMessage format therefore the unmarshal fails and there might be simply an http error like could not find the server, etc.. so we return the response error in that case.
I think it need to have more clear comment.
or maybe we change it to a better way. do you have any suggestions in mind?
There was a problem hiding this comment.
pushed a new change, hope the new one is more clear. let me know plz
There was a problem hiding this comment.
I got it now. Thank you for clarifying 👍
| // Therefore we just return the original error from the request | ||
| return resErr | ||
| } | ||
| return Error{Message: msg} |
There was a problem hiding this comment.
It's currently not clear to me when it succeeds and when it fails.
There was a problem hiding this comment.
Well the HTTP request can succeed, but the requested action might fail. for example when user requests to start a packetloss on a non existing network interface. The HTTP request to the endpoint succeeds, but the underlying service returns an error in form of MetaMessage with details.
So there are two layers of errors here.
Not sure how to make it clearer.
There was a problem hiding this comment.
pushed a proposal fix
There was a problem hiding this comment.
The new comment explains it better now. Also, the new structure looks good 👍
| } | ||
|
|
||
| func (c *Client) PacketlossStatus() (*MetaMessage, error) { | ||
| return c.getServiceStatus("/packetloss/status") | ||
| return c.getServiceStatus(endpointPrefix + "/packetloss/status") |
There was a problem hiding this comment.
i think it'd be nice to do something like api.BuildEndpoint("/packetloss/status") for things like this, and lose the endpointPrefix, then you could bundle any other thing that comes up (ie: potentially checking for accidental double // or double whack whack as i've heard it called 😸 ) inside that method
i'd maybe even go a step further, and set up iota/enum for different xdp capabilities and build a nice way of building these with code, maybe like:
api.BuildEndpoint(capabilities.Packetloss).Status() which returns a string or error or something, where the Packetloss iota type could then look up the url string etc. Would make ti really nice to test and not have strings all around, just a suggestion
There was a problem hiding this comment.
Great suggestions Ramin, Thank you man. I'll go for it.
There was a problem hiding this comment.
I will apply this in another PR as this PR is for the error stuff.
Would you please elaborate more on the .Status(), I am not sure if I got it properly.
There was a problem hiding this comment.
ah i'm definitely getting carried away so feel free to ignore me, or we can pair on it, but it the response is some kind of "string builder" that calling Status() on merely returns the endpoint string WITH the /status added, then its just a helper/builder that knows your url format, vs having to explicitly name a status endpoint of some kind
say BuildURL() returns a ApiEndpoint{ path: string } and func(a *ApiEndpiint) Status() could return the string, or add status: true to ApiEndpoint which implements a String() method that builds the full url.
There was a problem hiding this comment.
oh got it, so that .Status was an example. Thanks man, that's good suggestion.
There was a problem hiding this comment.
Keeping a single source of truth for URI paths sounds better if we want in the future to add more.
Having '/packet/status' concatenated to the endpointPrefix is not great for readability. Ramin's idea indeed sounds cool
sdk/client.go
Outdated
| resp, err := c.getResource(resPath) | ||
| if err != nil { | ||
| return nil, err | ||
| resp, resErr := c.getResource(resPath) |
There was a problem hiding this comment.
i would definitely keep to just calling these err all the time as its easier to just scan them and ignore as "error handling boilerplate"
There was a problem hiding this comment.
good catch, pushed a fix
| } | ||
| msg := api.MetaMessage{} | ||
|
|
||
| if err := json.Unmarshal(resp, &msg); err != nil { |
There was a problem hiding this comment.
i'd rely on http StatusCodes for this vs serialization success/failure and not try and overcomplicate it. Someone could return a 500 from api, but implement a MetaMessage response successfully and then this will be trouble
There was a problem hiding this comment.
Well the API endpoints should return 200 status only on success of the operation. I'll update where it does it if there is any. So, the it will be easier for the SDK users to use it. WDYT?
There was a problem hiding this comment.
Ok I pushed a proposed fix for it.
Now it should not happen, in any case when an error is returned from the endpoint SDK follows that if status ok is returned it returns nil.
| } | ||
|
|
||
| func (c *Client) BandwidthStart(req BandwidthStartRequest) error { | ||
| _, err := c.postResource("/bandwidth/start", req) | ||
| return err | ||
| return c.postServiceAction(endpointPrefix+"/bandwidth/start", req) |
There was a problem hiding this comment.
[nit]: consistency across endpointPrefix+ and endpointPrefix + is needed to be the same. fmt can help with this
There was a problem hiding this comment.
Thanks I'll fix this one and the endpoint paths will be resolved in another PR.
| } | ||
|
|
||
| func (c *Client) PacketlossStatus() (*MetaMessage, error) { | ||
| return c.getServiceStatus("/packetloss/status") | ||
| return c.getServiceStatus(endpointPrefix + "/packetloss/status") |
There was a problem hiding this comment.
Keeping a single source of truth for URI paths sounds better if we want in the future to add more.
Having '/packet/status' concatenated to the endpointPrefix is not great for readability. Ramin's idea indeed sounds cool
API endpoints return the errors in
MetaMessageformat which makes it easier for the end user to match and handle errors.This PR attempts to add such functionality to the SDK functions; moreover, it adds some helper functions for the users to match errors easier.