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: the database url is missing ":\\" when sqlite or postgresql is not used #5492

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

qq745639151
Copy link

If I set the environment varialbe LANGFLOW_DATABASE_URL start with "mysql+aiomysql://", the function _create_engine split it to judge the database will be used. But if sqlite or postgresql is not used. the url is missing "://"

@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. bug Something isn't working labels Dec 31, 2024
@github-actions github-actions bot added bug Something isn't working and removed bug Something isn't working labels Dec 31, 2024
Copy link

codspeed-hq bot commented Dec 31, 2024

CodSpeed Performance Report

Merging #5492 will degrade performances by 23.93%

Comparing qq745639151:main (f9383d6) with main (870aedf)

Summary

❌ 1 regressions
✅ 14 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main qq745639151:main Change
test_successful_run_with_input_type_any 274.4 ms 360.7 ms -23.93%

@ogabrielluiz ogabrielluiz requested a review from Copilot January 2, 2025 12:38
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

@@ -93,7 +93,9 @@ def _create_engine(self) -> AsyncEngine:
"pool_size": self.settings_service.settings.pool_size,
"max_overflow": self.settings_service.settings.max_overflow,
}
database_url = "postgresql+psycopg://" if url_components[0].startswith("postgresql") else url_components[0]
database_url = (
Copy link
Preview

Copilot AI Jan 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code should check if '://' is already present in the URL before appending it to avoid creating an invalid URL.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Copy link
Contributor

@ogabrielluiz ogabrielluiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @qq745639151

Good catch. Please take a look at my comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about something like this:

    def _create_engine(self) -> AsyncEngine:
        """Create the engine for the database."""
        url_components = self.database_url.split("://", maxsplit=1)
        if url_components[0].startswith("sqlite"):
            scheme = "sqlite+aiosqlite"
            kwargs = {}
        else:
            kwargs = {
                "pool_size": self.settings_service.settings.pool_size,
                "max_overflow": self.settings_service.settings.max_overflow,
            }
            scheme = "postgresql+psycopg" if url_components[0].startswith("postgresql") else url_components[0]
        database_url = f"{scheme}://{url_components[1]}"
        return create_async_engine(
            database_url,
            connect_args=self._get_connect_args(),
            **kwargs,
        )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants