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.

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

Generate Unique Bidrequest IDs for Amp Auctions #1938

Merged
merged 3 commits into from
Sep 14, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ type Configuration struct {
AutoGenSourceTID bool `mapstructure:"auto_gen_source_tid"`
//When true, new bid id will be generated in seatbid[].bid[].ext.prebid.bidid and used in event urls instead
GenerateBidID bool `mapstructure:"generate_bid_id"`
// GenerateRequestID overrides the bidrequest.id in an AMP Request or an App Stored Request with a generated UUID if set to true. The default is false.
GenerateRequestID bool `mapstructure:"generate_request_id"`
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
}

const MIN_COOKIE_SIZE_BYTES = 500
Expand Down Expand Up @@ -943,6 +945,7 @@ func SetupViper(v *viper.Viper, filename string) {
v.SetDefault("certificates_file", "")
v.SetDefault("auto_gen_source_tid", true)
v.SetDefault("generate_bid_id", false)
v.SetDefault("generate_request_id", false)

v.SetDefault("request_timeout_headers.request_time_in_queue", "")
v.SetDefault("request_timeout_headers.request_timeout_in_queue", "")
Expand Down
12 changes: 12 additions & 0 deletions endpoints/openrtb2/amp_auction.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"github.com/golang/glog"
"github.com/julienschmidt/httprouter"
"github.com/mxmCherry/openrtb/v15/openrtb2"
"github.com/prebid/prebid-server/util/uuidutil"
)

const defaultAmpRequestTimeoutMillis = 900
Expand All @@ -45,6 +46,7 @@ type AmpResponse struct {
// NewAmpEndpoint modifies the OpenRTB endpoint to handle AMP requests. This will basically modify the parsing
// of the request, and the return value, using the OpenRTB machinery to handle everything in between.
func NewAmpEndpoint(
uuidGenerator uuidutil.UUIDGenerator,
ex exchange.Exchange,
validator openrtb_ext.BidderParamValidator,
requestsById stored_requests.Fetcher,
Expand All @@ -69,6 +71,7 @@ func NewAmpEndpoint(
}

return httprouter.Handle((&endpointDeps{
uuidGenerator,
ex,
validator,
requestsById,
Expand Down Expand Up @@ -345,6 +348,15 @@ func (deps *endpointDeps) loadRequestJSONForAmp(httpRequest *http.Request) (req
return
}

if deps.cfg.GenerateRequestID {
newBidRequestId, err := deps.uuidGenerator.Generate()
if err != nil {
errs = []error{err}
return
}
req.ID = newBidRequestId
}
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved

if ampParams.Debug {
req.Test = 1
}
Expand Down
112 changes: 87 additions & 25 deletions endpoints/openrtb2/amp_auction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import (
"strconv"
"testing"

"github.com/julienschmidt/httprouter"
"github.com/mxmCherry/openrtb/v15/openrtb2"
"github.com/prebid/prebid-server/analytics"
analyticsConf "github.com/prebid/prebid-server/analytics/config"
"github.com/prebid/prebid-server/config"
Expand All @@ -19,7 +21,6 @@ import (
"github.com/prebid/prebid-server/openrtb_ext"
"github.com/prebid/prebid-server/stored_requests/backends/empty_fetcher"

"github.com/mxmCherry/openrtb/v15/openrtb2"
"github.com/stretchr/testify/assert"
)

Expand All @@ -38,6 +39,7 @@ func TestGoodAmpRequests(t *testing.T) {
}

endpoint, _ := NewAmpEndpoint(
fakeUUIDGenerator{},
&mockAmpExchange{},
newParamsValidator(t),
&mockAmpStoredReqFetcher{goodRequests},
Expand Down Expand Up @@ -91,6 +93,7 @@ func TestAMPPageInfo(t *testing.T) {
exchange := &mockAmpExchange{}

endpoint, _ := NewAmpEndpoint(
fakeUUIDGenerator{},
exchange,
newParamsValidator(t),
&mockAmpStoredReqFetcher{stored},
Expand Down Expand Up @@ -188,6 +191,7 @@ func TestGDPRConsent(t *testing.T) {
// Build Exchange Endpoint
mockExchange := &mockAmpExchange{}
endpoint, _ := NewAmpEndpoint(
fakeUUIDGenerator{},
mockExchange,
newParamsValidator(t),
&mockAmpStoredReqFetcher{stored},
Expand Down Expand Up @@ -340,6 +344,7 @@ func TestCCPAConsent(t *testing.T) {
// Build Exchange Endpoint
mockExchange := &mockAmpExchange{}
endpoint, _ := NewAmpEndpoint(
fakeUUIDGenerator{},
mockExchange,
newParamsValidator(t),
&mockAmpStoredReqFetcher{stored},
Expand Down Expand Up @@ -450,6 +455,7 @@ func TestConsentWarnings(t *testing.T) {
mockExchange = &mockAmpExchange{}
}
endpoint, _ := NewAmpEndpoint(
fakeUUIDGenerator{},
mockExchange,
newParamsValidator(t),
&mockAmpStoredReqFetcher{stored},
Expand Down Expand Up @@ -542,6 +548,7 @@ func TestNewAndLegacyConsentBothProvided(t *testing.T) {
// Build Exchange Endpoint
mockExchange := &mockAmpExchange{}
endpoint, _ := NewAmpEndpoint(
fakeUUIDGenerator{},
mockExchange,
newParamsValidator(t),
&mockAmpStoredReqFetcher{stored},
Expand Down Expand Up @@ -593,6 +600,7 @@ func TestAMPSiteExt(t *testing.T) {
}
exchange := &mockAmpExchange{}
endpoint, _ := NewAmpEndpoint(
fakeUUIDGenerator{},
exchange,
newParamsValidator(t),
&mockAmpStoredReqFetcher{stored},
Expand Down Expand Up @@ -629,6 +637,7 @@ func TestAmpBadRequests(t *testing.T) {
}

endpoint, _ := NewAmpEndpoint(
fakeUUIDGenerator{},
&mockAmpExchange{},
newParamsValidator(t),
&mockAmpStoredReqFetcher{badRequests},
Expand Down Expand Up @@ -659,6 +668,7 @@ func TestAmpDebug(t *testing.T) {
}

endpoint, _ := NewAmpEndpoint(
fakeUUIDGenerator{},
&mockAmpExchange{},
newParamsValidator(t),
&mockAmpStoredReqFetcher{requests},
Expand Down Expand Up @@ -731,6 +741,7 @@ func TestQueryParamOverrides(t *testing.T) {
}

endpoint, _ := NewAmpEndpoint(
fakeUUIDGenerator{},
&mockAmpExchange{},
newParamsValidator(t),
&mockAmpStoredReqFetcher{requests},
Expand Down Expand Up @@ -883,6 +894,7 @@ func (s formatOverrideSpec) execute(t *testing.T) {
}

endpoint, _ := NewAmpEndpoint(
fakeUUIDGenerator{},
&mockAmpExchange{},
newParamsValidator(t),
&mockAmpStoredReqFetcher{requests},
Expand Down Expand Up @@ -1249,31 +1261,8 @@ func TestBuildAmpObject(t *testing.T) {
recorder := httptest.NewRecorder()

for _, test := range testCases {

// Set up test, declare a new mock logger every time
actualAmpObject := new(analytics.AmpObject)

logger := newMockLogger(actualAmpObject)

mockAmpFetcher := &mockAmpStoredReqFetcher{
data: map[string]json.RawMessage{
test.inTagId: json.RawMessage(test.inStoredRequest),
},
}

endpoint, _ := NewAmpEndpoint(
&mockAmpExchange{},
newParamsValidator(t),
mockAmpFetcher,
empty_fetcher.EmptyFetcher{},
&config.Configuration{MaxRequestSize: maxSize},
&metricsConfig.DummyMetricsEngine{},
logger,
map[string]string{},
[]byte{},
openrtb_ext.BuildBidderMap(),
)

actualAmpObject, endpoint := AmpObjectTestSetup(t, test.inTagId, test.inStoredRequest, false)
// Run test
endpoint(recorder, request, nil)

Expand All @@ -1286,3 +1275,76 @@ func TestBuildAmpObject(t *testing.T) {
assert.Equalf(t, test.expectedAmpObject.Origin, actualAmpObject.Origin, "Amp Object Origin field doesn't match expected: %s\n", test.description)
}
}

func TestIdGeneration(t *testing.T) {
uuid := "foo"

testCases := []struct {
description string
givenInStoredRequest json.RawMessage
givenGenerateRequestID bool
expectedID string
}{
{
description: "The givenGenerateRequestID flag is set to true, so even though the stored amp request already has an id, we should still generate a new uuid",
givenInStoredRequest: json.RawMessage(`{"id":"ThisID","site":{"page":"prebid.org"},"imp":[{"id":"some-imp-id","banner":{"format":[{"w":300,"h":250}]},"ext":{"appnexus":{"placementId":1}}}],"tmax":1}`),
givenGenerateRequestID: true,
expectedID: uuid,
},
{
description: "The givenGenerateRequestID flag is set to true and the stored amp request ID is blank, so we should generate a new uuid for the request",
givenInStoredRequest: json.RawMessage(`{"id":"","site":{"page":"prebid.org"},"imp":[{"id":"some-imp-id","banner":{"format":[{"w":300,"h":250}]},"ext":{"appnexus":{"placementId":1}}}],"tmax":1}`),
givenGenerateRequestID: true,
expectedID: uuid,
},
{
description: "The givenGenerateRequestID flag is false, so the ID shouldn't change",
givenInStoredRequest: json.RawMessage(`{"id":"ThisID","site":{"page":"prebid.org"},"imp":[{"id":"some-imp-id","banner":{"format":[{"w":300,"h":250}]},"ext":{"appnexus":{"placementId":1}}}],"tmax":1}`),
givenGenerateRequestID: false,
expectedID: "ThisID",
},
{
description: "The givenGenerateRequestID flag is true, and if the id field isn't included in the stored request",
givenInStoredRequest: json.RawMessage(`{"site":{"page":"prebid.org"},"imp":[{"id":"some-imp-id","banner":{"format":[{"w":300,"h":250}]},"ext":{"appnexus":{"placementId":1}}}],"tmax":1}`),
givenGenerateRequestID: true,
expectedID: uuid,
},
SyntaxNode marked this conversation as resolved.
Show resolved Hide resolved
}

request := httptest.NewRequest("GET", "/openrtb2/auction/amp?tag_id=test", nil)
recorder := httptest.NewRecorder()

for _, test := range testCases {
// Set up and run test
actualAmpObject, endpoint := AmpObjectTestSetup(t, "test", test.givenInStoredRequest, test.givenGenerateRequestID)
endpoint(recorder, request, nil)

assert.Equalf(t, test.expectedID, actualAmpObject.Request.ID, "Bid Request ID is incorrect: %s\n", test.description)
}
}

func AmpObjectTestSetup(t *testing.T, inTagId string, inStoredRequest json.RawMessage, generateRequestID bool) (*analytics.AmpObject, httprouter.Handle) {
actualAmpObject := analytics.AmpObject{}
logger := newMockLogger(&actualAmpObject)

mockAmpFetcher := &mockAmpStoredReqFetcher{
data: map[string]json.RawMessage{
inTagId: json.RawMessage(inStoredRequest),
},
}

endpoint, _ := NewAmpEndpoint(
fakeUUIDGenerator{id: "foo", err: nil},
&mockAmpExchange{},
newParamsValidator(t),
mockAmpFetcher,
empty_fetcher.EmptyFetcher{},
&config.Configuration{MaxRequestSize: maxSize, GenerateRequestID: generateRequestID},
&metricsConfig.DummyMetricsEngine{},
logger,
map[string]string{},
[]byte{},
openrtb_ext.BuildBidderMap(),
)
return &actualAmpObject, endpoint
}
Loading