Adds error handling for unexpected responses for create/update/acquir…

[percygrunwald]

Hi, I am using this library for an Elixir application and I would like to pattern match when an operation fails for whatever reason. I noticed that the case ... of pattern for deciding on the response is only used on a few of the existing requests. I simply extended this handling of the response to more of the create/update/acquire/delete operations.

Thanks.

[dkataskin]

@pgrunwald First of all, thanks for the contribution! I can merge this pull requests as is but if you have time to implement one small refactoring it would be nice. I suggest extracting response code checking into a separate function and do pattern matching against response there, I think it will be better. Azure may change response code from, let's say no_content to ok, right now it will be counted as error but in fact it should be ok. Let me know if you want this to be merged or will do small refactoring.

[percygrunwald]

@dkataskin, I'm not 100% sure what the refactored function would look like. Do you mean a private function to wrap the case like this:

...
{Code, Body} = execute_request(ServiceContext, ReqContext),
return_response(Code, Body, State, ?http_created, created);

Where the return_response/3 looks like this:

return_response(Code, Body, State, ExpectedResponseCode, SuccessAtom) ->
  case Code of
    ExpectedResponseCode ->
      {reply, {ok, SuccessAtom}, State}.
    _ ->
      {reply, {error, Body}, State}
  end.

I feel like I've misinterpreted something because I don't think this will isolate us from needing to change if Azure change the response code, although the way I suggested is a little cleaner than my first attempt.

[dkataskin]

There are two parts, first is extracting, as you mentioned, a common function return_response/3. The second part is that I believe we should check only if http status code is >= 200 and < 300 to return successful response instead of pattern matching against specific codes like 204 No Content and 201 Created. But I'm not 100% sure. What do you think?

[percygrunwald]

@dkataskin, I see your point, but I'm not sure that adding this extra level of refactoring adds that much value right now. We would still need to define the success atom in the function call anyway. The level of complexity would remain largely the same. Yes, we would be protected if Azure were to change the success code for an operation, but I think that's highly unlikely.

I'm personally in favor of doing the refactoring I mentioned in my comment above, but I don't think it's necessary to get rid of the pattern matching on specific status codes.

I could do the refactoring this week.

[dkataskin]

Thank you, let me know when refactoring is done and I will merge the PR.

[percygrunwald]

@dkataskin, I completed the refactoring as we discussed.

[dkataskin]

@pgrunwald thank you, merged.