search

palantir / dialogue

A client-side RPC library for conjure-java
Apache License 2.0
29 stars 14 forks source link

Can't call service that requires trailing slash #1503

Open gatesn opened 2 years ago

gatesn commented 2 years ago

What happened?

The readme suggests I can use Dialogue to call non-Conjure servers, but I've run into a bit of trouble: https://github.com/palantir/dialogue/tree/96ae91db83aa9ccdc5939a934ab84b33691ec240#dialogue-annotations-processor-generated-client-bindings

Internally, Dialogue passes paths around as a list of segments, throwing away information about whether or not a trailing slash exists: https://github.com/palantir/dialogue/blob/c8c1a7fae13eb446a8ca0b779e23d48ef89fb3ee/dialogue-annotations-processor/src/main/java/com/palantir/dialogue/annotations/processor/data/HttpPath.java#L23

This results in the server issuing a 301 redirect to the path ending in a slash. However, the Apache client is configured to ignore redirects: https://github.com/palantir/dialogue/blob/dffea948719b5f810f5edf8c10e52d52d41d20ab/dialogue-apache-hc5-client/src/main/java/com/palantir/dialogue/hc5/ApacheHttpClientChannels.java#L547

I thought about implementing a channel to handle the 301 and resubmit the request with the slash, but I think the Channel API is too high-level since it also talks in terms of UrlBuilders, and therefore path segments.

It also seems there's a lot of logic wrapped up in ApacheHttpClientChannels.ClientBuilder that I don't (or can't, due to being package-private) want to replicate to enable redirects on the Apache client.

What did you want to happen?

I think I agree with the principle of no redirects, so it perhaps makes sense to support trailing slashes rather than 301s in this case?

My proposal might be to have HttpPath capture whether or not there exists a trailing slash (special-casing trailing slashes, vs any other duplicate slashes in the path). PathTemplate can then have a method to append a slash, templated in to the generated classes by the EndpointsEnumGenerator.

Unfortunately, I imagine this could be a breaking change for anyone relying on the current path template parsing behaviour...

carterkozak commented 2 years ago

We should be able to support explicit trailing slashes with the annotation processor by providing a final empty-string path segment (much like an empty path parameter). There's some ambiguity when the endpoint is the root of the api path, because we tend to assume path=/ means no trailing slash, so that would be a little bit special and I'm not sure we should support that path directly. I imagine we may have some normalization that occurs in our parser which prevents this from occurring out of the box.

Supporting this sort of redirect can be dangerous in less obvious ways, for example a non-retryable binary upload may be burned on the first request attempt, while other types of requests would work just fine in this case, so you're correct that we'd prefer not to support that.

gatesn commented 2 years ago

Can now workaround the trailing slashes using the annotation processor API, but it would still be useful to say "follow redirects", perhaps that could be a parameter to com.palantir.dialogue.annotations.Request ?