416 Adds DRP exercise endpoint #496
Conversation
| multidimensional_urlencode==0.0.4 | ||
| pyjwt |
There was a problem hiding this comment.
this is also a dep of fidesctl, so I've kept version out here to let package manager handle it. Maybe there's a better way to avoid package conflicts?
| value, | ||
| ) | ||
| # handle nested dict/objects | ||
| if not isinstance(value, (bytes, str, int, float)): |
There was a problem hiding this comment.
cache doesn't handle vals as bytes/str/int/float. Perhaps we should instead cache the whole req body as a str (json.loads()) instead of separating the top level props as cache keys. But this works as far as caching for debugging purposes only.
| @@ -22,7 +24,7 @@ class DrpPrivacyRequestCreate(BaseSchema): | |||
|
|
|||
| meta: DrpMeta | |||
| regime: Optional[DrpRegime] | |||
| exercise: DrpAction | |||
| exercise: List[DrpAction] | |||
There was a problem hiding this comment.
DRP spec dictates that this is a list. So we'll take in a list even though we don't support > 1 item
|
Verified both endpoints manually in postman:
|
| status_code=HTTP_424_FAILED_DEPENDENCY, | ||
| detail=exc.args[0], | ||
| ) | ||
| except Exception as exc: |
There was a problem hiding this comment.
These except blocks should surround only the minimal code, and we should avoid using broad excepts — unless there's a specific reason?
There was a problem hiding this comment.
this was to be consistent with what we have for POST "/privacy-request". But I can remove it, just lemme know your preference
| "email": drp_identity.email, | ||
| "phone_number": drp_identity.phone_number, | ||
| } | ||
| return PrivacyRequestIdentity(**identity_kwargs) |
There was a problem hiding this comment.
Do we want to discard any extra attributes on DrpIdentity silently?
There was a problem hiding this comment.
I'll surface a warning log message using a validator or a check within this mapper
| if not isinstance(value, (bytes, str, int, float)): | ||
| cache.set_with_autoexpire( | ||
| get_drp_request_body_cache_key(self.id, key), | ||
| repr(value), |
There was a problem hiding this comment.
I know this probably feels hacky but good choice in effort/reward trade-off if this is only for debug purposes as you say.
Looks like value can only ever be a key from this schema, which should be just fine.
If we want to do more with this data though let's make sure we're breaking it down and storing it by key. We could write a util method to handle that automatically for Pydantic schemas for instance.
|
Thanks for the review @seanpreston! This is good to go. I've added a warning message in case we don't support the specific identity props passed in: https://github.com/ethyca/fidesops/pull/496/files#diff-e2fc0ccbfb0913f3f97fd1f425377e29beb158ea9581cc5e171e044b8927c4a4R30 |
* initial infra for drp exercise endpoint * formatting, refactoring out drp status endpoint into drp-specific endpoint * gets tests passing * formatting * removes ignore_missing_imports * adds back import checks to mypy.ini * adds postman collection * cr comments * fix test
* initial infra for drp exercise endpoint * formatting, refactoring out drp status endpoint into drp-specific endpoint * gets tests passing * formatting * removes ignore_missing_imports * adds back import checks to mypy.ini * adds postman collection * cr comments * fix test
Purpose
Adds DRP exercise endpoint
Changes
Checklist
Run Unsafe PR Checkslabel has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #416