Fix panic observed in setSiteImplicitly function

[ashishshinde-pubm]

No description provided.

[hhhjort]

Fixes #3052

[AlexBVolcy]

There's another way that I think this check can be made that could improve the flow of the overall function:

I think you can pull this check to the beginning of the function, which will then allow the other r.Site nil checks to be removed.

So the overall function would maybe look something like this:

func setSiteImplicitly(httpReq *http.Request, r *openrtb_ext.RequestWrapper) {
	if r.Site == nil {
		r.Site = &openrtb2.Site{}
	}

	referrerCandidate := httpReq.Referer()
	if referrerCandidate == "" && r.Site.Page != "" {
		referrerCandidate = r.Site.Page // If http referer is disabled and thus has empty value - use site.page instead
	}

	if referrerCandidate != "" {
		setSitePageIfEmpty(r.Site, referrerCandidate)
		if parsedUrl, err := url.Parse(referrerCandidate); err == nil {
			setSiteDomainIfEmpty(r.Site, parsedUrl.Host)
			if publisherDomain, err := publicsuffix.EffectiveTLDPlusOne(parsedUrl.Host); err == nil {
				setSitePublisherDomainIfEmpty(r.Site, publisherDomain)
			}
		}
	}

	if siteExt, err := r.GetSiteExt(); err == nil && siteExt.GetAmp() == nil {
		siteExt.SetAmp(&notAmp)
	}
}

What do you think about this?

[ashishshinde-pubm]

@AlexBVolcy
Setting it at the top of function will change the existing behaviour of function, will provide the wrong message in bid-response.

Example -
Moving the condition to top breaks the TestJsonSampleRequests when request does not contain both site and app object.

Test: TestJsonSampleRequests/Asserts_we_return_400s_on_requests_that_are_not_supposed_to_pass_validation#49
Actual: Invalid request: request.site should include at least one of request.site.id or request.site.page.
Expected: Invalid request: request.site or request.app must be defined, but not both.
Filename: sample-requests/invalid-whole/no-site-or-app.json

[ashishshinde-pubm]

@AlexBVolcy what is your suggestion ? should we move it to top or keep it as it is ?

[hhhjort]

I think it is good to move it to the top. Since this is only called when app is nil, we know we always want to have a site object by the time we are done. So might as well make sure we have one at the top, and then we don't have to check for the remainder of the function.

[ashishshinde-pubm]

okay, moved the nil-check at the top.
Also, modified the error-message in no-site-or-app.json file

[AlexBVolcy]

@ashishshinde-pubm Apologies for the late reply, but yes I agree with @hhhjort, thank you for moving it.

[hhhjort]

LGTM

[AlexBVolcy]

LGTM