Create and throw dedicated serialization exception.

[robsiera]

Sometimes Arango returns a Response that is marked as 'no-errors' but the result value is null.

Example:
When you call

PostCursorAsync<int>("RETURN MAX('Books'[*].Stock)")

and when there are no documents with a 'Stock' field, you get this response

{ "result": [null], "hasMore": false, "cached": false, "extra": { "stats": { "writesExecuted": 0, "writesIgnored": 0, "scannedFull": 0, "scannedIndex": 0, "filtered": 0, "httpRequests": 0, "executionTime": 3.802776336669922e-4, "peakMemoryUsage": 66480 }, "warnings": [] }, "error": false, "code": 201 }

This response will throw an error, as [null] cannot be deserialized to int.

So who is to blame?
Is this a Arango problem? Should a null result be marked as error?
What is the role of code 201 in this, if any? Is it relevant that we do not get a 200 code? If so, shouldn't the client handle this relevant information?
If [null] is a valid answer, then why does the client try to deserializing it.

I would love to hear your thoughts, before I try create a PR.

Personally, I think Arango is in the wrong. The return value of a MAX operation is an INT. If Arango thinks that a value of ZERO is not appropriate in this case (which is a valid point of view) then that should result in an error or at least a warning.

EDIT
Or am I wrong to call this query with PostCursorAsync in the first place?

[robsiera]

I'm trying to create a Unit Test to showcase this issue, but for now I cannot reproduce it in a unit test. So it might be an issue with my code. I'll be back with more info shortly.

I've created a unit test to to showcase this issue.

[imtrobin]

I have also reported some inconsistencies with arangodb response error flag, where it is not set when it should be. They are aware but they won't change due to backward compatibility until the next major release.

In the library, then I'm not sure what is the best way to handle this. Should it throw exception? Actually I prefer not to.

[DiscoPYF]

Hi @robsiera , I think this behavior is as expected from ArangoDB. The query execution will use null as fallback when something doesn't exist in the query.

Example, the following AQL query will return null:

LET myobj = {}
RETURN myobj.prop

The same goes for RETURN MAX(Books[*].Stock). Those queries are valid. Then, according to the documentation:

HTTP 201 is returned if the result set can be created by the server.

Which it was, [ null ].

So who is to blame?

Here I think the writer of the query is to blame 😛 (knowing how ArangoDB behaves)
There are a few solutions I can think of.
First, I would recommend changing the return type to int? to make sure you can differentiate between a null result and a number.
Here is a test method that you can run, as example:

[Fact]
public async Task PostCursorAsync_ShouldSucceed_WhenUsingNullableType()
{
    var response = await _cursorApi.PostCursorAsync<int?>(
        "LET myobj = {} RETURN myobj.myprop");

    Assert.Single(response.Result);
    Assert.Null(response.Result.First());
}

Another solution is to handle null values inside the query: RETURN MAX(Books[*].Stock) || -1. Note that in that case you may have to adjust the logic to account for 0, negative values, etc.

Or LET maxResult = MAX(Books[*].Stock) RETURN maxResult == null ? -1 : maxResult.

Would that help?

I don't think the library should do anything special in that case, as there isn't anything fundamentally wrong. It would be better to keep special handling or behaviors to the minimum.

[robsiera]

Ok, if it is expected behaviour of Arango, then for me the 'problem' shifts towards the fact that the Deserialization in the client throws NewtonSoft exceptions without giving more context. I will draw up a proposal to improve the error message of the DeserializeFromStream() method.

[DiscoPYF]

JsonSerializationException provides all the context consumers need to understand the issue though. It will tell you what JSON type cannot be converted into what CLR type.

Message: 
    Newtonsoft.Json.JsonSerializationException : Error converting value {null} to type 'System.Int32'. Path 'result[0]', line 1, position 15.
    ---- System.InvalidCastException : Null object cannot be converted to a value type.

When using API instances with JsonNetApiClientSerialization, the default serializer which uses Json.NET, JsonSerializationException is the exception type thrown to indicate a problem with deserialization. You can then simply look at the message or inner exception to get more information.

With that information, you can make the necessary changes to the query or your logic.

[DiscoPYF]

We could document that more explicitly, the serialization section in the readme seems like a good place.

[rossmills99]

I think the key thing looking at the comments here is that the result from the query can in fact be null. Whether it's correct or not for ArangoDB to do that, if you expect a query result can possibly be null, then I think it's up to you to make sure the type passed is nullable. @DiscoPYF's suggestion to use int? as the type seems reasonable to me.

I also agree there may be some benefit to us catching serializer exceptions and wrapping them in our own exception type. I would suggest including the caught exception as an inner exception, and create our own Exception subclass. Then at least a consistent exception type is used regardless of different serializer implementations.

[imtrobin]

newton json already sertializersettings.OnError so I don't think it's good to wrap it again. It's back to exception decision, to throw or not, maybe should be having an OnError like newton for users to decide

[robsiera]

yes, being able to set Newtonsoft Serializationsettings would come in handy. I already missed that myself to not being able to set those. But that is beside to point for me. In that sense that I need an easy and clear indication where serialization went wrong, not just the Newtonsoft library saying that it hit a problem. These days we all use Json/Newtonsoft. I serialize and deserialize in different locations of my code. I want to easily see who is responsible.

Since I am using the PR version myself, I have already hit that exception twice (I'm converting an old app that uses an old Arango.net client). It was great to immediately see where the problem was, twice in a different location.

Hence I hope that this small improvement (see progress in issue 275) makes it in. Users of this library will most of the time encounter two problems: connection problems or Serialization/Deserialization problems. We better make is as easy as possible to pinpoint the problem in those cases.