[ENG-871] Addresses circular import and cleans up some tests #6383

JadeCara · 2025-07-25T15:56:36Z

Description Of Changes

Due to some speed required to get Manual Tasks up and running some comments on PRs did not have time to be addressed. This PR addresses this one: #6261 (comment) regarding circular imports.

Code Changes

Created a new module for ManualTaskAddress to remove circular dependency issues.
Cleaned up some tests.
Reduced some nesting by leaning on unique connection_config/manual_task inherent structure.

Steps to Confirm

There are no intended changes to functionality. The tests should all pass. For additional functional testing:

Run with FidesPlus
Create 1 or more ManualTask integrations with access and or erasure tasks
Create 1 or more Privacy requests - verify you can complete/skip etc and that they all complete as expected.

Pre-Merge Checklist

vercel · 2025-07-25T15:56:41Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Skipped Deployments

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	⬜️ Ignored (Inspect)	Visit Preview		Jul 28, 2025 4:36pm
fides-privacy-center	⬜️ Ignored (Inspect)			Jul 28, 2025 4:36pm

JadeCara · 2025-07-25T16:15:24Z

src/fides/api/task/manual/manual_task_graph_task.py

@@ -1,4 +1,4 @@
-from typing import Any, Dict, List, Optional
+from typing import Any, Optional


My crusade to replace List, Dict with the built ins to reduce imports continues 😂

JadeCara · 2025-07-25T16:16:34Z

src/fides/api/task/manual/manual_task_graph_task.py

@@ -111,67 +110,65 @@ def access_request(self, *inputs: List[Row]) -> List[Row]:
    def _ensure_manual_task_instances(
        self,
        db: Session,
-        manual_tasks: List[ManualTask],
+        manual_task: ManualTask,
        privacy_request: PrivacyRequest,
        allowed_config_type: "ManualTaskConfigurationType",
    ) -> None:
        """Create ManualTaskInstances for configs matching `allowed_config_type` if they don't exist."""



Able to un-nest this function because we are only looking at one ManualTask at a time.

JadeCara · 2025-07-25T16:17:03Z

src/fides/api/task/manual/manual_task_graph_task.py

@@ -182,79 +179,74 @@ def _format_size(size_bytes: int) -> str:
                size /= 1024.0
            return f"{size:.1f} PB"



Able to un-nest this function because we are only looking at one ManualTask at a time.

JadeCara · 2025-07-25T16:17:48Z

src/fides/api/task/manual/manual_task_utils.py


 # TYPE_CHECKING import placed after all runtime imports to avoid lint issues
 if TYPE_CHECKING:  # pragma: no cover
    from fides.api.graph.traversal import TraversalNode  # noqa: F401
    from fides.api.models.policy import Policy  # noqa: F401


-class ManualTaskAddress:


Moved to it separate module to remove circular imports

JadeCara · 2025-07-25T16:24:28Z

src/fides/api/task/manual/manual_task_utils.py

@@ -98,26 +56,20 @@ def get_manual_task_addresses(db: Session) -> List[CollectionAddress]:
    return manual_task_addresses


-def get_manual_tasks_for_connection_config(
+def get_manual_task_for_connection_config(


connection_config and manual_task have a 1:1 unique relationship so we will always be getting just one ManualTask from this query. Updated naming and usage across files.

just a question, do we enforce uniqueness at the DB level, or just at the application level ? If we do it at the app level can we maybe add a warning if we find more than one?

also I think we might want to use something like one_or_none in the query rather than first -- this makes it super clear that we only ever expect a single result there and it will actually error if it finds more

It is explicitly defined on both the model and the migration :)

updated to use one_or_none!

JadeCara · 2025-07-25T16:24:59Z

src/fides/api/task/manual/manual_task_utils.py

@@ -130,24 +82,23 @@ def create_manual_data_traversal_node(
    connection_key = address.dataset

    # Get manual tasks for this connection to determine fields
-    manual_tasks = get_manual_tasks_for_connection_config(db, connection_key)
+    manual_task = get_manual_task_for_connection_config(db, connection_key)

    # Create fields based on ManualTaskConfigFields


Able to un-nest here as well.

JadeCara · 2025-07-25T16:25:10Z

src/fides/api/task/manual/manual_task_utils.py

-    # Create Node and TraversalNode (import locally to avoid cyclic import)
-    from fides.api.graph.traversal import TraversalNode  # local import
-


JadeCara · 2025-07-25T16:25:36Z

src/fides/api/task/manual/manual_task_utils.py


        # Manual task collections act as root nodes - they don't need identity dependencies
        # since they provide manually-entered data rather than consuming identity data.
-        for manual_task in manual_tasks:


Able to un-nest this as well.

JadeCara · 2025-07-25T16:26:26Z

tests/api/task/manual/conftest.py

I moved the various task tests for Manual Tasks into the same dir and consolidated the fixtures to this file.

JadeCara · 2025-07-25T16:28:36Z

tests/api/task/manual/test_manual_task_integration.py

This file was also moved, but due to some removal of outdated tests and other changes it is a big enough change to look like a whole new file.

codecov · 2025-07-25T16:46:27Z

Codecov Report

❌ Patch coverage is 76.28866% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.98%. Comparing base (018ab1d) to head (1501c2f).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...rc/fides/api/task/manual/manual_task_graph_task.py	57.77%	12 Missing and 7 partials ⚠️
src/fides/api/task/manual/manual_task_address.py	91.66%	1 Missing and 1 partial ⚠️
src/fides/api/task/manual/manual_task_utils.py	91.66%	1 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (76.28%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6383      +/-   ##
==========================================
+ Coverage   86.80%   86.98%   +0.18%     
==========================================
  Files         453      454       +1     
  Lines       28907    28909       +2     
  Branches     3215     3211       -4     
==========================================
+ Hits        25092    25146      +54     
+ Misses       3095     3046      -49     
+ Partials      720      717       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…mports

tests/api/task/manual/conftest.py

erosselli

Thanks for all the cleanup, this was such a nice PR to review 😌 Remember to add the changelog entry; also I left a suggestion in a comment.

Haven't tested manually but since this is just a refactor and tests are still passing I think that's probably ok?

erosselli · 2025-07-28T13:39:26Z

src/fides/api/task/manual/manual_task_utils.py

@@ -98,26 +56,20 @@ def get_manual_task_addresses(db: Session) -> List[CollectionAddress]:
    return manual_task_addresses


-def get_manual_tasks_for_connection_config(
+def get_manual_task_for_connection_config(


just a question, do we enforce uniqueness at the DB level, or just at the application level ? If we do it at the app level can we maybe add a warning if we find more than one?

also I think we might want to use something like one_or_none in the query rather than first -- this makes it super clear that we only ever expect a single result there and it will actually error if it finds more

erosselli · 2025-07-28T13:47:47Z

tests/api/task/manual/test_manual_task_instance_filter.py

            ),
        ],
    )
-    def test_artificial_graphs_filter_by_policy_type(
+    @pytest.mark.usefixtures("manual_setup")
+    def test_artificial_graphs_include_all_fields(


did we change what we're testing here? or is the git diff just showing up annoyigly?

This is actually a change. I am not sure why this test was even passing before. The Graph should create initially with all potential nodes. When the traversal runs and the graph executes is where the filtering actually occurs. I updated this test to follow the correct behavior.

JadeCara · 2025-07-28T14:58:41Z

Haven't tested manually but since this is just a refactor and tests are still passing I think that's probably ok?

I have been heavily manually testing. I will run through it one more time before merging :)

…mports

cypress · 2025-07-28T18:07:09Z

fides Run #13167

Run Properties: Passed #13167 • fcb49b2eaa: [ENG-871] Addresses circular import and cleans up some tests (#6383)

Project	`fides`
Branch Review	`main`
Run status	`Passed #13167`
Run duration	`00m 53s`
Commit	`fcb49b2eaa: [ENG-871] Addresses circular import and cleans up some tests (#6383)`
Committer	`JadeWibbels`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`5`
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

addresses circular import and cleans up some tests

28f76a2

JadeCara commented Jul 25, 2025

View reviewed changes

removed nested import statement

84d004a

JadeCara commented Jul 25, 2025

View reviewed changes

JadeCara mentioned this pull request Jul 25, 2025

[Manual Task PR #2] Adding manual tasks to DSR execution #6261

Merged

16 tasks

JadeCara changed the title ~~addresses circular import and cleans up some tests~~ [ENG-871] Addresses circular import and cleans up some tests Jul 25, 2025

JadeCara added 2 commits July 25, 2025 12:50

Merge branch 'main' into ENG-871-manual-task-dsr-follow-up-circular-i…

9b93726

…mports

Merge branch 'main' into ENG-871-manual-task-dsr-follow-up-circular-i…

a8a05ff

…mports

vercel bot deployed to Preview – fides-plus-nightly July 25, 2025 20:20 View deployment

JadeCara marked this pull request as ready for review July 25, 2025 20:22

JadeCara requested a review from erosselli July 25, 2025 20:22

JadeCara commented Jul 25, 2025

View reviewed changes

tests/api/task/manual/conftest.py Outdated Show resolved Hide resolved

Update tests/api/task/manual/conftest.py

df1207f

erosselli approved these changes Jul 28, 2025

View reviewed changes

updated first to one_or_none

ff9956d

JadeCara added 2 commits July 28, 2025 08:58

Merge branch 'main' into ENG-871-manual-task-dsr-follow-up-circular-i…

b359f39

…mports

Merge branch 'main' into ENG-871-manual-task-dsr-follow-up-circular-i…

edcbec9

…mports

vercel bot deployed to Preview – fides-plus-nightly July 28, 2025 16:03 View deployment

updated Changelog

c6387c8

Merge branch 'main' into ENG-871-manual-task-dsr-follow-up-circular-i…

1501c2f

…mports

JadeCara merged commit fcb49b2 into main Jul 28, 2025
40 of 41 checks passed

JadeCara deleted the ENG-871-manual-task-dsr-follow-up-circular-imports branch July 28, 2025 17:50

		@@ -1,4 +1,4 @@
		from typing import Any, Dict, List, Optional
		from typing import Any, Optional

		@@ -182,79 +179,74 @@ def _format_size(size_bytes: int) -> str:
		size /= 1024.0
		return f"{size:.1f} PB"

		# Create Node and TraversalNode (import locally to avoid cyclic import)
		from fides.api.graph.traversal import TraversalNode # local import

[ENG-871] Addresses circular import and cleans up some tests #6383

[ENG-871] Addresses circular import and cleans up some tests #6383

Uh oh!

Conversation

JadeCara commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description Of Changes

Code Changes

Steps to Confirm

Pre-Merge Checklist

Uh oh!

vercel bot commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JadeCara Jul 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

erosselli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JadeCara commented Jul 28, 2025

Uh oh!

Uh oh!

cypress bot commented Jul 28, 2025 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

JadeCara commented Jul 25, 2025 •

edited

Loading

vercel bot commented Jul 25, 2025 •

edited

Loading

JadeCara Jul 28, 2025 •

edited

Loading

codecov bot commented Jul 25, 2025 •

edited

Loading

cypress bot commented Jul 28, 2025 •

edited by atlassian bot

Loading