error_data

[davidgovea]

Attempts to implement #296

Based on comment here, this might not be the correct approach (for api/ specifically?):

I'm not sure yet whether error_data should be exposed directly or wrapped in a JS Error

This PR just takes the expose-directly approach.
Some of the tests assert against errors thrown by appenders, so maybe you're thinking of integrating the error codes into that process more natively?

[jraymakers]

Yeah, I've been unsure how best to expose the new information in error_data.

Up to now, I've taken the approach of checking for errors in the C layer and throwing exceptions across the C/JS boundary instead of returning a value (e.g. duckdb_state) that needs to be checked. This seems more appropriate for a JS API.

I think this is still the right approach for the api layer, but I'm open to other ideas for the bindings layer, because the latter is a JS API that's meant to adhere closely to a C API.

Checking for errors and throwing exceptions across the C/JS boundary works fine when the only information that needs to be conveyed is the error message, because the Napi::Error class supports this. But error_data contains a type as well, and that's not supported out-of-the-box by Napi::Error. So another approach is needed.

Some options:

• Find a way to subclass Napi::Error, in the C code, so it can include the type. (I'm unsure if this is possible, or, if possible, if it comes with downsides.) Continue checking for errors and converting them to exceptions, extracting the type from error_data when available.
• Expose the error_data functions through the bindings layer, as you've done here. Continue to throw exceptions; the error_data functions allow getting more detail (e.g. the error type) if desired.
• Change the bindings layer to not throw exceptions, and instead adhere more closely to the C API, by returning duckdb_state where the underlying C API does, and requiring additional functions to be called to get error information (such as the error_data functions). This would require a fair amount of work to change existing code, and would be a breaking change. Also, solutions would need to be engineered for errors that occur in helper functions or in async workers.

I'm not inclined to take this third option. Either of the first two seem reasonable.

Certainly the least effort is the second, not only because it's the one you've implemented in this PR, but also because it doesn't change any existing APIs. The only downside is the somewhat strange API, where the thrown exception doesn't contain all relevant information, and a separate function call is required to get the additional information.

I'd be interested to explore whether a custom Napi::Error subclass is possible. If we (as I propose) are going to keep throwing exceptions across the C/JS layer, then that would be a nice mechanism to have in general, not just for this reason.

Regardless of which approach we take at the bindings layer, I feel more strongly that, at the api layer, we should throw exceptions that contain all relevant information. Creating and throwing custom Error subclasses is much easier at this layer, because it's pure JS.

If we're able to create a custom Napi::Error subclass, then we can just let that exception pass through the api layer as well, as is done today with the Napi::Errors thrown currently. If that doesn't pan out, then, to throw an Error subclass at the api layer, we may need to catch the underlying error, query error_data, and throw a new (JS-defined) custom Error. That's requires more code for each error-producing function in the api layer, which I don't love.

So, before going too far with error_data, I'd like to resolve the question of the Napi::Error subclass. Let me know if that's something you're interested in exploring or not.

[davidgovea]

sure, I can play around with it. I also don't see any way to do exception->custom JS error, but you can attach arbitrary keys to the exception that get carried over, so there is some opportunity to do error checks in C. Perhaps error wrapping could be avoided with hacked types..
I will sketch a few approaches out