UID2-2760 Add IdentityMapClient #39
Conversation
uid2_client/identity_map_client.py
Outdated
|
|
||
|
|
||
| class IdentityMapClient: | ||
| """Client for interacting with UID2 Identity Map services |
There was a problem hiding this comment.
note that this class needs to work for EUID as well. Instead of UID2 in this file we can say UID2 and EUID or UID
There was a problem hiding this comment.
I updated it to UID
| "email_hash": self.hashed_normalized_emails, | ||
| "phone_hash": self.hashed_normalized_phones | ||
| } | ||
| return json.dumps({k: v for k, v in json_object.items() if v is not None}) |
There was a problem hiding this comment.
Initialize the variables hashed_normalized_emails and hashed_normalized_phones as None, and then set their values later. This approach allows us to create objects like {"email_hash":[]} instead of {} when the input is empty. Passing {} to the operator would result in a 400 bad request error.
uid2_client/identity_map_input.py
Outdated
| if identity_type == IdentityType.Email: | ||
| self.hashed_normalized_emails = [] | ||
| for email in emails_or_phones: | ||
| if already_hashed: |
There was a problem hiding this comment.
probably don't need to enter the for loop if already_hashed is true . You can set self.hashed_normalized_emails.append( emails_or_phones)
There was a problem hiding this comment.
That makes sense. I change it to self.hashed_normalized_emails = emails_or_phones
self.hashed_normalized_emails.append( emails_or_phones) will add emails_or_phones as one element in the hashed_normalized_emails list.
uid2_client/identity_map_input.py
Outdated
| @@ -0,0 +1,63 @@ | |||
| import json | |||
|
|
|||
| from uid2_client import IdentityType, normalize_email_string, get_base64_encoded_hash, is_phone_number_normalized, \ | |||
There was a problem hiding this comment.
some imports appear unused
There was a problem hiding this comment.
Good catch, thank you. I may copy paste from other class....
| # which contains raw uid or the reason why it is unmapped | ||
|
|
||
| def _usage(): | ||
| print('Usage: python3 sample_sharing_client.py <base_url> <api_key> <client_secret> <email_1> <email_2> ... <email_n>' |
There was a problem hiding this comment.
sample_identity_map_client.py instead of sample_sharing_client.py
There was a problem hiding this comment.
My bad, good catch!
| from uid2_client import IdentityMapClient, IdentityMapInput | ||
|
|
||
|
|
||
| # this sample client takes email addresses or phone numbers as input and generates an IdentityMapResponse object |
There was a problem hiding this comment.
I know this example demonstrates mapping emails only, so either we should mention only emails here or mention all 4 types including hashes of emails and phones
There was a problem hiding this comment.
Yeah, I updated the description
uid2_client/identity_map_response.py
Outdated
| if not self.is_success(): | ||
| raise ValueError("Got unexpected identity map status: " + self.status) | ||
|
|
||
| body = self._get_body_as_json(response_json) |
There was a problem hiding this comment.
nitpicking here: I don't see much value in refactoring this into a separate method since its not called from anywhere else and its just one line
There was a problem hiding this comment.
Yeah, I agree, I removed that method.
uid2_client/identity_map_response.py
Outdated
|
|
||
| class MappedIdentity: | ||
| def __init__(self, raw_uid, bucket_id): | ||
| self.raw_uid = raw_uid |
There was a problem hiding this comment.
to denote that properties should not be accessed directly it is a convention to start the name with an underscore. See EncryptionDataResponse for example
There was a problem hiding this comment.
Thank you, I updated it.
There was a problem hiding this comment.
probably also change the vars raw_uid and bucket_id to _raw_uid and _bucket_id
There was a problem hiding this comment.
Yeah, that makes sense. I updated it.
tests/test_identity_map_client.py
Outdated
| cls.UID2_API_KEY = os.getenv("UID2_API_KEY") | ||
| cls.UID2_SECRET_KEY = os.getenv("UID2_SECRET_KEY") | ||
|
|
||
| print(cls.UID2_BASE_URL, cls.UID2_API_KEY, cls.UID2_SECRET_KEY) |
There was a problem hiding this comment.
unnecessary print statement ?
There was a problem hiding this comment.
Yeah, removed it.
tests/test_identity_map_client.py
Outdated
| self.assertRaises(HTTPError, client.generate_identity_map, | ||
| identity_map_input) | ||
|
|
||
| def assert_mapped(self, response, ddi): |
There was a problem hiding this comment.
Thank you, I updated it.
| # this sample client takes email addresses or phone numbers as input and generates an IdentityMapResponse object | ||
| # which contains raw uid or the reason why it is unmapped | ||
| # this sample client takes email addresses or phone numbers or hashed versions of either as input and generates | ||
| # an IdentityMapResponse object which contains raw uid or the reason why it is unmapped |
There was a problem hiding this comment.
this comment doesn't match what's in _usage()
There was a problem hiding this comment.
I updated the comment
uid2_client/identity_map_input.py
Outdated
| self._add_hashed_to_raw_dii_mapping(hashed_normalized_email, email) | ||
| self.hashed_normalized_emails.append(hashed_normalized_email) | ||
| else: # phone | ||
| self.hashed_normalized_phones = [] |
There was a problem hiding this comment.
could the above line go inside the else block at line 27?
There was a problem hiding this comment.
Yeah, I moved it.
| self._add_hashed_to_raw_dii_mapping(hashed_normalized_email, email) | ||
| self.hashed_normalized_emails.append(hashed_normalized_email) | ||
| else: # phone | ||
| if already_hashed: |
There was a problem hiding this comment.
nit: if already_hashed block should not depend on whether it's an email or phone and should go outside the phone/email check
There was a problem hiding this comment.
Hmm, it depends on the type. If it's a hashed email list, it gets assigned to hashed_normalized_emails, and if it's a hashed phone list, it's assigned to hashed_normalized_phones.
IdentityMapClientmake dockerand remove broken commands in readme