Skip to content
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

fix(backend): Fix DatabaseManager usage by calling it on-demand #8404

Merged
merged 2 commits into from
Oct 23, 2024

Conversation

majdyz
Copy link
Contributor

@majdyz majdyz commented Oct 23, 2024

Background

We unintentionally cached the DatabaseManager connection object and shared it across different threads in SupabaseIntegrationCredentialsStore

Changes 🏗️

  • Make DatabaseManager connection acquisition on the fly and cache it within the class.
  • Remove the need to provide a port number for connecting to pyro service (inferred from the class).
  • Moved utils/cache.py to autogptlib.

Testing 🔍

Note

Only for the new autogpt platform, currently in autogpt_platform/

  • Create from scratch and execute an agent with at least 3 blocks
  • Import an agent from file upload, and confirm it executes correctly
  • Upload agent to marketplace
  • Import an agent from marketplace and confirm it executes correctly
  • Edit an agent from monitor, and confirm it executes correctly

@majdyz majdyz requested a review from a team as a code owner October 23, 2024 00:46
@github-actions github-actions bot added platform/backend AutoGPT Platform - Back end size/l labels Oct 23, 2024
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Thread Safety
The use of thread_cached_property for db_manager might lead to thread safety issues if not properly handled. Ensure that the DatabaseManager client is thread-safe.

Error Handling
The removal of the db_manager parameter from IntegrationCredentialsManager initialization might lead to unexpected behavior if the database connection is not properly established elsewhere.

Code Duplication
The get_host and get_port methods are implemented in multiple places. Consider creating a base class or utility function to reduce duplication.

@majdyz majdyz merged commit 17e79ad into dev Oct 23, 2024
7 checks passed
@majdyz majdyz deleted the zamilmajdy/fix-credentials-manager branch October 23, 2024 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants