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

Handle Keys and Collections with a double underscore #3688

Merged
merged 10 commits into from
Jun 30, 2023

Conversation

SteveDMurphy
Copy link
Contributor

@SteveDMurphy SteveDMurphy commented Jun 27, 2023

Closes #3687

Description Of Changes

Found by a user with naming conventions that use a double underscore, which we weren't handling previously

Code Changes

  • Adds a test for the edge case
  • Create and use a function that extracts the address information and includes the edge case

Steps to Confirm

  • While you can set up a table named something like my__table__with__underscores add some data, and run a DSR against it, running the failing test against main will also show the result where only underscores would be returned

Pre-Merge Checklist

@SteveDMurphy SteveDMurphy force-pushed the SteveDMurphy-3687-key-error-double-underscore branch from 282c5be to 23baca6 Compare June 27, 2023 13:26
@cypress
Copy link

cypress bot commented Jun 27, 2023

Passing run #3002 ↗︎

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

Details:

Merge b57bf1e into f63a843...
Project: fides Commit: 008a093fd6 ℹ️
Status: Passed Duration: 01:11 💡
Started: Jun 30, 2023 9:42 AM Ended: Jun 30, 2023 9:43 AM

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

@codecov
Copy link

codecov bot commented Jun 27, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (f63a843) 87.05% compared to head (b57bf1e) 87.06%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3688   +/-   ##
=======================================
  Coverage   87.05%   87.06%           
=======================================
  Files         310      310           
  Lines       18990    18997    +7     
  Branches     2446     2446           
=======================================
+ Hits        16532    16539    +7     
  Misses       2029     2029           
  Partials      429      429           
Impacted Files Coverage Δ
src/fides/api/task/graph_task.py 93.17% <100.00%> (+0.02%) ⬆️
src/fides/api/task/task_resources.py 83.69% <100.00%> (+0.36%) ⬆️
src/fides/api/util/collection_util.py 96.77% <100.00%> (+0.47%) ⬆️

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

@SteveDMurphy SteveDMurphy requested a review from seanpreston June 27, 2023 14:24
@SteveDMurphy SteveDMurphy self-assigned this Jun 27, 2023
@SteveDMurphy SteveDMurphy marked this pull request as ready for review June 27, 2023 14:28
@SteveDMurphy SteveDMurphy force-pushed the SteveDMurphy-3687-key-error-double-underscore branch from 23baca6 to 91b467c Compare June 27, 2023 23:46
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.

nice fix - generally looks good to me, just had a small suggestion for what i think is a slightly simpler implementation! i'm also not 100% sure on your open question, so it may be good to get @pattisdr or @seanpreston's input on that one.

src/fides/api/task/task_resources.py Outdated Show resolved Hide resolved
src/fides/api/task/graph_task.py Outdated Show resolved Hide resolved
@SteveDMurphy SteveDMurphy requested a review from adamsachs June 30, 2023 00:11
Due to differences in the expected strings for an access or an erasure, it was determined that we may need to remove 2 or 3 items from the leading edge of the dataset. To avoid having the same function twice a parameter is now passed based on the string argument passed. The parameter name was also updated to be more descriptive
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.

i like where you landed on this @SteveDMurphy! the number_of_leading_strings_to_exclude arg makes sense, and the docstring of that function helps to make clear the functionality that otherwise may be a bit confounding for someone coming back to this code. nice work! 👍

@SteveDMurphy SteveDMurphy merged commit f4ca8cc into main Jun 30, 2023
@SteveDMurphy SteveDMurphy deleted the SteveDMurphy-3687-key-error-double-underscore branch June 30, 2023 11:53
@SteveDMurphy SteveDMurphy mentioned this pull request Jul 5, 2023
37 tasks
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.

Filter errors for Collections with a double underscore (__) in the name
3 participants