Refactor Usersync

[AlexBVolcy]

This PR represents a large refactor of usersync, primarily focused on refactoring usersync/cookie.go

This PR builds off of these smaller PRs, that started the refactoring process by removing unused code (#2766, #2768)

The main refactoring that is done in this PR:

• Renaming of variables
• Create interfaces for encoding and decoding a cookie
• Simplifying the functions that Read and Write a cookie, replacing ParseCookieFromRequest and SetCookieOnResponse
• An optimization for how we eject uids when a cookie is too full (extension of @guscarreon's work). This ejection framework will be built upon with Priority Bidder Ejection in a future PR
• The changes in this file will then be reflected throughout the code base, primarily in openrtb2/setuid.go
• The test framework in usersync/cookie_test.go has been majorly re-worked with new tests for the new functions from the refactor, and a re-working/consolidation of older tests.

[VeronikaSolovei9]

This check looks a little weird cfg.MaxCookieSizeBytes > 0. Should it be a part of validation at the server start up?
Later in this function you use isCookieTooBig = len(encodedCookie) > cfg.MaxCookieSizeBytes. Should this line be the same?

[AlexBVolcy]

The validation for MaxCookieSizeBytes, just checks if the value is greater than MIN_COOKIE_SIZE_BYTES, but there is no non-negative check, so that's why I have this here.

The reason that this check is not present later in the function, is once it's checked once, I no longer need to check it again since the value never changes.

[VeronikaSolovei9]

Just to confirm. You sort cookies from oldest to newest and then return first cookie that is not too big. What if all of them are too big and all of them are deleted?

[VeronikaSolovei9]

Do you think this whole function code should be inside of for loop (except of sort)?
Something like:

// PrepareCookieForWrite ejects UIDs as long as the cookie is too full
func (cookie *Cookie) PrepareCookieForWrite(cfg *config.HostCookie, ttl time.Duration, encoder Base64Encoder) string {

	uuidKeys := sortUIDs(cookie.uids)

	i := 0
	isCookieTooBig := true

	for isCookieTooBig && len(cookie.uids) > 0 {
		encodedCookie := encoder.Encode(cookie)

		isCookieTooBig = len(encodedCookie) > cfg.MaxCookieSizeBytes && cfg.MaxCookieSizeBytes > 0
		if !isCookieTooBig {
			return encodedCookie
		}

		uidToDelete := uuidKeys[i]
		delete(cookie.uids, uidToDelete)

		i++
	}
	return ""
}

or

// PrepareCookieForWrite ejects UIDs as long as the cookie is too full
func (cookie *Cookie) PrepareCookieForWrite(cfg *config.HostCookie, ttl time.Duration, encoder Base64Encoder) string {

	uuidKeys := sortUIDs(cookie.uids)

	for ind := range uuidKeys {
		encodedCookie := encoder.Encode(cookie)

		isCookieTooBig := len(encodedCookie) > cfg.MaxCookieSizeBytes && cfg.MaxCookieSizeBytes > 0
		if !isCookieTooBig {
			return encodedCookie
		}

		uidToDelete := uuidKeys[ind]
		delete(cookie.uids, uidToDelete)

	}
	return ""
}

This code passes existing unit tests. Please make sure the logic is still correct

[AlexBVolcy]

What this function does is eject the oldest UIDs from the Cookie as long as the cookie is still too full. So the sort function, sorts the UIDs from oldest to newest, and then we eject the UIDs in this order as long as the cookie is too full. Once the cookie isn't too full anymore, we return that updated cookie.

[AlexBVolcy]

When it comes to your re-write, I like the idea of consolidating the function, I'll look into this!

[guscarreon]

In the previous version of this file, the cookie was trimmed according to it's cookie.String() size as ilustrated in line 168 below:

143 // SetCookieOnResponse is a shortcut for "ToHTTPCookie(); cookie.setDomain(domain); setCookie(w, cookie)"
144 func (cookie *Cookie) SetCookieOnResponse(w http.ResponseWriter, setSiteCookie bool, cfg *config.HostCookie, ttl time.Duration) {
145     httpCookie := cookie.ToHTTPCookie(ttl)
146     var domain string = cfg.Domain
147
148     if domain != "" {
149         httpCookie.Domain = domain
150     }
151
152     var currSize int = len([]byte(httpCookie.String()))
153     for cfg.MaxCookieSizeBytes > 0 && currSize > cfg.MaxCookieSizeBytes && len(cookie.uids) > 0 {
154         var oldestElem string = ""
155         var oldestDate int64 = math.MaxInt64
156         for key, value := range cookie.uids {
157             timeUntilExpiration := time.Until(value.Expires)
158             if timeUntilExpiration < time.Duration(oldestDate) {
159                 oldestElem = key
160                 oldestDate = int64(timeUntilExpiration)
161             }
162         }
163         delete(cookie.uids, oldestElem)
164         httpCookie = cookie.ToHTTPCookie(ttl)
165         if domain != "" {
166             httpCookie.Domain = domain
167         }
168         currSize = len([]byte(httpCookie.String())) // <- OLD VERSION USES httpCookie.String()
169     }
170
171     if setSiteCookie {
172         httpCookie.Secure = true
173         httpCookie.SameSite = http.SameSiteNoneMode
174     }
175     w.Header().Add("Set-Cookie", httpCookie.String())
176 }
usersync/cookie.go

To be consistent, the refactored version could also make use of it:

61   func (cookie *Cookie) PrepareCookieForWrite(cfg *config.HostCookie, encoder Encoder) (string, error) {
62       uuidKeys := sortUIDs(cookie.uids)
63  
64       i := 0
65       for len(cookie.uids) > 0 {
66           encodedCookie, err := encoder.Encode(cookie)
67           if err != nil {
68               return encodedCookie, nil
69           }
   +         
   +         oldFashionedCookieSize := len([]byte(&http.Cookie{
   +             Name:    uidCookieName,
   +             Value:   b64,
   +             Expires: time.Now().Add(ttl),
   +             Path:    "/",
   +         }).String())
70  
71 -         isCookieTooBig := len(encodedCookie) > cfg.MaxCookieSizeBytes && cfg.MaxCookieSizeBytes > 0
   +         isCookieTooBig := len(oldFashionedCookieSize) > cfg.MaxCookieSizeBytes && cfg.MaxCookieSizeBytes > 0
72           if !isCookieTooBig {
73               return encodedCookie, nil
74           }
75  
76           uidToDelete := uuidKeys[i]
77           delete(cookie.uids, uidToDelete)
78  
79           i++
80       }
81       return "", nil
82   }
usersync/cookie.go

Back when I coded PR #2819 I was split on this. I couldn't decide on whether or not to keep using httpCookie.String() because it seemed wasteful and thought about simply using the base 64 encoded value like you are doing in this PR. But, after some tests, I noticed I wasn't getting the same results and decided to keep using httpCookie.String() in an effort to not risk changing the outcome with respect to the current version in production.

Why did you decide to not use the httpCookie.String() method anymore? Was this discussed offline?

[AlexBVolcy]

This is a good question! I decided to just use the base 64 encoded value since I assumed that that value represented the entirety of the cookie. However, you make a good point that this value eventually gets added to an HttpCookie and thus the size of base64 vs. httpCookie.String() are different. I think it makes sense that we utilize the oldFashionedCookieSize, but it's something that I'll bring up in team time to confirm.

[guscarreon]

LGTM

[AlexBVolcy]

I'm going to be running an additional test today, so let's wait to merge until after I do this.

[AlexBVolcy]

The final test that I ran went smoothly! This is ready to be merged in