Skip to content

Conversation

@aug24
Copy link

@aug24 aug24 commented Aug 8, 2018

This is a trivial change which ensures the user has a leading slash on the URI.

As currently written, the user can pass a request without a slash. Without this, the request is not strictly valid according to the HTTP spec. Nonetheless, requests to ES still work, but requests via an AWS ELB/ALB fail.

@colings86 colings86 added >bug :Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch labels Aug 9, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@javanna
Copy link
Member

javanna commented Aug 10, 2018

We have discussed this in the past, and if I remember correctly we said we are not in favor of modifying the request by adding /. That sounds like a subtle change to what users request, as users are providing the requests and can prepend them with / if they need to (see #30119 (comment) for this discussion). We did encounter this problem though with proxies and we went for a slightly different solution, which allows to set pathPrefix to / in case we want to add a / prefix to every request when missing. See #30119.

That said I am closing this PR, but thanks a lot for opening it, and hopefully you will find some other improvement to work on ;)

@javanna javanna closed this Aug 10, 2018
@javanna javanna removed the >bug label Aug 10, 2018
@aug24
Copy link
Author

aug24 commented Aug 13, 2018

I've done some more digging, and I would like to discuss this further if that's OK? I believe the current position is inconsistent.

In RestClientBuilder.setPathPrefix, there is the following code:

if (cleanPathPrefix.startsWith("/") == false) {
    cleanPathPrefix = "/" + cleanPathPrefix;
}

ie paths are forced to have a leading slash.

There is also this:

if (cleanPathPrefix.isEmpty() || "/".equals(cleanPathPrefix)) {
    throw new IllegalArgumentException("pathPrefix must not be empty or '/': [" + pathPrefix + "]");
}

ie you cannot simply handle the trivial case of "always prepend a slash" because the code actually doesn't let you.

As far as I can tell, there is no correct use case for the user providing a relative path URI without a pathPrefix. If we don't automatically fix this error (per the fix I suggested) then we should throw an exception with advice that either a pathPrefix or a leading slash is required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Clients/Java Low Level REST Client Minimal dependencies Java Client for Elasticsearch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants