Skip to content

Identity Map V3 #83

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

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Identity Map V3 #83

wants to merge 16 commits into from

Conversation

aulme
Copy link

@aulme aulme commented Jun 3, 2025

No description provided.

@aulme aulme requested a review from jon8787 June 5, 2025 06:17

Response response = new Response(identityMapInput);

response.assertUnmapped("INVALID", "this is not a hashed email");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious, why have the errors changed to CAPS and a single word?

Copy link
Author

@aulme aulme Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the code we return from the APIs. I don't know why we've changed the codes from the previous version. Perhaps @gmsdelmundo has some context one he's back next week. I didn't realize it was not always like that.
I guess this makes it look more like an error code and less like an end-user message.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this means error handling is no longer a drop-in replacement? Especially concerned about optout - if we change this to caps we will break someone.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, but a discussion on the API side, not in SDK - here we just return whatever APIs return.
We'll consider changing this in API.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jon8787 after some discussion, we've decided to keep the APIs as is and change the SDKs to return an enum. Any unknown unmaped statuses will become "UNKNOWN".
This will still be a breaking change, but it will be obviously breaking instead of silently breaking with changing strings.

Copy link
Contributor

@jon8787 jon8787 Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning an enum sounds good, although will there still be a way to get the unknown status?

Not very convinced about changing lowercase "optout" to CAPS. There may be consumers using the API directly who now have to change their optout handling when migrating to v3. What's the benefit?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm planning to expose the rawReason String together with reason. We'll use the Enum in all examples but if people want to examine the String, they can.
I've made parsing to enum work case-insensitive so we can decide on the casing outside of this PR.

return new IdentityMapV3Input().withHashedPhones(hashedPhones);
}

private transient final Map<String, List<String>> diiMappings = new HashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment about the use of transient here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

@SerializedName("phone_hash")
private final List<Identity> hashedPhones = new ArrayList<>();

public IdentityMapV3Input() {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want/need a public ctor?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from memory, might be to help consumers of the SDK to write their own tests

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also for cases where clients have a mix of different DII types, which we now support.
You could do IdentityMapV3Input.fromPhones(...).withEmails(...) if you have those cleanly separated. If you have a mix of records you can do something like:

input = new IdentityMapV3Input()
for dii in diis:
  if dii is email:
    input.addEmail(dii)
  if dii is phone:
    input.addPhone(dii)

}
}

private static IdentityMapV3Input getIdentityMapInput(IdentityMapV3Input identityMapInput) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just returns its input and isn't used anywhere

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting, refactoring gone wrong at some point

return new IdentityMapV3Input().withHashedPhones(hashedPhones);
}

private transient final Map<String, List<String>> diiMappings = new HashMap<>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment about what the keys/values are, or maybe change the name? Maybe something like hashedDiiToRawDii to line up with the param names of addToDiiMappings?

}
}

static public class UnmappedIdentity {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and elsewhere, can we use public static instead of static public?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had it in the original version, will change though, @gmsdelmundo also noticed.

return new IdentityMapV3Response(decryptedResponseString, identityMapInput);
}

Uid2Helper uid2Helper;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be private final

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

diiMappings.computeIfAbsent(hashedDii, k -> new ArrayList<>()).add(rawDii);
}

private String getEncodedDii(String identityType, int i) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "encoded" refer to here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Hashed" would be the correct word, will change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to have some unit tests, so we have some test coverage in the CI/CD pipeline. It also means I don't have to run an operator just to test changes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some unit tests.

}

public HashMap<String, MappedIdentity> getMappedIdentities() {
return mappedIdentities;
Copy link
Contributor

@mcollins-ttd mcollins-ttd Jun 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to return a copy here and in getUnmappedIdentities, or make mappedIdentities/unmappedIdentities immutable after they're populated.

Copy link
Author

@aulme aulme Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added copying. The Mapped/Unmapped Identity is already immutable so not a problem there.

@aulme aulme force-pushed the aul-UID2-5485-identity-map-v3 branch from af5a5a1 to c70243d Compare June 11, 2025 08:05
@aulme aulme force-pushed the aul-UID2-5485-identity-map-v3 branch from c70243d to b8163ce Compare June 11, 2025 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants