Skip to content

WIP: Feature/1258 ldap schema move from models to directories#958

Open
milov-dmitriy wants to merge 25 commits intodevfrom
feature/1258_ldap_schema_move_from_models_to_directories
Open

WIP: Feature/1258 ldap schema move from models to directories#958
milov-dmitriy wants to merge 25 commits intodevfrom
feature/1258_ldap_schema_move_from_models_to_directories

Conversation

@milov-dmitriy
Copy link
Collaborator

No description provided.

@milov-dmitriy milov-dmitriy self-assigned this Mar 5, 2026
Copilot AI review requested due to automatic review settings March 5, 2026 12:47
@milov-dmitriy milov-dmitriy added the python Pull requests that update Python code label Mar 5, 2026
Copy link
Contributor

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.

Pull request overview

This PR continues the migration of LDAP schema handling from in-model entities to directory-backed storage, introducing new schema directory creation use-cases and “deprecated” appendix DAOs/use-cases to support migration paths.

Changes:

  • Reworked AttributeType / ObjectClass persistence to rely on Directory + Attribute rows, with new “create directory like …” use-cases.
  • Updated LDAP search/subschema generation to use schema use-cases and raw-definition renderers (*RawDisplay).
  • Adjusted setup/migrations/tests to seed schema and entity types accordingly (with several tests temporarily stubbed).

Reviewed changes

Copilot reviewed 62 out of 64 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/test_shedule.py Switches test dependency from DAO to EntityType use-case.
tests/test_ldap/test_util/test_search.py Temporarily disables some LDAP utility integration tests.
tests/test_ldap/test_util/test_modify.py Temporarily disables some modify-related integration tests.
tests/test_ldap/test_util/test_add.py Temporarily disables ldapadd access-control integration test.
tests/test_ldap/test_roles/test_search.py Temporarily disables multiple role-based search tests.
tests/test_ldap/test_roles/test_multiple_access.py Temporarily disables multiple-ACE role test.
tests/test_ldap/test_ldap_schema/test_attribute_type_use_case.py Updates replication-flag tests to create test attribute types first.
tests/test_ldap/test_ldap3_definition_parse.py Updates raw-definition parsing tests to operate on DTOs + RawDisplay renderers.
tests/test_api/test_main/test_router/test_search.py Expands expected subtree dirs to include Configuration.
tests/test_api/test_main/test_router/conftest.py Updates test setup to use EntityTypeUseCase in SetupGateway wiring.
tests/test_api/test_ldap_schema/test_object_class_router.py Adjusts pagination size; temporarily disables part of modification assertions.
tests/test_api/test_ldap_schema/test_attribute_type_router_datasets.py Uses AttributeTypeUpdateSchema objects for patch datasets.
tests/test_api/test_ldap_schema/test_attribute_type_router.py Uses model_dump() for patch JSON, adjusts page size.
tests/test_api/test_auth/test_router.py Adds setup_session fixture for disabled-user auth test.
tests/constants.py Adds configuration directory + explicit entity_type_name fields across test seed data.
tests/conftest.py Major test-container wiring changes for new schema flow + deprecated appendix use-cases.
interface Updates git submodule pointer.
docker-compose.test.yml Adds -W ignore::SyntaxWarning during pytest in container.
app/repo/pg/tables.py Imports schema entities from entities_appendix instead of entities.
app/ldap_protocol/utils/raw_definition_parser.py Refactors raw-definition parser to produce DTOs (no DB session).
app/ldap_protocol/policies/password/dao.py Minor formatting change for create() signature.
app/ldap_protocol/ldap_schema/object_class_use_case.py Moves object-class creation logic to directory-backed creation + validation.
app/ldap_protocol/ldap_schema/object_class_raw_display.py New: render ObjectClass DTO as RFC4512-style raw definition.
app/ldap_protocol/ldap_schema/object_class_dir_create_use_case.py New: creates object-class directories under Configuration.
app/ldap_protocol/ldap_schema/object_class_dao.py Reworks DAO to read from Directories/Attributes instead of schema tables.
app/ldap_protocol/ldap_schema/entity_type_use_case.py Adds attach-to-directories logic and optional validation bypass for first setup.
app/ldap_protocol/ldap_schema/entity_type_dao.py Removes DAO-level attach logic; shifts object-class validation out of DAO.
app/ldap_protocol/ldap_schema/constants.py Removes compiled regex constant; keeps pattern.
app/ldap_protocol/ldap_schema/attribute_type_use_case.py Reworks attribute-type creation via directory-backed creation and deprecated exceptions.
app/ldap_protocol/ldap_schema/attribute_type_raw_display.py New: render AttributeType DTO as RFC4512-ish raw definition.
app/ldap_protocol/ldap_schema/attribute_type_dir_create_use_case.py New: creates attribute-type directories under Configuration.
app/ldap_protocol/ldap_schema/attribute_type_dao.py Reworks DAO to read from Directories/Attributes instead of schema tables.
app/ldap_protocol/ldap_schema/appendix/object_class_appendix/object_class_appendix_use_case.py New: deprecated object-class use-case for migration.
app/ldap_protocol/ldap_schema/appendix/object_class_appendix/object_class_appendix_dao.py New: deprecated object-class DAO for migration + table drop helper.
app/ldap_protocol/ldap_schema/appendix/attribute_type_appendix/attribute_type_appendix_use_case.py New: deprecated attribute-type use-case for migration.
app/ldap_protocol/ldap_schema/appendix/attribute_type_appendix/attribute_type_appendix_dao.py New: deprecated attribute-type DAO for migration + table drop helper.
app/ldap_protocol/ldap_requests/search.py Switches subschema generation to use schema use-cases + RawDisplay.
app/ldap_protocol/ldap_requests/modify.py Switches entity-type attach from DAO to use-case.
app/ldap_protocol/ldap_requests/contexts.py Updates request contexts to carry schema use-cases and EntityTypeUseCase.
app/ldap_protocol/ldap_requests/add.py Switches entity-type lookup/attach from DAO to use-case.
app/ldap_protocol/filter_interpreter.py Reworks ANR attribute lookup to query directory-backed attribute-type flags.
app/ldap_protocol/auth/use_cases.py Seeds new directory-based schema from deprecated tables during setup.
app/ldap_protocol/auth/setup_gateway.py Uses EntityTypeUseCase to attach entity types during environment setup.
app/ioc.py Adds wiring for deprecated appendix DAOs/use-cases and dir-create use-cases.
app/extra/scripts/add_domain_controller.py Uses EntityTypeUseCase instead of EntityTypeDAO.
app/extra/alembic_utils.py Adds temporary_stub_column2() helper for migrations.
app/enums.py Adds EntityTypeNames for Configuration/Attribute Type/Object Class; removes auth rule constant.
app/entities_appendix.py New: deprecated schema dataclasses (AttributeType/ObjectClass).
app/entities.py Moves schema dataclasses out; keeps AttributeType type via appendix import.
app/constants.py Adds Configuration dir in first setup data + entity types for schema entities.
app/api/ldap_schema/schema.py Switches constants import to API-local constants module.
app/api/ldap_schema/attribute_type_router.py Formatting-only change for update call.
app/api/ldap_schema/adapters/object_class.py Replaces adaptix converter with manual mapping supporting Directory/DTO.
app/api/ldap_schema/adapters/entity_type.py Switches constants import to API-local constants module.
app/api/ldap_schema/adapters/attribute_type.py Switches constants import to API-local constants module.
app/alembic/versions/f24ed0e49df2_add_filter_anr.py Migration now uses deprecated use-case via dishka + add temp stub column.
app/alembic/versions/c4888c68e221_fix_admin_attr_and_policy.py Uses EntityTypeUseCase for attaching entity types during migration.
app/alembic/versions/ba78cef9700a_initial_entity_type.py Uses EntityTypeUseCase; defers creation of new schema-related entity types.
app/alembic/versions/759d196145ae_.py New migration to create new schema-related entity types + copy schema to directories.
app/alembic/versions/2dadf40c026a_add_system_flags_to_attribute_types.py Migration now uses deprecated use-case for replication flags.
app/alembic/versions/275222846605_initial_ldap_schema.py Refactors initial schema seed to use DTO collection + deprecated use-cases.
app/alembic/versions/01f3f05a5b11_add_primary_group_id.py Uses EntityTypeUseCase for directory entity-type attachment in migration.
Comments suppressed due to low confidence (8)

app/ldap_protocol/utils/raw_definition_parser.py:1

  • tmp.values() yields AttributeTypeInfo/ObjectClassInfo objects, but _list_to_string() is meant for iterables of strings and returns str | None. As written, both methods will return the wrong type (and may return None) despite their return annotations, breaking parsing at runtime. Fix by returning the single parsed info object directly (e.g., pick the only value from the mapping) and keep _list_to_string() for actual string-list fields like info.name/info.superior.
    app/ldap_protocol/utils/raw_definition_parser.py:1
  • This error message is misleading in the object-class path (collect_object_class_dto_from_info). It should reference 'Object Class name' (not 'Attribute Type name') to help diagnose parsing failures correctly.
    app/ldap_protocol/ldap_schema/entity_type_use_case.py:1
  • EntityTypeUseCase uses self.__session, but the diff does not show it being initialized in __init__. This will raise AttributeError when attach_entity_type_to_directories() runs (e.g., from migrations). Inject AsyncSession into the use-case (like other use-cases) and assign it, or refactor to execute these queries via a DAO that owns the session.
    app/ldap_protocol/ldap_schema/object_class_dir_create_use_case.py:1
  • This uses the loop variable value from the prior for value in values loop, so the isinstance(value, str) check is unrelated to dir_.object_class and will be wrong (or even reference a stale value). Use dir_.object_class consistently in the check (as done in the AttributeType variant), or remove this extra objectClass insertion entirely if data['attributes']['objectClass'] already sets it. Also consider setting Directory.object_class to a meaningful value (e.g., classSchema) instead of \"\" to avoid creating an empty objectClass attribute.
    app/ldap_protocol/ldap_schema/object_class_dao.py:1
  • This update path is inconsistent with the new directory-backed schema storage: self.get(name) returns a DTO, so mutating obj.attribute_types_* won't persist changes. Additionally, AttributeType here is imported from entities_appendix (a dataclass), so select(AttributeType) is not a valid SQLAlchemy selectable and will fail at runtime. To fix, load the underlying Directory + its Attribute rows (via get_dir()), then update/replace the appropriate Attribute records (attribute_types_must / attribute_types_may, superior_name, etc.), and flush.
    app/ldap_protocol/ldap_schema/object_class_dao.py:1
  • The method name get_object_class_names_include_attribute_type1 looks like a temporary name and is unclear/confusing. Rename it to a stable API name (or remove it if superseded by get_object_class_names_include_attribute_type).
    tests/test_ldap/test_util/test_search.py:1
  • Early return effectively disables the test while still reporting it as passed, which can hide regressions in the schema-migration work. Prefer pytest.skip(\"TODO: ...\") (or pytest.mark.xfail) so the test suite clearly reports the missing coverage; the same applies to the other newly-added early returns in this PR.
    docker-compose.test.yml:1
  • Ignoring SyntaxWarning in CI can mask real issues introduced by the refactor (especially around SQLAlchemy queries and deprecated APIs). Consider removing this ignore, or narrowing it to a specific known warning source (module/class) so newly introduced warnings still fail or are visible.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@milov-dmitriy milov-dmitriy requested a review from Copilot March 5, 2026 15:11
Copy link
Contributor

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.

Pull request overview

Copilot reviewed 62 out of 64 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (11)

app/ldap_protocol/utils/raw_definition_parser.py:1

  • _get_attribute_type_info() / get_object_class_info() now return _list_to_string(tmp.values()), but _list_to_string() returns str | None (and is intended for strings), not AttributeTypeInfo / ObjectClassInfo. This will break parsing at runtime. Restore returning the single ...Info instance (e.g., take the only element from tmp.values()), or generalize _list_to_string() with a TypeVar so it can return the single element of any iterable while preserving the return type.
    app/ldap_protocol/utils/raw_definition_parser.py:1
  • The error message is incorrect for this context: this is parsing an Object Class name, but it raises ValueError(\"Attribute Type name is required\"). Update the message to refer to the Object Class name to make debugging actionable.
    app/ldap_protocol/ldap_schema/object_class_dir_create_use_case.py:1
  • value here comes from the for name, values in data[\"attributes\"].items(): for value in values: loop above, so after the loop it will be the last iterated attribute value (or may be undefined if attributes is empty). This makes the objectClass attribute value incorrect/non-deterministic. Use dir_.object_class (and check isinstance(dir_.object_class, str)), or explicitly set the intended objectClass value(s) without relying on a loop-scoped variable.
    app/ldap_protocol/ldap_schema/object_class_dao.py:1
  • attributes_dict.get(\"...\") can return None, and indexing [0] will raise TypeError. This is especially likely for optional fields like superior_name. Consider using a safe default (e.g., dir_.attributes_dict.get(\"superior_name\", [None])[0]) and similarly guard required fields with a clearer error when missing.
    app/ldap_protocol/ldap_schema/object_class_dao.py:1
  • update() now loads obj via self.get(name), which returns an ObjectClassDTO, not a mutable ORM model/Directory entity. The method then tries to mutate obj.attribute_types_must with ORM AttributeType objects, which will either fail (DTO shape) or corrupt the DTO type (list[str] vs list[AttributeType]). Refactor update() to load and mutate the underlying Directory + Attribute rows (or delegate updates to the new Directory-backed create/update use case) instead of using the DTO.
    app/ldap_protocol/ldap_schema/object_class_dao.py:1
  • update() now loads obj via self.get(name), which returns an ObjectClassDTO, not a mutable ORM model/Directory entity. The method then tries to mutate obj.attribute_types_must with ORM AttributeType objects, which will either fail (DTO shape) or corrupt the DTO type (list[str] vs list[AttributeType]). Refactor update() to load and mutate the underlying Directory + Attribute rows (or delegate updates to the new Directory-backed create/update use case) instead of using the DTO.
    app/ldap_protocol/ldap_schema/attribute_type_dao.py:1
  • get_all() converts each Directory using attributes_dict, but the query does not eager-load Directory.attributes. That risks N+1 lazy loads and may produce incomplete attributes_dict depending on how it's computed. Align with other queries in this PR by adding selectinload(qa(Directory.attributes)) (and any other required relationships) before converting.
    app/ldap_protocol/ldap_schema/attribute_type_use_case.py:1
  • set_attr_replication_flag() still exists on the use case, but its permission mapping was removed. If authorization enforcement relies on PERMISSIONS, this creates an authorization gap or breaks expected policy checks. Either re-add a permission mapping for set_attr_replication_flag (and keep/restore the corresponding AuthorizationRules entry), or remove/relocate the method if it is intentionally no longer part of the public service surface.
    app/ldap_protocol/ldap_schema/object_class_use_case.py:1
  • superior_name is always stored as [str(dto.superior_name)]. When dto.superior_name is None, this persists the literal string 'None', which will later be treated as a real superior name. Only include superior_name when it is not None, or store an empty list/omit the attribute entirely.
    tests/test_ldap/test_util/test_search.py:1
  • Returning early from a test makes it pass without exercising behavior or assertions, which can hide regressions in CI. Prefer pytest.skip(\"...\") (explicitly marking it as skipped with a reason) or pytest.xfail(\"...\") if it is expected to fail until the refactor is complete.
    docker-compose.test.yml:1
  • Ignoring SyntaxWarning globally can mask real issues (especially around invalid escapes, deprecated syntax, or runtime parsing warnings) and makes the test signal weaker. Consider scoping the filter to the specific warning source/module or fixing the underlying warnings instead of suppressing them across the entire test run.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

Labels

python Pull requests that update Python code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants