[Timebox: 2d] Address Sonar Analysis issues - part 1 (#1011)

* Refactoring cookie_sync.go

* Couple of changes in exchange.go

* Removed unnecessary TODO comment

* Scott's Oct 15th comments

* Scott's Oct 15th comments - part 2

cache/filecache/filecache.go

@@ -76,8 +76,7 @@ func New(filename string) (*Cache, error) {
 	}, nil
 }
 
-// Close does nothing
-// TODO: close the file
+// This empty function exists so the Cache struct implements the Cache interface defined in cache/legacy.go
 func (c *Cache) Close() error {
 	return nil
 }

endpoints/cookie_sync.go

@@ -78,28 +78,13 @@ func (deps *cookieSyncDeps) Endpoint(w http.ResponseWriter, r *http.Request, _ h
 	}
 
 	parsedReq := &cookieSyncRequest{}
-	if err := json.Unmarshal(bodyBytes, parsedReq); err != nil {
+	if err := parseRequest(parsedReq, bodyBytes, deps.gDPR.UsersyncIfAmbiguous); err != nil {
 		co.Status = http.StatusBadRequest
-		co.Errors = append(co.Errors, fmt.Errorf("JSON parsing failed: %v", err))
-		http.Error(w, "JSON parsing failed: "+err.Error(), http.StatusBadRequest)
+		co.Errors = append(co.Errors, err)
+		http.Error(w, co.Errors[len(co.Errors)-1].Error(), co.Status)
 		return
 	}
 
-	if parsedReq.GDPR != nil && *parsedReq.GDPR == 1 && parsedReq.Consent == "" {
-		co.Status = http.StatusBadRequest
-		co.Errors = append(co.Errors, errors.New("gdpr_consent is required if gdpr is 1"))
-		http.Error(w, "gdpr_consent is required if gdpr=1", http.StatusBadRequest)
-		return
-	}
-	// If GDPR is ambiguous, lets untangle it here.
-	if parsedReq.GDPR == nil {
-		var gdpr = 1
-		if deps.gDPR.UsersyncIfAmbiguous {
-			gdpr = 0
-		}
-		parsedReq.GDPR = &gdpr
-	}
-
 	if len(biddersJSON) == 0 {
 		parsedReq.Bidders = make([]string, 0, len(deps.syncers))
 		for bidder := range deps.syncers {
@@ -160,6 +145,26 @@ func (deps *cookieSyncDeps) Endpoint(w http.ResponseWriter, r *http.Request, _ h
 	enc.Encode(csResp)
 }
 
+func parseRequest(parsedReq *cookieSyncRequest, bodyBytes []byte, usersyncIfAmbiguous bool) error {
+	if err := json.Unmarshal(bodyBytes, parsedReq); err != nil {
+		return fmt.Errorf("JSON parsing failed: %s", err.Error())
+	}
+
+	if parsedReq.GDPR != nil && *parsedReq.GDPR == 1 && parsedReq.Consent == "" {
+		return errors.New("gdpr_consent is required if gdpr=1")
+	}
+	// If GDPR is ambiguous, lets untangle it here.
+	if parsedReq.GDPR == nil {
+		var gdpr = new(int)
+		*gdpr = 1
+		if usersyncIfAmbiguous {
+			*gdpr = 0
+		}
+		parsedReq.GDPR = gdpr
+	}
+	return nil
+}
+
 func gdprToString(gdpr *int) string {
 	if gdpr == nil {
 		return ""

endpoints/openrtb2/amp_auction.go

@@ -93,7 +93,7 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h
 		Source:        pbsmetrics.DemandWeb,
 		RType:         pbsmetrics.ReqTypeAMP,
 		PubID:         pbsmetrics.PublisherUnknown,
-		Browser:       pbsmetrics.BrowserOther,
+		Browser:       getBrowserName(r),
 		CookieFlag:    pbsmetrics.CookieFlagUnknown,
 		RequestStatus: pbsmetrics.RequestStatusOK,
 	}
@@ -103,11 +103,6 @@ func (deps *endpointDeps) AmpAuction(w http.ResponseWriter, r *http.Request, _ h
 		deps.analytics.LogAmpObject(&ao)
 	}()
 
-	isSafari := checkSafari(r)
-	if isSafari {
-		labels.Browser = pbsmetrics.BrowserSafari
-	}
-
 	// Add AMP headers
 	origin := r.FormValue("__amp_source_origin")
 	if len(origin) == 0 {

endpoints/openrtb2/auction.go

@@ -90,7 +90,7 @@ func (deps *endpointDeps) Auction(w http.ResponseWriter, r *http.Request, _ http
 		Source:        pbsmetrics.DemandUnknown,
 		RType:         pbsmetrics.ReqTypeORTB2Web,
 		PubID:         pbsmetrics.PublisherUnknown,
-		Browser:       pbsmetrics.BrowserOther,
+		Browser:       getBrowserName(r),
 		CookieFlag:    pbsmetrics.CookieFlagUnknown,
 		RequestStatus: pbsmetrics.RequestStatusOK,
 	}
@@ -100,11 +100,6 @@ func (deps *endpointDeps) Auction(w http.ResponseWriter, r *http.Request, _ http
 		deps.analytics.LogAuctionObject(&ao)
 	}()
 
-	isSafari := checkSafari(r)
-	if isSafari {
-		labels.Browser = pbsmetrics.BrowserSafari
-	}
-
 	req, errL := deps.parseRequest(r)
 
 	if fatalError(errL) && writeError(errL, w, &labels) {
@@ -1126,16 +1121,27 @@ func setUAImplicitly(httpReq *http.Request, bidReq *openrtb.BidRequest) {
 	}
 }
 
-// Check if a request comes from a Safari browser
-func checkSafari(r *http.Request) (isSafari bool) {
-	isSafari = false
+// parseUserID gets this user's ID for the host machine, if it exists.
+func parseUserID(cfg *config.Configuration, httpReq *http.Request) (string, bool) {
+	if hostCookie, err := httpReq.Cookie(cfg.HostCookie.CookieName); hostCookie != nil && err == nil {
+		return hostCookie.Value, true
+	} else {
+		return "", false
+	}
+}
+
+// getBrowserName checks if a request comes from a Safari browser.
+// Returns pbsmetrics.BrowserSafari or pbsmetrics.BrowserOther
+// depending on the value of the "User-Agent" header of our http.Request
+func getBrowserName(r *http.Request) pbsmetrics.Browser {
+	var browser pbsmetrics.Browser = pbsmetrics.BrowserOther
 	if ua := user_agent.New(r.Header.Get("User-Agent")); ua != nil {
 		name, _ := ua.Browser()
 		if name == "Safari" {
-			isSafari = true
+			browser = pbsmetrics.BrowserSafari
 		}
 	}
-	return
+	return browser
 }
 
 // Write(return) errors to the client, if any. Returns true if errors were found.

endpoints/openrtb2/video_auction.go

@@ -75,7 +75,7 @@ func (deps *endpointDeps) VideoAuctionEndpoint(w http.ResponseWriter, r *http.Re
 		Source:        pbsmetrics.DemandUnknown,
 		RType:         pbsmetrics.ReqTypeVideo,
 		PubID:         pbsmetrics.PublisherUnknown,
-		Browser:       pbsmetrics.BrowserOther,
+		Browser:       getBrowserName(r),
 		CookieFlag:    pbsmetrics.CookieFlagUnknown,
 		RequestStatus: pbsmetrics.RequestStatusOK,
 	}
@@ -85,11 +85,6 @@ func (deps *endpointDeps) VideoAuctionEndpoint(w http.ResponseWriter, r *http.Re
 		deps.analytics.LogAuctionObject(&ao)
 	}()
 
-	isSafari := checkSafari(r)
-	if isSafari {
-		labels.Browser = pbsmetrics.BrowserSafari
-	}
-
 	lr := &io.LimitedReader{
 		R: r.Body,
 		N: deps.cfg.MaxRequestSize,

exchange/exchange.go

@@ -76,13 +76,9 @@ func NewExchange(client *http.Client, cache prebid_cache_client.Client, cfg *con
 
 func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidRequest, usersyncs IdFetcher, labels pbsmetrics.Labels, categoriesFetcher *stored_requests.CategoryFetcher) (*openrtb.BidResponse, error) {
 	// Snapshot of resolved bid request for debug if test request
-	var resolvedRequest json.RawMessage
-	if bidRequest.Test == 1 {
-		if r, err := json.Marshal(bidRequest); err != nil {
-			glog.Errorf("Error marshalling bid request for debug: %v", err)
-		} else {
-			resolvedRequest = r
-		}
+	resolvedRequest, err := buildResolvedRequest(bidRequest)
+	if err != nil {
+		glog.Errorf("Error marshalling bid request for debug: %v", err)
 	}
 
 	for _, impInRequest := range bidRequest.Imp {
@@ -100,14 +96,8 @@ func (e *exchange) HoldAuction(ctx context.Context, bidRequest *openrtb.BidReque
 	cleanRequests, aliases, errs := cleanOpenRTBRequests(ctx, bidRequest, usersyncs, blabels, labels, e.gDPR, e.UsersyncIfAmbiguous)
 
 	// List of bidders we have requests for.
-	liveAdapters := make([]openrtb_ext.BidderName, len(cleanRequests))
-	i := 0
-	for a := range cleanRequests {
-		liveAdapters[i] = a
-		i++
-	}
-	// Randomize the list of adapters to make the auction more fair
-	randomizeList(liveAdapters)
+	liveAdapters := listBiddersWithRequests(cleanRequests)
+
 	// Process the request to check for targeting parameters.
 	var targData *targetData
 	shouldCacheBids := false
@@ -600,3 +590,24 @@ func (e *exchange) makeBid(Bids []*pbsOrtbBid, adapter openrtb_ext.BidderName) (
 	}
 	return bids, errList
 }
+
+// Returns a snapshot of resolved bid request for debug if test field is set in the incomming request
+func buildResolvedRequest(bidRequest *openrtb.BidRequest) (json.RawMessage, error) {
+	if bidRequest.Test == 1 {
+		return json.Marshal(bidRequest)
+	}
+	return nil, nil
+}
+
+func listBiddersWithRequests(cleanRequests map[openrtb_ext.BidderName]*openrtb.BidRequest) []openrtb_ext.BidderName {
+	liveAdapters := make([]openrtb_ext.BidderName, len(cleanRequests))
+	i := 0
+	for a := range cleanRequests {
+		liveAdapters[i] = a
+		i++
+	}
+	// Randomize the list of adapters to make the auction more fair
+	randomizeList(liveAdapters)
+
+	return liveAdapters
+}