-
Notifications
You must be signed in to change notification settings - Fork 748
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
Conversation
@@ -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)) | |||
} |
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.
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
}
....
}
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.
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.
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.
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
}
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 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.")
}
No description provided.