avoid circular deps caused within `api.api.v1` #3692

adamsachs · 2023-06-27T22:32:13Z

‼️
note that we will need to update fidesplus accordingly if we move urn_registry.py as is proposed here currently!
‼️

Description Of Changes

The root cause here is that the __init__.py of our api.api.v1 module brings in a lot of dependencies via ADMIN_ROUTER. this means we need to be judicious when to depend on code within api.api.v1 so as to keep it as self-contained as possible. in this case, the immediate cause of the problem is that postgres_setup.py depends on our connectors module, and our connectors module was bringing in api.api.v1 via some utilities (namely consent_util which brought in endpoint.utils.)

it seems like an unambiguously good update to move our api.api.v1.endpoint.utils up a few levels into api.util.endpoint_utils, alongside other API related utilities that are not only used by code within api.api.v1, per the general principle above. i've made that change in 5bee963, and that does help us resolve the initial error here.

but we quickly hit another error (stack trace below), this time because our fides_connector (specifically the fides_client it is highly dependent on), which is also initialized by the connectors module, relies directly on the urn_registry that's within api.api.v1. the solution to this feels less straightforward to me -- i see 3 potential paths forward but none seem like an obvious best choice:

we could move urn_registry out of api.api.v1, ~~but that doesn't feel right, because it's pretty strictly related/coupled to the code in that module~~ -- @ThomasLaPiana also suggested this, and given some more thought, this feels like it might be the right path forward. my proposed change here is to move it to a common.api.v1 module/directory, so that it's clear it refers to our api.v1 endpoints/code.
we could update the __init__.py of api.api.v1 to not bring in so many dependencies via the ADMIN_ROUTER, but this feels like a tall order and larger scope than what we're doing here. i'm not even sure it's desirable, it may lead to other problems.
we could move fides_connector (and fides_client) out of the connectors module, or at least not have it initialized as part of the module's __init__.py, so that it's not initialized with postgres_setup.py. this feels simpler and better, it's not crazy to have the fides_connector special-cased, since it's a pretty special connector in how self-referential it is. but where's the better spot for it so that we're sure it gets properly initialized on normal app runtime/startup, but it's not causing dep issues?
we could update fides_client to not rely on the urn_registry and instead just use string literals for its URL/URN references. this would be simple, but it feels like it makes that code materially worse, and it just doesn't feel like the right solution here - constants are good :)

new error with the update contained in 5bee963:

Traceback (most recent call last):
  File "/fides/tests/ops/integration_tests/setup_scripts/postgres_setup.py", line 15, in <module>
    from fides.api.service.connectors.sql_connector import PostgreSQLConnector
  File "/fides/src/fides/api/service/connectors/__init__.py", line 23, in <module>
    from fides.api.service.connectors.fides_connector import (
  File "/fides/src/fides/api/service/connectors/fides_connector.py", line 19, in <module>
    from fides.api.service.connectors.fides.fides_client import FidesClient
  File "/fides/src/fides/api/service/connectors/fides/fides_client.py", line 9, in <module>
    from fides.api.api.v1 import urn_registry as urls
  File "/fides/src/fides/api/api/v1/__init__.py", line 8, in <module>
    from .endpoints.admin import ADMIN_ROUTER
  File "/fides/src/fides/api/api/v1/endpoints/admin.py", line 7, in <module>
    from fides.api.db.database import configure_db, reset_db
  File "/fides/src/fides/api/db/database.py", line 17, in <module>
    from fides.api.db.seed import load_default_resources, load_samples
  File "/fides/src/fides/api/db/seed.py", line 12, in <module>
    from fides.api.api.v1.endpoints.saas_config_endpoints import (
  File "/fides/src/fides/api/api/v1/endpoints/saas_config_endpoints.py", line 58, in <module>
    from fides.api.util.connection_util import validate_secrets
  File "/fides/src/fides/api/util/connection_util.py", line 40, in <module>
    from fides.api.service.privacy_request.request_runner_service import (
  File "/fides/src/fides/api/service/privacy_request/request_runner_service.py", line 25, in <module>
    from fides.api.graph.analytics_events import (
  File "/fides/src/fides/api/graph/analytics_events.py", line 16, in <module>
    from fides.api.task.task_resources import TaskResources
  File "/fides/src/fides/api/task/task_resources.py", line 17, in <module>
    from fides.api.service.connectors import (
ImportError: cannot import name 'BigQueryConnector' from partially initialized module 'fides.api.service.connectors' (most likely due to a circular import) (/fides/src/fides/api/service/connectors/__init__.py)

Code Changes

moved api.api.v1.endpoint.utils.py to api.util.endpoint_utils.py and updated references
figure out something for above problem with fides_connector.py / fides_client.py when running postgres_setup.py --> proposed update here is to move urn_registry.py to common.api.v1 rather than api.api.v1

Steps to Confirm

nox -s quickstart and nox -s dev -- shell postgres should both work fine
general smoke testing of application with fides_env(test) (just given the number of files touched) - privacy request execution and system management still worked fine.

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation:
- documentation complete, PR opened in fidesdocs
- documentation issue created in fidesdocs
Issue Requirements are Met
Relevant Follow-Up Issues Created
Update CHANGELOG.md
For API changes, the Postman collection has been updated

adamsachs · 2023-06-27T22:33:17Z

@ThomasLaPiana @pattisdr @galvana i've made some progress but my brain's a bit stuck here as i wrap up my day - wrote up where i am and the problem as i see it.

maybe there's a totally different approach here that's either simpler or preferred - i went down this path and didn't really look back!

cypress · 2023-06-27T22:49:40Z

Passing run #2993 ↗︎

0	4	0	0	0
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.

Details:

Merge `8e7406b` into `cd579f7`...
Project: fides	Commit: `6251ac2dde ℹ️`
Status: Passed	Duration: 00:49 💡
Started: Jun 29, 2023 9:04 PM	Ended: Jun 29, 2023 9:05 PM

_{This comment has been generated by cypress-bot as a result of this
project's GitHub integration settings.}

codecov · 2023-06-27T23:14:53Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (cd579f7) 87.05% compared to head (8e7406b) 87.05%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3692   +/-   ##
=======================================
  Coverage   87.05%   87.05%           
=======================================
  Files         310      310           
  Lines       18990    18990           
  Branches     2446     2446           
=======================================
  Hits        16532    16532           
  Misses       2029     2029           
  Partials      429      429

Impacted Files	Coverage Δ
src/fides/api/util/endpoint_utils.py	`81.96% <ø> (ø)`
src/fides/common/api/v1/urn_registry.py	`100.00% <ø> (ø)`
src/fides/api/api/v1/endpoints/config_endpoints.py	`100.00% <100.00%> (ø)`
...fides/api/api/v1/endpoints/connection_endpoints.py	`93.16% <100.00%> (ø)`
.../api/api/v1/endpoints/connection_type_endpoints.py	`100.00% <100.00%> (ø)`
.../api/api/v1/endpoints/consent_request_endpoints.py	`85.12% <100.00%> (ø)`
...rc/fides/api/api/v1/endpoints/dataset_endpoints.py	`96.29% <100.00%> (ø)`
src/fides/api/api/v1/endpoints/drp_endpoints.py	`92.10% <100.00%> (ø)`
...fides/api/api/v1/endpoints/encryption_endpoints.py	`100.00% <100.00%> (ø)`
...pi/v1/endpoints/identity_verification_endpoints.py	`100.00% <100.00%> (ø)`
... and 30 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

adamsachs · 2023-06-28T12:25:09Z

@ThomasLaPiana @pattisdr @galvana i've made some progress but my brain's a bit stuck here as i wrap up my day - wrote up where i am and the problem as i see it.

maybe there's a totally different approach here that's either simpler or preferred - i went down this path and didn't really look back!

proposed update is to move urn_registry.py to common.api.v1 rather than api.api.v1

pattisdr

Local testing worked well, thanks for digging into these circular dependency issues - they can be so time-consuming 😓

src/fides/common/api/v1/urn_registry.py

src/fides/api/util/utils.py

…rcular deps

pattisdr

🏆

ThomasLaPiana · 2023-06-29T21:19:22Z

I'm just saying this out loud...the majority of these issues were caused specifically by the admin router due to it's need to directly modify the database. While we probably could have avoided most of these changes by directly moving that endpoint into main.py or something (like 4 PRs ago, haha), I still feel these refactors meaningfully contribute to the code being more navigable 🙂

Thanks for the changes here @adamsachs !

adamsachs · 2023-06-30T11:16:01Z

I'm just saying this out loud...the majority of these issues were caused specifically by the admin router due to it's need to directly modify the database. While we probably could have avoided most of these changes by directly moving that endpoint into main.py or something (like 4 PRs ago, haha), I still feel these refactors meaningfully contribute to the code being more navigable 🙂

Thanks for the changes here @adamsachs !

ha - i also made that observation @ThomasLaPiana , thank you for calling it out here! i felt a bit hesitant to move it and "special-case" the endpoint for lack of consistency, but i think you're right to make a case for it - it does feel like a good change to simplify the dependency tree longer-term!

adamsachs self-assigned this Jun 28, 2023

adamsachs marked this pull request as ready for review June 28, 2023 13:15

adamsachs requested review from ThomasLaPiana, pattisdr and galvana June 28, 2023 13:15

pattisdr reviewed Jun 28, 2023

View reviewed changes

src/fides/common/api/v1/urn_registry.py Outdated Show resolved Hide resolved

src/fides/api/util/utils.py Outdated Show resolved Hide resolved

adamsachs added 5 commits June 29, 2023 16:47

move api.api.v1.endpoint.utils to api.util.endpoint_utils to avoid ci…

9d69e87

…rcular deps

move urn_registry to common.api.v1 for dependency management

0c5db8d

fix up mypy errors

6aae94a

update changelog

cf16945

cleanup based on code review

8e7406b

adamsachs force-pushed the asachs/3691-circular-dep branch from 53b3611 to 8e7406b Compare June 29, 2023 20:47

pattisdr approved these changes Jun 29, 2023

View reviewed changes

adamsachs merged commit 1594e81 into main Jun 30, 2023

adamsachs deleted the asachs/3691-circular-dep branch June 30, 2023 11:16

adamsachs mentioned this pull request Jul 6, 2023

move db-dependent routers up a few levels to reduce circular dependencies #3741

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid circular deps caused within `api.api.v1` #3692

avoid circular deps caused within `api.api.v1` #3692

adamsachs commented Jun 27, 2023 •

edited

Loading

adamsachs commented Jun 27, 2023

cypress bot commented Jun 27, 2023 •

edited

Loading

codecov bot commented Jun 27, 2023 •

edited

Loading

adamsachs commented Jun 28, 2023

pattisdr left a comment

pattisdr left a comment

ThomasLaPiana commented Jun 29, 2023 •

edited

Loading

adamsachs commented Jun 30, 2023 •

edited

Loading

avoid circular deps caused within api.api.v1 #3692

avoid circular deps caused within api.api.v1 #3692

Conversation

adamsachs commented Jun 27, 2023 • edited Loading

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

adamsachs commented Jun 27, 2023

cypress bot commented Jun 27, 2023 • edited Loading

Passing run #2993 ↗︎

codecov bot commented Jun 27, 2023 • edited Loading

Codecov Report

adamsachs commented Jun 28, 2023

pattisdr left a comment

Choose a reason for hiding this comment

pattisdr left a comment

Choose a reason for hiding this comment

ThomasLaPiana commented Jun 29, 2023 • edited Loading

adamsachs commented Jun 30, 2023 • edited Loading

avoid circular deps caused within `api.api.v1` #3692

avoid circular deps caused within `api.api.v1` #3692

adamsachs commented Jun 27, 2023 •

edited

Loading

cypress bot commented Jun 27, 2023 •

edited

Loading

codecov bot commented Jun 27, 2023 •

edited

Loading

ThomasLaPiana commented Jun 29, 2023 •

edited

Loading

adamsachs commented Jun 30, 2023 •

edited

Loading