PIR Database Migrations to Address Integrity Issues & Related Feature Flagger #2997

aataraxiaa · 2024-07-19T09:36:39Z

Task/Issue URL: https://app.asana.com/0/1206488453854252/1207806240565841/f
Tech Design URL: https://app.asana.com/0/481882893211075/1207878066929518/f

Description: This PR adds a v3 PIR database migration. This migration is intended solely to address PIR integrity issues. We address these issues by:

Cleaning up orphaned records
Adding cascading deletes to all foreign key references

Specific Changes Introduced:

Extract Migration code out of the database provider and into a dedicated type, with a protocol interface DataBrokerProtectionDatabaseMigrationsProvider
Add migration code to clean up orphaned records and recreate existing tables with the correct cascading deletes
Add additional precautionary logic to the migration to cleanup any remaining integrity issues after recreation of the tables
Add a feature flagger type to allow for % rollout to users
Add a method on DataBrokerProtectionDatabaseProvider to enable creation based on the feature flag, and allow injection of the migrations
Add test helper methods on DataBrokerProtectionDatabaseProvider
Update DataBrokerProtectionDatabaseProvider.deleteProfileData to delete all records
Add tests for migration, feature flagging. Add test .sql database file

Steps to test this PR:

Ensure you have PIR data in your debug vault (run main branch in Xcode, check PIR dashboard or debug DB viewer)
Pull this branch
Put a breakpoint at line 102 of DefaultDataBrokerProtectionDatabaseProvider (so you can confirm that you were migrated to v3)
Run the app from Xcode, check that you still have data in the PIR dashboard debug DB viewer

Definition of Done:

Does this PR satisfy our Definition of Done?

—

Internal references:

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

github-actions · 2024-07-19T09:38:34Z

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

Generated by 🚫 dangerJS against 495608f

…p, add tests and test data

…, update test

…ve tests

… & vault creation for testability. Add tests & mocks. Remove some previous changes.

… flag for all.

THISISDINOSAUR

It mostly looks good, although I haven't physically tested it yet since I've left quite a few comments (so would want to retest it if there are many changes)

It's a little difficult to review some of the database provider stuff because git gets confused with some things moving. It would be easier to review if it was separated out into multiple PRs. In future please do that, or we can discuss the PR strategy at the start of the project.

THISISDINOSAUR · 2024-07-24T16:25:07Z

...on/Sources/DataBrokerProtection/Storage/DataBrokerProtectionDatabaseMigrationsProvider.swift

+        // Recreate tables to add correct foreign key constraints
+        try recreateTablesV3(database: database)
+
+        // As a precaution, re-run orphan deletion if necessary


In what case would this be necessary?

I could not re-create any situation in which this is required. This was added to mitigate any unknown risks (of which I currently am unaware). We might feel this indicates lack of confidence in our approach. I don’t see it that way. I see it as an extra pre-caution to mitigate a possibly ‘high-risk’ migration.

I think we're double dipping since we have this and "// As a precaution, explicitly check for any foreign key violations which were missed".
In general I think there are a lot of parts of the codebase that do things just in case and lots of them are nonsensical, so I do have an immediate visceral reaction to it.

I think I'm unconvinced it derisks it, because if it's necessary, what happened? What's broken? I assume something

I'm personally undecided on keeping it at the moment, but I do think it's worth more of a convo

Understandable. I can’t see a way that foreign keys would be missed. To validate that we have:

Implemented a orphan cleanup strategy which we believe won’t leave any integrity issues

Added tests for this

Put these changes through code review

So, we can remove this. However, if we do admit there is some risk in a migration (and most of the risk of failing comes from integrity checks IMO), and we are feature flagging and taking other precautions, I’m unsure where I sit with this one.

Cool, I’ll ping on MM for a conversation.

We chatted about it sync and I'm fine with leaving it. It's well encapsulated, and a migration is essentially tech debt anyway...

...on/Sources/DataBrokerProtection/Storage/DataBrokerProtectionDatabaseMigrationsProvider.swift

THISISDINOSAUR · 2024-07-24T17:59:54Z

...tion/Sources/DataBrokerProtection/Storage/DataBrokerProtectionMigrationsFeatureFlagger.swift

+         It will be removed via a PR to the release branch before the end of the internal testing week
+         See: https://app.asana.com/0/0/1207876679488680/f
+         */
+        return true


I could see this going badly wrong as a strategy. Did we talk about it anywhere in asana?

Understandable concern. It could go wrong, but I think risk can be mitigated with planning. No, we didn’t discuss in Asana, but that probably would have been a better first step here. I’ll remove this for now, and initiate a conversation in Asana.

Asana: Async Chat: Enabling the New Migrations for Internal Users

I’ve pushed an implementation for this. Note that I only added option 3 as discussed in Asana - the time-based check. This is all that is required.

...erProtection/Tests/DataBrokerProtectionTests/DataBrokerProtectionDatabaseProviderTests.swift

...tion/Tests/DataBrokerProtectionTests/DataBrokerProtectionMigrationsFeatureFlaggerTests.swift

...ion/Sources/DataBrokerProtection/Database/DataBrokerProtectionSecureVaultErrorReporter.swift

...erProtection/Sources/DataBrokerProtection/Storage/DataBrokerProtectionDatabaseProvider.swift

aataraxiaa · 2024-07-25T09:36:57Z

It mostly looks good, although I haven't physically tested it yet since I've left quite a few comments (so would want to retest it if there are many changes)

It's a little difficult to review some of the database provider stuff because git gets confused with some things moving. It would be easier to review if it was separated out into multiple PRs. In future please do that, or we can discuss the PR strategy at the start of the project.

Thanks for the thorough review and comments @THISISDINOSAUR , super useful.

I’ve updated the PR and responded to all comments.

Re: the difficulty in reviewing the PR - Totally agree. The nature of the changes and presentation in Git make it hard to review. Apologies for that. As you suggest a good approach here would have been to open separate PRs - maybe some early PRs with move-related changes. I’ll keep that in mind. I don’t think a specific PR strategy discussion would have helped in this case, as I really didn’t anticipate the types of changes we ended up with. However, maybe a general ‘PR Strategy’ discussion would have at least planted the seed to break up the PRs at a later stage.

… users

...tion/Sources/DataBrokerProtection/Storage/DataBrokerProtectionMigrationsFeatureFlagger.swift

THISISDINOSAUR

Testing works, so LGTM!

For prosperity, we also chatted at length on MM exactly what causes orphaned records happen to try and make sure any of this clean up shouldn't cause a problem. ( I was specifically checking for collisions with the weird "deprecated" data thing we do and if it could cause problems there).

I'm as happy as it's possible to be there, whilst acknowledging this work is always risky.

… Flagger (#2997) Task/Issue URL: https://app.asana.com/0/1206488453854252/1207806240565841/f Tech Design URL: https://app.asana.com/0/481882893211075/1207878066929518/f **Description**: This PR adds a v3 PIR database migration. This migration is intended solely to address PIR integrity issues.

aataraxiaa added 10 commits July 23, 2024 10:29

Extract Migrations to dedicated type, add initial v3 migration cleanu…

f7219c4

…p, add tests and test data

Implement correct passing test, cleanup Swiftlint

faba82d

Add explicit foreign key cleanup as a precaution after orphan cleanup…

44e7db2

…, update test

Ensure all table data is deleted when clearing profile data

3e8a5a9

Re-create tables with cascading deletes and add some testing

b0e07fb

Rerun orphan cleanup as last migration step, add documentation, impro…

8d6bc3b

…ve tests

Add test to check migration of very invalid database data

8caac04

Add specific migration failed integrity error and propagate to pixel

42af11d

Add feature flag and temp local BSK reference

9a0bf61

Implement PIR Migrations feature flagger & tests. Refactor migrations…

f0a8e90

… & vault creation for testability. Add tests & mocks. Remove some previous changes.

aataraxiaa force-pushed the pete/pir-database-integrity branch from 3a06484 to f0a8e90 Compare July 24, 2024 11:21

aataraxiaa changed the title ~~Extract Migrations to dedicated type, add initial v3 migration cleanu…~~ PIR Database Migrations to Address Integrity Issues & Related Feature Flagger Jul 24, 2024

aataraxiaa added 2 commits July 24, 2024 13:41

Correctly name file

2a498f3

Repeat DB integrity issues test and add documentation. Enable feature…

d374d4a

… flag for all.

aataraxiaa marked this pull request as ready for review July 24, 2024 15:17

aataraxiaa requested a review from THISISDINOSAUR July 24, 2024 15:18

Temporarily disable test due to bypassed feature flag

dbbbdfa

THISISDINOSAUR reviewed Jul 24, 2024

View reviewed changes

PR feedback changes and improvements

9cb5635

aataraxiaa added 2 commits July 25, 2024 15:24

Implement temporary date range check to allow migrations for internal…

8a0d316

… users

Fix comment

f0b76ed

THISISDINOSAUR reviewed Jul 25, 2024

View reviewed changes

...tion/Sources/DataBrokerProtection/Storage/DataBrokerProtectionMigrationsFeatureFlagger.swift Outdated Show resolved Hide resolved

PR feedback

495608f

THISISDINOSAUR approved these changes Jul 25, 2024

View reviewed changes

aataraxiaa merged commit 727561d into main Jul 25, 2024
24 checks passed

aataraxiaa deleted the pete/pir-database-integrity branch July 25, 2024 16:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PIR Database Migrations to Address Integrity Issues & Related Feature Flagger #2997

PIR Database Migrations to Address Integrity Issues & Related Feature Flagger #2997

aataraxiaa commented Jul 19, 2024 •

edited

Loading

github-actions bot commented Jul 19, 2024 •

edited

Loading

THISISDINOSAUR left a comment

THISISDINOSAUR Jul 24, 2024

aataraxiaa Jul 25, 2024

THISISDINOSAUR Jul 25, 2024

aataraxiaa Jul 25, 2024

THISISDINOSAUR Jul 25, 2024

THISISDINOSAUR Jul 24, 2024

aataraxiaa Jul 25, 2024

aataraxiaa Jul 25, 2024 •

edited

Loading

aataraxiaa commented Jul 25, 2024

THISISDINOSAUR left a comment

PIR Database Migrations to Address Integrity Issues & Related Feature Flagger #2997

PIR Database Migrations to Address Integrity Issues & Related Feature Flagger #2997

Conversation

aataraxiaa commented Jul 19, 2024 • edited Loading

Internal references:

github-actions bot commented Jul 19, 2024 • edited Loading

THISISDINOSAUR left a comment

Choose a reason for hiding this comment

THISISDINOSAUR Jul 24, 2024

Choose a reason for hiding this comment

aataraxiaa Jul 25, 2024

Choose a reason for hiding this comment

THISISDINOSAUR Jul 25, 2024

Choose a reason for hiding this comment

aataraxiaa Jul 25, 2024

Choose a reason for hiding this comment

THISISDINOSAUR Jul 25, 2024

Choose a reason for hiding this comment

THISISDINOSAUR Jul 24, 2024

Choose a reason for hiding this comment

aataraxiaa Jul 25, 2024

Choose a reason for hiding this comment

aataraxiaa Jul 25, 2024 • edited Loading

Choose a reason for hiding this comment

aataraxiaa commented Jul 25, 2024

THISISDINOSAUR left a comment

Choose a reason for hiding this comment

aataraxiaa commented Jul 19, 2024 •

edited

Loading

github-actions bot commented Jul 19, 2024 •

edited

Loading

aataraxiaa Jul 25, 2024 •

edited

Loading