palantir / conjure-java-runtime

Opinionated libraries for HTTP&JSON-based RPC using Dialogue, Feign, OkHttp as clients and Jetty/Jersey as servers
Apache License 2.0
79 stars 95 forks source link

Unexpected null body when deserializing empty optional serialized by conjure-undertow #1288

Open jonsyu1 opened 5 years ago

jonsyu1 commented 5 years ago

What happened?

com.palantir.logsafe.exceptions.SafeNullPointerException: Unexpected null body is thrown when calling a conjure-undertow server endpoint via conjure-retrofit.

Repro steps:

  1. conjure an endpoint with return type any
  2. have the conjure-undertow server return Optional.empty()
  3. use the conjure-retrofit client to call the endpoint

What did you want to happen?

The conjure-retrofit client should understand the conjure-undertow server when an Optional.empty() is serialized and deserialized.

carterkozak commented 5 years ago

In this case, I wonder if we should use returns: optional<any>?

jonsyu1 commented 5 years ago

This endpoint serves both old and new summaries. The old summaries could return raw types like optional but the new summaries return a conjure-defined struct or union type that wraps the primitive types.

Changing the endpoint to return optional<any> is not desirable because our new summaries will never return Optional.empty() and would require an API break.

JacekLach commented 5 years ago

an empty optional is surely within the domain of any. It seems reasonably that it might not serialize as a 204 and instead as null or whatever else - as long as, knowing to expect an optional in this particular case, the client can deserialize that object as an optional.

JacekLach commented 5 years ago

FYI we had to go back to conjure-jersey over this as it proved to be a bigger break than we thought

iamdanfox commented 5 years ago

Can I double check what the signature of your retrofit client was? I remember some tricky inconsistencies in null coercion a while ago.

Was it a Call<Object>?

carterkozak commented 5 years ago

I think the issue was conjure-undertow serializing json null for returns: any when the returned Object is an empty optional. The spec isn't really clear about what we expect here. optional<any> would allow this to work properly without a real wire break, but it would change bindings that may be used by existing consumers, blocking library upgrades.

iamdanfox commented 5 years ago

Honestly the whole thing feels super ambiguous because you can happily define Foo as an alias of optional, so then in a nice typed world the client could always know that 204 should map to Foo.of(Optional.empty()) but when the client doesn't have a useful signature (i.e. any), it's impossible to differentiate between the server returning Foo.of(Optional.empty()), Bar.of(Optional.empty()) and plain Optional.empty().

JacekLach commented 5 years ago

Isn't it always up to the code calling a conjure client to know what the exact type of an any endpoint is (presumably based on the query they sent) and deserialise properly? If I get an Object foo = endpoint.callAnyMethod() on the other side, and I know it should be optional, I think it's fine if foo is null instead of Optional.empty(); the same way that if callAnyMethod returns a conjure blob, I don't think foo would be of that type, and would be a Map<String, Object> instead?