Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7133 +/- ##
==========================================
- Coverage 87.24% 87.24% -0.01%
==========================================
Files 535 535
Lines 35335 35324 -11
Branches 4114 4117 +3
==========================================
- Hits 30828 30817 -11
+ Misses 3617 3615 -2
- Partials 890 892 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…des into ENG-2185-add-new-jsob-tree-col
| # TypeAdapter for deserializing JSONB to Condition (handles Union discrimination) | ||
| ConditionTypeAdapter: TypeAdapter[Condition] = TypeAdapter(Condition) | ||
|
|
There was a problem hiding this comment.
I'm not sure I follow here -- probably cause I don't know enough pydantic heh -- what union are we discriminating?
There was a problem hiding this comment.
A Condition is a single Pydantic type that can be a group or a leaf. So the Type Adapter works for deserializing groups correctly and leafs correctly :)
| # JSONB storage for full condition tree | ||
| condition_tree = Column(JSONB, nullable=True) | ||
|
|
There was a problem hiding this comment.
I'm curious about the reasoning behind storing the full condition tree in the condition -- is it performance gains , so we avoid calculating the condition tree at runtime?
also I'm realizing I'm not entirely sure I understand what we mean by condition tree 😅 I was expecting this model to have some sort of relationship to itself/other conditions but don't see that?
There was a problem hiding this comment.
Yeah - the original design was that each condition was stored in its own line. To get the full condition tree you needed to know what conditions were grouped together and rebuild the tree.
The actual usage pattern turns out to be more - remove all, create a new tree than update a single line. Its not a huge time spent doing it, but there is more risks for bugs when we have to rebuild the tree when we want to use it than just keeping it built.
The actual trees are validated using the pydantic models. This will remove a lot of moving pieces. The next PR in the series removes the un-used columns (after doing good tests/verification in nightly etc) that all works as expected.
| def get_root_condition(cls, db: Session, **kwargs: Any) -> Optional[Condition]: | ||
| """Get the root condition tree for a parent entity. | ||
| """Get the condition tree for a parent entity. |
There was a problem hiding this comment.
should this be renamed to get_condition_tree or similar ?
| - Multi-type hierarchy means one digest_config can have multiple independent | ||
| condition trees, each with a different digest_condition_type (RECEIVER, CONTENT, PRIORITY) | ||
| - Within each tree, all nodes must have the same digest_condition_type | ||
| - This enables separate condition logic for different aspects of digest processing | ||
| Each digest_config can have up to three independent condition trees, | ||
| one per digest_condition_type (RECEIVER, CONTENT, PRIORITY). | ||
| This enables separate condition logic for different aspects of digest processing. |
There was a problem hiding this comment.
do we want to keep the "within each tree, all nodes must have the same digest_condition_type ?
There was a problem hiding this comment.
Yes! good catch thank you :)
| id = Column(String(255), primary_key=True, default=FidesBase.generate_uuid) | ||
|
|
||
| # Foreign key relationships | ||
| # Foreign key to parent digest config |
There was a problem hiding this comment.
ah I see , the relationship I was asking about is in the specific model and not the base one. Okay so each condition contains the tree starting from itself and down? e.g if my tree is
A
|_B
|_C
|_D
|_E
A will contain the entire tree, B will contain a tree with B as the root and C and D as the children , E will contain a tree with just itself, and so will C and D?
There was a problem hiding this comment.
Right now we are just putting the whole thing in A.
The next PR removes all relationship cols and all orphaned dependents. The whole tree will just be in A.
| __table_args__ = ( | ||
| # TODO update to this in next migration to use UniqueConstraint |
There was a problem hiding this comment.
is this todo intentional?
There was a problem hiding this comment.
Yes :) It gets resolved in the next PR. I can take it out here though since that PR is almost ready for review as well :)
| privacy_request_field_addresses: set[str] = set( | ||
| address | ||
| for address in all_field_addresses | ||
| if address.startswith("privacy_request.") |
There was a problem hiding this comment.
the raw string here makes me a bit nervous, do we have other places that use it? can we move it to a constant ?
There was a problem hiding this comment.
Great point and we do already have it as a constant! Let me update that!
erosselli
left a comment
There was a problem hiding this comment.
I haven't tested manually but the code looks good, thanks for making the changes. I think codecov is reporting some uncovered lines (and the changelog is missing :) still the old-school entry since I haven't merged my changelog PR yet)
|
@greptile please review |
...des/api/alembic/migrations/versions/xx_2025_12_16_1630_85ce2c1c9579_add_jsonb_tree_column.py
Show resolved
Hide resolved
...des/api/alembic/migrations/versions/xx_2025_12_16_1630_85ce2c1c9579_add_jsonb_tree_column.py
Show resolved
Hide resolved
...des/api/alembic/migrations/versions/xx_2025_12_16_1630_85ce2c1c9579_add_jsonb_tree_column.py
Outdated
Show resolved
Hide resolved
|
@greptile please review |
...des/api/alembic/migrations/versions/xx_2025_12_16_1630_85ce2c1c9579_add_jsonb_tree_column.py
Show resolved
Hide resolved
|
@greptile please review |
Co-authored-by: Jade Wibbels <jade@ethyca.com>
Co-authored-by: Jade Wibbels <jade@ethyca.com>
Co-authored-by: Jade Wibbels <jade@ethyca.com>
Ticket ENG-2185
Description Of Changes
🎯 Create manual tasks on the consent graph.
There are going to be a few PRs cleaning up some un-used functionality or adjusting functionality for improved usage patterns. This PR adds a condition tree which will hold the full conditional dependency tree in a jsonb column on the "root condition." It also migrates current conditions to store their full tree on their root node. This will allow us to access the full tree without building it or having to build out all of the conditions each time. It is a clean up based on the first few rounds of customer use and learning actual usage patterns.
This PR:
Future PRs:
Code Changes
.fides/db_dataset.yml- add new columnsrc/fides/api/alembic/migrations/versions/adds new colunsrc/fides/api/db/base.py- adds new column and updates the existing functionality to use the new tree columns. Removes some functionality around rebuilding the tree details.src/fides/api/models/digest/conditional_dependencies.py- updates some doc strings/comments and some functionality around returning the treesrc/fides/api/models/digest/conditional_dependencies.py- updates some docstrings and comments, updatessrc/fides/api/models/manual_task/manual_task.py- Updates some docstrings and removes some un-needed code.Steps to Confirm
There should be NO change in functionality.
If you have some manual tasks/conditions and/or digests hanging around feel free to skip to step 5)
In the DB you can view the conditional dependencies (for the ones in the example it looks like this)
Digests auto create their own conditions for now. It should look something like this in the db
Example the root condition should now look like this:
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works