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

fix: added url path encoding #376

Merged
merged 4 commits into from
Aug 31, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions Sources/Common/Extensions/StringExtensions.swift
Expand Up @@ -31,6 +31,11 @@ public extension String {
String((0 ..< length).map { _ in abcLetters.randomElement()! })
}

static func randomStringWithSpecialCharacters(length: Int = 20) -> String {
Copy link
Member

Choose a reason for hiding this comment

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

If we do end up using this function, move it into Tests/Shared since it's a function used only for tests.

let characters = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-._~:/?#[]@!$&'()*+,;=%"
return String((0 ..< length).map { _ in characters.randomElement()! })
}

/**
Checks if the string matches the regex pattern.

Expand Down
3 changes: 2 additions & 1 deletion Sources/Common/Service/CIOApiEndpoint.swift
Expand Up @@ -53,7 +53,8 @@ public extension CIOApiEndpoint {
baseUrl = String(baseUrl.dropLast())
}

return baseUrl + path
let fullPath = baseUrl + (path.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed) ?? path)
return fullPath
}
}

Expand Down
25 changes: 25 additions & 0 deletions Tests/Common/Service/HttpEndpointTest.swift
Expand Up @@ -44,4 +44,29 @@ class HttpEndpointTest: UnitTest {

XCTAssertEqual(actual, expected)
}

func test_getUrlString_givenRandomPathNeedsEncoding_expectEncodedPath() {
let identifierWithSpecialChar = String.randomStringWithSpecialCharacters()
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer if we hard-coded test data instead of randomly generate it. Maybe the randomly generated string gives us a char with no special chars, for example.

The other test function is great in that it includes a special character that we have identified causes issues for customers. I like the idea of hard-coding test strings that we know could cause an issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point, but random values does extend range for testing more characters. We can perhaps update randomStringWithSpecialCharacters to must include special character every time.

Copy link
Member

Choose a reason for hiding this comment

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

What if we hard-code abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-._~:/?#[]@!$&'()*+,;=% as the identifier?

I think that would do the same thing as randomStringWithSpecialCharacters if not be more valuable because every time we run the test suite, we are checking all special characters intead of some.

let endpoint = CIOApiEndpoint.identifyCustomer(identifier: identifierWithSpecialChar)

let expectedIdentifier = identifierWithSpecialChar.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed) ?? identifierWithSpecialChar
let expected = "https://customer.io/api/v1/customers/\(expectedIdentifier)"

setHttpBaseUrls(trackingApi: "https://customer.io")

let actual = endpoint.getUrlString(baseUrls: httpBaseUrls)

XCTAssertEqual(actual, expected)
}

func test_getUrl_givenUnencodedPathWithSpecialCharacter_expectValidUrl() {
let identifierWithSpecialChar = "social-login|1234567890abcde"
let endpoint = CIOApiEndpoint.identifyCustomer(identifier: identifierWithSpecialChar)

setHttpBaseUrls(trackingApi: "https://customer.io")

let actualUrl = endpoint.getUrl(baseUrls: httpBaseUrls)

XCTAssertNotNil(actualUrl, "Expected valid URL but got nil. Ensure path is encoded correctly.")
levibostian marked this conversation as resolved.
Show resolved Hide resolved
}
}