-
Notifications
You must be signed in to change notification settings - Fork 0
Сached get_base_directories #889
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
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request implements a caching mechanism for the get_base_directories function to improve performance by avoiding repeated database queries. The implementation introduces an async LRU cache decorator with TTL support and SQLAlchemy object expiration detection.
Changes:
- Added
async_lru_cachedecorator andhas_expired_sqla_objshelper function inhelpers.pyfor caching async functions with TTL-based expiration - Modified
get_base_directoriesto returnDirectoryDTOobjects instead of SQLAlchemyDirectoryentities and applied caching - Refactored
create_pathmethod signature to acceptparent_pathlist instead of parent object, updating all call sites accordingly
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| app/ldap_protocol/utils/helpers.py | Implements async LRU cache decorator with TTL and SQLAlchemy object expiration detection |
| app/ldap_protocol/utils/queries.py | Applies caching to get_base_directories and changes return type to DTOs; updates create_group to use parent.path |
| app/entities.py | Refactors create_path to accept parent_path list instead of parent Directory object |
| app/ldap_protocol/auth/setup_gateway.py | Updates create_dir to extract parent.path before calling create_path and sets parent_id manually |
| app/ldap_protocol/ldap_requests/add.py | Updates create_path call to pass parent.path instead of parent object |
| app/ldap_protocol/ldap_requests/modify_dn.py | Updates create_path call to pass parent_dir.path instead of parent object |
| app/ldap_protocol/ldap_requests/search.py | Updates type hint for base_directories parameter to accept DirectoryDTO |
| app/ldap_protocol/roles/role_use_case.py | Updates type hint for parent_directory parameter to accept DirectoryDTO |
| app/alembic/versions/71e642808369_add_directory_is_system.py | Refactors migration to use SQL update instead of modifying objects in loop |
| app/alembic/versions/16a9fa2c1f1e_rename_readonly_group.py | Updates create_path calls to extract parent.path before passing |
| tests/test_api/test_auth/test_router.py | Reorders imports to follow project conventions (moved PasswordUtils import) |
| pyproject.toml | Adds "dtos" to known-first-party imports list for import sorting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self._session.add(dir_) | ||
| await self._session.flush() | ||
| dir_.parent_id = parent.id if parent else None |
Copilot
AI
Jan 20, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parent_id is set after the flush but before the refresh. If parent is None, this sets parent_id to None. However, the refresh on line 144 only includes "id" in the attribute_names, not "parent_id". If the relationship between parent_id and parent needs to be maintained correctly, consider refreshing both or setting parent_id before the initial flush at line 142.
| self._session.add(dir_) | |
| await self._session.flush() | |
| dir_.parent_id = parent.id if parent else None | |
| dir_.parent_id = parent.id if parent else None | |
| self._session.add(dir_) | |
| await self._session.flush() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Почему то в таком случае parent_id не сохраняется
|
Я хз как сейчас реализовать механизм инвалидации кэша помимо TTL. Можно было бы переделать это на редис и привязаться к алхимическим сигналам, но не хочется эти сигналы трогать(мне они не нравятся). Возможно, учитывая, что эти "базовые директории" не будут меняться особо, механизм инвалидации не требуется |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rimu-stack, что скажешь насчет расположения этого модуля ?
| is_system: bool, | ||
| domain: Directory, | ||
| parent: Directory | None = None, | ||
| domain: Directory | DirectoryDTO, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Зачем здесь оставлять Directory ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Тут может приходить Directory (app/ldap_protocol/auth/setup_gateway.py::72)
|
|
||
| def create_object_sid( | ||
| domain: Directory, | ||
| domain: Directory | DirectoryDTO, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Здесь вроде тоже можно оставить только dto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Вызывается внутри create_dir, там могут быть оба варианта
Я точно против сигналов. Как реализовать механизм инвалидации кеша ? Как написал выше, предлагаю уйти от общего случая кеширования к кешированию конкретно get_base_directories. Соответственно, инвалидировать этот кеш нужно, когда мы добавляем новую корневую директорию. Корневые директории могут быть трех типов: Конфигурация (Она есть в AD, но её нет у нас), Схема (но она не хранится у нас в дереве) и домен. Возможность добавления нового домена пока даже не планируется. Соответственно, пока достаточно добавить инвалидацию только в момент добавления нашего домена, а именно в first_setup. *Сейчас еще подумал, что могут быть изменены атрибуты базовой директории, проверь это, возможно инвалидацию еще нужно добавить в modify \ modify_dn |
Задача: закэшировать результат работы get_base_directories
Решение: добавил механизм кэширования результатов асинхронных функций с учетом ttl и session.expire_all()