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.

Sign up for GitHub

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

Remove Legacy UIDs From Cookie #2768

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5307c7b
Remove legacy UIDs
AlexBVolcy May 10, 2023
27a1610
Simplify Unmarshal Logic
AlexBVolcy May 15, 2023
de6a020
Unmarshal update
AlexBVolcy May 16, 2023
b330580
Handle edge case
AlexBVolcy May 21, 2023
e88d036
Tweak to test
AlexBVolcy May 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 21 additions & 46 deletions usersync/cookie.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,9 @@ func (cookie *Cookie) TrySync(key string, uid string) error {
// This exists so that Cookie (which is public) can have private fields, and the rest of
// the code doesn't have to worry about the cookie data storage format.
type cookieJson struct {
LegacyUIDs map[string]string `json:"uids,omitempty"`
UIDs map[string]uidWithExpiry `json:"tempUIDs,omitempty"`
OptOut bool `json:"optout,omitempty"`
Birthday *time.Time `json:"bday,omitempty"`
UIDs map[string]uidWithExpiry `json:"tempUIDs,omitempty"`
OptOut bool `json:"optout,omitempty"`
Birthday *time.Time `json:"bday,omitempty"`
}

func (cookie *Cookie) MarshalJSON() ([]byte, error) {
Expand All @@ -240,53 +239,29 @@ func (cookie *Cookie) MarshalJSON() ([]byte, error) {
})
}

// UnmarshalJSON holds some transition code.
//
// "Legacy" cookies had UIDs *without* expiration dates, and recognized "0" as a legitimate UID for audienceNetwork.
// "Current" cookies always include UIDs with expiration dates, and never allow "0" for audienceNetwork.
//
// This Unmarshal method interprets both data formats, and does some conversions on legacy data to make it current.
// If you're seeing this message after March 2018, it's safe to assume that all the legacy cookies have been
// updated and remove the legacy logic.
func (cookie *Cookie) UnmarshalJSON(b []byte) error {
var cookieContract cookieJson
err := json.Unmarshal(b, &cookieContract)
if err == nil {
cookie.optOut = cookieContract.OptOut
cookie.birthday = cookieContract.Birthday

if cookie.optOut {
cookie.uids = make(map[string]uidWithExpiry)
} else {
cookie.uids = cookieContract.UIDs

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

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

@SyntaxNode SyntaxNode May 16, 2023 •
edited
Loading

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
Collaborator

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.")
}

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

// Interpret "legacy" UIDs as having been expired already.
// This should cause us to re-sync, since it would be time for a new one.
for bidder, uid := range cookieContract.LegacyUIDs {
if _, ok := cookie.uids[bidder]; !ok {
cookie.uids[bidder] = uidWithExpiry{
UID: uid,
Expires: time.Now().Add(-5 * time.Minute),
}
}
}
cookie.optOut = cookieContract.OptOut
cookie.birthday = cookieContract.Birthday

// Any "0" values from audienceNetwork really meant "no ID available." This happens if they've never
// logged into Facebook. However... once we know a user's ID, we stop trying to re-sync them until the
// expiration date has passed.
//
// Since users may log into facebook later, this is a bad strategy.
// Since "0" is a fake ID for this bidder, we'll just treat it like it doesn't exist.
if id, ok := cookie.uids[string(openrtb_ext.BidderAudienceNetwork)]; ok && id.UID == "0" {
delete(cookie.uids, string(openrtb_ext.BidderAudienceNetwork))
}
}
if !cookie.optOut {
cookie.uids = cookieContract.UIDs
}
return err

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

// Audience Network / Facebook Handling
if id, ok := cookie.uids[string(openrtb_ext.BidderAudienceNetwork)]; ok && id.UID == "0" {
delete(cookie.uids, string(openrtb_ext.BidderAudienceNetwork))
}

return nil
}

func timestamp() *time.Time {
Expand Down
27 changes: 0 additions & 27 deletions usersync/cookie_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package usersync

import (
"encoding/base64"
"encoding/json"
"net/http"
"net/http/httptest"
"strings"
Expand Down Expand Up @@ -260,32 +259,6 @@ func TestCookieReadWrite(t *testing.T) {
assert.Len(t, received.uids, 2, "Sync Count")
}

func TestPopulatedLegacyCookieRead(t *testing.T) {
legacyJson := `{"uids":{"adnxs":"123","audienceNetwork":"456"},"bday":"2017-08-03T21:04:52.629198911Z"}`
var cookie Cookie
json.Unmarshal([]byte(legacyJson), &cookie)

if cookie.HasAnyLiveSyncs() {
t.Error("Expected 0 user syncs. Found at least 1.")
}
if cookie.HasLiveSync("adnxs") {
t.Errorf("Received cookie should act like it has no ID for adnxs.")
}
if cookie.HasLiveSync("audienceNetwork") {
t.Errorf("Received cookie should act like it has no ID for audienceNetwork.")
}
}

func TestEmptyLegacyCookieRead(t *testing.T) {
legacyJson := `{"bday":"2017-08-29T18:54:18.393925772Z"}`
var cookie Cookie
json.Unmarshal([]byte(legacyJson), &cookie)

if cookie.HasAnyLiveSyncs() {
t.Error("Expected 0 user syncs. Found at least 1.")
}
}

func TestNilCookie(t *testing.T) {
var nilCookie *Cookie

Expand Down