Return errors rather than throw exceptions

[apawsey]

Follow up from #201

I changed the default behaviour of all the ApiClient objects to return a response with an error, rather than throw an exception. The reason for this is exception handling is relatively expensive Why Exceptions should be Exceptional, and more importantly (to me), bulky in a code sense.

Following on from the behaviour of the HttpClient itself, non-200 status codes, are not thrown as exceptions, but rather returned, for handling. I would argue the same basis here, that calling a web service to retrieve a document for example, and getting a 404 response is not an exceptional circumstance, but rather a pretty likely scenario. However, I can also see that creating a document and getting a non-success return code may be a more exceptional scenario, depending on the details, so perhaps the behaviour could vary by operation?

I do like the ability though not to have to keep writing try catch for lots of single one line method calls, so a universal switch is still my preferred choice.

[imtrobin]

I prefer exceptions actually, As a library, it would be better to leave the choice to the user. Perhaps it could be set a func delegate in the global, which the api can also override.

[imtrobin]

I will try to take stab at this. I want to throw my own exception class so I added a delegate in DocumentApiClient, which we can set in ctor

        Action <IApiClientResponse> errorCallback;

        void DefaultErrorHandler (IApiClientResponse response)
        {
            throw  GetApiErrorException (response).GetAwaiter ().GetResult ();
        }

      // this is to handle different responses
        T           ReportError <T> (IApiClientResponse response)
        {
            errorCallback (response);
            return default (T);
        }

        /// <summary>
        /// Get an existing document based on its Document ID.
        /// </summary>
        /// <typeparam name="T"></typeparam>
        /// <param name="documentId"></param>
        /// <returns></returns>
        public async Task<T> GetDocumentAsync<T>(string documentId)
        {
            ValidateDocumentId(documentId);
            var response = await _client.GetAsync(_docApiPath + "/" + documentId);
            if (response.IsSuccessStatusCode)
            {
                var stream = await response.Content.ReadAsStreamAsync();
                var document = DeserializeJsonFromStream<T>(stream);
                return document;
            }
            return ReportError<T> (response); // MODIFIED
        }

To move it further, if we want to be able to return error code instead throw, we probably need to standardize the response like

#205

However, it still won't work on things like GetDocumentAsync,, because returns only T. So I can achieve to customize exception class, but not return error code properly.

[DiscoPYF]

@imtrobin Thanks for taking a look at this.

The approach you mention is a good compromise to work with the existing design of the API. It has the benefit to be free of breaking changes.
However it seems quite difficult to use on a per-endpoint basis.
Also, I am not sure I see the benefit of allowing consumers to throw custom exception types, even though it seems straightforward to do with this approach.

As you mentioned, @apawsey proposed to return a wrapper/base response for all endpoint. The class would contain the response and an error property.
It seems more likely to create a breaking change, so we would probably want to do that sooner than later.

Another alternative is to add an out parameter to each endpoint that would be set with the error. Probably not something we want to do.

I think we need to make a decision in accordance with the API design and ease of use.

[imtrobin]

The other library I used, take in an optional Action user error callback per operation.

public async Task<T> GetDocumentAsync<T>(string documentId, Action<Response> errorCallback = null)
{
  if (null != errorCallback)
     errorCallback (response);
}

I prefer to use a global error handling mechanism which I set once, instead of per operation. But there is another problem in 214, which the current success it not correct. We many need to handle that per operation.

[endeffects]

Hi, today i started to use the library too and i came up with the same concern.
I think the exception pattern used here is kind of overused currently.

From my experience in the past with other db clients the common standard
for read and write operation is:

• Return null on read operations where the resource does not exist.
• Throw an exception on write operations where the resource does not exist.

This comes important for many modern design patterns, where the client
generates the primary keys (guids). So due the underlaying arangodb api
and the missing create or update operation we have to check first if a document
exists and than we can post or put the document to the db.

Currently this throws exceptions for new documents each time by using
the GetDocument operation. Which is bad by design, because the write process
becomes much more expensive. This also has a hugh impact to the overall application
performance and it cuts the decision to use arangodb for its good statistics.

[imtrobin]

Yes, I want to throw my own exceptions because I do not want leak Arangodb exceptions to the user level. So I can check for common errors like Get returns null, and I throw my own "Invalid ID message". We cannot rely on HTTP IsSuccessStatusCode because for multiple id operations like DeleteDocumentsAsync, it always return true!

[endeffects]

Well, if you like to delete a document in a fire and forget operation it doesn't matter if the document still exists, so it's fine if the operation allways returns a success. If you need to verify that the document is still available you have to get it first. It's the same prefly check as for a create or update operation to identify the next required step (post or put). There is no need to throw any exception here. If you have to deal with a hugh amount of database operations per second throwing exceptions for each use case is a really bad deal and decreases the performance alot.

Anyways, if this is your personal design decision you are fine to go with it, but it should not be the default behavior of such a db driver / client. Well, and please don't get me wrong, i know the underlaying arangodb api is not perfect in the way it behaves, but there is no reason to make it more worse than it is.

[imtrobin]

I don't agree that we should use get first before we delete, because arangodb api will already return an error ID with doc not found. What I want is a way to access that and customize how I handle it.

For example, doc update may result in 1200 write conflict if multiple writes happen concurrently. In some cases, I expect and want to handle, so I have overload Put/Post Document with versions that return the response directly.

[DiscoPYF]

This comes important for many modern design patterns, where the client
generates the primary keys (guids). So due the underlaying arangodb api
and the missing create or update operation we have to check first if a document
exists and than we can post or put the document to the db.

Currently this throws exceptions for new documents each time by using
the GetDocument operation.

For that specific use case, I would recommend using the HEAD operation: DocumentApiClient.HeadDocumentAsync. I think it fits better the approach of "check first if exists" rather than trying to get the whole document.

https://www.arangodb.com/docs/stable/http/document-working-with-documents.html#read-document-header

[imtrobin]

The "Check Exist, Operation" does not work if there are multiple threads (or rest api) that can be deleting it while the check Exist succeed.