Skip to content

Conversation

@JadeCara
Copy link
Contributor

@JadeCara JadeCara commented Jan 27, 2026

Ticket ENG-2195

Description Of Changes

🎯 Create a Policy Evaluation Engine for Fides

This PR creates a new policy evaluation engine to accept a PrivacyRequest object transform it to evaluation data dictionary then compare to policies with conditions. For each policy, check if conditions match and return the matching policy + evaluation result. There should also be a fallback to default if no policies match. There is a check in place when creating a condition to validate there are no identical conditions. So there should be 1 or 0 matches.

Code Changes

  • src/fides/api/task/conditional_dependencies/policy_evaluation.py - new engine
  • src/fides/api/task/conditional_dependencies/util.py added field address extraction function
  • src/fides/api/task/manual/manual_task_conditional_evaluation.py updated to import new function
  • src/fides/api/task/manual/manual_task_utils.py updated to use new function
  • tests/api/task/conditional_dependencies/test_policy_evaluation.py tests!

Steps to Confirm

  1. This is not live yet. All tests should pass.

Pre-Merge Checklist

  • Issue requirements met
  • All CI pipelines succeeded
  • CHANGELOG.md updated
    • Add a db-migration This indicates that a change includes a database migration label to the entry if your change includes a DB migration
    • Add a high-risk This issue suggests changes that have a high-probability of breaking existing code label to the entry if your change includes a high-risk change (i.e. potential for performance impact or unexpected regression) that should be flagged
    • Updates unreleased work already in Changelog, no new entry necessary
  • UX feedback:
    • All UX related changes have been reviewed by a designer
    • No UX review needed
  • Followup issues:
    • Followup issues created
    • No followup issues
  • Database migrations:
    • Ensure that your downrev is up to date with the latest revision on main
    • Ensure that your downgrade() migration is correct and works
      • If a downgrade migration is not possible for this change, please call this out in the PR description!
    • No migrations
  • Documentation:
    • Documentation complete, PR opened in fidesdocs
    • Documentation issue created in fidesdocs
    • If there are any new client scopes created as part of the pull request, remember to update public-facing documentation that references our scope registry
    • No documentation updates required

@vercel
Copy link
Contributor

vercel bot commented Jan 27, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

2 Skipped Deployments
Project Deployment Actions Updated (UTC)
fides-plus-nightly Ignored Ignored Preview Jan 30, 2026 11:44pm
fides-privacy-center Ignored Ignored Jan 30, 2026 11:44pm

Request Review

@JadeCara JadeCara marked this pull request as ready for review January 27, 2026 01:18
@JadeCara JadeCara requested a review from a team as a code owner January 27, 2026 01:18
@JadeCara JadeCara requested review from erosselli and removed request for a team January 27, 2026 01:18
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 27, 2026

Greptile Overview

Greptile Summary

This PR introduces a new policy evaluation engine (PolicyEvaluator) that selects the most appropriate policy for a privacy request based on conditional matching with sophisticated specificity-based prioritization.

Key Changes:

  • Created PolicyEvaluator class in policy_evaluation.py that evaluates privacy requests against policies with conditions
  • Implements a two-tier specificity system: (1) condition count (more conditions = more specific), (2) location hierarchy tier (exact location > country > groups > regulations)
  • Raises PolicyEvaluationError when multiple policies have identical specificity to prevent ambiguous matches
  • Falls back to action-type-specific default policies when no conditions match
  • Refactored extract_field_addresses from manual_task_utils.py to util.py for better code reusability
  • Made location fields available for consent requests (previously unavailable)
  • Added comprehensive test coverage with parameterized tests for all specificity scenarios

Architecture:
The engine queries all PolicyCondition records filtered by action type, evaluates each condition tree against privacy request data, calculates specificity scores, and returns the most specific match or the default policy.

The specificity calculation ensures deterministic policy selection while the ambiguity detection prevents misconfiguration.

Confidence Score: 4/5

  • This PR is safe to merge with minimal risk - introduces well-tested core functionality with proper error handling
  • Score reflects excellent test coverage and clean architecture, but deducted one point due to being a new core engine that hasn't been battle-tested in production and one minor syntax issue
  • All files are well-implemented. The main policy evaluation engine file should be monitored in early production deployments to validate the specificity logic works as expected across real-world scenarios

Important Files Changed

Filename Overview
src/fides/api/task/conditional_dependencies/policy_evaluation.py New policy evaluation engine with sophisticated specificity-based matching logic - well-structured with proper error handling for ambiguous policy matches
tests/api/task/conditional_dependencies/test_policy_evaluation.py Comprehensive test coverage with parameterized tests for policy specificity hierarchy and error cases
src/fides/api/task/conditional_dependencies/schemas.py Added PolicyEvaluationResult and PolicyEvaluationSpecificity schemas for evaluation results

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@JadeCara
Copy link
Contributor Author

@greptile please review

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

@JadeCara
Copy link
Contributor Author

@greptile please review

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No files reviewed, no comments

Edit Code Review Agent Settings | Greptile

Comment on lines 103 to 107
logger.error(
f"Error evaluating policy {policy_condition.policy.key}: "
f"{type(exc).__name__}: {exc}",
exc_info=True,
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this equivalent to using logger.exception ? will this include a stack trace in dev envs? (and what about prod ones?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to use exception

Comment on lines 75 to 78
# Default to access if no action_type provided
return _get_default_policy_result(
db, action_type or ActionType.access
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have little context on the policy conditions, but why is access the default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating to make Action type not optional :)

Comment on lines 88 to 90
root_condition = ConditionTypeAdapter.validate_python(
policy_condition.condition_tree
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm slightly confused as to what this is doing here, I thought now we had the entire condition tree on the root node?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the name to be more clear. This is loading the json version of a condition tree into the Condition schema.

Comment on lines 143 to 145
return PolicyEvaluationResult(
policy=policy, evaluation_result=None, is_default=True
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow why we need the "default policy" if we're just returning None for the result ? can't we just leave policy as None as well ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is evaluating conditions to select a policy - so we need to return a policy. The evaluation result is the set of conditions which matched for logging purposes.

For defaults there was no set of conditions that matched, its just the default (fallback)

db, action_type or ActionType.access
)

evaluator = ConditionEvaluator(db)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think of having a PolicyEvaluator class that contains a evaluate_policy_conditions method , instead of just having it be a function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no issue with that, refactoring to make it a class. :)

assert result.policy.key == "conditional"


class TestEdgeCases:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I don't love "TestEdgeCases" as a test suite name hah , arguably both the tests in this class can be moved into TestDefaultFallback ?

@JadeCara JadeCara requested a review from erosselli January 30, 2026 01:49
@JadeCara
Copy link
Contributor Author

@greptile please review

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Comment on lines 100 to 107
query = (
self.db.query(PolicyCondition)
.join(Policy, PolicyCondition.policy_id == Policy.id)
.options(contains_eager(PolicyCondition.policy))
.join(Rule, Policy.id == Rule.policy_id)
.filter(Rule.action_type == action_type)
)
return query.all()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

query doesn't specify ordering - when multiple policies match, the result is non-deterministic

add explicit ordering (e.g. by created_at or a dedicated priority field) to ensure consistent policy selection

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a check to prevent multiple policies from having the same conditions. There should be only 1 match.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could return query.one() so if at any point this somehow returns multiple ones, we get a failure :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated for better prioritizing- included errors for ambiguous selections.

@pytest.mark.asyncio
@pytest.fixture(scope="function")
def seed_data(session):
def seed_data(db: Session):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this change related?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to use seed data to get all of the premade policies but when I used this fixture I got an error saying there was no session fixture. Pretty sure no other tests were using this or we would have seen a lot more errors!

Copy link
Contributor

@erosselli erosselli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, left two comments

@JadeCara
Copy link
Contributor Author

@greptile please review

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 files reviewed, no comments

Edit Code Review Agent Settings | Greptile

@JadeCara
Copy link
Contributor Author

@greptile please review

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants