POC - DBP Processor Refactoring (Abstraction & Testability) #2730

aataraxiaa · 2024-05-01T14:58:32Z

Task/Issue URL:
Tech Design URL:
CC:

Description:
This PR is a proof-of-concept. It demonstrates how we can make our processor logic more testable. If the proof-of-concept is satisfactory, and we get consensus on the approach, we can use this POC as the basis for a final implementation.

The following changes are included:

Extracts OperationCollection building into a dedicated protocol and concrete type - DataBrokerOperationsCollectionBuilder
Implements DataBrokerProtectionQueueManager, a refactoring of the current DataBrokerProtectionProcessor type, which enables testing of it’s operation queue state
Implements QueueManagerMode, which captures the state of the queue manager
Creates protocol interfaces for dependencies of DataBrokerProtectionQueueManager, which can be mocked
Adds mocks for all dependencies required to test DataBrokerProtectionQueueManager
Adds tests for DataBrokerProtectionQueueManager and QueueManagerMode, and related mocks

Steps to test this PR:
1.

—

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

…ete implementation. Add dependencies protocol and concrete impl.

…. This replaces our processor.

github-actions · 2024-05-02T08:56:22Z

	Fails
🚫	Please, don't forget to add a link to the internal task

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against 16acb56

…erTests

THISISDINOSAUR

Awesome work! I think this goes a really long way to moving us in the right direction

THISISDINOSAUR · 2024-05-02T12:49:41Z

...otection/Sources/DataBrokerProtection/Operations/DataBrokerOperationsCollectionBuilder.swift

@@ -0,0 +1,81 @@
+//
+//  DataBrokerOperationsCollectionBuilder.swift


I think the introduction of this class is awesome. One minor thing is it really highlights the need to rename DataBrokerOperationsCollection, since this is technically building a DataBrokerOperationsCollectionCollection.

It's my understanding that a DataBrokerOperationsCollection is all of the operations (even operation I think we should rename to avoid confusion with NSOperation) for a single broker. I think using that information if we first come up with a different name for scan/opt out "operations"we should be able to come up with a better name

Agree on the need for renaming. I thought about this, and came up with different ideas -e.g renaming DataBrokerOperationsCollection to DataBrokerJob, or DataBrokerJobGroup, and other variations. In the end, I decided that as DataBrokerOperationsCollection is in fact an Operation, and it is handled as such by an OperationQueue, it should just be called DataBrokerOperation. However, to implement this I needed to change conflicting type names.

Ultimately, what I propose is in this commit.

Note that we could go further with this - i.e rename other references to operation which should now say task based on these changes. At the moment I think we overuse ‘Operation’. However, want to share this and get input before we make any more related changes.

...okerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionQueueManager.swift

THISISDINOSAUR · 2024-05-02T12:53:55Z

...okerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionQueueManager.swift

+        self.brokerUpdater = brokerUpdater
+    }
+
+    func startManualScans(showWebView: Bool, operationDependencies: OperationDependencies, completion: ((DataBrokerProtectionSchedulerErrorCollection?) -> Void)?) {


I'd maybe change "startManualScans" to something like startManualScansIfPermitted, although they should always be. But since it does check for interruptions I think we should capture that somehow

@THISISDINOSAUR what would be the cases for the manual scan to not start? In other words, how can it not be permitted? Is the depencency other scans running or initial data state, like, not having a profile data

I think at present this function should never be called if it can't start, but because it does checks to see if they can and could fail, I think that should be captured in the signature.

I’ve renamed the DataBrokerProtectionQueueManager methods in this commit. Note that I have also renamed the QueueManagerMode cases.

THISISDINOSAUR · 2024-05-02T12:56:01Z

...okerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionQueueManager.swift

+        mode = .manual
+
+        // New Manual scans ALWAYS interrupt (i.e cancel) ANY current Manual/Scheduled scans
+        operationQueue.cancelAllOperations()


I'd maybe consider making an explicit tear down function, even if it only contains this one line for now, or maybe a single function that handles state transitions

Also worth noting this will still have the completion/barrier block bug, where if we interrupt a state, the completion (and anything else in the barrier block) won't be called.

THISISDINOSAUR · 2024-05-02T12:57:31Z

...okerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionQueueManager.swift

+
+    func startManualScans(showWebView: Bool, operationDependencies: OperationDependencies, completion: ((DataBrokerProtectionSchedulerErrorCollection?) -> Void)?) {
+
+        guard mode.canInterrupt(forNewMode: .manual) else { return }


You mentioned it but recording for prosperity, if we decide not to interrupt, we should call the completion of the function that isn't going to execute (probably with a new error type?)

THISISDINOSAUR · 2024-05-02T12:58:10Z

...okerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionQueueManager.swift

+        }
+    }
+
+    func runAllOptOutOperations(showWebView: Bool, operationDependencies: OperationDependencies, completion: ((DataBrokerProtectionSchedulerErrorCollection?) -> Void)?) {


I think we can get rid of some of these, although need to check which ones.
You mentioned it, but recording for prosperity, we should rename them too, since they're not so clear

I’ve removed the unused runAllOperations method - See this commit

THISISDINOSAUR · 2024-05-02T12:59:37Z

...okerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionQueueManager.swift

+        } catch {
+            os_log("DataBrokerProtectionProcessor error: runOperations, error: %{public}@", log: .error, error.localizedDescription)
+            operationQueue.addBarrierBlock {
+                completion(DataBrokerProtectionSchedulerErrorCollection(oneTimeError: error))


Hold over from before, but it seems weird and wrong that we don't call the completion immediately if there's an error. So if there's already things on the queue unrelated to this set of scans, it won't run for ages.
(with the current design I don't think there's a case where that's ever true, but no reason to encourage it).

You mean here it should call completion directly instead of adding the barrierBlock?

Yeah, or if not we should record that and make it clear it's deliberate

THISISDINOSAUR · 2024-05-02T13:00:10Z

...aBrokerProtection/Sources/DataBrokerProtection/Scheduler/DataBrokerProtectionScheduler.swift

+                                                           pixelHandler: pixelHandler)
+
+        var brokerUpdater: DataBrokerProtectionBrokerUpdater?
+        if let vault = try? DataBrokerProtectionSecureVaultFactory.makeVault(reporter: nil) {


It's tangental and out of scope, but I don't think we should ever be making the vault directly, and should be striving to use a single DB abstraction (Database)

THISISDINOSAUR · 2024-05-02T13:00:54Z

...otection/Sources/DataBrokerProtection/Operations/DataBrokerOperationsCollectionBuilder.swift

+
+typealias OperationType = DataBrokerOperationsCollection.OperationType
+
+protocol OperationDependencies {


I think this is a big improvement, although I personally don't have any great ideas on what we should be doing with our dependancies in DBP more broadly, so if you have any ideas there it would be good.

Bunn · 2024-05-02T15:31:54Z

LocalPackages/DataBrokerProtection/Tests/DataBrokerProtectionTests/QueueManagerModeTests.swift

+        XCTAssertTrue(result)
+    }
+
+    // TODO: More tests to be added as we define expected interruption behaviorgst


I see the TODO here, just adding the suggestion that the most important thing to add here is the case where it's manual scan interrupting manual scan since it's the "edge case" of all other cases.

Thanks @Bunn, I’ve added a test for this scenario in this commit.

…, `IPCServerInterface`, and `XPCServerInterface` interfaces. Remove related Pixel.

…Mode` cases for consistency and clarity

aataraxiaa · 2024-05-03T16:55:54Z

FYI @THISISDINOSAUR and @Bunn - I’m not going to continue any development here. I’ll instead work on new branches to enable opening focused PRs to a feature branch. However, I’ll keep this open for the moment for reference.

aataraxiaa requested a review from THISISDINOSAUR May 1, 2024 15:18

aataraxiaa added 5 commits May 2, 2024 09:45

Extract Operation Collection building to dedicated protocol and concr…

5172709

…ete implementation. Add dependencies protocol and concrete impl.

Implement DataBrokerProtectionQueueManager protocol and concrete impl…

0b34b8a

…. This replaces our processor.

Rebase conflict fixes

27d8b22

Rebase conflict fix

6947410

Tidy up

1b369a5

aataraxiaa force-pushed the pete/poc-dbp-processor-refactoring branch from 47bdf76 to 1b369a5 Compare May 2, 2024 08:54

Iterate on queue state, add empty DataBrokerOperationsCollectionBuild…

716c5c0

…erTests

THISISDINOSAUR reviewed May 2, 2024

View reviewed changes

Bunn reviewed May 2, 2024

View reviewed changes

aataraxiaa added 3 commits May 3, 2024 09:30

Remove unused runAllOperations from DataBrokerProtectionScheduler…

16acb56

…, `IPCServerInterface`, and `XPCServerInterface` interfaces. Remove related Pixel.

Rename DataBrokerProtectionOperationQueue methods and `QueueManager…

8670832

…Mode` cases for consistency and clarity

Rename OperationCollection, related files, and rename conflicting types

c4c044b

aataraxiaa closed this May 9, 2024

aataraxiaa deleted the pete/poc-dbp-processor-refactoring branch July 17, 2024 14:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POC - DBP Processor Refactoring (Abstraction & Testability) #2730

POC - DBP Processor Refactoring (Abstraction & Testability) #2730

aataraxiaa commented May 1, 2024

github-actions bot commented May 2, 2024 •

edited

Loading

THISISDINOSAUR left a comment

THISISDINOSAUR May 2, 2024

aataraxiaa May 3, 2024

THISISDINOSAUR May 2, 2024

Bunn May 2, 2024

THISISDINOSAUR May 2, 2024

aataraxiaa May 3, 2024

THISISDINOSAUR May 2, 2024

THISISDINOSAUR May 2, 2024

THISISDINOSAUR May 2, 2024

THISISDINOSAUR May 2, 2024

aataraxiaa May 3, 2024

THISISDINOSAUR May 2, 2024

Bunn May 2, 2024

THISISDINOSAUR May 2, 2024

THISISDINOSAUR May 2, 2024

THISISDINOSAUR May 2, 2024

Bunn May 2, 2024

aataraxiaa May 3, 2024 •

edited

Loading

aataraxiaa commented May 3, 2024

		@@ -0,0 +1,81 @@
		//
		// DataBrokerOperationsCollectionBuilder.swift


		func startManualScans(showWebView: Bool, operationDependencies: OperationDependencies, completion: ((DataBrokerProtectionSchedulerErrorCollection?) -> Void)?) {

		guard mode.canInterrupt(forNewMode: .manual) else { return }


		typealias OperationType = DataBrokerOperationsCollection.OperationType

		protocol OperationDependencies {

POC - DBP Processor Refactoring (Abstraction & Testability) #2730

POC - DBP Processor Refactoring (Abstraction & Testability) #2730

Conversation

aataraxiaa commented May 1, 2024

Internal references:

github-actions bot commented May 2, 2024 • edited Loading

THISISDINOSAUR 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aataraxiaa May 3, 2024 • edited Loading

Choose a reason for hiding this comment

aataraxiaa commented May 3, 2024

github-actions bot commented May 2, 2024 •

edited

Loading

aataraxiaa May 3, 2024 •

edited

Loading