Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update the get_cached_identity_data function to support non-JSON-encoded value #4855

Merged
merged 5 commits into from
May 3, 2024

Conversation

galvana
Copy link
Contributor

@galvana galvana commented May 2, 2024

Closes PROD-2009

Description Of Changes

This is a fix for a defect that was introduced with the custom identities feature, specifically this line. Moving forward, we want to make sure that any non-JSON-encoded values in the cache can still be read. There is no data lost between the two caching methods since we're just formatting the values differently. This means the change can be targeted and everything can continue to work as expected as long as get_cached_identity_data returns the expected response.

Code Changes

  • Updated get_cached_identity_data and added a test

Steps to Confirm

  • This one is tricky to verify since it involves queuing a privacy request pre-2.34, upgrading to 2.34 then approving the request. I just wrote a test to make sure we could reproduce the issue reported in prod and verify the fix.

Pre-Merge Checklist

  • All CI Pipelines Succeeded
  • Issue Requirements are Met
  • Update CHANGELOG.md

Copy link

vercel bot commented May 2, 2024

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

Name Status Preview Comments Updated (UTC)
fides-plus-nightly ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 3, 2024 3:15pm

@galvana galvana requested a review from adamsachs May 2, 2024 17:37
Copy link

cypress bot commented May 2, 2024

Passing run #7577 ↗︎

0 4 0 0 Flakiness 0

Details:

Merge 79d0cd9 into 8165baf...
Project: fides Commit: 11ba5ead22 ℹ️
Status: Passed Duration: 00:35 💡
Started: May 2, 2024 6:50 PM Ended: May 2, 2024 6:50 PM

Review all test suite changes for PR #4855 ↗︎

Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

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

this makes sense to me @galvana! just one minor nit on potentially adding some more context around the code comment, to be extra sure that this isn't confusing for anyone looking at this moving forward.

also, as i mentioned separately - it was a bit surprising to me that we're keeping the cache in tact across upgrades, but i guess that makes sense. in any case, this is a good, defensive measure 👍

oh, and nice job on adding in the test coverage!🏅

src/fides/api/models/privacy_request.py Outdated Show resolved Hide resolved
Copy link

codecov bot commented May 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.79%. Comparing base (8165baf) to head (79d0cd9).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4855   +/-   ##
=======================================
  Coverage   86.78%   86.79%           
=======================================
  Files         346      346           
  Lines       20913    20917    +4     
  Branches     2733     2733           
=======================================
+ Hits        18150    18154    +4     
  Misses       2288     2288           
  Partials      475      475           

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

@galvana galvana merged commit 43b62d4 into main May 3, 2024
12 checks passed
@galvana galvana deleted the PROD-2009-privacy-request-hangs-in-approved-state branch May 3, 2024 15:13
Kelsey-Ethyca pushed a commit that referenced this pull request May 3, 2024
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.

2 participants