sch-UID2-5486 added identity map v3 client and binary support#58
sch-UID2-5486 added identity map v3 client and binary support#58sophia-chen-ttd merged 10 commits intomainfrom
Conversation
| self._hashed_dii_to_raw_diis[hashed_dii].append(raw_dii) | ||
|
|
||
| def _get_hashed_dii(self, identity_type: str, index: int) -> str: | ||
| if identity_type == "email_hash": |
There was a problem hiding this comment.
Possible improvement: use Literal["email_hash", "phone"hash"] - this documents what the valid values are as well. https://typing.python.org/en/latest/spec/literal.html
Can use it all the way upstream with identity_type too.
Need to check if version of Python we're using supports this though.
There was a problem hiding this comment.
Thanks, added this in
| body = response_json["body"] | ||
| self._populate_identities(body, identity_map_input) | ||
|
|
||
| def _populate_identities(self, api_response: Dict[str, List[Dict]], identity_map_input): |
There was a problem hiding this comment.
Why List[Dict] without the inner Dict having types?
It would also be good to have some validation that what we get is what we expect - in Java SDK the type system does it for us at deserialization.
There was a problem hiding this comment.
Good catch. I've also added some validation in the ApiResponse and ApiIdentity. If the structure isn't as we expect, it will throw an error
uid2_client/request_response_util.py
Outdated
|
|
||
|
|
||
| def make_v2_request(secret_key, now, data=None): | ||
| def create_envelope(secret_key, now, data=None) -> 'Envelope': |
There was a problem hiding this comment.
It's nice that we started adding typing. Could we add them consistently to function inputs and outputs in this file as well?
There was a problem hiding this comment.
where is typing being consumed? are we adding mypy or similar in the pipelines?
There was a problem hiding this comment.
I've just added typing to the function signatures to make it clearer for us to know what the function takes in and returns, especially since we're returning custom class objects now. I haven't added in any type checker but that could be an option, in another mr?
There was a problem hiding this comment.
Have you tested with a type checker locally though? Good to check all the typing is correct before merging
Sounds good to add one in pipelines later - do you mind making a ticket?
There was a problem hiding this comment.
Checked with mypy locally and created a ticket
| from uid2_client.unmapped_identity_reason import UnmappedIdentityReason | ||
|
|
||
|
|
||
| @unittest.skipIf( |
There was a problem hiding this comment.
I don't think this is a good idea, we should break if those are missing instead. Otherwise we may have those tests skipped permanently in the pipeline without anyone noticing.
There was a problem hiding this comment.
these are integration tests, are they running in the pipeline?
There was a problem hiding this comment.
No, it doesn't look like they've been running in the pipeline
There was a problem hiding this comment.
Curious, what prevents them from running in the pipeline? (if not this skipIf ) ?
There was a problem hiding this comment.
I've removed all the @unittest.skipIf(), now the tests require the env variables to set
There was a problem hiding this comment.
Curious, what prevents them from running in the pipeline? (if not this
skipIf) ?
Ah its because we've set the 'vulnerability_scan_only' param to true in our workflow so it will only run the vulnerability scan. I think if we want to turn this off and run the tests in the pipeline, we'll need to keep the unittest.skipIf or set the env variables in the workflow
There was a problem hiding this comment.
Can we try to turn off vulnerability_scan_only ? (and keep skipIf)
There was a problem hiding this comment.
Just tried it but it didn't work because the shared testing pipeline only works for java tests. I'm thinking to keep the vulnerability_scan_only on but then write a custom testing job in this repo
There was a problem hiding this comment.
ah right I found UID2-3648 for that so we can leave it for now (skipIf should remain though)
IdentityMapV3
Tests
Additional refactoring to support binary requests and responses