Unify Configuration Management #241

jaceklyp · 2023-02-22T17:16:51Z

Task/Issue URL: https://app.asana.com/0/72649045549333/1203797041821336/f
iOS PR: duckduckgo/iOS#1512
macOS PR: duckduckgo/macos-browser#990
What kind of version bump will this require?: Major

Description:
Use common API and Configuration modules located in BSK for fetching privacy related assets.
Add payload validation.

Steps to test this PR:

Run tests
Check if app downloads assets.
Check if app sends etag in requests.
Check if app handles 304s.
Check if app stores the resources.

Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

…side enum

ayoy · 2023-02-23T14:32:31Z

Hi @jaceklyp! I'm getting around to checking your code. Have just compiled and run it for the first time and I figured it would be great to have more debug logging for network requests. Perhaps controlled by a boolean flag. WDYT?

bwaresiak · 2023-02-24T11:04:11Z

Sources/API/APIHeaders.swift

+
+    public var defaultHeaders: HTTPHeaders {
+        guard let userAgent = APIHeaders.userAgent else {
+            fatalError("Please set the userAgent before accessing defaultHeaders.")


This is time-sensitive, based on when setUserAgent is called, right? Shouldn't it rather be an assertion, to not crash the app on production?

bwaresiak · 2023-02-24T13:01:11Z

Sources/API/Extensions/URLRequestAttribution.swift

+import Foundation
+import Common
+
+public enum URLRequestAttribution {


Is that used? Was that used? :)

bwaresiak · 2023-02-24T13:04:36Z

Sources/API/HTTPConstants.swift

+    case trace = "TRACE"
+    case patch = "PATCH"
+
+}


Shouldn't these be put in a tighter scope instead of on Module level? E.g. under APIRequest

bwaresiak · 2023-02-24T13:05:25Z

Sources/Configuration/Configuration.swift

+
+import Foundation
+
+public protocol ConfigurationURLProvider {


ConfigurationURLProviding ?

bwaresiak · 2023-02-24T13:08:37Z

Sources/API/Extensions/HTTPURLResponseExtension.swift

+    enum Constants {
+
+        static let weakEtagPrefix = "W/"
+        static let successfulStatusCodes = Array(200..<300)


Maybe use Range instead?

bwaresiak · 2023-02-24T13:09:21Z

Sources/API/Extensions/HTTPURLResponseExtension.swift

+        }
+    }
+
+    func etag(shouldDropWeakPrefix: Bool) -> String? {


droppingWeakPrefix ?

Tests/APITests/APIRequestTests.swift

ayoy

I noticed I’ve had 2 pending comments. Both rather minor, but perhaps worth considering. Dropping them now before I jump into reviewing the rest.

ayoy · 2023-02-23T17:42:42Z

Sources/API/APIResponseRequirements.swift

+
+    public static let nonEmptyData = APIResponseRequirements(rawValue: 1 << 0)
+    public static let etag = APIResponseRequirements(rawValue: 1 << 1)
+    public static let allow304 = APIResponseRequirements(rawValue: 1 << 2) // setting this overrides nonEmptyData since urlSession will actually return empty data


I'd prefer to either have docs for all the options, or user more descriptive naming. Ideally both :) I'd like the options to tell what the Requirement actually enforces, i.e:

requireNonEmptyData

requireETagHeader

allowHTTPNotModified

ayoy · 2023-02-24T10:52:55Z

Sources/API/APIRequest.swift

+        return (data, httpResponse)
+    }
+
+    private func getHTTPResponse(from response: URLResponse?) throws -> HTTPURLResponse {


I think you could remove this method and just define URLResponse.asHTTPURLResponse as throwable function.

ayoy

LGTM! 👍 Leaving a bunch of final comments, I’ve also tested macOS and it works great. Will check the code now.

ayoy · 2023-03-06T20:35:18Z

Sources/Configuration/ConfigurationFetching.swift

+    /**
+     Downloads and stores the configurations provided in parallel.
+
+     - Parameters:
+        - configurations: An array of `Configuration` enums that need to be downloaded and stored.
+
+     - Throws:
+        If any configuration fails to fetch or validate, an `Error` of type `.aggregated` is thrown.
+        The `.aggregated` case of the `Error` enum contains a dictionary of type `[Configuration: Error]`
+        that associates each failed configuration with its corresponding error.
+
+     - Important:
+        If any task fails, the error is recorded but the group continues processing the remaining tasks.
+        The task group is not cancelled automatically when a task throws an error.
+     */


Right now this documentation applies to aggregatedError (when you option+click it). Please move aggregatedError elsewhere so that it applies to fetch(any:).

ayoy · 2023-03-06T21:13:00Z

Sources/Configuration/ConfigurationFetching.swift

+
+protocol ConfigurationFetching {
+
+    func fetch(any configurations: [Configuration]) async throws


I’d suggest fetch(anyOf:) and fetch(allOf:) but I’m not strong on that.

ayoy · 2023-03-06T21:20:46Z

Sources/Networking/APIRequest.swift

+        self.urlSession = urlSession
+        self.log = log
+
+        assertUserAgentIsPresent()


ayoy · 2023-03-06T21:24:44Z

Sources/Networking/APIRequest.swift

+        }
+    }
+
+    /// This method is deprecated. Please use the 'fetch()' async method instead.


If it’s only used on iOS, perhaps worth guarding this function with #if os(iOS)?
Or maybe add the deprecation notice to the os_log too?

ayoy · 2023-03-06T21:30:41Z

Sources/Networking/APIResponseRequirements.swift

+    /// Allows HTTP Not Modified responses.
+    /// Setting this overrides requireNonEmptyData since urlSession will actually return empty data.


How about the following docs:

Allows HTTP 304 (Not Modified) response status code. When this is set, requireNonEmptyData is not honored, since URLSession returns empty data on HTTP 304.

ayoy · 2023-03-06T21:35:19Z

Tests/ConfigurationTests/ConfigurationFetcherTests.swift

+
+    let privacyConfigurationData = Data("Privacy Config".utf8)
+
+    // Tests for fetch(any: [Configuration])


Cosmetic: I'd use // MARK: - here

ayoy · 2023-03-06T22:08:23Z

Sources/Networking/Extensions/URLSessionExtension.swift

+    private static let mainThreadCallback = URLSession(configuration: .default, delegate: nil, delegateQueue: OperationQueue.main)
+    private static let mainThreadCallbackEphemeral = URLSession(configuration: .ephemeral, delegate: nil, delegateQueue: OperationQueue.main)
+
+    public static func makeSession(useMainThreadCallbackQueue: Bool = false, ephemeral: Bool = true) -> URLSession {


One more thing, sorry – I’d avoid calling this function makeSession, because it’s not making anything. It only picks one of the existing sessions.

I’ve noticed it while reading through macOS app code where it looks like we were creating sessions with every call. How about just session(useMainThreadCallbackQueue: Bool = false, ephemeral: Bool = true)?

# By Alexey Martemyanov (8) and others # Via GitHub * main: Make FeatureName a struct so it can be extended from client code (#276) add count of all bookmarks in domain to view model (#272) Configuration updates optimizations (#269) Roll-back InteractiveUserScript (#268) Update C-S-S to 4.4.4 (#260) Add invalid payload pixel handler (#264) Fix Error pixel misfiring on valid scenario (#261) add didCancelNavigationAction (#262) Opt-outable private API usage (#254) HTTPS upgrade threading (#256) DDGSync module (#253) fix delayed new window with empty url creation handling (#255) remove major tracker network (#251) Enable username&password passed in URL (#245) fix assertion for popup about:blank navigations without WKNavigation (#247) fix about: scheme fragments dropping (#246) Unify Configuration Management (#241) # Conflicts: # Package.resolved # Package.swift

jaceklyp added 25 commits February 10, 2023 09:12

Configuration PoC

156dd1d

More tests, and cleaner API

0b0e725

Add more tests

3dae118

Add payload validation

5f3e07c

Get rid of ConfigurationTasks and add possibility to override urls in…

4be949c

…side enum

Move code around

a1a13dd

Move onDidStore to fetch method

bf766e7

Merge branch 'main' into jacek/unify-configuration

0bb3ea5

APIRequest refactor part1

e240c13

Continue refactoring

2261fe2

Use OptionSet instead of array of options

147a030

Fix tests

b78d5e2

Add more tests

2b4d58f

Create ConfigurationURLProvider to customize urls for Configuration

cf88a48

Fix typo

25006d4

Provide setUserAgent method

2ebbd14

Merge branch 'main' into jacek/unify-configuration

c8242f9

Fix deployment target

156654d

Fix compilation errors

35c33d7

Fix tests failing on broken locale

0ccd2ff

Error messages + handling 304

f3650bc

Fix for 304 handling logic

dfe4307

Update tests

6bfae72

Fix tests

a493f4c

Fix naming

4a37765

jaceklyp requested review from brindy, bwaresiak, mallexxx, THISISDINOSAUR and tomasstrba as code owners February 22, 2023 17:16

jaceklyp requested a review from samsymons as a code owner February 22, 2023 17:16

Fix test

d379db2

ayoy self-assigned this Feb 23, 2023

bwaresiak reviewed Feb 24, 2023

View reviewed changes

Tests/APITests/APIRequestTests.swift Outdated Show resolved Hide resolved

Address PR comments

c971ba9

ayoy reviewed Mar 6, 2023

View reviewed changes

jaceklyp added 4 commits March 6, 2023 12:43

Address PR comments #2

e3ba7d9

Remove prints

74333ef

Fix warnings and add logger option

219280d

Make Error public

c2a4ff1

ayoy approved these changes Mar 6, 2023

View reviewed changes

ayoy reviewed Mar 6, 2023

View reviewed changes

jaceklyp added 4 commits March 7, 2023 12:20

Add single configuration fetch method

54010a3

Get rid of fetch(any:) method

2f3dee2

Merge branch 'main' into jacek/unify-configuration

2f3bd90

Fix compilation errors

351d4d9

ayoy assigned jaceklyp and unassigned ayoy Mar 8, 2023

jaceklyp merged commit c10bf52 into main Mar 8, 2023

jaceklyp deleted the jacek/unify-configuration branch March 8, 2023 11:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify Configuration Management #241

Unify Configuration Management #241

jaceklyp commented Feb 22, 2023 •

edited

Loading

ayoy commented Feb 23, 2023

bwaresiak Feb 24, 2023

bwaresiak Feb 24, 2023

bwaresiak Feb 24, 2023

bwaresiak Feb 24, 2023

bwaresiak Feb 24, 2023

bwaresiak Feb 24, 2023

ayoy left a comment

ayoy Feb 23, 2023

ayoy Feb 24, 2023

ayoy left a comment

ayoy Mar 6, 2023

ayoy Mar 6, 2023

ayoy Mar 6, 2023

ayoy Mar 6, 2023

ayoy Mar 6, 2023

ayoy Mar 6, 2023

ayoy Mar 6, 2023


		protocol ConfigurationFetching {

		func fetch(any configurations: [Configuration]) async throws

		/// Allows HTTP Not Modified responses.
		/// Setting this overrides requireNonEmptyData since urlSession will actually return empty data.


		let privacyConfigurationData = Data("Privacy Config".utf8)

		// Tests for fetch(any: [Configuration])

Unify Configuration Management #241

Unify Configuration Management #241

Conversation

jaceklyp commented Feb 22, 2023 • edited Loading

Internal references:

ayoy commented Feb 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayoy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaceklyp commented Feb 22, 2023 •

edited

Loading