-
Notifications
You must be signed in to change notification settings - Fork 17
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
Safe URL encoding #38
Conversation
@@ -41,19 +41,6 @@ final class PollerTests: XCTestCase { | |||
XCTAssertEqual(poller.etag, "W/\"7c-GUwjw43L+nPpd9TY5PHtsXJueiM\"") | |||
} | |||
|
|||
func testStartCompletesWithUrlError() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this scenario is impossible now
@@ -40,19 +40,22 @@ public class UnleashClientBase { | |||
var metrics: Metrics | |||
|
|||
public init(unleashUrl: String, clientKey: String, refreshInterval: Int = 15, metricsInterval: Int = 30, disableMetrics: Bool = false, appName: String = "unleash-swift-client", environment: String? = nil, poller: Poller? = nil, metrics: Metrics? = nil) { | |||
guard let url = URL(string: unleashUrl), url.scheme != nil else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sure URL passed to the client is correct so that we eliminate impossible URLs down the line
return unleashUrl + "?" + params.joined(separator: "&") | ||
func formatURL(context: [String: String]) -> URL? { | ||
var components = URLComponents(url: unleashUrl, resolvingAgainstBaseURL: false) | ||
components?.queryItems = context.map { key, value in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
safe URL manipulation instead of string concatenation. No matter what strings we put into context the URL is always correctly escaped
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Very happy to see the use of URL instead of String where possible 🙌🏼 Does this change our public API? I would guess this requires a major version update before it can be released?
@thomasheartman Actually I deliberately left the public API of the Unleash client as string and we turn it into a URL in the very first step, failing otherwise. API of the poller changes but it's used internally. |
About the changes
Important files
Discussion points