Skip to content

Make sqlalchemy engine connect listener recorder specific #34908

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.

Sign up for GitHub

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

Merged
balloob merged 5 commits into from
Apr 30, 2020
Merged

Make sqlalchemy engine connect listener recorder specific #34908

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c10a74e
Make sqlalchemy engine connect listener recorder specific
bdraco Apr 30, 2020
4a90a41
Fix sqlite url check
bdraco Apr 30, 2020
ae9a0d0
Only set connect args/poolclass with no path
bdraco Apr 30, 2020
897025c
No need to set wal for in memory
bdraco Apr 30, 2020
ac7f598
event naming
bdraco Apr 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions homeassistant/components/recorder/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@
import time
from typing import Any, Dict, Optional

from sqlalchemy import create_engine, exc, select
from sqlalchemy.engine import Engine
from sqlalchemy.event import listens_for
from sqlalchemy import create_engine, event as sqlalchemy_event, exc, select
from sqlalchemy.orm import scoped_session, sessionmaker
from sqlalchemy.pool import StaticPool
import voluptuous as vol
Expand Down Expand Up @@ -488,15 +486,13 @@ def _setup_connection(self):
"""Ensure database is ready to fly."""
kwargs = {}

# pylint: disable=unused-variable
@listens_for(Engine, "connect")

@balloob balloob Apr 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ha oops 😆

def setup_connection(dbapi_connection, connection_record):
def setup_recorder_connection(dbapi_connection, connection_record):
"""Dbapi specific connection settings."""

# We do not import sqlite3 here so mysql/other
# users do not have to pay for it to be loaded in
# memory
if self.db_url == "sqlite://" or ":memory:" in self.db_url:

@balloob balloob Apr 30, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This changes behavior.

If you specify just sqlite://, you get a SQLite in-memory database. If it starts with sqlite://, it can also be file based, like the default.

During tests we run with an in-memory DB.

@bdraco bdraco Apr 30, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

That logic should still be below. This fixes a regression I caused in b09b572

if self.db_url.startswith("sqlite://"):
old_isolation = dbapi_connection.isolation_level
dbapi_connection.isolation_level = None
cursor = dbapi_connection.cursor()
Expand All @@ -519,6 +515,9 @@ def setup_connection(dbapi_connection, connection_record):
self.engine.dispose()

self.engine = create_engine(self.db_url, **kwargs)

sqlalchemy_event.listen(self.engine, "connect", setup_recorder_connection)

Base.metadata.create_all(self.engine)
self.get_session = scoped_session(sessionmaker(bind=self.engine))

Expand Down