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

[FX-580] Add trailing slash to all requests #121

Conversation

dleis612
Copy link
Contributor

@dleis612 dleis612 commented Oct 4, 2023

What

Add trailing slashes to all http requests that were missing them.

Test Plan

I tested this locally by running through a checkout and looking at the datadog logs.

@@ -37,6 +37,7 @@ internal class EncryptionKeyService(
.newBuilder()
.addPathSegment(ForageConstants.PathSegment.ISO_SERVER)
.addPathSegment(ForageConstants.PathSegment.ENCRYPTION_ALIAS)
.addPathSegment("")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doing addPathSegment("") is not clear that we're adding a trailing slash.

Would doing something like adding this extension work:

// in `OkHttpClientBuilder.kt` or new file: `HttpUrlExtensions.kt`
fun HttpUrl.Builder.addTrailingSlash(): HttpUrl.Builder {
    return this.addPathSegment("")
}

// in encryption key service

 return httpUrl
            .newBuilder()
            .addPathSegment(ForageConstants.PathSegment.ISO_SERVER)
            .addPathSegment(ForageConstants.PathSegment.ENCRYPTION_ALIAS)
            .addTrailingSlash()
            .build()

alternatively, can you add comments for each call to addPathSegment("")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty cool optimization I didn't know was possible :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dleis612 dleis612 merged commit cd3a733 into main Oct 4, 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.

2 participants