[http-remoting] Serialize Conjure error parameters to JSON rather than with Objects.toString()

[mateuszlitwin]

Currently in Java implementation error parameters are serialized with Objects.toString() (see SerializableError.forException(ServiceException exception)). This leads to different formatting of the same error depending on the language used for the error implementation.

I suggest serializing parameters (safe/unsafe error arguments) as JSON - this representation is consistent across languages because parameters are Conjure types. This way it is also possible to deserialize parameter back into proper Conjure type.

Posting here given this should be expected behavior for all Conjure implementation.

[uschi2000]

@iamdanfox the wire format docs claims errors should be serialized using the JSON format, probably including errors. did you already have a plan here?

[sfackler]

What's the path forward here? conjure-typescript generates code that follows the spec, but won't be able to deserialize errors produced by conjure-java: https://github.com/palantir/conjure-typescript/blob/develop/src/commands/generate/tests/resources/errors/primitiveError.ts. I'm trying to figure out what to do in conjure-rust.

It seems like there are a couple of options:

• Fix conjure-java to JSON-encode parameters. This is an API break on its end due to the new signature of the parameters map in ServiceException, but brings it in line with the spec.
• Change the spec to use the PLAIN encoding for error parameters. This is a breaking spec change since it will be forced to disallow list/set/object/etc parameters.

[iamdanfox]

I'd prefer to keep the conjure error parameters as plain strings (i.e. a spec update), to discourage people from putting lots of deeply-nested information in their errors. I think we should steer them towards union response types instead.

Conjure errors have been quite tricky so far - for java clients, the ergonomics are pretty bad - you don't have any typed way of differentiating which error you received or what values it contained. There's also no enforcement for which endpoints will return which errors, so an exhaustive visitor becomes a bit impractical.

More importantly, I'm concerned about how to promote backwards compatiblity if users invest lots of effort in defining really detailed Conjure errors. The scenario I want to avoid is:

• backend supports two types of errors, ALREADY_EXISTS and LOCKED
• some frontend builds really detailed error handling for these conjure errors
• backend decides to send more granular information and stops sending LOCKED in favour of more granular LOCKED_PERMANENTLY and LOCKED_TEMPORARILY

In this scenario, the frontend's really detailed error handling probably stops working. If people start relying on these params, does this mean adding a new error type actually results in a functionality regression from a users point of view? How can frontend devs pre-empt this without compiler support. Are frontend devs really supposed to just react to constant reports of regressions in error-handling?

In summary, I'd prefer to keep the conjure error params as plain strings because I think expanding them to deeply nested JSON might have unintended consequences for API design and result in harder to maintain codebases further down the line.

[sfackler]

I thought the entire purpose of the new error system was so that clients could introspect into them and e.g. have localized templates of the various error conditions. If clients aren't supposed to understand anything about the contents of the errors, then why are we parameterizing them at all?

[abless]

I agree that it feels a bit contradictory. At the very least the current implementation is faulty.

@iamdanfox, instead of breaking the API what about introducing a new field (Map<String, Object> unserializedParameters)? That would not break existing clients yet allow clients read more complex types (where a complex type might be a simple as a list of integers).

[ferozco]

Caught up offline with @carterkozak and @iamdanfox to discuss a way forward on this. We arrived at there being two issues with service exceptions in Java land:

• Server produces errors with string params violating the spec compliant
• Client bindings for the errors assume that all params are errors and there is no type safe way to catch a specific type of service exception.

We decided that we could solve both issues at once by changing the generated error bindings. The new error bindings would have distinct client and server side types, extending from RemoteException and ServiceException respectively. Server side types would correctly serialize parameters as JSON and the client side types would allow for type safe interaction.

We would hide all of these changes behind a conjure-java feature flag since the server side change is a break, allowing individual product teams to opt in as necessary.

Unfortunately, we aren't able to prioritize this work right now, so in the short term this problem won't be going away.

[ash211]

It's ugly, but typescript code is now starting to handle this by parsing the Objects.toString() back out to a typescript array, like this:

// Conjure types lists of string in errors as string[], but they actually look like this:
// "[ri.foundry.main.dataset.1, ri.foundry.main.dataset.2]"
// So check if they're a string (for forwards-compatibility if it gets fixed) and then parse
// <snip internal issue link...>
export function conjureErrorRidsToRidsList(errorRids: string | string[]) {
    if (Array.isArray(errorRids)) {
        return errorRids;
    }
    // The array isn't valid JSON, we need to strip the square brackets and parse by hand
    const withoutBrackets = errorRids.substring(1, errorRids.length - 1);
    if (withoutBrackets.length === 0) {
        return [];
    }
    try {
        return withoutBrackets.split(", ");
    } catch (e) {
        console.error("conjureErrorRidsToRidsList: Unable to parse rids", e);
        return [];
    }
}

I intend to continue using this pattern until conjure-java follows the wire spec.

[ash211]

The problem is the Objects.toString here: https://github.com/palantir/conjure-java-runtime-api/blob/f1d8a5465ac7485ac93d19512644bdf6d110693d/errors/src/main/java/com/palantir/conjure/java/api/errors/SerializableError.java#L120.

It should instead be done with an ObjectMapper