-
Notifications
You must be signed in to change notification settings - Fork 3
sch-UID2-5486 added identity map v3 client and binary support #58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is typing being consumed? are we adding mypy or similar in the pipelines?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these are integration tests, are they running in the pipeline?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it doesn't look like they've been running in the pipeline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, what prevents them from running in the pipeline? (if not this skipIf ) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed all the @unittest.skipIf(), now the tests require the env variables to set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we try to turn off vulnerability_scan_only ? (and keep skipIf)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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