[sql-54] firewalldb: handle deleted sessions during the kv stores + privacy mapper migration #1153
Conversation
Summary of ChangesHello @ViktorT-11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical edge case in the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
ViktorT-11
added
the
no-changelog
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an edge case in the database migration where kv entries and privacy mapper pairs could point to a deleted session. The solution is to fetch all existing sessions at the start of the migration and pass them as a map to the migration functions. This map is then used to check for a session's existence before migrating any session-specific data, correctly ignoring data for deleted sessions. This approach is not only correct but also improves performance by replacing multiple database queries with in-memory map lookups. The changes are well-structured and include thorough test cases for the new logic. I have one minor suggestion to clean up an unused parameter.
| groupAlias := getNewSessionAlias(t, ctx, sessionStore) | ||
|
|
||
| _ = insertTempAndPermEntry( | ||
| t, ctx, boltDB, testRuleName, groupAlias, fn.None[string](), | ||
| testEntryKey, testEntryValue, | ||
| ) |
There was a problem hiding this comment.
nit: you could perhaps add one kv entry (not deleting its session), to ensure that there is actually a kv entry created using this pattern in the happy case in addition to this code, same in the other test
There was a problem hiding this comment.
Thanks 🙏! Added two extra tests for this, as I do think it adds value to have a test that clearly shows that no kv entries are migrated at all if all linked sessions are deleted :)
In the upcoming commits, we will update the kv stores and the privacy mapper migration to not migrate entries if their linked session has been deleted. As those checks will need to query the SQL db to see if the session still exists, we move the session alias to session map to not only be used in the actions migration, but throughout the migration when ever we need to query a session by its alias. This is done to avoid multiple queries to the SQL db for the same session alias, to improve the performance of the migration.
If the user has deleted their session.db file, but kept their rules.db file, there can exist kv entry values that point to a now deleted session ID. Such kv entries should be ignored during the migration, as they are cannot be used anymore. This commit updates the migration to handle this case.
If the user has deleted their session.db file, but kept their rules.db file, there can exist privacy mapper pairs that point to a now deleted session ID. Such pairs should be ignored during the migration, as they are cannot be used anymore. This commit updates the migration to handle this case.
ViktorT-11
force-pushed
the
2025-10-firewalldb-mig-deleted-session-db-fix
branch
from
60be78b to
11cb808
Compare
3 participants
If the user has deleted their
session.dbfile, but kept theirrules.dbfile, there can exist kv entries + privacy mapper pairs that point to a now deleted session ID. Such kv entries and privacy mapper pairs should be ignored during the migration, as they are cannot be used anymore.This PR addresses that edge case.