Flatten API module structure, add session management, consolidate DI#7451
Flatten API module structure, add session management, consolidate DI#7451
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
galvana
changed the title
galvana
requested review from
gilluminate
and removed request for
a team and
gilluminate
Greptile SummaryThis PR performs a structural refactoring of the Fides API module layout, session management, and dependency injection:
The refactoring is largely mechanical (import path updates) with focused architectural changes in the service and session layers. One test mock target ( This PR changes 78 files, which exceeds the recommended 15-file limit for a single PR. While this is understandable given the nature of a structural refactoring (most changes are mechanical import updates), it does make review more difficult. Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 41499ac |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
src/fides/api/service/deps.py
Outdated
| """ | ||
| Re-export shim -- canonical location is fides.api.deps. | ||
| """ | ||
|
|
||
| from fides.api.deps import ( # noqa: F401 | ||
| get_connection_service, | ||
| get_dataset_config_service, | ||
| get_dataset_service, | ||
| get_event_audit_service, | ||
| get_messaging_service, | ||
| get_privacy_request_service, | ||
| get_system_service, | ||
| get_taxonomy_service, | ||
| get_user_service, | ||
| ) |
There was a problem hiding this comment.
are we moving these in a follow up PR?
There was a problem hiding this comment.
Yeah, just added this temporarily
There was a problem hiding this comment.
didn't Adam also add this file in his PR?
github-actions
bot
requested review from
eastandwestwind
and removed request for
a team
erosselli
left a comment
erosselli
left a comment
There was a problem hiding this comment.
Approving with a comment
There was a problem hiding this comment.
I now see two new files src/fides/common/session/session_management.py and src/fides/common/session_management.py -- but this file should already be added by Adam's PR , can you check to make sure the merge was handled ok? :)
…ate deps - Flatten api/api/ into api/ (move deps.py and v1/ up one level) - Create common/session/session_management.py with composable transaction boundary decorators (ported from fidesplus v3) - Add service/exceptions.py with base domain exception hierarchy - Consolidate service factory functions from api/service/deps.py into api/deps.py as the single canonical DI location - Convert api/service/deps.py to a re-export shim - Update all imports across src/ and tests/ Co-authored-by: Cursor <cursoragent@cursor.com>
DSR 3.0 is now the only execution path. This removes the deprecated DSR 2.0 sequential/Dask-based execution model including: - deprecated_graph_task.py and scheduler_utils.py modules - Redis caching methods from TaskResources - DSR 2.0 fallback paths in privacy_request model methods - to_mock_execution_node() from traversal.py - use_dsr_2_0 test fixture and all DSR version parameterization - Deleted test files: test_deprecated_graph_task, test_task_resources, test_dsr_3_0_default Moved format_data_use_map_for_caching() to create_request_tasks.py as it is still used by DSR 3.0. Co-authored-by: Cursor <cursoragent@cursor.com>
DSR 3.0 is the only execution path. Remove all dsr_version/use_dsr_3_0 test parameterization, conditional DSR version logic, and the no-op use_dsr_3_0 fixture and config setting across 44 files. Co-authored-by: Cursor <cursoragent@cursor.com>
service/exceptions.py was added but never imported anywhere — removing to keep the PR focused on the structural refactoring. Co-authored-by: Cursor <cursoragent@cursor.com>
- Move get_api_session + engine singleton and get_autoclose_db_session from deps.py to common/session/session_management.py - Migrate all consumers to import from fides.common.session - Remove TYPE_CHECKING block and deferred imports from deps.py; promote service imports to top-level - Remove unused domain exceptions module - Break circular import between deps.py and privacy_request_service.py Co-authored-by: Cursor <cursoragent@cursor.com>
Extract messaging config status logic into MessagingService, compose MessagingService into UserService to eliminate service-to-route import, and promote all service imports to top-level in deps.py. Co-authored-by: Cursor <cursoragent@cursor.com>
Use the 'import X as X' pattern so mypy recognizes these as explicit re-exports from fides.api.deps. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Eliana Rosselli <67162025+erosselli@users.noreply.github.com>
galvana
force-pushed
the
flatten-api-structure
branch
from
09f6ccf to
b6baf7c
Compare
adamsachs
left a comment
adamsachs
left a comment
There was a problem hiding this comment.
👍 generally looks good to me, thanks for continuing to push this forward!
only one question really, i'm not sure i agree with the DI consolidation - not that this is worse than before, but i just think another approach may be better, if we're updating that anyway? maybe i'm not thinking about it correctly though, open to hear a different POV or continue to iterate as needed.
There was a problem hiding this comment.
OK - so common will be like lib/shared utils, i think that's probably better...and core will be for core 'features', e.g. Systems, etc? or do we want to get rid of core?
There was a problem hiding this comment.
I'm treating common as shared utils (session management, encryption, constants, etc). I think core originally referred to the actions that could be taken by fidesctl through the CLI. Not sure if core is a concept that we want to apply any meaning to now. Fides will just have a base set of "domains" (privacy requests, systems, datasets) but I'm deferring the decision as to what is considered core functionality.
| # --------------------------------------------------------------------------- | ||
| # Service factory functions for FastAPI dependency injection. | ||
| # --------------------------------------------------------------------------- |
There was a problem hiding this comment.
you think this is better here, rather than a deps.py module within each 'feature'/service package?
i feel like coupling them all together in a single module here gives us this central deps.py module with a huge dependency tree, and that doesn't seem great. whereas each 'feature' exposing its own deps.py allows those dependencies to be pulled in selectively by the endpoints that need them.
There was a problem hiding this comment.
I agree that this file could get huge, but I also think that Depends is an API-layer concern. I've been thinking about keeping the routes under a root-level api directory along cli. These are just two different ways that we expose features/services. Something like this:
api -> deps -> services
cli -> services
We can chat about this more.
There was a problem hiding this comment.
I think I tend to agree with Adam about having module-level deps. Even though they're an API-layer concern, the api endpoints themselves are also going to be defined within each module right?
lmk if you're meeting to chat about this, can I join?
3 participants
Summary
Structural refactoring to flatten the nested
api/api/package, introduce shared session management infrastructure incommon/session, consolidate dependency injection, and break a circular import cycle.API module flatten
api/api/deps.py→api/deps.pyandapi/api/v1/→api/v1/, eliminating the redundant nestedapi/packagesrc/andtests/Session management (
common/session)common/session/session_management.pyas the canonical owner of:get_api_session()— engine singleton + session factory (moved fromdeps.py)get_autoclose_db_session()— context-managed session lifecycle@with_optional_sync_session/@with_optional_async_session— composable transaction-boundary decorators (ported from the fidesplus v3 layer)graph_task,saas_connector,sql_connector,middleware,identity_salt,memory_watchdog,hash_migration_job,encrypted_large_data,connector_registry_service,app_setup) to import session utilities fromfides.common.sessioninstead offides.api.depsdeps.py↔privacy_request_service.pyby having the service import fromcommon.sessionDI consolidation
api/service/deps.pyintoapi/deps.pyas the single canonical DI locationapi/service/deps.pyremains as a re-export shim for backward compatibilityTYPE_CHECKINGblock or deferred in-body imports)Break circular import cycle
get_messaging_config_status()andis_email_invite_enabled()business logic frommessaging_endpoints.pyroute handlers intoMessagingServiceMessagingServiceviaDepends(get_messaging_service)MessagingServiceintoUserService(injected viaget_user_servicefactory) to replace the service→route import ofuser_email_invite_statusdeps.py→UserService→messaging_endpoints→deps.pycycleCode Changes
api/api/deps.py→api/deps.pyandapi/api/v1/→api/v1/(46 endpoint files)common/session/session_management.py(~127 lines) with engine singleton, session factory, and transaction decoratorsget_api_session+ engine singleton fromdeps.pytocommon/session(single connection pool)fides.api.deps→fides.common.sessionfor session importsMessagingService.get_messaging_config_status()andis_email_invite_enabled()MessagingServiceintoUserServiceto eliminate service→route importSteps to Confirm
nox -s static_checkspassesnox -s pytest— all existing tests passfides_user_device_idtoProvidedIdentityTypeEnum #3131 imports alignPre-Merge Checklist
CHANGELOG.mdupdated