-
Notifications
You must be signed in to change notification settings - Fork 74
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
Validate a standalone dataset #5549
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
fides Run #11262
Run Properties:
|
Project |
fides
|
Branch Review |
refs/pull/5549/merge
|
Run status |
Passed #11262
|
Run duration | 00m 51s |
Commit |
76272a0ece ℹ️: Merge b25bf2f45a41da1c44a0f03d21fdfb836c7f7c93 into a28ae2f9de42c2e93246c1201be9...
|
Committer | Adrian Galvan |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
View all changes introduced in this branch ↗︎ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice work on this @galvana !
I'm impressed with the detail you put into the BE tests, as well as the little features you added along the way that really add to the "enterprise" user experience.
As we discussed in our pair CR session, in addition to the comments I left, I'd like to see:
- In the UI, add some tooltips for a) note that results are raw results, not based on policy, and b) note that
Run
will run a test access request using the given identity data - Write docs, or defer to separate PR
- New ticket to write FE tests
- Put the ability to test datasets / privacy requests behind a plus flag for now, but let's add a new ticket to move these new endpoints to plus
- In the YAML editor, if there is a generic error in yaml format, let's try to surface a user-friendly error
clients/admin-ui/src/features/test-datasets/TestRunnerSection.tsx
Outdated
Show resolved
Hide resolved
graph_dataset = dataset_config.get_graph() | ||
for collection in graph_dataset.collections: | ||
for field in collection.fields: | ||
for ref, edge_direction in field.references[:]: | ||
if edge_direction == "from" and ref.dataset != dataset_config.fides_key: | ||
field.identity = f"{ref.dataset}_{ref.collection}_{'_'.join(ref.field_path.levels)}" | ||
field.references.remove((ref, "from")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we extract this into a separate function? Since we're mutating the graph_dataset
var, we can just have the function return that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this?
def _replace_references_with_identities(dataset_key: str, graph_dataset: GraphDataset):
"""
Replace external field references with identity values for testing.
Creates a copy of the graph dataset and replaces dataset references with
equivalent identity references that can be seeded directly. This allows
testing a single dataset in isolation without needing to load data from
referenced external datasets.
"""
modified_graph_dataset = deepcopy(graph_dataset)
for collection in modified_graph_dataset.collections:
for field in collection.fields:
for ref, edge_direction in field.references[:]:
if edge_direction == "from" and ref.dataset != dataset_key:
field.identity = f"{ref.dataset}_{ref.collection}_{'_'.join(ref.field_path.levels)}"
field.references.remove((ref, "from"))
return modified_graph_dataset
graph_dataset = dataset_config.get_graph() | ||
for collection in graph_dataset.collections: | ||
for field in collection.fields: | ||
for ref, edge_direction in field.references[:]: | ||
if edge_direction == "from" and ref.dataset != dataset_config.fides_key: | ||
field.identity = f"{ref.dataset}_{ref.collection}_{'_'.join(ref.field_path.levels)}" | ||
field.references.remove((ref, "from")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like this?
def _replace_references_with_identities(dataset_key: str, graph_dataset: GraphDataset):
"""
Replace external field references with identity values for testing.
Creates a copy of the graph dataset and replaces dataset references with
equivalent identity references that can be seeded directly. This allows
testing a single dataset in isolation without needing to load data from
referenced external datasets.
"""
modified_graph_dataset = deepcopy(graph_dataset)
for collection in modified_graph_dataset.collections:
for field in collection.fields:
for ref, edge_direction in field.references[:]:
if edge_direction == "from" and ref.dataset != dataset_key:
field.identity = f"{ref.dataset}_{ref.collection}_{'_'.join(ref.field_path.levels)}"
field.references.remove((ref, "from"))
return modified_graph_dataset
clients/admin-ui/src/features/test-datasets/TestRunnerSection.tsx
Outdated
Show resolved
Hide resolved
} | ||
} catch (error) { | ||
toast(errorToastParams(getErrorMessage(error as FetchBaseQueryError))); | ||
datasetValues = yaml.load(editorContent) as Dataset; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put this in a try/catch so that we can catch any parsing exceptions and show a toast
const updatedDatasetConfig: DatasetConfigSchema = { | ||
fides_key: currentDataset.fides_key, | ||
ctl_dataset: { | ||
...currentDataset.ctl_dataset, | ||
...datasetValues, | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was causing some weird consolidation issues, it was better to just pass the datasetValues
directly to the updateDataset
@@ -169,7 +184,7 @@ const EditorSection = ({ connectionKey }: EditorSectionProps) => { | |||
<Button | |||
htmlType="submit" | |||
size="small" | |||
data-testid="save-btn" | |||
data-testid="refresh-btn" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding test IDs for later
Run | ||
</Button> | ||
<HStack> | ||
<QuestionTooltip label="Run a test access request using the provided test input data" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a tooltip for an extra bit of context
@@ -153,3 +151,25 @@ def run_test_access_request( | |||
privacy_request_proceed=False, | |||
) | |||
return privacy_request | |||
|
|||
|
|||
def _replace_references_with_identities(dataset_key: str, graph_dataset: GraphDataset): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Broke this out to it's own function, and made it non-destructive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this a lot better, thanks!
@@ -2632,7 +2632,7 @@ def get_access_results_urls( | |||
status_code=HTTP_200_OK, | |||
response_model=FilteredPrivacyRequestResults, | |||
) | |||
def get_filtered_results( | |||
def get_test_privacy_request_results( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
@@ -153,3 +151,25 @@ def run_test_access_request( | |||
privacy_request_proceed=False, | |||
) | |||
return privacy_request | |||
|
|||
|
|||
def _replace_references_with_identities(dataset_key: str, graph_dataset: GraphDataset): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this a lot better, thanks!
fides Run #11263
Run Properties:
|
Project |
fides
|
Branch Review |
main
|
Run status |
Passed #11263
|
Run duration | 00m 46s |
Commit |
966a0e3279: Validate a standalone dataset (#5549)
|
Committer | Adrian Galvan |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
0
|
Pending |
0
|
Skipped |
0
|
Passing |
4
|
View all changes introduced in this branch ↗︎ |
Closes LA-157
Description Of Changes
Adds three new API endpoints to assist with the testing of individual datasets
GET /connection/{connection_key}/dataset/{datset_key}/inputs
GET /connection/{connection_key}/dataset/{datset_key}/reachability
POST /connection/{connection_key}/dataset/{datset_key}/test
Dataset test
and are omitted from the Request manager pageGET /privacy_request/{privacy_request_id}/filtered-results
Dataset test
)Also includes a test dataset page that uses the new dataset test endpoints
Code Changes
DatasetEditorSection.tsx
andTestRunnerSection.tsx
components using thedataset-test.slice.ts
Steps to Confirm
nox -s demo -- dev
so we have access to the "Cookie House PostgreSQL Database" systemTest datasets
buttonTest results
sectionPre-Merge Checklist
CHANGELOG.md
updatedmain
downgrade()
migration is correct and works