Add a program which verifies that an identity has been correctly deleted #1283
Conversation
…ng the allocating identity
tnotheis
left a comment
tnotheis
left a comment
There was a problem hiding this comment.
As discussed in our call, there are 3 bigger topics:
I think there should be tests for this. At least the following:
- The check finds a simple address
- The check finds an address concatenated with something else
The test should be a unit test. This means that it should NOT run the CLI command. Instead, there should be a class responsible for the check, which is called by the CheckCommand, as well as by the test.
I just had a crazy idea. :) Wouldn't it be awesome if we could run the logic of the check command from within the ActualDeletion job after an identity was deleted? This way we would be notified if there is undeleted data.
But this involves some optimizations in your code. Because currently you read the whole file content (=> the whole database export) into memory. In a production database this would lead to a memory overflow. This means that you would have to refactor your code so that it reads line by line.
You cannot use the export command of the Admin CLI, as the generated files only contain a subset of all data.
.github/workflows/test.yml
Outdated
| docker compose -f ./.ci/compose.test.yml -f ./.ci/compose.test.${{matrix.database.type}}.yml wait admin-cli | ||
| - name: Create identities, relationships etc. and initiate deletion processes | ||
| run: dotnet run --no-build --no-restore --project ./Applications/IdentityDeletionVerifier/src/IdentityDeletionVerifier/IdentityDeletionVerifier.csproj init --consumerBaseUrl http://localhost:5000 --adminBaseUrl http://localhost:5173 |
There was a problem hiding this comment.
| run: dotnet run --no-build --no-restore --project ./Applications/IdentityDeletionVerifier/src/IdentityDeletionVerifier/IdentityDeletionVerifier.csproj init --consumerBaseUrl http://localhost:5000 --adminBaseUrl http://localhost:5173 | |
| run: dotnet run --no-build --no-restore --project ./Applications/IdentityDeletionVerifier/src/IdentityDeletionVerifier/IdentityDeletionVerifier.csproj init --consumerApiBaseUrl http://localhost:5000 --adminApiBaseUrl http://localhost:5173 |
| using System.Text; | ||
| using Backbone.IdentityDeletionVerifier.Commands; | ||
|
|
||
| Console.OutputEncoding = Encoding.UTF8; |
There was a problem hiding this comment.
Out of curiosity: why do we need this?
There was a problem hiding this comment.
For Spectre.Console to be able to write emojis and better spinners
| public static readonly string PATH_TO_IDENTITIES_FILE = Path.Combine(PATH_TO_TEMP_DIR, IDENTITIES_FILENAME); | ||
|
|
||
| [GeneratedRegex(@"export-\d{8}_\d{6}\.zip$")] | ||
| private static partial Regex MyRegex(); |
| return -1; | ||
| } | ||
|
|
||
| var a = await GetIdentityToCheck(); |
.github/workflows/test.yml
Outdated
| Database__Provider: Postgres | ||
| Database__ConnectionString: "Server=postgres;Database=enmeshed;User Id=devices;Password=Passw0rd;Port=5432" |
There was a problem hiding this comment.
Please stay with the typical config hierarchy, so that
- It's consistent
- I don't have to provide a separate config during deployment
In this case, this means:
| Database__Provider: Postgres | |
| Database__ConnectionString: "Server=postgres;Database=enmeshed;User Id=devices;Password=Passw0rd;Port=5432" | |
| Infrastructure__SqlDatabase__Provider: Postgres | |
| Infrastructure__SqlDatabase__ConnectionString: "Server=postgres;Database=enmeshed;User Id=devices;Password=Passw0rd;Port=5432" |
| if (!DirectoryExists()) | ||
| { | ||
| AnsiConsole.MarkupLineInterpolated($"[red bold]The temp directory[/][grey bold]{FilePaths.PATH_TO_TEMP_DIR} [/][red bold]doesn't exist.[/]"); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
IdentitiesFileExists already checks implicitly for the existence of the directory
| private bool DirectoryExists() => Directory.Exists(FilePaths.PATH_TO_TEMP_DIR); | ||
| private bool IdentitiesFileExists() => File.Exists(FilePaths.PATH_TO_IDENTITIES_FILE); | ||
| private bool ExportFileExists() => Directory.EnumerateFiles(FilePaths.PATH_TO_TEMP_DIR).Any(FilePaths.EXPORT_FILE_PATTERN.IsMatch); | ||
| private string GetLatestExportFile() => Directory.EnumerateFiles(FilePaths.PATH_TO_TEMP_DIR).Where(e => FilePaths.EXPORT_FILE_PATTERN.IsMatch(e)).Max()!; |
There was a problem hiding this comment.
I'd suggest a (static?) class AppDirectory (I'm not quite happy with the name, maybe you find a better one), which encapsulates the regular expressions and constants for the file paths and has the following methods:
- Exists() (only if you want to keep the check for the existence of the directoy)
- IdentitiesFileExists()
- ExportFileExists()
- GetLatestExportFile()
| return -1; | ||
| } | ||
|
|
||
| var a = await GetIdentityToCheck(); |
There was a problem hiding this comment.
I don't like the name of the method either. How about DoesIdentityFileHaveContent?
| var count = line | ||
| .Split(',') | ||
| .Count(s => string.Equals(s, identityToCheck, StringComparison.OrdinalIgnoreCase)); |
There was a problem hiding this comment.
There is a problem with this solution: if the address is concatenated with something else, it doesn't match. I'd suggest:
| var count = line | |
| .Split(',') | |
| .Count(s => string.Equals(s, identityToCheck, StringComparison.OrdinalIgnoreCase)); | |
| var count = Regex.Matches(line, identityToCheck).Count |
|
|
||
| if (!IdentitiesFileExists()) | ||
| { | ||
| AnsiConsole.MarkupLine("[red bold]The deleted identities file doesn't exist. Run the Init command first.[/]"); |
There was a problem hiding this comment.
The first part of the error message sounds incorrect if you read it without any special knowledge. Because OF COURSE the identities file doesn't exist if it was deleted. 😉
… identity-deletion-verifier
Readiness checklist