What error status code should we return?

Development
1

In this PR we found a gray area of error status code and I would like to hear your opinion about it.

From a UI perspective, when an API request is invoked, it will look something like:

try {
    const { data } = await API.get(url);
    handleSuccess(data);
  } catch (error) {
    handleFailure(error);
  }

while based on the response status code it will decide which way to go.

on that PR the rails API code looks like that:

def power_status
        render json: PowerManager::PowerStatus.new(host: @host).power_state
      rescue => e
        Foreman::Logging.exception("Failed to fetch power status", e)

        resp = {
          id: @host.id,
          statusText: _("Failed to fetch power status: %s") % e,
        }

        render json: resp.merge(PowerManager::PowerStatus::HOST_POWER[:na])
      end

and while exception would occur, the returned json would be in status code that will translate to SUCCESS in the UI, so I added internal_server_error:

render json: resp.merge(PowerManager::PowerStatus::HOST_POWER[:na]), status: :internal_server_error

I know that it is too generic, but couldn’t find something that suits more.
What is your idea ? How should we handle this type of errors? I would like to hear it,
thanks

2

as the person who wrote the original code, I can share what was my thinking process:

From foreman perspective, there are multiple states, e.g.
Host is On
Host is Off
Host state is unknown (e.g. not responding withing xx ms).

From a UI perspective, its:
I have a known state (on, off)
I have an unknown state (e.g. foreman doesn’t know the state, or transient or server error).

IMHO, only the last state (e.g. 500 server error or timeout etc) requires the UI to respond to failure.

It just happens to be that if the host state is unknown or if the UI failed to fetch the state, the visualization happens to be the same.

2 Likes
3

Thanks for the clarification @ohadlevy :+1:

I thought that the code under rescue should be treated as an exception, an error
but I guess that for APIs the approach is different or is it just this specific case?

4

I think we should distinguish between expected failure and exceptiom failure. Backend should reply with 2xx if host is on/off/info not available, e.g. on bmc. If we get timeout or any other unexpected failure, we should reply with 5xx. Info not available and 5xx should be visualized differently, while none of them provides power status. The 5xx can still contain more data in json about the actual error. While it may be overkill for this widget, it may be handy to choose different 5xx code per rescued exception, e.g. timeout should respons with try later, while unknown exception should say general server error.

1 Like
5

I was thinking exactly the same as @ohadlevy which is why I raised my concerns. Glad to hear I wasn’t the only one thinking that.