feat: add ExceptionHandler

[wojiaodoubao]

Since we are implementing namespace, we need to throw errors, e.g. resource not found(#135 (comment),).

I think we need to first handle errors. Then we can throw errors in namespace impl.

[jackye1995]

for this constructor, I think the expectation is to get the errorResponse from the ApiException, instead of hard-coding it to the values below

[wojiaodoubao]

Thanks! I would like to parse errorResponse from ApiException in the way below, please help review whether it is ok.

There are 2 cases of ApiException.

1. Server-side error. ApiException is constructed from HttpResponse. Since the server-side error follows spec(https://github.com/lancedb/lance-namespace/blob/main/docs/src/spec/rest.yaml#L2513), we can parse error response from responseBody, which is a json object.
2. Client-side error, such as io error, request serialize error. In this case, the response body is either null or can't be deserialized as json. We need to hard-code the errorResponse.

[jackye1995]

For 1, I think we should still handle the case if server returns an error that is not following the spec and handle it probably as a 500 with logging.

For 2, I think it won't throw ApiException? Maybe we should define this as LanceNamespaceServerException, and have another LanceNamespaceClientException to handle the case you are describing, what do you think?

[wojiaodoubao]

For 1, I think we should still handle the case if server returns an error that is not following the spec and handle it probably as a 500 with logging.

Agree. We can use 'whether containing both response header and response body' as the condition to identify if it is a server-side error or a client-side error.

For 2, I think it won't throw ApiException? Maybe we should define this as LanceNamespaceServerException, and have another LanceNamespaceClientException to handle the case you are describing, what do you think?

The client-side error will throw ApiException too, the code is auto-generated.
e.g.

• https://github.com/lancedb/lance-namespace/blob/main/java/lance-namespace-apache-client/src/main/java/com/lancedb/lance/namespace/client/apache/ApiClient.java#L650
• https://github.com/lancedb/lance-namespace/blob/main/java/lance-namespace-apache-client/src/main/java/com/lancedb/lance/namespace/client/apache/ApiClient.java#L681
• https://github.com/lancedb/lance-namespace/blob/main/java/lance-namespace-apache-client/src/main/java/com/lancedb/lance/namespace/client/apache/ApiClient.java#L1017

Since the rest client always throwing ApiException and LanceNamespaceException is able to parse error response from ApiException, I think a LanceNamespaceException should be enough.

[jackye1995]

I see.

If any error occurs, construct ErrorResponse with 500.

I think we cannot do that, because 500 indicates something is wrong with the server, but as you shown the error could be client side.

In that case, I would suggest let's make private final ErrorResponse errorResponse; as private final Optional<ErrorResponse> errorResponse; and only set that if it is available. Otherwise, just use the default Java Exception constructor to set super(message, throwable)

[wojiaodoubao]

I just found a way to identify 'server-side error' and 'client-side error'. Please help review whether the idea is ok.

We can use 'whether containing both response header and response body' as the condition to identify if it is a server-side error or a client-side error. Then we can use the correct code.

  private static ErrorResponse parse(ApiException e) {
    boolean serverSideError = e.getResponseHeaders() != null && e.getResponseBody() != null;
    ......
  }

[jackye1995]

I guess to clarify our terms, there are actually 3 cases:

1. something went wrong in the client before even sending the request to server
2. server returns a 4XX error indicating client request has some issues
3. server returns a 5XX error indicating server has some issues
4. something went wrong in the client for processing the error response after receiving it from the server (e.g. error response structure is invalid and fails to be parsed)

I was talking about using Optional<ErrorResponse> errorResponse; and set it as empty for case 1 and 4, because it does not even make sense to say it belongs to any HTTP error code and we don't really have a valid error response.

This means we can do something like

public LanceNamespaceException(ApiException e) {
    // try parse response to ErrorResponse
    // if succeed, use this(errorResponse) constructor
    // if null or fail parsing, just do super(e)
}

[wojiaodoubao]

Thanks your explanation, it is clear, let me fix it.

[jackye1995]

the instance should be the actual URI route that failed, not hard-coded

[wojiaodoubao]

Thanks! I misunderstood the meaning of instance, let me fix it.

I also made 'type' hard-coded, is it ok?

[jackye1995]

I think the intention for type is to provide more detailed error type beyond the standard HTTP error codes. For example if there is any error code from HMS, or in the example above we could also make UnknownDBException an error type.

Also for title vs detail, my thinking was that an error message like String.format("Database %s doesn't exist.", database) should be the title, and things like stacktrace should be detail.

[geruh]

@jackye1995 @wojiaodoubao for non rest implementations do we still want to use this instance type?

[wojiaodoubao]

Hi @geruh , I think we still need this even for non rest impl. Mostly is because 'non rest impl' could be used by the rest server. Following this rule can make the response more clear.

If we are using the 'non rest impl' directly, we might not need the status part and detail part(containing the stack trace).

My idea is:

1. We use zero as undefined status.
2. We ignore undefined status and detail in Exception message. So when we directly use impl, we won't see the confusing message.

What do you think?

[jackye1995]

looks good to me!

[jackye1995]

@wojiaodoubao I just invited you as maintainer, so we can iterate quickly on this and you can just merge by yourself after rebase.