Rename class/method/variable names to improve token generation code readability by sunnywu · Pull Request #1080 · IABTechLab/uid2-operator

sunnywu · 2024-10-14T01:37:00Z

Refer to ticket description on https://atlassian.thetradedesk.com/jira/browse/UID2-4159

…f variables which don't just include tokens

…ts for clarity

…LevelHashIdentity and RawUidIdentity as UserIdentity is a very vague term and one needs to read function calls to really understand whether the UserIdentity class has hashed DII, first level hash or raw Uid.

2. Rename RefreshToken class to RefreshTokenInput 3. Rename createAdvertisingToken/createRefreshToken methods to createAdvertisingTokenInput/createRefreshTokenInput

…ii/FirstLevelHash/RawUidIdentity sub-classes

…ponse

…ertisingId to generateRawUid

2. Renamed AdvertisingId to RawUid in most operator codes (except optout store related)

…onse

mcollins-ttd · 2024-10-15T02:16:20Z

+    public IdentityScope identityScope;
+    public IdentityType identityType;
+    public int privacyBits;
+    public Instant establishedAt;
+    public Instant refreshedAt;


Can we make these final and set them in a protected ctor.

mcollins-ttd · 2024-10-15T02:18:18Z

+    public IdentityScope GetIdentityScope() { return identityScope; }
+    public IdentityType GetIdentityType() { return identityType; }
+    public Instant GetEstablishedAt() { return establishedAt; };
+    public Instant GetIRefreshedAt() { return refreshedAt; }


Should start with get

Why no getter for privacyBits?

Do we need getters if the fields are public? Or should we keep the getters and make the fields private?

Typo in GetIRefreshedAt

mcollins-ttd · 2024-10-15T02:45:57Z

                this.identityV3Enabled
-                    ? TokenUtils.getAdvertisingIdV3(firstLevelHashIdentity.identityScope, firstLevelHashIdentity.identityType, firstLevelHashIdentity.id, rotatingSalt.getSalt())
-                    : TokenUtils.getAdvertisingIdV2(firstLevelHashIdentity.id, rotatingSalt.getSalt()),
+                    ? TokenUtils.getRawUidV3(firstLevelHashIdentity.identityScope,
+                        firstLevelHashIdentity.identityType, firstLevelHashIdentity.firstLevelHash, rotatingSalt.getSalt())
+                    : TokenUtils.getRawUidV2(firstLevelHashIdentity.firstLevelHash, rotatingSalt.getSalt()),


Can we introduce a variable here? This is hard to read.

gmsdelmundo · 2024-10-15T03:11:52Z

    public static final byte[] ValidateIdentityForEmailHash = EncodingUtils.getSha256Bytes(IdentityConst.ValidateIdentityForEmail);
    public static final byte[] ValidateIdentityForPhoneHash = EncodingUtils.getSha256Bytes(IdentityConst.ValidateIdentityForPhone);
+
+    // DIIs to use when you want to generate a optout response in token generation or identity map


Maybe we can add some similar comments for the above/below groups of variables too?

gmsdelmundo · 2024-10-15T03:14:14Z

+
+    public IdentityScope GetIdentityScope() { return identityScope; }
+    public IdentityType GetIdentityType() { return identityType; }
+    public Instant GetEstablishedAt() { return establishedAt; };


Extra ; at the end

removed the get methods for now

gmsdelmundo · 2024-10-15T03:14:57Z

+    public IdentityScope GetIdentityScope() { return identityScope; }
+    public IdentityType GetIdentityType() { return identityType; }
+    public Instant GetEstablishedAt() { return establishedAt; };
+    public Instant GetIRefreshedAt() { return refreshedAt; }


Extra I after Get
i.e. this should be GetRefreshedAfterAt()

gmsdelmundo · 2024-10-15T03:21:02Z

+
+import java.time.Instant;
+
+// Contains a hash computed from a raw email/phone number DII input or the hash is provided by the UID Participant


I think better to split this into two lines as such:

// Contains a hashed DII // The hash can either be computed from a raw email/phone number DII input or provided by the UID Participant directly

gmsdelmundo · 2024-10-15T03:27:40Z

+import java.time.Instant;
+
+//base class for all other HshedDii/FirstLevelHash/RawUIDIdentity class and define the basic common fields
+public class UserIdentity {


I don't think we're instantiating UserIdentity anymore, so I suggest making this an abstract class, i.e. public abstract class UserIdentity

As for the matches() method, you can also move matches() here as an abstract method and make other classes implement it, i.e. public abstract boolean matches(UserIdentity that);, or use generics on the class type so you can do public abstract boolean matches (T that); if you want to force the input parameter type to be the same as the derived class, but your call on this

i have made UserIdentity abstract - regarding matches , let me see if i can refactor the codes of the various subclasses first

gmsdelmundo · 2024-10-15T03:53:17Z

+
+// Contains the computed raw UID and its bucket ID from identity/map request
+public class RawUidResult {
+    public static RawUidResult OptoutIdentity = new RawUidResult(new byte[33], "");


For public static, the naming convention is capitalized snake case, i.e. public static RawUidResult OPT_OUT_IDENTITY

On a separate note, not a really big deal because I think both make sense, but I noticed we sometimes refer to "optout" as "OptOut" and sometimes as "Optout" - do we want this standardized too?

i'll keep the convention for now due to reason given above.

I'll note down the Optout/OptOut point - seems they are split 50/50 ish. Not too important in context of this ticket work tho

gmsdelmundo · 2024-10-15T03:54:08Z

-    public static RefreshResponse Expired = new RefreshResponse(Status.Expired, IdentityTokens.LogoutToken);
-    public static RefreshResponse Deprecated = new RefreshResponse(Status.Deprecated, IdentityTokens.LogoutToken);
-    public static RefreshResponse NoActiveKey = new RefreshResponse(Status.NoActiveKey, IdentityTokens.LogoutToken);
+    public static RefreshResponse Invalid = new RefreshResponse(Status.Invalid, IdentityResponse.OptOutIdentityResponse);


Similar comment to the other public static comments, but it should be capitalized snake case here too

gmsdelmundo · 2024-10-15T03:58:47Z

        this.webClient.get(remoteApiPort, remoteApiHost, remoteApiPath).
-            addQueryParam("identity_hash", EncodingUtils.toBase64String(firstLevelHashIdentity.id))
+            addQueryParam("identity_hash", EncodingUtils.toBase64String(firstLevelHashIdentity.firstLevelHash))
            .addQueryParam("advertising_id", EncodingUtils.toBase64String(advertisingId))


Do we want to add a comment here that advertising id is equivalent to raw UID?

gmsdelmundo · 2024-10-15T04:04:09Z

            final int length = b3.getInt(4);

-            final byte[] advertisingId = EncodingUtils.fromBase64(b3.slice(8, 8 + length).getBytes());
+            final byte[] getRawUid = EncodingUtils.fromBase64(b3.slice(8, 8 + length).getBytes());


I think just rawUid is fine here

fixed also renamed ``idtorawUid``` in decodeAdvertisingTokenV3orV4 method below

gmsdelmundo · 2024-10-15T04:07:37Z

+    private byte[] encryptIdentityV2(SourcePublisher sourcePublisher, FirstLevelHashIdentity firstLevelHashIdentity, KeysetKey key) {
+        return encryptIdentityV2(sourcePublisher, firstLevelHashIdentity.firstLevelHash, firstLevelHashIdentity.privacyBits,
+                firstLevelHashIdentity.establishedAt, key);
+


Extra newline

2. Renamed RawUidResult to RawUidResponse 3. rename some id variable to rawUid to make it clearer 4. more comments

…xed the UserIdentity/FirstLevelHashIdentity class uses to HashedDiiIdentity instead in IdentityMapBenchmark and TokenEndecBenchmark

methods to encodeIntoAdvertisingToken or encodeIntoRefreshToken to make it clear what we are encoding into

sunnywu added 15 commits October 10, 2024 17:08

Renamed IdentityToken class and variables to Identity as it's a set o…

1693ea8

…f variables which don't just include tokens

Renamed Identity.LogoutToken to InvalidIdentity and added some commen…

14fd733

…ts for clarity

Renamed Identity.LogoutToken to InvalidIdentity and added some commen…

7dd3a69

…ts for clarity

renamed getAdvertisingId to MappedIdentity

f2e7a87

Split UserIdentity into 3 distinct classes - HashedDiiIdentity, First…

c0cb6df

…LevelHashIdentity and RawUidIdentity as UserIdentity is a very vague term and one needs to read function calls to really understand whether the UserIdentity class has hashed DII, first level hash or raw Uid.

Added brief comment to Identity/MappedIdentity

0fccf8c

Renamed MappedIdentity to MappedIdentityResult

113e8ca

1. Rename AdvertisingToken class to AdvertisingTokenInput

ae03ff5

2. Rename RefreshToken class to RefreshTokenInput 3. Rename createAdvertisingToken/createRefreshToken methods to createAdvertisingTokenInput/createRefreshTokenInput

Renamed ITokenEncoder.encode to ITokenEncoder.encodeIntoIdentityResponse

5595a15

Renamed Identity.isEmptyToken method to Identity.isNotValid

c6bc08f

Renamed PublisherIdentity to SourcePublisher

639b799

Refactored UserIdentity as a base class which is inherited by HashedD…

d9c730e

…ii/FirstLevelHash/RawUidIdentity sub-classes

Renamed Identity to IdentityResponse

42c5890

Fixed using raw identity in testGenerateTokenForOptOutUser

b62aa54

Renamed RefreshResponse#getIdentity to RefreshResponse#getIdentityRes…

b272d40

…ponse

sunnywu changed the title ~~Syw UI d2 4159 token gen code renaming~~ Rename class/variable names to improve token generation code readability Oct 14, 2024

sunnywu mentioned this pull request Oct 14, 2024

Refactor Token Generation Logic to improve readability #1081

Closed

sunnywu changed the title ~~Rename class/variable names to improve token generation code readability~~ Rename class/method/variable names to improve token generation code readability Oct 14, 2024

sunnywu added 4 commits October 14, 2024 15:24

Renamed MappedIdentity to RawUid2Result and UIDOperatorService.getAdv…

8dc0a41

…ertisingId to generateRawUid

1. Fixed RawUid2Result to RawUidResult

91ffa12

2. Renamed AdvertisingId to RawUid in most operator codes (except optout store related)

Renamed IdentityResponse.optOutIdentityResponse to OptOutIdentityResp…

4bb4ccb

…onse

Renamed IdentityResponse.optoutIdentityResponse to OptOutIdentityResp…

8ae5e08

…onse

mcollins-ttd reviewed Oct 15, 2024

View reviewed changes

gmsdelmundo reviewed Oct 15, 2024

View reviewed changes

sunnywu added 6 commits October 15, 2024 18:23

Fixed UserIdentity member variables as final and added constructor

d8eda03

1. Addressed code review feedbacks

e74e8da

2. Renamed RawUidResult to RawUidResponse 3. rename some id variable to rawUid to make it clearer 4. more comments

Made UserIdentity abstract first

7f4ebf5

Fixed a few id variable to rawUid/firstLevelHash to make it clear. Fi…

72f7be4

…xed the UserIdentity/FirstLevelHashIdentity class uses to HashedDiiIdentity instead in IdentityMapBenchmark and TokenEndecBenchmark

1. Refactored EncryptedTokenEncoder to rename encode/encodev2/encodev3

21f0c60

methods to encodeIntoAdvertisingToken or encodeIntoRefreshToken to make it clear what we are encoding into

fixed variable name

53f4de1

sunnywu mentioned this pull request Oct 28, 2024

Final Refactoring - Syw UID2-4159 token gen code refactoring user identity #1123

Open


		import java.time.Instant;

		// Contains a hash computed from a raw email/phone number DII input or the hash is provided by the UID Participant

Conversation

sunnywu commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sunnywu commented Oct 14, 2024 •

edited

Loading