Skip to content

Conversation

@saltanovas
Copy link
Member

@saltanovas saltanovas commented Jun 27, 2023

Starting with Ktor 2.x, leading and trailing slashes are extremely important:

Note that trailing slash in base URL and leading slash in request URL are important. The rules to calculate a final URL:

  1. Request URL doesn't start with slash
    • Base URL ends with slash -> concat strings. Example: base = https://example.com/dir/, request = file.html, result = https://example.com/dir/file.html
    • Base URL doesn't end with slash -> remove last path segment of base URL and concat strings. Example: base = https://example.com/dir/deafult_file.html, request = file.html, result = https://example.com/dir/file.html
  2. Request URL starts with slash -> use request path as is. Example: base = https://example.com/dir/deafult_file.html, request = /root/file.html, result = https://example.com/root/file.html

There is io.ktor.http.appendPathSegments extension method, but it uses segments.flatMap { it.split('/') } under the hood instead, which would add two slashes then

@saltanovas saltanovas self-assigned this Jun 27, 2023
@spoonman01
Copy link
Contributor

@saltanovas What was the actual issue here? As far as I know we do not access static files from kevin-jvm and do not do any path concatenation.

Paths are all specified with a leading slash and no trailing slash
Does no trailing slash in ktor 2 point to different URLs than ktor 1 did ?
If that is the case, wouldn't the the IgnoreTrailingSlash plugin fix this without changing other code ?

@stosik
Copy link
Collaborator

stosik commented Jun 28, 2023

I would also think of a more client-wide solution, one small oversee and path can be propagated wrongly, either adding a plugin or maybe defaultRequest with custom UrlBuilder implementation, then this specific configuration would be project-wide

@saltanovas
Copy link
Member Author

The reason why I haven't installed the IgnoreTrailingSlash plugin is that it would be additional dependency for the ones who uses the library, so I wanted to have as plain solution as possible. Or you guys don't see it as a problem?

I will give a try @stosik second suggestion

@annasp
Copy link

annasp commented Jun 28, 2023

@saltanovas I updated the version of the kevin-jvm on whitelabel demo app the previous week and I found the issue. I just tested the app locally using your current fix and it works as expected. Just a confirmation that the current PR fixes the issue.

@grantas33
Copy link
Contributor

grantas33 commented Jun 28, 2023

could we just remove leading slashes from all the paths in eu.kevin.api.Endpoint for this to work without custom extension function appendPath? Also unrelated, but we could probably simplify how we set baseUrl in io.ktor.client.plugins.DefaultRequestKt#defaultRequest by using url("$apiUrl/.${Endpoint.VERSION}/")

@saltanovas
Copy link
Member Author

I would choose what @grantas33 propose 😄 because after testing things, IgnoreTrailingSlash does not affect HttpClient requests, and the way how paths are being resolved is just part of default URLBuilder :(

url("$apiUrl/.${Endpoint.VERSION}/") does not resolve uri. Current solution was chosen so that if one defines apiUrl, trailing slash would be solved correctly

@saltanovas saltanovas merged commit f96175b into getkevin:master Jun 29, 2023
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

Successfully merging this pull request may close these issues.

5 participants