Flatten API module structure, add session management, consolidate DI#7451
Merged
Flatten API module structure, add session management, consolidate DI#7451
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Contributor
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 |
erosselli
reviewed
Feb 26, 2026
src/fides/api/service/deps.py
Outdated
Comment on lines
+1
to
+15
| """ | ||
| 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, | ||
| ) |
Contributor
There was a problem hiding this comment.
are we moving these in a follow up PR?
Contributor
Author
There was a problem hiding this comment.
Yeah, just added this temporarily
Contributor
There was a problem hiding this comment.
didn't Adam also add this file in his PR?
erosselli
approved these changes
Feb 27, 2026
Contributor
erosselli
left a comment
There was a problem hiding this comment.
Approving with a comment
Contributor
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>
09f6ccf to
b6baf7c
Compare
adamsachs
approved these changes
Feb 28, 2026
Contributor
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.
Contributor
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?
Contributor
Author
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.
Comment on lines
+79
to
+81
| # --------------------------------------------------------------------------- | ||
| # Service factory functions for FastAPI dependency injection. | ||
| # --------------------------------------------------------------------------- |
Contributor
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.
Contributor
Author
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.
Contributor
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?
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates and relocates FastAPI dependency providers and session management: adds Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as FastAPI Endpoint
participant DI as Depends (src/fides/api/deps.py)
participant Service as MessagingService / OtherService
participant DB as Database (SQLAlchemy)
participant Cache as Redis
Client->>API: HTTP request
API->>DI: Resolve dependencies (get_db / get_autoclose_db_session, get_cache, get_messaging_service)
DI->>DB: get_autoclose_db_session -> Session
DI->>Cache: get_cache -> Redis (if configured)
API->>Service: Call injected service method
Service->>DB: queries/commits via Session
Service->>Cache: read/write cache
Service-->>API: Result
API-->>Client: HTTP response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/ops/api/v1/endpoints/test_saas_config_endpoints.py (1)
481-483:⚠️ Potential issue | 🟡 MinorUpdate patch path to match flattened module structure.
Line 481-483 uses
fides.api.api.v1.endpoints.saas_config_endpointswhile line 572 correctly usesfides.api.v1.endpoints.saas_config_endpoints. The actual module is located atsrc/fides/api/v1/endpoints/saas_config_endpoints.py, so the patch at lines 481-483 should be updated to remove the duplicate "api" in the path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ops/api/v1/endpoints/test_saas_config_endpoints.py` around lines 481 - 483, Update the mock.patch target string to match the flattened module path used elsewhere: replace "fides.api.api.v1.endpoints.saas_config_endpoints.ConnectorRegistry.get_connector_template" with "fides.api.v1.endpoints.saas_config_endpoints.ConnectorRegistry.get_connector_template" in the test where ConnectorRegistry.get_connector_template is being patched so it points to the actual module location.
🧹 Nitpick comments (5)
src/fides/api/v1/endpoints/manual_webhook_endpoints.py (1)
27-27: Move_get_connection_configto shared deps module to reduce endpoint-to-endpoint coupling.Line 27 imports
_get_connection_configfromdataset_config_endpoints. This function is a FastAPI dependency that's also imported byprivacy_request_endpoints.py. Move it tofides.api.depsas a public function (e.g.,get_connection_config) to establish a single source of truth and reduce cross-endpoint coupling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/v1/endpoints/manual_webhook_endpoints.py` at line 27, The private dependency function _get_connection_config should be moved out of dataset_config_endpoints and into the shared deps module as a public dependency named get_connection_config in fides.api.deps; copy the implementation from _get_connection_config into fides.api.deps, keep the same signature and behavior but rename to get_connection_config, then update all imports that reference _get_connection_config (e.g., in manual_webhook_endpoints.py and privacy_request_endpoints.py) to import get_connection_config from fides.api.deps and remove the original import from dataset_config_endpoints; ensure dataset_config_endpoints no longer defines or exports _get_connection_config.src/fides/service/user/user_service.py (1)
43-58: Consider consolidating invite dispatch intoMessagingService.Line 43 now gates through
MessagingService, but Lines 50-55 still dispatch directly. Moving dispatch behindMessagingServicewould keep enablement + delivery logic in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/service/user/user_service.py` around lines 43 - 58, The invite dispatch is done directly via dispatch_message (dispatch_message, MessagingActionType.USER_INVITE, Identity, UserInviteBodyParams) instead of through the existing MessagingService; refactor by adding a method on MessagingService (e.g., send_user_invite or dispatch_user_invite) that accepts the invite details (username, invite_code, to_identity/email, and message params) and encapsulates feature gating (is_email_invite_enabled) and service_type selection (config_proxy.notifications.notification_service_type), then replace the direct call to dispatch_message in this block with a call to that new MessagingService method while leaving FidesUserInvite.create and user.update as-is.src/fides/api/deps.py (3)
56-62: Consider adding return type parameter for the cache generator.The
get_cache()function yields a Redis connection but the return type is unparameterized. Additionally, consider whether this should be a dependency that yields (likeget_db) rather than directly yielding the connection, since the current pattern doesn't have cleanup logic.🔧 Suggested improvement
-def get_cache() -> Generator: +def get_cache() -> Generator[FidesopsRedis, None, None]: """Return a connection to our Redis cache""" if not CONFIG.redis.enabled: raise RedisNotConfigured( "Application redis cache required, but it is currently disabled! Please update your application configuration to enable integration with a Redis cache." ) yield get_redis_connection()Note: You'll need to import the appropriate Redis type from
fides.api.util.cache.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/deps.py` around lines 56 - 62, The get_cache() dependency currently has an unparameterized Generator return type and yields a Redis connection without cleanup; update its signature to Generator[Redis, None, None] (import Redis from fides.api.util.cache), instantiate the connection into a local variable via get_redis_connection(), use a try/finally around the yield to mirror the get_db pattern, and in finally ensure the connection is properly closed/cleaned up (call the appropriate close/disconnect method on that Redis instance) so the dependency has correct typing and cleanup.
43-49: Same type annotation improvement applies here.🔧 Optional type annotation improvement
-def get_readonly_db() -> Generator: +def get_readonly_db() -> Generator[Session, None, None]: """Return our readonly database session"""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/deps.py` around lines 43 - 49, The get_readonly_db generator is annotated only as Generator; update its return type to a precise Generator[...] using the DB session type (e.g., Generator[Session, None, None] or Generator[AsyncSession, None, None] as appropriate) so callers and type checkers know the yielded value is a DB session; adjust imports to include typing.Generator and the correct SQLAlchemy Session type and update the signature for get_readonly_db (and keep get_readonly_api_session usage unchanged).
34-40: Consider adding explicit type parameters for better type inference.The return type
Generatorwithout type parameters reduces IDE support and type checker effectiveness. Consider specifyingGenerator[Session, None, None].🔧 Optional type annotation improvement
-def get_db() -> Generator: +def get_db() -> Generator[Session, None, None]: """Return our database session"""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/deps.py` around lines 34 - 40, The get_db dependency currently types its return as plain Generator which hurts type inference; update the signature of get_db to use a parameterized generator type such as Generator[Session, None, None] (import Session from your ORM/session module and Generator from typing) so IDEs and type checkers can infer the yielded session from get_api_session and callers of get_db.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fides/api/middleware.py`:
- Line 36: The code creates a DB session via get_api_session() and assigns it to
db but never closes it, risking a session leak; update the call site that uses
write_audit_log_resource_record to ensure the session is always closed by
wrapping the usage in a try/finally (call db.close() in finally) or use a
context manager that closes the Session, referencing the get_api_session() call
and the db variable so the cleanup happens after write_audit_log_resource_record
returns or on exception.
In `@src/fides/common/session_management.py`:
- Around line 52-56: The finally block calls db.close() even if
get_api_session() or get_readonly_api_session() raised before db was assigned,
causing UnboundLocalError; initialize db = None before the try in the
generator/contextmanager functions (the ones that call get_api_session() and
get_readonly_api_session()) and change the finally to only call db.close() when
db is not None (or check 'if db:'/'if db is not None:') so the original
exception is preserved.
---
Outside diff comments:
In `@tests/ops/api/v1/endpoints/test_saas_config_endpoints.py`:
- Around line 481-483: Update the mock.patch target string to match the
flattened module path used elsewhere: replace
"fides.api.api.v1.endpoints.saas_config_endpoints.ConnectorRegistry.get_connector_template"
with
"fides.api.v1.endpoints.saas_config_endpoints.ConnectorRegistry.get_connector_template"
in the test where ConnectorRegistry.get_connector_template is being patched so
it points to the actual module location.
---
Nitpick comments:
In `@src/fides/api/deps.py`:
- Around line 56-62: The get_cache() dependency currently has an unparameterized
Generator return type and yields a Redis connection without cleanup; update its
signature to Generator[Redis, None, None] (import Redis from
fides.api.util.cache), instantiate the connection into a local variable via
get_redis_connection(), use a try/finally around the yield to mirror the get_db
pattern, and in finally ensure the connection is properly closed/cleaned up
(call the appropriate close/disconnect method on that Redis instance) so the
dependency has correct typing and cleanup.
- Around line 43-49: The get_readonly_db generator is annotated only as
Generator; update its return type to a precise Generator[...] using the DB
session type (e.g., Generator[Session, None, None] or Generator[AsyncSession,
None, None] as appropriate) so callers and type checkers know the yielded value
is a DB session; adjust imports to include typing.Generator and the correct
SQLAlchemy Session type and update the signature for get_readonly_db (and keep
get_readonly_api_session usage unchanged).
- Around line 34-40: The get_db dependency currently types its return as plain
Generator which hurts type inference; update the signature of get_db to use a
parameterized generator type such as Generator[Session, None, None] (import
Session from your ORM/session module and Generator from typing) so IDEs and type
checkers can infer the yielded session from get_api_session and callers of
get_db.
In `@src/fides/api/v1/endpoints/manual_webhook_endpoints.py`:
- Line 27: The private dependency function _get_connection_config should be
moved out of dataset_config_endpoints and into the shared deps module as a
public dependency named get_connection_config in fides.api.deps; copy the
implementation from _get_connection_config into fides.api.deps, keep the same
signature and behavior but rename to get_connection_config, then update all
imports that reference _get_connection_config (e.g., in
manual_webhook_endpoints.py and privacy_request_endpoints.py) to import
get_connection_config from fides.api.deps and remove the original import from
dataset_config_endpoints; ensure dataset_config_endpoints no longer defines or
exports _get_connection_config.
In `@src/fides/service/user/user_service.py`:
- Around line 43-58: The invite dispatch is done directly via dispatch_message
(dispatch_message, MessagingActionType.USER_INVITE, Identity,
UserInviteBodyParams) instead of through the existing MessagingService; refactor
by adding a method on MessagingService (e.g., send_user_invite or
dispatch_user_invite) that accepts the invite details (username, invite_code,
to_identity/email, and message params) and encapsulates feature gating
(is_email_invite_enabled) and service_type selection
(config_proxy.notifications.notification_service_type), then replace the direct
call to dispatch_message in this block with a call to that new MessagingService
method while leaving FidesUserInvite.create and user.update as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: abcdb9af-331f-4b51-b045-76eb43b8b686
📒 Files selected for processing (83)
docs/fides/docs/system-integration-link/02-technical-design.mdsrc/fides/api/api/__init__.pysrc/fides/api/api/deps.pysrc/fides/api/app_setup.pysrc/fides/api/cryptography/identity_salt.pysrc/fides/api/db/seed.pysrc/fides/api/deps.pysrc/fides/api/middleware.pysrc/fides/api/migrations/hash_migration_job.pysrc/fides/api/models/field_types/encrypted_large_data.pysrc/fides/api/oauth/system_manager_oauth_util.pysrc/fides/api/oauth/utils.pysrc/fides/api/service/connectors/saas/connector_registry_service.pysrc/fides/api/service/connectors/saas_connector.pysrc/fides/api/service/connectors/sql_connector.pysrc/fides/api/service/deps.pysrc/fides/api/task/graph_task.pysrc/fides/api/util/connection_util.pysrc/fides/api/util/memory_watchdog.pysrc/fides/api/v1/__init__.pysrc/fides/api/v1/api.pysrc/fides/api/v1/endpoints/__init__.pysrc/fides/api/v1/endpoints/admin.pysrc/fides/api/v1/endpoints/config_endpoints.pysrc/fides/api/v1/endpoints/connection_endpoints.pysrc/fides/api/v1/endpoints/connection_type_endpoints.pysrc/fides/api/v1/endpoints/connector_template_endpoints.pysrc/fides/api/v1/endpoints/consent_request_endpoints.pysrc/fides/api/v1/endpoints/dataset_config_endpoints.pysrc/fides/api/v1/endpoints/drp_endpoints.pysrc/fides/api/v1/endpoints/dsr_package_link.pysrc/fides/api/v1/endpoints/encryption_endpoints.pysrc/fides/api/v1/endpoints/generate.pysrc/fides/api/v1/endpoints/generic.pysrc/fides/api/v1/endpoints/generic_overrides.pysrc/fides/api/v1/endpoints/health.pysrc/fides/api/v1/endpoints/identity_verification_endpoints.pysrc/fides/api/v1/endpoints/manual_webhook_endpoints.pysrc/fides/api/v1/endpoints/masking_endpoints.pysrc/fides/api/v1/endpoints/messaging_endpoints.pysrc/fides/api/v1/endpoints/oauth_endpoints.pysrc/fides/api/v1/endpoints/partitioning_endpoints.pysrc/fides/api/v1/endpoints/policy_endpoints.pysrc/fides/api/v1/endpoints/policy_webhook_endpoints.pysrc/fides/api/v1/endpoints/pre_approval_webhook_endpoints.pysrc/fides/api/v1/endpoints/privacy_request_endpoints.pysrc/fides/api/v1/endpoints/privacy_request_redaction_patterns_endpoints.pysrc/fides/api/v1/endpoints/registration_endpoints.pysrc/fides/api/v1/endpoints/router_factory.pysrc/fides/api/v1/endpoints/saas_config_endpoints.pysrc/fides/api/v1/endpoints/storage_endpoints.pysrc/fides/api/v1/endpoints/system.pysrc/fides/api/v1/endpoints/user_endpoints.pysrc/fides/api/v1/endpoints/user_permission_endpoints.pysrc/fides/api/v1/endpoints/validate.pysrc/fides/api/v1/endpoints/view.pysrc/fides/api/v1/endpoints/worker_endpoints.pysrc/fides/api/v1/exception_handlers.pysrc/fides/common/session_management.pysrc/fides/core/repository/__init__.pysrc/fides/core/repository/session_management.pysrc/fides/service/messaging/messaging_service.pysrc/fides/service/privacy_request/privacy_request_service.pysrc/fides/service/user/user_service.pysrc/fides/system_integration_link/repository.pysrc/fides/system_integration_link/service.pytests/ctl/api/test_admin.pytests/ctl/api/test_generate.pytests/ctl/api/test_validate.pytests/ctl/core/test_api.pytests/ops/api/test_deps.pytests/ops/api/v1/endpoints/privacy_request/test_privacy_request_endpoints.pytests/ops/api/v1/endpoints/test_dataset_config_endpoints.pytests/ops/api/v1/endpoints/test_encryption_endpoints.pytests/ops/api/v1/endpoints/test_messaging_endpoints.pytests/ops/api/v1/endpoints/test_oauth_endpoints.pytests/ops/api/v1/endpoints/test_registration_endpoints.pytests/ops/api/v1/endpoints/test_saas_config_endpoints.pytests/ops/api/v1/endpoints/test_storage_endpoints.pytests/ops/api/v1/endpoints/test_user_endpoints.pytests/ops/models/test_consent_request.pytests/ops/util/test_memory_watchdog.pytests/service/test_taxonomy_service.py
💤 Files with no reviewable changes (4)
- tests/service/test_taxonomy_service.py
- src/fides/core/repository/session_management.py
- src/fides/api/api/deps.py
- src/fides/api/service/deps.py
| "extra_data": None, | ||
| } | ||
| db: Session = deps.get_api_session() | ||
| db: Session = get_api_session() |
There was a problem hiding this comment.
Potential session leak: session created but not explicitly closed.
The session is obtained via get_api_session() but never closed after write_audit_log_resource_record completes. Unlike the FastAPI dependency injection flow (which uses a generator with finally: db.close()), direct calls to get_api_session() require manual cleanup.
Consider wrapping in a try/finally or using a context manager:
🔧 Suggested fix
- db: Session = get_api_session()
+ db: Session = get_api_session()
+ try:
+ # get the user id associated with the request
+ token = request.headers.get("authorization")
+ if token:
+ client = await get_client_user_id(db, token)
+ audit_log_resource_data["user_id"] = client
- # get the user id associated with the request
- token = request.headers.get("authorization")
- if token:
- client = await get_client_user_id(db, token)
- audit_log_resource_data["user_id"] = client
+ # Access request body to check for fides_keys
+ await set_body(request, await request.body())
- # Access request body to check for fides_keys
- await set_body(request, await request.body())
+ body = await get_body(request)
+ fides_keys: List = await extract_data_from_body(body)
+ audit_log_resource_data["fides_keys"] = fides_keys
- body = await get_body(request)
- fides_keys: List = await extract_data_from_body(body)
- audit_log_resource_data["fides_keys"] = fides_keys
-
- # write record to server
- await write_audit_log_resource_record(db, audit_log_resource_data)
+ # write record to server
+ await write_audit_log_resource_record(db, audit_log_resource_data)
+ finally:
+ db.close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fides/api/middleware.py` at line 36, The code creates a DB session via
get_api_session() and assigns it to db but never closes it, risking a session
leak; update the call site that uses write_audit_log_resource_record to ensure
the session is always closed by wrapping the usage in a try/finally (call
db.close() in finally) or use a context manager that closes the Session,
referencing the get_api_session() call and the db variable so the cleanup
happens after write_audit_log_resource_record returns or on exception.
Comment on lines
+52
to
+56
| try: | ||
| db = get_api_session() | ||
| yield db | ||
| finally: | ||
| db.close() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/fides/common/session_management.py | head -80Repository: ethyca/fides
Length of output: 3365
🏁 Script executed:
cat -n src/fides/common/session_management.py | sed -n '165,180p'Repository: ethyca/fides
Length of output: 522
🏁 Script executed:
rg -n "def get_.*api_session|try:|finally:|\.close\(\)" src/fides/common/session_management.py | head -30Repository: ethyca/fides
Length of output: 242
🏁 Script executed:
# Check if Optional[Session] is imported and available
grep -n "from typing import\|Optional" src/fides/common/session_management.pyRepository: ethyca/fides
Length of output: 388
🏁 Script executed:
# Let's get the full context of both functions to ensure we understand them completely
sed -n '43,57p' src/fides/common/session_management.pyRepository: ethyca/fides
Length of output: 448
🏁 Script executed:
sed -n '160,175p' src/fides/common/session_management.pyRepository: ethyca/fides
Length of output: 443
Guard db.close() when session acquisition fails.
If get_api_session() or get_readonly_api_session() raises an exception before db is assigned, the finally block at lines 56 and 174 will attempt to close an unbound variable, raising UnboundLocalError and masking the original exception.
🐛 Proposed fix
`@contextmanager`
def get_autoclose_db_session() -> Generator[Session, None, None]:
"""
Return a database session as a context manager that automatically closes
when the context exits.
Use this when you need manual control over the session lifecycle
outside of API endpoints.
"""
- try:
- db = get_api_session()
- yield db
- finally:
- db.close()
+ db: Optional[Session] = None
+ try:
+ db = get_api_session()
+ yield db
+ finally:
+ if db is not None:
+ db.close() `@contextmanager`
def get_readonly_autoclose_db_session() -> Generator[Session, None, None]:
"""
Return a database session as a context manager that automatically closes
when the context exits. Falls back to the standard session if read-only
is not configured.
Use this when you need manual control over the session lifecycle
outside of API endpoints.
"""
if not CONFIG.database.sqlalchemy_readonly_database_uri:
with get_autoclose_db_session() as db:
yield db
return
- try:
- db = get_readonly_api_session()
- yield db
- finally:
- db.close()
+ db: Optional[Session] = None
+ try:
+ db = get_readonly_api_session()
+ yield db
+ finally:
+ if db is not None:
+ db.close()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| db = get_api_session() | |
| yield db | |
| finally: | |
| db.close() | |
| db: Optional[Session] = None | |
| try: | |
| db = get_api_session() | |
| yield db | |
| finally: | |
| if db is not None: | |
| db.close() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fides/common/session_management.py` around lines 52 - 56, The finally
block calls db.close() even if get_api_session() or get_readonly_api_session()
raised before db was assigned, causing UnboundLocalError; initialize db = None
before the try in the generator/contextmanager functions (the ones that call
get_api_session() and get_readonly_api_session()) and change the finally to only
call db.close() when db is not None (or check 'if db:'/'if db is not None:') so
the original exception is preserved.
…es.common Move scope_registry and urn_registry up from fides.common.api(.v1) to fides.common, removing the unnecessary api/v1 nesting. Delete the now-empty fides.common.api package. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/ops/api/v1/endpoints/test_saas_config_endpoints.py (1)
481-485:⚠️ Potential issue | 🔴 CriticalInconsistent mock patch path - tests will fail to patch the correct target.
This mock patch still references the old module path
fides.api.api.v1.endpoints.saas_config_endpoints, but line 572 was updated to use the new flattened pathfides.api.v1.endpoints.saas_config_endpoints. The same inconsistency exists at line 510-511.Since the module has moved, these mock patches need to be updated to match, otherwise they won't intercept the actual
ConnectorRegistry.get_connector_templatecalls and the tests will either fail or produce false positives.🐛 Proposed fix for both occurrences
with mock.patch( - "fides.api.api.v1.endpoints.saas_config_endpoints.ConnectorRegistry.get_connector_template" + "fides.api.v1.endpoints.saas_config_endpoints.ConnectorRegistry.get_connector_template" ) as mock_template:Apply the same fix at line 510-511.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ops/api/v1/endpoints/test_saas_config_endpoints.py` around lines 481 - 485, Update the mock.patch targets so they patch the actual moved module path: replace uses of "fides.api.api.v1.endpoints.saas_config_endpoints.ConnectorRegistry.get_connector_template" with "fides.api.v1.endpoints.saas_config_endpoints.ConnectorRegistry.get_connector_template" wherever it appears (e.g., the two occurrences around the ConnectorRegistry.get_connector_template mocks in test_saas_config_endpoints.py) so the MagicMock will intercept the real calls.
🧹 Nitpick comments (4)
src/fides/api/v1/endpoints/system.py (1)
66-66: Avoid importing shared flow from another endpoint module.
instantiate_connectionstill lives undersrc/fides/api/v1/endpoints/saas_config_endpoints.py:347-402, so importing it here keeps two router modules coupled at import time. In a PR focused on breaking cycles, this is a good candidate to extract into a service/helper outsidefides.api.v1.endpointsand have both handlers call that shared layer instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/v1/endpoints/system.py` at line 66, The system endpoint currently imports instantiate_connection from saas_config_endpoints, coupling two router modules; extract the shared logic into a new service/helper module (for example a new module named something like fides.api.v1.services.saas_config_service or fides.api.services.saas_helper) and move the instantiate_connection function there without changing its public signature, then update the import in both src/fides/api/v1/endpoints/saas_config_endpoints.py and src/fides/api/v1/endpoints/system.py to import instantiate_connection from the new service module; ensure tests and any other callers import the new module and run existing linters to confirm there are no remaining cross-endpoint imports.tests/api/v1/endpoints/test_dsr_package_link.py (1)
288-294: Unused variable:rate_limit_responsesis computed but never asserted.The test computes
rate_limit_responsesto capture 429 responses, but the only assertion checks that some 302 responses occur. This means the test doesn't actually verify rate limiting is working—it would pass even if rate limiting were disabled.Consider adding an assertion on
rate_limit_responsesor removing the unused variable if rate limiting behavior cannot be reliably tested in this context.💡 Option: Assert rate limiting if intended
# Check if we got any rate limit responses (429 Too Many Requests) - # If rate limiting is working, we should see some 429s rate_limit_responses = [r for r in responses if r == 429] - # At least some requests should succeed (rate limiting might not be very strict in tests) - # But we should see evidence of rate limiting if it's working assert HTTP_302_FOUND in responses, "Some requests should succeed" + # Optionally assert rate limiting is triggered: + # assert len(rate_limit_responses) > 0, "Rate limiting should trigger some 429 responses"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/api/v1/endpoints/test_dsr_package_link.py` around lines 288 - 294, The test computes rate_limit_responses but never asserts it; either assert that rate limiting occurred (e.g., check that rate_limit_responses is non-empty or that any(r == 429 for r in responses)) or remove the unused rate_limit_responses variable and its computation if rate limiting cannot be reliably tested; update the assertion logic around responses and HTTP_302_FOUND accordingly so the test verifies both success and rate-limit behavior when intended.tests/ops/api/v1/endpoints/test_policy_endpoints.py (1)
16-19: Use neutral URI aliases here.
POLICY_LIST/RULE_LISTbeing renamed to*_CREATE_URIis a bit misleading, since these constants back both the list and patch routes in this file. Names likePOLICY_LIST_URIandRULE_LIST_URIwould make the tests easier to scan.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ops/api/v1/endpoints/test_policy_endpoints.py` around lines 16 - 19, The import aliases POLICY_CREATE_URI and RULE_CREATE_URI are misleading because POLICY_LIST and RULE_LIST back both list and patch routes; change the aliases to POLICY_LIST_URI and RULE_LIST_URI (while keeping POLICY_DETAIL and RULE_DETAIL as-is) and update every usage of POLICY_CREATE_URI and RULE_CREATE_URI in this test file (e.g., in the test functions that call list/patch endpoints) to use the new POLICY_LIST_URI and RULE_LIST_URI names so intent matches the constants POLICY_LIST and RULE_LIST.src/fides/api/v1/endpoints/oauth_endpoints.py (1)
39-41: Extractverify_oauth_connection_configout of the endpoints layer.
oauth_endpoints.pystill depends on another endpoint module for a pure validation helper. That keeps route modules coupled and makes future import cycles easier to reintroduce. Moving this into a shared service/util module would fit the rest of this refactor better.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/fides/api/v1/endpoints/oauth_endpoints.py` around lines 39 - 41, The endpoint module oauth_endpoints.py currently imports the helper verify_oauth_connection_config from another endpoint module (saas_config_endpoints); extract verify_oauth_connection_config into a shared service or util module (e.g., create fides.api.v1.services.oauth or fides.api.v1.util.oauth_validation), move the function implementation there, update oauth_endpoints.py to import verify_oauth_connection_config from the new service/util, update saas_config_endpoints (and any other callers) to import the helper from the new location, and run/update tests to ensure no import cycles were introduced and all references to verify_oauth_connection_config are corrected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/fides/api/v1/endpoints/admin.py`:
- Line 9: The get_db dependency can raise an UnboundLocalError because db is
closed in a finally block even when get_api_session() fails before assigning db;
update src/fides/api/deps.py's get_db to initialize db = None at the top, wrap
the session construction in try/except, and only call db.close() in the finally
if db is not None (and re-raise the original exception on failure) so endpoints
that import get_db (e.g., the admin endpoint referencing get_db) surface the
real DB initialization error instead of an UnboundLocalError.
In `@src/fides/api/v1/endpoints/messaging_endpoints.py`:
- Around line 764-770: The endpoint user_email_invite_status is missing the
standard security dependency; add Security(verify_oauth_client,
scopes=[MESSAGING_READ]) to its dependencies (same pattern used by other
endpoints like MESSAGING_STATUS) by updating the function signature to include
the Security dependency so the route enforces OAuth client verification with the
MESSAGING_READ scope; ensure you use the existing verify_oauth_client and
MESSAGING_READ symbols already used elsewhere in this file.
In `@tests/ops/api/v1/endpoints/test_oauth_endpoints.py`:
- Around line 495-496: The mock.patch targets in test_oauth_endpoints.py are
incorrect: replace the patches that currently target
fides.api.v1.endpoints.saas_config_endpoints.OAuth2AuthorizationCodeAuthenticationStrategy.get_access_token
with a patch pointing to where oauth_endpoints imports the class
(fides.api.v1.endpoints.oauth_endpoints.OAuth2AuthorizationCodeAuthenticationStrategy.get_access_token)
or patch the class definition itself
(fides.api.service.authentication.authentication_strategy_oauth2_authorization_code.OAuth2AuthorizationCodeAuthenticationStrategy.get_access_token)
so the get_access_token mocks actually intercept calls in the oauth_endpoints
code; leave the connection_status patches as-is because connection_status is
imported into oauth_endpoints.
---
Outside diff comments:
In `@tests/ops/api/v1/endpoints/test_saas_config_endpoints.py`:
- Around line 481-485: Update the mock.patch targets so they patch the actual
moved module path: replace uses of
"fides.api.api.v1.endpoints.saas_config_endpoints.ConnectorRegistry.get_connector_template"
with
"fides.api.v1.endpoints.saas_config_endpoints.ConnectorRegistry.get_connector_template"
wherever it appears (e.g., the two occurrences around the
ConnectorRegistry.get_connector_template mocks in test_saas_config_endpoints.py)
so the MagicMock will intercept the real calls.
---
Nitpick comments:
In `@src/fides/api/v1/endpoints/oauth_endpoints.py`:
- Around line 39-41: The endpoint module oauth_endpoints.py currently imports
the helper verify_oauth_connection_config from another endpoint module
(saas_config_endpoints); extract verify_oauth_connection_config into a shared
service or util module (e.g., create fides.api.v1.services.oauth or
fides.api.v1.util.oauth_validation), move the function implementation there,
update oauth_endpoints.py to import verify_oauth_connection_config from the new
service/util, update saas_config_endpoints (and any other callers) to import the
helper from the new location, and run/update tests to ensure no import cycles
were introduced and all references to verify_oauth_connection_config are
corrected.
In `@src/fides/api/v1/endpoints/system.py`:
- Line 66: The system endpoint currently imports instantiate_connection from
saas_config_endpoints, coupling two router modules; extract the shared logic
into a new service/helper module (for example a new module named something like
fides.api.v1.services.saas_config_service or fides.api.services.saas_helper) and
move the instantiate_connection function there without changing its public
signature, then update the import in both
src/fides/api/v1/endpoints/saas_config_endpoints.py and
src/fides/api/v1/endpoints/system.py to import instantiate_connection from the
new service module; ensure tests and any other callers import the new module and
run existing linters to confirm there are no remaining cross-endpoint imports.
In `@tests/api/v1/endpoints/test_dsr_package_link.py`:
- Around line 288-294: The test computes rate_limit_responses but never asserts
it; either assert that rate limiting occurred (e.g., check that
rate_limit_responses is non-empty or that any(r == 429 for r in responses)) or
remove the unused rate_limit_responses variable and its computation if rate
limiting cannot be reliably tested; update the assertion logic around responses
and HTTP_302_FOUND accordingly so the test verifies both success and rate-limit
behavior when intended.
In `@tests/ops/api/v1/endpoints/test_policy_endpoints.py`:
- Around line 16-19: The import aliases POLICY_CREATE_URI and RULE_CREATE_URI
are misleading because POLICY_LIST and RULE_LIST back both list and patch
routes; change the aliases to POLICY_LIST_URI and RULE_LIST_URI (while keeping
POLICY_DETAIL and RULE_DETAIL as-is) and update every usage of POLICY_CREATE_URI
and RULE_CREATE_URI in this test file (e.g., in the test functions that call
list/patch endpoints) to use the new POLICY_LIST_URI and RULE_LIST_URI names so
intent matches the constants POLICY_LIST and RULE_LIST.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc0b572d-96c7-4e19-a374-cb6eee56de7c
📒 Files selected for processing (113)
scripts/quickstart.pysrc/fides/api/common_exceptions.pysrc/fides/api/models/privacy_request/webhook.pysrc/fides/api/oauth/roles.pysrc/fides/api/oauth/system_manager.pysrc/fides/api/oauth/utils.pysrc/fides/api/schemas/user_permission.pysrc/fides/api/service/connectors/fides/fides_client.pysrc/fides/api/service/connectors/query_configs/saas_query_config.pysrc/fides/api/service/privacy_request/request_runner_service.pysrc/fides/api/util/connection_util.pysrc/fides/api/util/endpoint_utils.pysrc/fides/api/v1/endpoints/admin.pysrc/fides/api/v1/endpoints/config_endpoints.pysrc/fides/api/v1/endpoints/connection_endpoints.pysrc/fides/api/v1/endpoints/connection_type_endpoints.pysrc/fides/api/v1/endpoints/connector_template_endpoints.pysrc/fides/api/v1/endpoints/consent_request_endpoints.pysrc/fides/api/v1/endpoints/dataset_config_endpoints.pysrc/fides/api/v1/endpoints/drp_endpoints.pysrc/fides/api/v1/endpoints/dsr_package_link.pysrc/fides/api/v1/endpoints/encryption_endpoints.pysrc/fides/api/v1/endpoints/generate.pysrc/fides/api/v1/endpoints/generic_overrides.pysrc/fides/api/v1/endpoints/identity_verification_endpoints.pysrc/fides/api/v1/endpoints/manual_webhook_endpoints.pysrc/fides/api/v1/endpoints/masking_endpoints.pysrc/fides/api/v1/endpoints/messaging_endpoints.pysrc/fides/api/v1/endpoints/oauth_endpoints.pysrc/fides/api/v1/endpoints/partitioning_endpoints.pysrc/fides/api/v1/endpoints/policy_endpoints.pysrc/fides/api/v1/endpoints/policy_webhook_endpoints.pysrc/fides/api/v1/endpoints/pre_approval_webhook_endpoints.pysrc/fides/api/v1/endpoints/privacy_request_endpoints.pysrc/fides/api/v1/endpoints/privacy_request_redaction_patterns_endpoints.pysrc/fides/api/v1/endpoints/registration_endpoints.pysrc/fides/api/v1/endpoints/router_factory.pysrc/fides/api/v1/endpoints/saas_config_endpoints.pysrc/fides/api/v1/endpoints/storage_endpoints.pysrc/fides/api/v1/endpoints/system.pysrc/fides/api/v1/endpoints/user_endpoints.pysrc/fides/api/v1/endpoints/user_permission_endpoints.pysrc/fides/api/v1/endpoints/validate.pysrc/fides/api/v1/endpoints/view.pysrc/fides/api/v1/endpoints/worker_endpoints.pysrc/fides/cli/utils.pysrc/fides/common/api/__init__.pysrc/fides/common/api/v1/__init__.pysrc/fides/common/scope_registry.pysrc/fides/common/urn_registry.pysrc/fides/config/security_settings.pysrc/fides/service/connection/connection_service.pysrc/fides/system_integration_link/routes.pytests/api/middleware/test_middleware.pytests/api/v1/endpoints/test_dataset_endpoints.pytests/api/v1/endpoints/test_dsr_package_link.pytests/conftest.pytests/ctl/cli/test_cli.pytests/ctl/core/test_api.pytests/lib/test_client_model.pytests/lib/test_oauth_util.pytests/lib/test_system_oauth_util.pytests/ops/api/v1/endpoints/privacy_request/conftest.pytests/ops/api/v1/endpoints/privacy_request/test_privacy_request_bulk_actions.pytests/ops/api/v1/endpoints/privacy_request/test_privacy_request_data_transfer.pytests/ops/api/v1/endpoints/privacy_request/test_privacy_request_diagnostics.pytests/ops/api/v1/endpoints/privacy_request/test_privacy_request_endpoints.pytests/ops/api/v1/endpoints/privacy_request/test_privacy_request_finalization.pytests/ops/api/v1/endpoints/privacy_request/test_privacy_request_location_filtering.pytests/ops/api/v1/endpoints/privacy_request/test_privacy_request_logs.pytests/ops/api/v1/endpoints/privacy_request/test_privacy_request_performance.pytests/ops/api/v1/endpoints/test_config_endpoints.pytests/ops/api/v1/endpoints/test_connection_config_endpoints.pytests/ops/api/v1/endpoints/test_connection_template_endpoints.pytests/ops/api/v1/endpoints/test_connector_template_endpoints.pytests/ops/api/v1/endpoints/test_consent_request_endpoints.pytests/ops/api/v1/endpoints/test_data_category_tagging_instructions.pytests/ops/api/v1/endpoints/test_dataset_config_endpoints.pytests/ops/api/v1/endpoints/test_dataset_test_endpoints.pytests/ops/api/v1/endpoints/test_drp_endpoints.pytests/ops/api/v1/endpoints/test_encryption_endpoints.pytests/ops/api/v1/endpoints/test_generic_overrides.pytests/ops/api/v1/endpoints/test_identity_verification_endpoints.pytests/ops/api/v1/endpoints/test_manual_webhooks.pytests/ops/api/v1/endpoints/test_masking_endpoints.pytests/ops/api/v1/endpoints/test_messaging_endpoints.pytests/ops/api/v1/endpoints/test_oauth_endpoints.pytests/ops/api/v1/endpoints/test_partitioning_endpoints.pytests/ops/api/v1/endpoints/test_policy_endpoints.pytests/ops/api/v1/endpoints/test_policy_webhook_endpoints.pytests/ops/api/v1/endpoints/test_pre_approval_webhook_endpoints.pytests/ops/api/v1/endpoints/test_privacy_request_redaction_patterns_endpoints.pytests/ops/api/v1/endpoints/test_registration_endpoints.pytests/ops/api/v1/endpoints/test_saas_config_endpoints.pytests/ops/api/v1/endpoints/test_storage_endpoints.pytests/ops/api/v1/endpoints/test_system.pytests/ops/api/v1/endpoints/test_system_data_steward.pytests/ops/api/v1/endpoints/test_system_sorting.pytests/ops/api/v1/endpoints/test_taxonomy_overrides.pytests/ops/api/v1/endpoints/test_user_endpoints.pytests/ops/api/v1/endpoints/test_user_permission_endpoints.pytests/ops/api/v1/test_exception_handlers.pytests/ops/api/v1/test_main.pytests/ops/integration_tests/test_connection_configuration_integration.pytests/ops/integration_tests/test_privacy_request_logging.pytests/ops/models/test_client.pytests/ops/schemas/test_user_permission.pytests/ops/service/privacy_request/test_request_runner_service.pytests/ops/service/privacy_request/test_request_service.pytests/ops/util/test_api_router.pytests/ops/util/test_oauth_util.pytests/system_integration_link/test_data_steward_role.pytests/system_integration_link/test_routes.py
✅ Files skipped from review due to trivial changes (6)
- tests/ops/api/v1/endpoints/test_masking_endpoints.py
- src/fides/system_integration_link/routes.py
- tests/ops/service/privacy_request/test_request_service.py
- tests/ops/service/privacy_request/test_request_runner_service.py
- tests/ops/api/v1/endpoints/test_partitioning_endpoints.py
- src/fides/api/v1/endpoints/partitioning_endpoints.py
🚧 Files skipped from review as they are similar to previous changes (12)
- src/fides/api/v1/endpoints/saas_config_endpoints.py
- src/fides/api/v1/endpoints/pre_approval_webhook_endpoints.py
- tests/ops/api/v1/endpoints/test_user_endpoints.py
- src/fides/api/v1/endpoints/connector_template_endpoints.py
- src/fides/api/v1/endpoints/storage_endpoints.py
- tests/ops/api/v1/endpoints/test_messaging_endpoints.py
- src/fides/api/v1/endpoints/config_endpoints.py
- src/fides/api/v1/endpoints/policy_webhook_endpoints.py
- src/fides/api/util/connection_util.py
- src/fides/api/v1/endpoints/dsr_package_link.py
- src/fides/api/v1/endpoints/consent_request_endpoints.py
- tests/ctl/core/test_api.py
| from fides.api.api.deps import get_db | ||
| from fides.api.api.v1.endpoints import API_PREFIX | ||
| from fides.api.db.database import configure_db, migrate_db, reset_db | ||
| from fides.api.deps import get_db |
There was a problem hiding this comment.
Guard get_db() against session-construction failures.
Line 9 now routes this endpoint through src/fides/api/deps.py:get_db, and that helper currently closes db unconditionally in finally. If get_api_session() fails before assignment, requests hitting Line 206 will raise UnboundLocalError and hide the real DB initialization error.
Suggested fix in src/fides/api/deps.py
def get_db() -> Generator:
"""Return our database session"""
+ db = None
try:
db = get_api_session()
yield db
finally:
- db.close()
+ if db is not None:
+ db.close()Also applies to: 206-206
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fides/api/v1/endpoints/admin.py` at line 9, The get_db dependency can
raise an UnboundLocalError because db is closed in a finally block even when
get_api_session() fails before assigning db; update src/fides/api/deps.py's
get_db to initialize db = None at the top, wrap the session construction in
try/except, and only call db.close() in the finally if db is not None (and
re-raise the original exception on failure) so endpoints that import get_db
(e.g., the admin endpoint referencing get_db) surface the real DB initialization
error instead of an UnboundLocalError.
Comment on lines
764
to
+770
| @router.get(MESSAGING_EMAIL_INVITE_STATUS) | ||
| def user_email_invite_status( | ||
| db: Session = Depends(deps.get_db), | ||
| config_proxy: ConfigProxy = Depends(deps.get_config_proxy), | ||
| messaging_service: MessagingService = Depends(get_messaging_service), | ||
| ) -> UserEmailInviteStatus: | ||
| """Returns whether or not all the necessary configurations are in place to be able to invite a user via email.""" | ||
|
|
||
| messaging_status = get_messaging_status( | ||
| db=db, messaging_method=MessagingMethod.EMAIL | ||
| ) | ||
| return UserEmailInviteStatus( | ||
| enabled=( | ||
| messaging_status.config_status == MessagingConfigStatus.configured | ||
| and config_proxy.admin_ui.url is not None | ||
| ) | ||
| ) | ||
| is_enabled = messaging_service.is_email_invite_enabled() | ||
| return UserEmailInviteStatus(enabled=is_enabled) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other status/config endpoints in the codebase are also public or protected
rg -n "MESSAGING_EMAIL_INVITE_STATUS|user_email_invite" --type py -C 3Repository: ethyca/fides
Length of output: 1975
🏁 Script executed:
# Check endpoints in messaging_endpoints.py to see security patterns
rg -n "@router\.(get|post|put|delete)" src/fides/api/v1/endpoints/messaging_endpoints.py -A 5 | head -100Repository: ethyca/fides
Length of output: 3349
🏁 Script executed:
# Continue viewing the file to see the email invite endpoint and check for pattern consistency
rg -n "@router\.(get|post|put|delete)" src/fides/api/v1/endpoints/messaging_endpoints.py -A 5 | tail -100Repository: ethyca/fides
Length of output: 3480
🏁 Script executed:
# Check if there's any documentation or comments explaining why this endpoint is public
cat -n src/fides/api/v1/endpoints/messaging_endpoints.py | sed -n '755,775p'Repository: ethyca/fides
Length of output: 871
Add Security dependency to match the file's established security pattern.
This endpoint is missing the Security(verify_oauth_client, scopes=[MESSAGING_READ]) dependency present on all other endpoints in this file. Even the similar MESSAGING_STATUS endpoint (line 254) requires this security check. Add it to maintain consistent security posture.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/fides/api/v1/endpoints/messaging_endpoints.py` around lines 764 - 770,
The endpoint user_email_invite_status is missing the standard security
dependency; add Security(verify_oauth_client, scopes=[MESSAGING_READ]) to its
dependencies (same pattern used by other endpoints like MESSAGING_STATUS) by
updating the function signature to include the Security dependency so the route
enforces OAuth client verification with the MESSAGING_READ scope; ensure you use
the existing verify_oauth_client and MESSAGING_READ symbols already used
elsewhere in this file.
Comment on lines
495
to
+496
| @mock.patch( | ||
| "fides.api.api.v1.endpoints.saas_config_endpoints.OAuth2AuthorizationCodeAuthenticationStrategy.get_access_token" | ||
| "fides.api.v1.endpoints.saas_config_endpoints.OAuth2AuthorizationCodeAuthenticationStrategy.get_access_token" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the endpoint modules exist at the new flattened paths
echo "=== Checking for saas_config_endpoints module ==="
fd -t f 'saas_config_endpoints.py' --exec echo "Found: {}"
echo ""
echo "=== Checking for oauth_endpoints module ==="
fd -t f 'oauth_endpoints.py' --exec echo "Found: {}"
echo ""
echo "=== Verifying OAuth2AuthorizationCodeAuthenticationStrategy.get_access_token exists ==="
rg -n "def get_access_token" --type py -g '*saas*' -A 2
echo ""
echo "=== Verifying connection_status function exists in oauth_endpoints ==="
rg -n "^def connection_status|^async def connection_status" --type py -g '*oauth_endpoints*'Repository: ethyca/fides
Length of output: 553
🏁 Script executed:
#!/bin/bash
# Check the actual contents of the endpoint modules
echo "=== Checking saas_config_endpoints.py for OAuth2AuthorizationCodeAuthenticationStrategy ==="
rg -n "OAuth2AuthorizationCodeAuthenticationStrategy" --type py
echo ""
echo "=== Checking what's actually imported/used in saas_config_endpoints ==="
head -50 ./src/fides/api/v1/endpoints/saas_config_endpoints.py
echo ""
echo "=== Checking oauth_endpoints.py for connection_status ==="
rg -n "connection_status" --type py | head -20
echo ""
echo "=== Checking the test file to understand mocking context ==="
sed -n '490,530p' ./tests/ops/api/v1/endpoints/test_oauth_endpoints.pyRepository: ethyca/fides
Length of output: 10581
🏁 Script executed:
#!/bin/bash
# Check where get_access_token is called in oauth_endpoints
echo "=== Checking oauth_endpoints around line 276 where auth_strategy is used ==="
sed -n '270,320p' ./src/fides/api/v1/endpoints/oauth_endpoints.py
echo ""
echo "=== Searching for get_access_token calls in oauth_endpoints ==="
rg -n "get_access_token" ./src/fides/api/v1/endpoints/oauth_endpoints.py -A 2 -B 2
echo ""
echo "=== Check if saas_config_endpoints is imported/used in oauth_endpoints ==="
rg -n "saas_config_endpoints" ./src/fides/api/v1/endpoints/oauth_endpoints.pyRepository: ethyca/fides
Length of output: 2913
🏁 Script executed:
#!/bin/bash
# Check what's imported from saas_config_endpoints into oauth_endpoints
echo "=== Check imports from saas_config_endpoints in oauth_endpoints ==="
sed -n '39,50p' ./src/fides/api/v1/endpoints/oauth_endpoints.py
echo ""
echo "=== Check if OAuth2AuthorizationCodeAuthenticationStrategy is used anywhere it's NOT directly imported ==="
rg -n "OAuth2AuthorizationCodeAuthenticationStrategy" ./src/fides/api/v1/endpoints/oauth_endpoints.py
echo ""
echo "=== Verify the actual import statement for OAuth2AuthorizationCodeAuthenticationStrategy ==="
head -50 ./src/fides/api/v1/endpoints/oauth_endpoints.py | grep -A 2 -B 2 "OAuth2AuthorizationCodeAuthenticationStrategy"Repository: ethyca/fides
Length of output: 930
🏁 Script executed:
#!/bin/bash
# Final verification: Show what actually happens with the current mock patch
echo "=== Full context of test_callback_for_valid_state ==="
sed -n '495,515p' ./tests/ops/api/v1/endpoints/test_oauth_endpoints.py
echo ""
echo "=== Verify: Is OAuth2AuthorizationCodeAuthenticationStrategy used as a class reference in saas_config_endpoints? ==="
rg -n "OAuth2AuthorizationCodeAuthenticationStrategy\." ./src/fides/api/v1/endpoints/saas_config_endpoints.py | head -10Repository: ethyca/fides
Length of output: 1088
Incorrect mock patch targets—patches will not intercept the mocked methods.
The mock patches target fides.api.v1.endpoints.saas_config_endpoints.OAuth2AuthorizationCodeAuthenticationStrategy.get_access_token, but the code being tested in oauth_endpoints.py imports OAuth2AuthorizationCodeAuthenticationStrategy directly from fides.api.service.authentication.authentication_strategy_oauth2_authorization_code, not from saas_config_endpoints. This means the patches will not intercept the method calls in the code under test.
For patches to work correctly, they must target where the class is imported in the module containing the code being tested. Update these patches to either:
- Target the import location in oauth_endpoints:
fides.api.v1.endpoints.oauth_endpoints.OAuth2AuthorizationCodeAuthenticationStrategy.get_access_token - Or target the actual definition:
fides.api.service.authentication.authentication_strategy_oauth2_authorization_code.OAuth2AuthorizationCodeAuthenticationStrategy.get_access_token
The connection_status patches at lines 551 and 589 are correct, as connection_status is imported into oauth_endpoints.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/ops/api/v1/endpoints/test_oauth_endpoints.py` around lines 495 - 496,
The mock.patch targets in test_oauth_endpoints.py are incorrect: replace the
patches that currently target
fides.api.v1.endpoints.saas_config_endpoints.OAuth2AuthorizationCodeAuthenticationStrategy.get_access_token
with a patch pointing to where oauth_endpoints imports the class
(fides.api.v1.endpoints.oauth_endpoints.OAuth2AuthorizationCodeAuthenticationStrategy.get_access_token)
or patch the class definition itself
(fides.api.service.authentication.authentication_strategy_oauth2_authorization_code.OAuth2AuthorizationCodeAuthenticationStrategy.get_access_token)
so the get_access_token mocks actually intercept calls in the oauth_endpoints
code; leave the connection_status patches as-is because connection_status is
imported into oauth_endpoints.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/ops/api/v1/endpoints/test_saas_config_endpoints.py (1)
510-516:⚠️ Potential issue | 🔴 CriticalInconsistent mock.patch target path - test will fail.
Line 511 still uses the old path
fides.api.api.v1.endpoints.saas_config_endpoints.ConnectorRegistry.get_connector_templatewith the doubleapi.api, while line 482 was correctly updated tofides.api.v1.endpoints.... This mock won't intercept the actual function call, causingtest_patch_after_delete_accepts_matching_allowed_valuesto fail or behave unexpectedly.🐛 Proposed fix
with mock.patch( - "fides.api.api.v1.endpoints.saas_config_endpoints.ConnectorRegistry.get_connector_template" + "fides.api.v1.endpoints.saas_config_endpoints.ConnectorRegistry.get_connector_template" ) as mock_template:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ops/api/v1/endpoints/test_saas_config_endpoints.py` around lines 510 - 516, The mock.patch target path is inconsistent and won't intercept the call to ConnectorRegistry.get_connector_template; update the mock.patch in the test (the one wrapping ConnectorRegistry.get_connector_template) to use the corrected module path "fides.api.v1.endpoints.saas_config_endpoints.ConnectorRegistry.get_connector_template" so it matches the other patch usage and the actual import location used by the code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/ops/api/v1/endpoints/test_saas_config_endpoints.py`:
- Around line 510-516: The mock.patch target path is inconsistent and won't
intercept the call to ConnectorRegistry.get_connector_template; update the
mock.patch in the test (the one wrapping
ConnectorRegistry.get_connector_template) to use the corrected module path
"fides.api.v1.endpoints.saas_config_endpoints.ConnectorRegistry.get_connector_template"
so it matches the other patch usage and the actual import location used by the
code under test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ce34d31-b48f-4f3c-aae7-bb73024bf4e1
📒 Files selected for processing (1)
tests/ops/api/v1/endpoints/test_saas_config_endpoints.py
mfbrown
pushed a commit
that referenced
this pull request
Mar 12, 2026
…7451) Co-authored-by: Adrian Galvan <galvana@uci.edu> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Eliana Rosselli <67162025+erosselli@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_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_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.mdupdatedSummary by CodeRabbit