PROD-2473 Update GET /dataset endpoint to support exclude_saas_datasets and only_unlinked_datasets params
#5132
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
|
Test summaryRun details
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5132 +/- ##
=======================================
Coverage 86.54% 86.55%
=======================================
Files 357 357
Lines 22331 22340 +9
Branches 2955 2957 +2
=======================================
+ Hits 19327 19336 +9
Misses 2480 2480
Partials 524 524 ☔ View full report in Codecov by Sentry. |
erosselli
changed the title
erosselli
changed the title
erosselli
force-pushed
the
PROD-2473
branch
2 times, most recently
from
f012119
to
8f70226
Compare
erosselli
changed the title
erosselli
changed the title
GET /dataset endpoint to support exclude_saas_datasets and only_unlinked_datasets params
| @@ -591,6 +591,7 @@ def get_ctl_datasets( | |||
| only_unlinked_datasets: bool = False, | |||
| ) -> List[Dataset]: | |||
| """ | |||
| Deprecated. Use `GET /datasets` instead. | |||
There was a problem hiding this comment.
You can add a deprecated flag to the router info, here's an example:
@router.get(
PRIVACY_REQUESTS,
dependencies=[Security(verify_oauth_client, scopes=[PRIVACY_REQUEST_READ])],
response_model=Page[
Union[
PrivacyRequestVerboseResponse,
PrivacyRequestResponse,
]
],
deprecated=True,
)
def get_request_status(
Then it will be nice and obvious in the Swagger docs
There was a problem hiding this comment.
ah cool! didn't know this 😎
| taxonomy_model=CtlDataset, | ||
| filter_params=filter_params, | ||
| if not page and not size: | ||
| return await list_resource(CtlDataset, db) |
There was a problem hiding this comment.
We would need to apply the exclude_saas_datasets and only_unlinked_datasets filters to the non-paginated response if we wanted to keep backwards compatibility. Did we make a decision to omit this?
There was a problem hiding this comment.
hmm that's a good point. I can definitely add them in -- however I think the idea was to at some point in the future deprecate the non-paginated response entirely since it has performance issues when there's a big number of elements, and the existing non-paginated endpoint with the filters (/filters/dataset) still exists so it can be used if that's still needed.
What do you think? I think ideally if anyone wants to move from the deprecated endpoint to the new one, they should probably add in the pagination support while they're at it?
There was a problem hiding this comment.
added it in 071258d
| @@ -2001,7 +2001,7 @@ def test_list_dataset_no_pagination( | |||
| api_client: TestClient, | |||
| generate_auth_header, | |||
| ctl_dataset, | |||
| saas_ctl_dataset, | |||
| secondary_sendgrid_instance, | |||
|
Test summaryRun details
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Cloud |
Closes PROD-2473
Description Of Changes
Added two query params
exclude_saas_datasetsandonly_unlinked_datasetsto theGET /datasetsendpoint to match those inGET /filters/datasets.Code Changes
list_dataset_paginatedto add the two new filter options,exclude_saas_datasetsandonly_unlinked_datasetsGET /filter/datasetsendpointSteps to Confirm
Pre-Merge Checklist
CHANGELOG.mdmaindowngrade()migration is correct and works