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

binary alias return type appears to have the wrong content-type header #711

Closed
robqliu opened this issue Sep 29, 2020 · 4 comments
Closed

Comments

@robqliu
Copy link

robqliu commented Sep 29, 2020

What happened?

I'm using the dialogue client and it threw this exception when attempting to hit a conjure endpoint of an undertow server whose return type was an alias for 'binary':

ERROR [2020-09-29T06:35:04.005393Z] <redacted> {}. (<redacted>, throwable0_received: application/json, throwable0_supportedEncodings: [EncodingDeserializerContainer{encoding=BinaryEncoding{application/octet-stream}, deserializer=InputStreamDeserializer{}}])
com.palantir.logsafe.exceptions.SafeRuntimeException: Unsupported Content-Type
        at com.palantir.conjure.java.dialogue.serde.ConjureBodySerDe$EncodingDeserializerRegistry$1.deserialize(ConjureBodySerDe.java:298)
        at com.palantir.conjure.java.dialogue.serde.ConjureBodySerDe$EncodingDeserializerRegistry.deserialize(ConjureBodySerDe.java:257)
        at com.palantir.dialogue.futures.DialogueDirectTransformationFuture.onSuccess(DialogueDirectTransformationFuture.java:103)
        at com.google.common.util.concurrent.Futures$CallbackListener.run(Futures.java:1021)
        at com.palantir.dialogue.futures.SafeDirectExecutor.execute(SafeDirectExecutor.java:32)
        at com.google.common.util.concurrent.AbstractFuture.executeListener(AbstractFuture.java:1137)
        at com.google.common.util.concurrent.AbstractFuture.complete(AbstractFuture.java:957)
        at com.google.common.util.concurrent.AbstractFuture.set(AbstractFuture.java:726)
        at com.google.common.util.concurrent.AbstractFuture.complete(AbstractFuture.java:957)
        at com.google.common.util.concurrent.AbstractFuture.set(AbstractFuture.java:726)
        at com.google.common.util.concurrent.SettableFuture.set(SettableFuture.java:47)
...
        Suppressed: com.palantir.logsafe.exceptions.SafeRuntimeException: Rethrown by dialogue
                at com.palantir.conjure.java.dialogue.serde.DefaultClients.block(DefaultClients.java:124)
...

I was able to fix this by directly returning the binary type in my endpoint:

      <redacted>:
        http: PUT /<redacted>
        args:
          request:
            param-type: body
            type: <redacted>
        returns: binary

to be clear, it looked like this before

      <redacted>:
        http: PUT /<redacted>
        args:
          request:
            param-type: body
            type: <redacted>
        returns: Foo

      Foo:
        alias: binary

What did you want to happen?

Seems like binary aliases should work as return types. Not sure if this is a bug in the dialogue client or in the undertow endpoint generated.

@bmoylan
Copy link
Contributor

bmoylan commented Oct 1, 2020

+1 I noticed this recently when trying to update conjure-go to correctly reflect the conjure-verification BinaryAliasExample test cases. The verifier is expecting these to be JSON serialized (quoted, base64, json content-type) where I expected to match the behavior for plain binary types.

@carterkozak
Copy link
Contributor

@bubchi89 Yep, this is a bug where we fail to dealias the top-level type, and incorrectly expect json instead of application/octet stream. Let's move this ticket to the correct project please https://github.com/palantir/conjure-java

@bmoylan I think the verifier is broken in a similar way. Would you mind filing an issue on the https://github.com/palantir/conjure-verification project?

@bmoylan
Copy link
Contributor

bmoylan commented Oct 1, 2020

Actually it looks like the verifier issue was already filed here: palantir/conjure-verification#296

@carterkozak
Copy link
Contributor

Closing this in favor of palantir/conjure-java#1099
fix is up: palantir/conjure-java#1102

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants