-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat(users): store and retrieve lineage_context from DB instead of Redis #7940
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR should also read from redis for backwards compatibility.
crates/router/src/core/user.rs
Outdated
let _ = state | ||
.global_store | ||
.update_user_by_user_id( | ||
&user_from_token.user_id, | ||
storage_user::UserUpdate::LineageContextUpdate { | ||
lineage_context: Some( | ||
serde_json::to_value(&lineage_context) | ||
.change_context(UserErrors::InternalServerError) | ||
.attach_printable("Failed to serialize LineageContext to JSON")?, | ||
), | ||
}, | ||
) | ||
.await | ||
.map_err(|e| { | ||
logger::error!( | ||
"Failed to update lineage context for user {}: {:?}", | ||
user_from_token.user_id, | ||
e | ||
) | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can implement this on LineageContext
or the UserFromStorage
type if it is still big after the other changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But generally, db calls are not supposed to be isolated right, so added the code in the main function itself. Do you recommend having this in the impl on lineageContext?
crates/router/src/db/user.rs
Outdated
storage::User { | ||
lineage_context: lineage_context.clone(), | ||
..user.to_owned() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not updating last_modified_at
here? Are we not supposed to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not updating last_modified_at in any kind of update for MockDb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated last_modified_at for all kinds of updates in MockDb now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the handling of the user's lineage_context by persisting it in PostgreSQL instead of Redis and updates several type references from the theme namespace to the user namespace. Key changes include adding the lineage_context column in the users table, updating DB interactions to handle the new column (including async updates via tokio::spawn), and refactoring type imports across multiple files.
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
crates/router/src/services/email/types.rs | Updated import for EmailThemeConfig to use the user namespace. |
crates/router/src/routes/user/theme.rs | Refactored ThemeLineage import to use the user namespace. |
crates/router/src/db/user/theme.rs | Updated ThemeLineage import to use the user namespace. |
crates/router/src/db/user.rs | Added lineage_context assignments and last_modified_at updates in user update functions. |
crates/router/src/db/kafka_store.rs | Updated ThemeLineage import to use the user namespace. |
crates/router/src/core/user/theme.rs | Updated ThemeLineage import to use the user namespace. |
crates/router/src/core/user.rs | Replaced lineage context cache update with an async DB update using tokio::spawn. |
crates/router/src/core/recon.rs | Updated ThemeLineage import to use the user namespace. |
crates/router/src/consts/user.rs | Removed unused lineage_context constants related to Redis. |
crates/router/src/configs/settings.rs | Updated EmailThemeConfig import to use the user namespace. |
crates/diesel_models/src/user/theme.rs | Updated EmailThemeConfig and ThemeLineage imports to use the user namespace. |
crates/diesel_models/src/user.rs | Added lineage_context field and updated UserUpdate variants accordingly. |
crates/diesel_models/src/schema_v2.rs & schema.rs | Added lineage_context column to the users table schema. |
crates/diesel_models/src/query/user/theme.rs | Updated ThemeLineage import to use the user namespace. |
crates/common_utils/src/types/user/core.rs | Introduced the new LineageContext type definition. |
crates/common_utils/src/types/user.rs & types.rs | Organized and re-exported user types instead of theme types. |
crates/api_models/src/user/theme.rs | Updated ThemeLineage and EmailThemeConfig imports to use the user namespace. |
lineage_context | ||
.try_set_lineage_context_in_cache(&state, user_from_token.user_id.as_str()) | ||
.await; | ||
tokio::spawn({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a retry mechanism for the asynchronous update of lineage_context in the background task to improve resilience against transient DB failures.
Copilot uses AI. Check for mistakes.
lineage_context | ||
.try_set_lineage_context_in_cache(&state, user_from_token.user_id.as_str()) | ||
.await; | ||
tokio::spawn({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Evaluate whether a consistent retry or fallback strategy is needed across all lineage_context update tasks (e.g. in switch_merchant and switch_profile) to ensure reliable DB updates.
Copilot uses AI. Check for mistakes.
Type of Change
Description
Previously, the user's
lineage_context
(userid, org_id, merchant_id, profile_id, role_id, tenant_id) was cached in Redis.This PR moves the storage and retrieval of
lineage_context
to the PostgreSQLusers
table instead of Redis.This ensures permanent persistence of lineage_context in DB, as opposed to the 7 days TTL when stored in Redis.
Key Changes
Schema Change:
lineage_context
as aJSONB
column in theusers
table.During Login:
lineage_context
from DB (users.lineage_context
).tokio::spawn
to avoid blocking response.During Org/Merchant/Profile Switch:
lineage_context
is serialized and persisted in theusers
table.tokio::spawn
with success and failure logs.Error Handling:
Additional Changes
Motivation and Context
By moving the
lineage_context
to theusers
table in DB:How did you test it?
Test Case 1: Lineage Context Caching on Login
lineage_context
is stored in theusers
table (PostgreSQL).Test Case 2: Lineage Context Update on Org/Profile Switch
lineage_context
field in the DB.Test Case 3: Fallback to Role Resolution upon clearing lineage_context in db (Tested on local)
lineage_context
from the DB for a user.Test Case 4: Fallback to Role Resolution upon deleting user_role (Tested on local)
user_role
from the DB for a user (user_role corresponds to user invited to another account)Checklist
cargo +nightly fmt --all
cargo clippy