Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jonsyu1 opened this issue Oct 23, 2019 · 8 comments
Labels

Comments

@jonsyu1
Copy link
Contributor

jonsyu1 commented Oct 23, 2019

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
Copy link
Contributor

carterkozak commented Oct 23, 2019

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

@jonsyu1
Copy link
Contributor Author

jonsyu1 commented Oct 23, 2019

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.

@jonsyu1 jonsyu1 pinned this issue Oct 23, 2019
@jonsyu1 jonsyu1 unpinned this issue Oct 23, 2019
@JacekLach
Copy link
Contributor

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
Copy link
Contributor

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

@iamdanfox
Copy link
Contributor

iamdanfox commented Nov 12, 2019

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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?

@jonsyu1 jonsyu1 added the bug label Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants