Exceptions thrown do not match documentation

[oliverjanik]

Here's one example:

https://rethinkdb.com/api/java/db_create/ says it will throw ReqlRuntimeError if the database exists.

I get AggregateException with inner ReqlDriverError which says:

The query response cannot be converted to an object of T or List<T>. This run helper works with SUCCESS_ATOM results. The server response was RUNTIME_ERROR. If the server response can be handled by this run method try converting to T or List<T>. Otherwise, if the server response cannot be handled by this run helper use another run helper like `.runCursor` or `.runResult<T>`.

The exceptions thrown are part of the API, I suggest they line up with the Java driver.

As a side note there seems to be a bit of schism in the driver w.r.t. error handling most of the regular operations like Insert or Update return errors in the Result whereas DbCreate or TableCreate just throw. Is there any rationale behind this divide?

Edit:

I've hooked up the TRACE from driver. This is a different exception:

 JSON Recv: Token: 1, JSON: {"t":18,"e":3000000,"r":["Database name `test-db` invalid (Use A-Za-z0-9_ only)."],"b":[0]}

So it obviously doesn't like hyphens in database names. Can this error message be exposed in ReqlDriverError? Without trace there's now way to tell what went wrong.

Edit 2:

My original statement stands. When database exists I get AggregateException->ReqlDriverError and the TRACE says this:

 JSON Recv: Token: 1, JSON: {"t":18,"e":4100000,"r":["Database `diabetes_australia` already exists."],"b":[]}

[bchavez]

Hey Oliver, how are you doing? Hope you're doing well. Sorry for the delayed response, still on vacation. Be back at work around April 25th or so. I'll try to answer as best I can. Thanks VERY much for the JSON traces -- they help a bunch.

First, it looks like you're using a Run Helper since the error message looks like:

The query response cannot be converted to an object of T or List<T>.
This run helper works with SUCCESS_ATOM results.....

Run Helpers aren't part of the official API, so I'm giving myself a little wiggle room here. :wink: :wink: .

_I think_, what's happening here is { "t":18 is an error response from the server and the .RunHelper*<T> is expecting a non-error response so what you're getting instead is a ReqlDriverError because the .RunHelper can't magically convert an error response into a data response you request (possibly .RunResult).. IIRC, the Run Helpers don't do any checks for error responses instead they make a direct attempt to convert the JSON on the wire to the type you're expecting.

If you use the dynamic .Run method, you should get the expected exception you're looking for and error responses should line up with the Java driver.

Regardless, you bring up a good usability issue. We should probably check for error responses before attempting a direct Run Helper conversion.

Also, the core of the driver is async so the TPL/CLR will generate an AggregateException wrapping the underlying exception. I'm not sure what the official guidance is on unwrapping these exceptions for framework libraries. I don't have my reference books with me at the moment. I'll have to check when I get home.

Hope this helps, Brian

[oliverjanik]

Thanks for the response, yes I am using the non dynamic versions like RunResult or RunAtom. I prefer not to deal with dynamic unless absolutely necessary.

W.r.t AggregateException, I don't mind them if they're documented. I'm not sure if there's a standard practice for C# libraries, but dealing with actual exceptions is way more friendly.

[bchavez]

Thanks again @oliverjanik. This is now fixed in master. Next release will contain the fix. We'll check and throw the response error for all Run Helpers before bottoming out with a ReqlDriverError in case of a mis-matched conversion.

[oliverjanik]

Did you keep AggregateExceptions? Or do you unwrap them?

[bchavez]

Originally, I kept them wrapped in AggregateException. But I had a change of heart :heart: after thinking about it driving home. They are now unwrapped when used against the synchronous API and work as naturally expected in a synchronous world. Hehe.... catch that ? "a synchronous" :sunglasses:

[oliverjanik]

Any ETA on the release? Would make my life easier.

[bchavez]

@oliverjanik v2.3.1-beta-1 released with changes thus far. Keyword: beta. There's some code reviewing in master that I still haven't finished. Also, have not tested new authentication in .NET Core yet.

[oliverjanik]

OK, I'll give it a spin. Thank you very much.