[sql-60] Add privacy mapper kvdb->SQL migration itest#1218
[sql-60] Add privacy mapper kvdb->SQL migration itest#1218ViktorT-11 wants to merge 1 commit intolightninglabs:migration-itest-kv-storesfrom
Conversation
Implement itest coverage for migrating privacy mapper pairs data from kvdb to sql. The itest coverage mimics the unit tests for privacy mapper in firewalldb/sql_migration_test.go, except for the randomized migration tests. This coverage ensures that the full kvdb->SQL migration flow for litd works as expected for the privacy mapper data.
ViktorT-11
added
the
no-changelog
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 significantly enhances the integration test suite by adding dedicated coverage for the migration of privacy mapper data from the key-value database (KVDB) to a SQL database. This ensures that the critical privacy mapping functionality remains intact and correctly migrates during the broader database transition, contributing to the overall reliability and data integrity of the system. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![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 adds valuable integration test coverage for the kvdb to SQL migration of privacy mapper values. The test cases are comprehensive and cover various scenarios, including single and multiple pairs, as well as handling of deleted sessions. The overall implementation is solid. I've identified a couple of areas for improvement related to database transaction efficiency and error handling within the test logic, which I've detailed in the specific comments.
| addPrivacyPairs := func(groupID session.ID, numPairs int) error { | ||
| pairs := make(map[string]string) | ||
| for i := 0; i < numPairs; i++ { | ||
| realKey := fmt.Sprintf("real-%d", i) | ||
| pseudoKey := fmt.Sprintf("pseudo-%d", i) | ||
| err := firewallStore.PrivacyDB(groupID).Update( | ||
| ctx, func(ctx context.Context, | ||
| tx firewalldb.PrivacyMapTx) error { | ||
|
|
||
| return tx.NewPair( | ||
| ctx, realKey, pseudoKey, | ||
| ) | ||
| }, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| pairs[realKey] = pseudoKey | ||
| } | ||
|
|
||
| data.privPairs[groupID] = pairs | ||
| return nil | ||
| } |
There was a problem hiding this comment.
The addPrivacyPairs function currently opens a new database transaction for each privacy pair it adds by calling Update inside a loop. This is inefficient, especially when adding multiple pairs. It's better to perform all NewPair calls within a single Update transaction to reduce database overhead.
addPrivacyPairs := func(groupID session.ID, numPairs int) error {
pairs := make(map[string]string)
err := firewallStore.PrivacyDB(groupID).Update(
ctx, func(ctx context.Context,
tx firewalldb.PrivacyMapTx) error {
for i := 0; i < numPairs; i++ {
realKey := fmt.Sprintf("real-%d", i)
pseudoKey := fmt.Sprintf("pseudo-%d", i)
err := tx.NewPair(
ctx, realKey, pseudoKey,
)
if err != nil {
return err
}
pairs[realKey] = pseudoKey
}
return nil
},
)
if err != nil {
return err
}
data.privPairs[groupID] = pairs
return nil
}| if err == sql.ErrNoRows { | ||
| continue | ||
| } |
There was a problem hiding this comment.
This check for sql.ErrNoRows is incorrect for a sqlc :many query like GetAllPrivacyPairs. These queries return an empty slice and a nil error if no rows are found, not sql.ErrNoRows. This if block is effectively dead code and can be removed. The require.NoError call on the next line is sufficient to handle any actual database errors.
|
@ViktorT-11, remember to re-request review from reviewers when ready |
2 participants
Based on #1217
Implements part of step 6. of "Phase 3" in #917.
This PR implements itest coverage for the kvdb->SQL migration of privacy mapper values.