-
Notifications
You must be signed in to change notification settings - Fork 5
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
advertiser and data provider support #55
Conversation
…into jon-UID2-2431-identity-map # Conflicts: # src/main/java/com/uid2/client/PublisherUid2Client.java
…of generic error response from Java network library on SDK side (#48)"
Could you please add descriptions on the readme.md for the IdentityMapClient, similar to PublisherClient description? https://github.com/IABTechLab/uid2-client-java#usage-for-publishers And also copy these new description onto uid2docs on this page? |
hashedNormalizedEmails = new ArrayList<>(); | ||
for (String email : emailsOrPhones) { | ||
if (alreadyHashed) { | ||
hashedNormalizedEmails.add(email); |
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 think the answer is yet, but does operator return hash exactly as passed without transformations? E.g. lowercase, etc. What happens if the same hash is specified multiple times on 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.
Yes, the hash is base64 encoded so it's case sensitive and is returned in the same case.
If same hash is specified multiple times, it is deduplicated in the response. I've added a test identityMapDuplicateHashedEmails
if (alreadyHashed) { | ||
hashedNormalizedEmails.add(email); | ||
} else { | ||
String hashedEmail = InputUtil.normalizeAndHashEmail(email); |
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.
What happens if input contains multiple instances of the same email or variants of it that result in the same 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.
thanks, good spot. Fixed and added identityMapDuplicateEmails
test
return new IdentityMapInput(IdentityType.Phone, hashedPhones, true); | ||
} | ||
|
||
private IdentityMapInput(IdentityType identityType, Iterable<String> emailsOrPhones, boolean alreadyHashed) { |
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.
What happens if emailsOrPhones
is empty?
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.
The request still works but getMappedIdentities() and getUnmappedIdentities return empty maps. Added test identityMapEmptyInput
import java.util.HashMap; | ||
|
||
public class IdentityMapResponse { | ||
IdentityMapResponse(String response, IdentityMapInput identityMapInput) { |
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.
nit: Is it worth turning this into a factory function?
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 anticipate implementation differences between UID2 & EUID that wouldn't also require an interface change, and that couldn't be handled server-side.
The place where we did use factories was Sharing encryption and decryption - but even there I think returning whether we're EUID vs UID2 from the server would've been better.
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 just meant changing to public static IdentityMapResponse fromJson(String responseJson, IdentityMapInput identityMapInput)
and making constructor private. Mainly because it is not intuitively clear what the input parameters to constructor are.
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.
Oh, the ctor is package-private. SDK consumers can't create these other than by calling IdentityMapClient.generateIdentityMap or IdentityMapHelper.createIdentityMapResponse
No description provided.