Skip to content
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

Remove Legacy UIDs From Cookie #2768

Merged
merged 5 commits into from May 22, 2023
Merged

Conversation

AlexBVolcy
Copy link
Contributor

No description provided.

@bsardo bsardo self-assigned this May 11, 2023
@@ -261,18 +252,7 @@ func (cookie *Cookie) UnmarshalJSON(b []byte) error {
cookie.uids = cookieContract.UIDs

if cookie.uids == nil {
cookie.uids = make(map[string]uidWithExpiry, len(cookieContract.LegacyUIDs))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that the legacy code has been removed, I think you can simplify this logic getting rid of the else and the second instance of the assignment to an empty map as such:

cookie.uids = make(map[string]uidWithExpiry)

if !cookie.optOut {
	if cookieContract.UIDs != nil {
		cookie.uids = cookieContract.UIDs
	}
    ....
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This code is unmarshaling an existing cookie. In the "happy path" cookie.uids = cookieContract.UIDs always runs and the new make statement is a wasted. Consider a compromise:

if err := json.Unmarshal(b, &cookieContract); err != nil {
  return err
}

cookie.optOut = cookieContract.OptOut
cookie.birthday = cookieContract.Birthday

if !cookie.optOut {
  cookie.uids = cookieContract.uids
}

if cookie.uids == nil {
  cookie.uids = make(map[string]uidWithExpiry)
}

...

This way uids is ensured to never be nil and only makes the map if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Comparing it to the previous functionality, I see an edge case we missed. If cookie.optOut is true and there are existing cookies in the collection, they will remain. I doubt it matters in practice, but perhaps we should protect against that case to be safe:

if cookie.optOut {
   cookie.uids = nil
} else {
  cookie.uids = cookieContract.uids
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The logic looks good. It's a bit alarming that the tests weren't failing when we missed the edge case. I see that the following tests are exactly the same:
TestOptOutCookie
TestEmptyOptOutCookie

I believe TestOptOutCookie should be modified to something like this:

func TestOptOutCookie(t *testing.T) {
	cookie := &Cookie{
		uids:     map[string]uidWithExpiry{"appnexus": {UID: "test"}},
		optOut:   true,
		birthday: timestamp(),
	}
	ensureConsistency(t, cookie)
}

and that we should then modify ensureConsistency so the cookie size after parsing is verified to be 0 if we are opting out:

if cookie.optOut {
	assert.Equal(t, 0, len(copiedCookie.uids), "Incorrect sync count on reparsed cookie.")
} else {
	assert.Equal(t, len(cookie.uids), len(copiedCookie.uids), "Incorrect sync count on reparsed cookie.")
}

@SyntaxNode SyntaxNode self-assigned this May 16, 2023
bsardo
bsardo previously approved these changes May 18, 2023
bsardo
bsardo previously approved these changes May 22, 2023
@bsardo bsardo self-requested a review May 22, 2023 18:13
@SyntaxNode SyntaxNode merged commit 5d4cb55 into prebid:master May 22, 2023
3 checks passed
blueseasx pushed a commit to blueseasx/prebid-server that referenced this pull request Jun 2, 2023
@AlexBVolcy AlexBVolcy mentioned this pull request Jun 7, 2023
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.

None yet

3 participants