Skip to content

Conversation

@DrPyser
Copy link
Contributor

@DrPyser DrPyser commented Mar 13, 2025

  • controller: fix db connection leak
  • alembic: fix downgrade
  • database.helpers: db_session utility context manager
  • controller: use db_session

@wazo-community-zuul
Copy link
Contributor

Build failed.
https://zuul.wazo.community/zuul/t/local/buildset/cf014f4eeb95419eafab02244fe5a31d

✔️ tox-linters SUCCESS in 5m 41s
✔️ wazo-tox-py39 SUCCESS in 6m 24s
✔️ debian-packaging-bullseye SUCCESS in 3m 48s
auth-tox-integration FAILURE in 9m 43s

@DrPyser DrPyser force-pushed the db-session-context-manager branch from 2b82001 to 6b964ee Compare March 13, 2025 20:42
@sonarqubecloud
Copy link

@wazo-community-zuul
Copy link
Contributor

Build failed.
https://zuul.wazo.community/zuul/t/local/buildset/8d9031fff46743b889d293899669d85d

✔️ tox-linters SUCCESS in 5m 49s
✔️ wazo-tox-py39 SUCCESS in 6m 30s
✔️ debian-packaging-bullseye SUCCESS in 3m 46s
auth-tox-integration FAILURE in 9m 36s

@DrPyser
Copy link
Contributor Author

DrPyser commented Mar 18, 2025

@CodiumAI-Agent /review

@QodoAI-Agent
Copy link

QodoAI-Agent commented Mar 18, 2025

PR Reviewer Guide 🔍

(Review updated until commit 10d7eba)

Here are some key observations to aid the review process:

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

Transaction Scope

Using db_session() wraps reads in a transaction; confirm that downstream services called after the context do not require the session/objects as they will be detached once the session closes.

with db_session():
    top_tenant_uuid = self.dao.tenant.find_top_tenant()
    visible_tenants = self.dao.tenant.list_visible_tenants(top_tenant_uuid)
    tenant_uuids = [tenant.uuid for tenant in visible_tenants]

self._default_policy_service.update_policies(top_tenant_uuid)
self._all_users_service.update_policies(tenant_uuids)
self._default_group_service.update_groups(tenant_uuids)
Session Cleanup

db_session() uses session.close() instead of scoped_session.remove(); verify this plays well with thread-local scoped sessions to avoid leaking connections in multi-threaded contexts.

@contextmanager
def db_session():
    session: BaseSession = get_db_session()
    try:
        with session.begin():
            yield session
    except Exception:
        session.rollback()
        raise
    finally:
        session.close()
Query Results Lifetime

Ensure 'tenants' and 'users' collections are fully materialized (e.g., not lazy-loaded attributes) before the session closes to prevent DetachedInstanceError when accessed later.

with db_session():
    # fetch tenants
    tenants = self.dao.tenant.get_missing_auth_methods(
        available_methods=available_authentication_methods
    )
    # fetch users
    users = self.dao.user.get_missing_auth_methods(
        available_methods=available_authentication_methods
    )

tenants_authentication_methods = {
    tenant['default_authentication_method'] for tenant in tenants
}

@DrPyser DrPyser force-pushed the db-session-context-manager branch from 6b964ee to 10d7eba Compare September 29, 2025 19:31
@QodoAI-Agent
Copy link

Persistent review updated to latest commit 10d7eba

@wazo-community-zuul
Copy link
Contributor

Build failed.
https://zuul.wazo.community/zuul/t/local/buildset/55dfc675a4ce43e79b210b021243469a

✔️ tox-linters SUCCESS in 7m 29s
✔️ wazo-tox-py39 SUCCESS in 6m 31s
✔️ debian-packaging-bullseye SUCCESS in 4m 05s
auth-tox-integration FAILURE in 9m 50s

@fblackburn1 fblackburn1 force-pushed the db-session-context-manager branch from 10d7eba to e292b0c Compare September 30, 2025 12:13
Copy link
Member

@fblackburn1 fblackburn1 left a comment

Choose a reason for hiding this comment

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

Out of Curiosity, did it cause you any issue?

@fblackburn1 fblackburn1 force-pushed the db-session-context-manager branch from e292b0c to a252f59 Compare September 30, 2025 12:15
@sonarqubecloud
Copy link

@wazo-community-zuul
Copy link
Contributor

Build failed.
https://zuul.wazo.community/zuul/t/local/buildset/a84f49af77204842b4cb8b0117bae2bd

✔️ tox-linters SUCCESS in 6m 57s
✔️ wazo-tox-py39 SUCCESS in 7m 40s
✔️ debian-packaging-bullseye SUCCESS in 3m 44s
auth-tox-integration FAILURE in 9m 13s

@DrPyser DrPyser marked this pull request as draft October 2, 2025 16:00
@DrPyser
Copy link
Contributor Author

DrPyser commented Oct 2, 2025

Out of Curiosity, did it cause you any issue?

Yep, the integration tests are failing. Will have to investigate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants