Create and throw dedicated serialization exception.

[robsiera]

See #274 for more details on the origin of this improvement

[DiscoPYF]

Thank you for submitting those changes. In overall, I can't see how returning a general Exception would help the caller of the method in any way.

I think those changes would only be truly useful if a specific exception type was thrown. Even then, I personally see little value in providing the same information that JsonSerializationException already provides.

There are many different type of serialization errors that can occur, not only when converting null to a value type.

[DiscoPYF]

We probably don't want to throw such a generic Exception type here. You are reducing the specific JsonSerializationException to a very general Exception. I don't understand how it is supposed to improve interpretation/catch for the consumer.

[robsiera]

I don't understand how it is supposed to improve interpretation/catch for the consumer.

Suddenly I was confronted with a "Error converting value {null} to type 'System.Int32'".
I was not expecting NULL from Arango. So I was unsure about what triggered this error. As a developer, you do not know the ins and outs of the client. For all I know it could have been a bug in the client.
So this is what I need as debugger: undoubtable information about the context of the error.

[DiscoPYF]

Following a discussion with @rossmills99 , we also think that having our own exception type for serializer exceptions would make sense. Since we support different searialization implementations, it would be good to be able to document a consistent exception type.

We have a single point where we call the serialization component in ApiClientBase. The methods DeserializeJsonFromStream and GetContent are good candidates where we can try/catch calls to the serializer, catch any generic Exception and throw the specific exception of our own creation (with the caught exception as inner exception).

@robsiera Would you be up to submit a pull request with those changes?

[robsiera]

@DiscoPYF I committed a new implemention. Is that one more to your liking?

[DiscoPYF]

@robsiera Thank you, I think that's exactly what we had in mind. 👍

I left a bunch of minor comments which I think should be addressed before merging. In addition, can you add some tests to make sure we cover the new logic?

I also noticed that the pull request has a lot of unrelated intermediary commits now. Can you rebase your latest changes on master only? The PR title should also be updated I think.

[DiscoPYF]

It's probably optimized away by the compiler but we don't need an interpolated string $ here do we?

[DiscoPYF]

Regarding the tests, I was thinking that implementing tests for the de/serialization error handling would be quite simple with the CursorApiClient.PostCursorAsync method. The class ihnerit from ApiClientBase so it would cover the new logic.

Something like this should be enough:

• PostCursorAsync_ShouldThrowException_WhenSerializationFailed
• PostCursorAsync_ShouldThrowException_WhenDeserializationFailed

What do you think?

[DiscoPYF]

@robsiera To test the exception when deserializing the error response, we can mock the transport layer and return a dummy response. Here is an example:

[Fact]
public async Task PostCursorAsync_ShouldThrow_WhenErrorCannotBeDeserialized()
{
    var mockTransport = new Mock<IApiClientTransport>();

    var mockResponse = new Mock<IApiClientResponse>();

    var mockResponseContent = new Mock<IApiClientResponseContent>();

    string mockJsonError = "{ errorNum: \"some_error\" }";

    mockResponseContent.Setup(x => x.ReadAsStreamAsync())
        .Returns(Task.FromResult<Stream>(
            new MemoryStream(Encoding.UTF8.GetBytes(mockJsonError))));

    mockResponse.Setup(x => x.Content)
        .Returns(mockResponseContent.Object);

    mockResponse.Setup(x => x.IsSuccessStatusCode)
        .Returns(false);

    mockTransport.Setup(x => x.PostAsync(
        It.IsAny<string>(),
        It.IsAny<byte[]>()))
        .Returns(Task.FromResult(mockResponse.Object));

    var cursorApi = new CursorApiClient(mockTransport.Object);

    var ex = await Assert.ThrowsAsync<JsonSerializationException>(async () =>
    {
        await cursorApi.PostCursorAsync<object>("RETURN true");
    });

    Assert.NotNull(ex.InnerException);
}

[robsiera]

Regarding the tests,
Something like this should be enough:

• PostCursorAsync_ShouldThrowException_WhenSerializationFailed
• PostCursorAsync_ShouldThrowException_WhenDeserializationFailed

I created the WhenDeserializationFailed variant as that was easy.
I did not create the WhenSerializationFailed variant as I do not know how to trigger that exception.

[robsiera]

I processed all the comments.

I also noticed that the pull request has a lot of unrelated intermediary commits now. Can you rebase your latest changes on master only? The PR title should also be updated I think.

Alas, I do not know how to cleanup the history within this branch. I do not have much experience with PRs. I can however create a new clean pull-request, but then it will create yet another ticket - not ideal.
Please advise.

[DiscoPYF]

@robsiera Thank you. The changes look good. 👍
I left a comment with a test you can add for testing the error deserialization exception.

Regarding updating the pull request, one way is to squash the commits.

First create a backup branch in case something goes wrong:

git checkout -b feature/issue274-backup

Then go back to your branch.
The simplest way to squash commits is to reset the commits you want to combine, up to a base commit:

git reset --soft f1fbee5f4b40007286ee029a4bc997a2047b4774

Here f1fbee5f4b40007286ee029a4bc997a2047b4774 is the hash of the latest commit from master on your branch.

Once done, you have local changes that have been combined. You can commit and force push.

git commit
git push origin feature/issue274 -f

Make sure you mention the issue number in the body of your commit message, e.g.:

Create and throw dedicated serialization exception.

fix #274

Then update the pull request title (maybe with the commit message), and we should be good to go.

[DiscoPYF]

@robsiera There are still 14 commits on your branch. Do you mind squashing them so that the previous unrelated commits get removed?

We can "Squash and merge" the pull request on our end, but if we do that I think you won't be marked as the author. So it would be better if you squash the commits on your branch directly.

I have posted the instruction above, but if you need further help with that with can chat.

[DiscoPYF]

@robsiera Actually I read that "Squash and merge" may use the author of the pull request as author for the squashed commit. Maybe that would work then.

I am unsure how that would behave exactly. So it would be great if you can cleanup your branch before we merge.
isaacs/github#1368

[robsiera]

I'm using SourceTree. By default SourceTree does not allow a push -force. But it has a setting allowing you to use it anyway.
So please confirm that all is Squach and Merged correctly now.

[DiscoPYF]

@robsiera Thank you, looks good. I use SourceTree as well but for some commands/worfklows it's just more efficient to use the command line directly.

I'll approve and we should be able to merge shortly 👍