Deserialize endpoint errors

[mpritham]

Before this PR

We'd like to support the a new method in BodySerDe that allows the creation of deserializers given an expected result type, and a set of expected error types.

A ConjureError is serialized and sent by servers. Dialogue deserializes the errors to a SerializableError which loses the type information of the parameters. By providing the type information of the parameters, Dialogue can deserialize the received ConjureError into the well-typed objects (instead of Strings).

After this PR

==COMMIT_MSG==

==COMMIT_MSG==

Possible downsides?

[changelog-app]

Generate changelog in changelog/@unreleased

What do the change types mean?

• feature: A new feature of the service.
• improvement: An incremental improvement in the functionality or operation of the service.
• fix: Remedies the incorrect behaviour of a component of the service in a backwards-compatible way.
• break: Has the potential to break consumers of this service's API, inclusive of both Palantir services
  and external consumers of the service's API (e.g. customer-written software or integrations).
• deprecation: Advertises the intention to remove service functionality without any change to the
  operation of the service itself.
• manualTask: Requires the possibility of manual intervention (running a script, eyeballing configuration,
  performing database surgery, ...) at the time of upgrade for it to succeed.
• migration: A fully automatic upgrade migration task with no engineer input required.

Note: only one type should be chosen.

How are new versions calculated?

• ❗The break and manual task changelog types will result in a major release!
• 🐛 The fix changelog type will result in a minor release in most cases, and a patch release version for patch branches. This behaviour is configurable in autorelease.
• ✨ All others will result in a minor version release.

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

Check the box to generate changelog(s)

• Generate changelog entry

[carterkozak]

Looks solid -- want to start on the codegen piece using an RC?

[mpritham]

Yep. Added do-not-merge. Need a +1 to cut the RC.

[carterkozak]

Approving to cut an RC for iteration with conjure-java codegen 👍

Invalidated by push of ae9dfff

[mpritham]

Actually instead of relying on reflection, we could use a custom deserializer while overriding getNullValue. I prefer this over using reflection. We could define the custom deserializer once in Conjure-Java in a shared utilities class:

abstract static class CustomNullDeserializer<T> extends JsonDeserializer<T> {
    public abstract T create();

    @Override
    public T deserialize(JsonParser _parser, DeserializationContext _ctxt) {
         throw ...
    }

    @Override
    public T getNullValue(DeserializationContext _ctxt) {
        return create();
    }
}

static class EmptyReturnValueDeserializer extends CustomNullDeserializer<EmptyReturnValue> {
    @Override
    public EmptyReturnValue create() {
        return new EmptyReturnValue();
    }
}

@Generated("by conjure-java")
@JsonDeserialize(using = EmptyReturnValueDeserializer.class)
record EmptyReturnValue() implements EmptyBodyEndpointReturnBaseType {}

[mpritham]

Updated.

[carterkozak]

This has quite a bit in common with our handling of null-to-empty coercion used for empty optionals (and aliases of optionals) when endpoints return 204 with no content -- we have to handle explicit null tokens and empty streams the same way.
I wonder if we could reuse any of that code here?

[carterkozak]

There's a trade-off between reflection, and a great deal of additional codegen we have to format/ship/compile/etc. In practice this deserializer codegen asks jackson to use reflection in a way that jackson understands, which may be much more verbose than writing something more specific into the deserializer.

[mpritham]

I wonder if we could reuse any of that code here?

I realize I didn't make a new commit when I replaced the old version with the custom deserializer approach. Updated with an implementation that refactors JacksonEmptyContainerLoader#constructEmptyInstance.

In practice this deserializer codegen asks jackson to use reflection in a way that jackson understands, which may be much more verbose than writing something more specific into the deserializer.

I agree. It is more generated code that needs to be packaged and compiled. I was initially hesitant on adding additional special handling here (i.e. do X if there is a 0-parameter static factory annotated with JsonCreator) because I thought it'd be nice to separate the concerns - the client code (Conjure-Java) that calls into Dialogue would have to be responsible for specifying how null values should be deserialized. But, ultimately this is used in the implementation of ConjureBodySerDe: the coupling is expected, and as you pointed out, there is some special handling for optionals and collections already.

[mpritham]

I also added methods to deserialize input streams and optional input streams. The idea is that the type T.Success here has a constructor with InputStream as the only parameter, and on a successful binary response body, a SuccessT(response.body()) will be returned.

[carterkozak]

in the else case, we should throw rather than returning a null ref that will cause something else to throw in a more confusing way later on!

I might suggest splitting this up a bit as well: We should be able to do the validation on successT.getType() once when the deserializer is first created, as well as the .getConstructor(<arg>), that way we only need to execute newInstance on a per-response basis.

[carterkozak]

Perhaps we check clazz.getRecordComponents().length == 0 to ensure we're not seeing a record with fields and a special no-arg ctor?

I prefer to be as precise as possible for these sorts of things, otherwise we can allow unexpected behavior

[mpritham]

Makes sense. I didn't think of that case.

[carterkozak]

I don't think the ConjureErrors type buys us anything here, can we remove that and make BaseEndpointError the top-level class? Might consider renaming it to EndpointError, I don't think Base adds a ton