Sailthru Integration: ignore valid 400 for user not existing #5145
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
SteveDMurphy
force-pushed
the
SteveDMurphy-fides-1082-ignore-400-sailthru
branch
from
457e78f
to
754d44a
Compare
|
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 |
| @@ -67,10 +67,12 @@ def marigold_engage_user_read( | |||
| method=HTTPMethod.GET, | |||
| path="/user", | |||
| query_params=signed_payload(secrets, payload), | |||
| ) | |||
| ), | |||
| ignore_errors=[400], | |||
There was a problem hiding this comment.
Because we're using a request_override for the user read, we can't define ignore_errors on the SaaS config, it needs to be defined on the client.send inside the request override. It's possible for a request override function to make multiple client.send calls so if we define ignore_errors at the SaaS config level, we couldn't programmatically determine which client.send to apply that to.
There was a problem hiding this comment.
We could define a variable that makes explicit for future programmers that we are ignoring the 400 error because the user might not exist, and also remove a magic number/list, by defining something like
error400ForUserNotFound = [400]
There was a problem hiding this comment.
Also, Do we have an explanation from their docs on why an UserNotFound is a .400 error instead of a 404 ? Usually NotFound errors are 404
Nevermind, found the reference on the Docs. I'ts odd that they override the usual 404 error.
Maybe we should check for the specific error message "User not found with {KEY}", just so we dont blank accept any 400 error
It did feel odd to see a 400 for this instead of the 404. Happy to see if we can dig in a bit more to ensure other errors aren't also being ignored. Sounds like that might be the case with the custom override we need to account for based on what @galvana was saying 👍🏽
There was a problem hiding this comment.
Its better now. But i still think we should rename the variable, just so we have more clarity on what type of "400" error are we ignoring
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5145 +/- ##
==========================================
- Coverage 86.49% 86.46% -0.03%
==========================================
Files 359 359
Lines 22484 22495 +11
Branches 2979 2981 +2
==========================================
+ Hits 19448 19451 +3
- Misses 2503 2511 +8
Partials 533 533 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I have some doubts after double checking the documentation on Error Codes for Marigold . This implementation allows for all 400 errors to pass, but there might be a few of them, like Missing required parameter or account not enabled for lookup that we wouldn't want to ignore. Maybe we can check directly the error message, and allow/deny success acordding to that?
...fides/api/service/saas_request/override_implementations/marigold_engage_request_overrides.py
Show resolved
Hide resolved
| @@ -67,10 +67,12 @@ def marigold_engage_user_read( | |||
| method=HTTPMethod.GET, | |||
| path="/user", | |||
| query_params=signed_payload(secrets, payload), | |||
| ) | |||
| ), | |||
| ignore_errors=[400], | |||
There was a problem hiding this comment.
We could define a variable that makes explicit for future programmers that we are ignoring the 400 error because the user might not exist, and also remove a magic number/list, by defining something like
error400ForUserNotFound = [400]
| @@ -67,10 +67,12 @@ def marigold_engage_user_read( | |||
| method=HTTPMethod.GET, | |||
| path="/user", | |||
| query_params=signed_payload(secrets, payload), | |||
| ) | |||
| ), | |||
| ignore_errors=[400], | |||
There was a problem hiding this comment.
Also, Do we have an explanation from their docs on why an UserNotFound is a .400 error instead of a 404 ? Usually NotFound errors are 404
Nevermind, found the reference on the Docs. I'ts odd that they override the usual 404 error.
Maybe we should check for the specific error message "User not found with {KEY}", just so we dont blank accept any 400 error
It did feel odd to see a 400 for this instead of the 404. Happy to see if we can dig in a bit more to ensure other errors aren't also being ignored. Sounds like that might be the case with the custom override we need to account for based on what @galvana was saying 👍🏽
...fides/api/service/saas_request/override_implementations/marigold_engage_request_overrides.py
Show resolved
Hide resolved
| @@ -67,10 +67,12 @@ def marigold_engage_user_read( | |||
| method=HTTPMethod.GET, | |||
| path="/user", | |||
| query_params=signed_payload(secrets, payload), | |||
| ) | |||
| ), | |||
| ignore_errors=[400], | |||
There was a problem hiding this comment.
Its better now. But i still think we should rename the variable, just so we have more clarity on what type of "400" error are we ignoring
|
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 FIDES-1082
Description Of Changes
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdmaindowngrade()migration is correct and works