various multi tenant improvements #2803
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This pull request introduces significant multi-tenant improvements across various components of the Danswer application. Key changes include:
- Added
tenant_idparameter to critical functions in connector deletion, periodic tasks, and indexing processes - Introduced tenant-specific database sessions using
get_session_with_tenantinstead ofget_sqlalchemy_engine - Modified
LocalFileConnectorto support tenant-specific contexts - Updated token rate limiting system to include tenant-specific logic
- Added
DocumentSourceRequiringTenantContextconstant to handle tenant-specific document sources - Improved logout process for cloud-enabled scenarios by deleting specific cookies
- Introduced
REGISTRATION_URLconstant for potential user registration functionality
15 file(s) reviewed, 23 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -23,7 +23,7 @@ | |||
| bind=True, | |||
| base=AbortableTask, | |||
| ) | |||
| def kombu_message_cleanup_task(self: Any) -> int: | |||
| def kombu_message_cleanup_task(self: Any, tenant_id: str | None) -> int: | |||
There was a problem hiding this comment.
style: Consider adding type hints for the return value of kombu_message_cleanup_task
| @@ -118,6 +118,9 @@ class DocumentSource(str, Enum): | |||
| NOT_APPLICABLE = "not_applicable" | |||
|
|
|||
|
|
|||
| DocumentSourceRequiringTenantContext: list[DocumentSource] = [DocumentSource.FILE] | |||
There was a problem hiding this comment.
style: Consider adding a comment explaining the purpose and usage of this new constant
| @@ -435,14 +435,13 @@ def cancel_indexing_attempts_for_ccpair( | |||
|
|
|||
| db_session.execute(stmt) | |||
There was a problem hiding this comment.
logic: The db_session.commit() call has been removed. Verify that the transaction is committed elsewhere to ensure changes are persisted.
There was a problem hiding this comment.
Ensured! Different ticket created to make sure we think about the proper level of abstraction to commit and create transactions
| for cc_pair in get_connector_credential_pairs(db_session): | ||
| resync_cc_pair(cc_pair, db_session=db_session) |
There was a problem hiding this comment.
style: consider wrapping this loop in a try-except block to handle potential errors during resync
|
|
||
|
|
||
| def _check_token_rate_limits(_: User | None) -> None: | ||
| _user_is_rate_limited_by_global() | ||
| def _check_token_rate_limits(_: User | None, tenant_id: str | None) -> None: |
There was a problem hiding this comment.
style: Update function signature to include type hint for tenant_id
| if (response && !response.ok) { | ||
| return new Response(response.body, { status: response?.status }); | ||
| } |
There was a problem hiding this comment.
style: Consider using response.status instead of response?.status to avoid potential undefined behavior
| export const REGISTRATION_URL = | ||
| process.env.INTERNAL_URL || "http://127.0.0.1:3001"; |
There was a problem hiding this comment.
logic: The REGISTRATION_URL uses INTERNAL_URL, which may not be appropriate for client-side usage
There was a problem hiding this comment.
This is changed in a separate PR
Description
Improves various flows for multi-tenant usage while being a no-op for non multi tenant