From 3335ba307bbde4ad01eec392a8fa34db35b0c41c Mon Sep 17 00:00:00 2001 From: Scott Kay Date: Mon, 14 Dec 2020 16:01:23 -0500 Subject: [PATCH 1/2] Refactor AMP Param Parsing --- amp/parse.go | 110 ++++++++++++++++++ endpoints/openrtb2/amp_auction.go | 150 ++++++++----------------- endpoints/openrtb2/amp_auction_test.go | 18 ++- 3 files changed, 163 insertions(+), 115 deletions(-) create mode 100644 amp/parse.go diff --git a/amp/parse.go b/amp/parse.go new file mode 100644 index 00000000000..bfe1f31ac03 --- /dev/null +++ b/amp/parse.go @@ -0,0 +1,110 @@ +package amp + +import ( + "errors" + "net/http" + "strconv" + "strings" + + "github.com/mxmCherry/openrtb" +) + +// Params defines the paramters of an AMP request. +type Params struct { + Account string + CanonicalURL string + Consent string + Debug bool + Origin string + Size Size + Slot string + StoredRequestID string + Timeout *uint64 +} + +// Size defines size information of an AMP request. +type Size struct { + Height uint64 + Multisize []openrtb.Format + OverrideHeight uint64 + OverrideWidth uint64 + Width uint64 +} + +// ParseParams parses the AMP request paramters from a http request. +func ParseParams(httpRequest *http.Request) (Params, error) { + query := httpRequest.URL.Query() + + tagID := query.Get("tag_id") + if len(tagID) == 0 { + return Params{}, errors.New("AMP requests require an AMP tag_id") + } + + params := Params{ + Account: query.Get("account"), + CanonicalURL: query.Get("curl"), + Consent: chooseConsent(query.Get("consent_string"), query.Get("gdpr_consent")), + Debug: query.Get("debug") == "1", + Origin: query.Get("__amp_source_origin"), + Size: Size{ + Height: parseInt(query.Get("h")), + Multisize: parseMultisize(query.Get("ms")), + OverrideHeight: parseInt(query.Get("oh")), + OverrideWidth: parseInt(query.Get("ow")), + Width: parseInt(query.Get("w")), + }, + Slot: query.Get("slot"), + StoredRequestID: tagID, + Timeout: parseIntPtr(query.Get("timeout")), + } + return params, nil +} + +func parseIntPtr(value string) *uint64 { + if parsed, err := strconv.ParseUint(value, 10, 64); err == nil { + return &parsed + } + return nil +} + +func parseInt(value string) uint64 { + if parsed, err := strconv.ParseUint(value, 10, 64); err == nil { + return parsed + } + return 0 +} + +func parseMultisize(multisize string) []openrtb.Format { + if multisize == "" { + return nil + } + + sizeStrings := strings.Split(multisize, ",") + sizes := make([]openrtb.Format, 0, len(sizeStrings)) + for _, sizeString := range sizeStrings { + wh := strings.Split(sizeString, "x") + if len(wh) != 2 { + return nil + } + f := openrtb.Format{ + W: parseInt(wh[0]), + H: parseInt(wh[1]), + } + if f.W == 0 && f.H == 0 { + return nil + } + + sizes = append(sizes, f) + } + return sizes +} + +func chooseConsent(consent, gdprConsent string) string { + if len(consent) > 0 { + return consent + } + + // Fallback to 'gdpr_consent' for compatability until it's no longer used. This was our original + // implementation before the same AMP macro was reused for CCPA. + return gdprConsent +} diff --git a/endpoints/openrtb2/amp_auction.go b/endpoints/openrtb2/amp_auction.go index 902af46421b..cd63107f23d 100644 --- a/endpoints/openrtb2/amp_auction.go +++ b/endpoints/openrtb2/amp_auction.go @@ -8,7 +8,6 @@ import ( "fmt" "net/http" "net/url" - "strconv" "strings" "time" @@ -17,6 +16,7 @@ import ( "github.com/julienschmidt/httprouter" "github.com/mxmCherry/openrtb" accountService "github.com/prebid/prebid-server/account" + "github.com/prebid/prebid-server/amp" "github.com/prebid/prebid-server/analytics" "github.com/prebid/prebid-server/config" "github.com/prebid/prebid-server/errortypes" @@ -314,43 +314,41 @@ func (deps *endpointDeps) loadRequestJSONForAmp(httpRequest *http.Request) (req req = &openrtb.BidRequest{} errs = nil - ampID := httpRequest.FormValue("tag_id") - if ampID == "" { - errs = []error{errors.New("AMP requests require an AMP tag_id")} - return + ampParams, err := amp.ParseParams(httpRequest) + if err != nil { + return nil, []error{err} } ctx, cancel := context.WithTimeout(context.Background(), time.Duration(storedRequestTimeoutMillis)*time.Millisecond) defer cancel() - storedRequests, _, errs := deps.storedReqFetcher.FetchRequests(ctx, []string{ampID}, nil) + storedRequests, _, errs := deps.storedReqFetcher.FetchRequests(ctx, []string{ampParams.StoredRequestID}, nil) if len(errs) > 0 { return nil, errs } if len(storedRequests) == 0 { - errs = []error{fmt.Errorf("No AMP config found for tag_id '%s'", ampID)} + errs = []error{fmt.Errorf("No AMP config found for tag_id '%s'", ampParams.StoredRequestID)} return } // The fetched config becomes the entire OpenRTB request - requestJSON := storedRequests[ampID] + requestJSON := storedRequests[ampParams.StoredRequestID] if err := json.Unmarshal(requestJSON, req); err != nil { errs = []error{err} return } - debugParam := httpRequest.FormValue("debug") - if debugParam == "1" { + if ampParams.Debug { req.Test = 1 } // Two checks so users know which way the Imp check failed. if len(req.Imp) == 0 { - errs = []error{fmt.Errorf("data for tag_id='%s' does not define the required imp array", ampID)} + errs = []error{fmt.Errorf("data for tag_id='%s' does not define the required imp array", ampParams.StoredRequestID)} return } if len(req.Imp) > 1 { - errs = []error{fmt.Errorf("data for tag_id '%s' includes %d imp elements. Only one is allowed", ampID, len(req.Imp))} + errs = []error{fmt.Errorf("data for tag_id '%s' includes %d imp elements. Only one is allowed", ampParams.StoredRequestID, len(req.Imp))} return } @@ -367,35 +365,30 @@ func (deps *endpointDeps) loadRequestJSONForAmp(httpRequest *http.Request) (req *req.Imp[0].Secure = 1 } - errs = deps.overrideWithParams(httpRequest, req) + errs = deps.overrideWithParams(ampParams, req) return } -func (deps *endpointDeps) overrideWithParams(httpRequest *http.Request, req *openrtb.BidRequest) []error { +func (deps *endpointDeps) overrideWithParams(ampParams amp.Params, req *openrtb.BidRequest) []error { if req.Site == nil { req.Site = &openrtb.Site{} } // Override the stored request sizes with AMP ones, if they exist. if req.Imp[0].Banner != nil { - width := parseFormInt(httpRequest, "w", 0) - height := parseFormInt(httpRequest, "h", 0) - overrideWidth := parseFormInt(httpRequest, "ow", 0) - overrideHeight := parseFormInt(httpRequest, "oh", 0) - if format := makeFormatReplacement(overrideWidth, overrideHeight, width, height, httpRequest.FormValue("ms")); len(format) != 0 { + if format := makeFormatReplacement(ampParams.Size); len(format) != 0 { req.Imp[0].Banner.Format = format - } else if width != 0 { - setWidths(req.Imp[0].Banner.Format, width) - } else if height != 0 { - setHeights(req.Imp[0].Banner.Format, height) + } else if ampParams.Size.Width != 0 { + setWidths(req.Imp[0].Banner.Format, ampParams.Size.Width) + } else if ampParams.Size.Height != 0 { + setHeights(req.Imp[0].Banner.Format, ampParams.Size.Height) } } - canonicalURL := httpRequest.FormValue("curl") - if canonicalURL != "" { - req.Site.Page = canonicalURL + if ampParams.CanonicalURL != "" { + req.Site.Page = ampParams.CanonicalURL // Fixes #683 - if parsedURL, err := url.Parse(canonicalURL); err == nil { + if parsedURL, err := url.Parse(ampParams.CanonicalURL); err == nil { domain := parsedURL.Host if colonIndex := strings.LastIndex(domain, ":"); colonIndex != -1 { domain = domain[:colonIndex] @@ -406,14 +399,13 @@ func (deps *endpointDeps) overrideWithParams(httpRequest *http.Request, req *ope setAmpExt(req.Site, "1") - setEffectiveAmpPubID(req, httpRequest.URL.Query()) + setEffectiveAmpPubID(req, ampParams.Account) - slot := httpRequest.FormValue("slot") - if slot != "" { - req.Imp[0].TagID = slot + if ampParams.Slot != "" { + req.Imp[0].TagID = ampParams.Slot } - policyWriter, policyWriterErr := readPolicyFromUrl(httpRequest.URL) + policyWriter, policyWriterErr := readPolicy(ampParams.Consent) if policyWriterErr != nil { return []error{policyWriterErr} } @@ -421,42 +413,38 @@ func (deps *endpointDeps) overrideWithParams(httpRequest *http.Request, req *ope return []error{err} } - if timeout, err := strconv.ParseInt(httpRequest.FormValue("timeout"), 10, 64); err == nil { - req.TMax = timeout - deps.cfg.AMPTimeoutAdjustment + if ampParams.Timeout != nil { + req.TMax = int64(*ampParams.Timeout) - deps.cfg.AMPTimeoutAdjustment } return nil } -func makeFormatReplacement(overrideWidth uint64, overrideHeight uint64, width uint64, height uint64, multisize string) []openrtb.Format { +func makeFormatReplacement(size amp.Size) []openrtb.Format { var formats []openrtb.Format - if overrideWidth != 0 && overrideHeight != 0 { + if size.OverrideWidth != 0 && size.OverrideHeight != 0 { formats = []openrtb.Format{{ - W: overrideWidth, - H: overrideHeight, + W: size.OverrideWidth, + H: size.OverrideHeight, }} - } else if overrideWidth != 0 && height != 0 { + } else if size.OverrideWidth != 0 && size.Height != 0 { formats = []openrtb.Format{{ - W: overrideWidth, - H: height, + W: size.OverrideWidth, + H: size.Height, }} - } else if width != 0 && overrideHeight != 0 { + } else if size.Width != 0 && size.OverrideHeight != 0 { formats = []openrtb.Format{{ - W: width, - H: overrideHeight, + W: size.Width, + H: size.OverrideHeight, }} - } else if width != 0 && height != 0 { + } else if size.Width != 0 && size.Height != 0 { formats = []openrtb.Format{{ - W: width, - H: height, + W: size.Width, + H: size.Height, }} } - if parsedSizes := parseMultisize(multisize); len(parsedSizes) != 0 { - formats = append(formats, parsedSizes...) - } - - return formats + return append(formats, size.Multisize...) } func setWidths(formats []openrtb.Format, width uint64) { @@ -471,42 +459,6 @@ func setHeights(formats []openrtb.Format, height uint64) { } } -func parseMultisize(multisize string) []openrtb.Format { - if multisize == "" { - return nil - } - - sizeStrings := strings.Split(multisize, ",") - sizes := make([]openrtb.Format, 0, len(sizeStrings)) - for _, sizeString := range sizeStrings { - wh := strings.Split(sizeString, "x") - if len(wh) != 2 { - return nil - } - f := openrtb.Format{ - W: parseIntErrorless(wh[0], 0), - H: parseIntErrorless(wh[1], 0), - } - if f.W == 0 && f.H == 0 { - return nil - } - - sizes = append(sizes, f) - } - return sizes -} - -func parseFormInt(req *http.Request, value string, defaultTo uint64) uint64 { - return parseIntErrorless(req.FormValue(value), defaultTo) -} - -func parseIntErrorless(value string, defaultTo uint64) uint64 { - if parsed, err := strconv.ParseUint(value, 10, 64); err == nil { - return parsed - } - return defaultTo -} - // AMP won't function unless ext.prebid.targeting and ext.prebid.cache.bids are defined. // If the user didn't include them, default those here. func defaultRequestExt(req *openrtb.BidRequest) (errs []error) { @@ -563,9 +515,7 @@ func setAmpExt(site *openrtb.Site, value string) { } } -func readPolicyFromUrl(url *url.URL) (privacy.PolicyWriter, error) { - consent := readConsentFromURL(url) - +func readPolicy(consent string) (privacy.PolicyWriter, error) { if len(consent) == 0 { return privacy.NilPolicyWriter{}, nil } @@ -583,17 +533,8 @@ func readPolicyFromUrl(url *url.URL) (privacy.PolicyWriter, error) { } } -func readConsentFromURL(url *url.URL) string { - if v := url.Query().Get("consent_string"); v != "" { - return v - } - - // Fallback to 'gdpr_consent' for compatability until it's no longer used by AMP. - return url.Query().Get("gdpr_consent") -} - // Sets the effective publisher ID for amp request -func setEffectiveAmpPubID(req *openrtb.BidRequest, urlQueryParams url.Values) { +func setEffectiveAmpPubID(req *openrtb.BidRequest, account string) { var pub *openrtb.Publisher if req.App != nil { if req.App.Publisher == nil { @@ -608,10 +549,9 @@ func setEffectiveAmpPubID(req *openrtb.BidRequest, urlQueryParams url.Values) { } if pub.ID == "" { - // For amp requests, the publisher ID could be sent via the account - // query string - if acc := urlQueryParams.Get("account"); acc != "" && acc != "ACCOUNT_ID" { - pub.ID = acc + // ACCOUNT_ID is the unresolved macro name and should be ignored. + if account != "" && account != "ACCOUNT_ID" { + pub.ID = account } } } diff --git a/endpoints/openrtb2/amp_auction_test.go b/endpoints/openrtb2/amp_auction_test.go index daa5c1feb20..84aa9eebbc8 100644 --- a/endpoints/openrtb2/amp_auction_test.go +++ b/endpoints/openrtb2/amp_auction_test.go @@ -1031,14 +1031,12 @@ func getTestBidRequest(nilUser bool, userExt *openrtb_ext.ExtUser, nilRegs bool, func TestSetEffectiveAmpPubID(t *testing.T) { testPubID := "test-pub" - testURLQueryParams := url.Values{} - testURLQueryParams.Add("account", testPubID) testCases := []struct { - description string - req *openrtb.BidRequest - urlQueryParams url.Values - expectedPubID string + description string + req *openrtb.BidRequest + account string + expectedPubID string }{ { description: "No publisher ID provided", @@ -1072,7 +1070,7 @@ func TestSetEffectiveAmpPubID(t *testing.T) { expectedPubID: testPubID, }, { - description: "Publisher ID present in account query parameter", + description: "Publisher ID present in account parameter", req: &openrtb.BidRequest{ App: &openrtb.App{ Publisher: &openrtb.Publisher{ @@ -1080,8 +1078,8 @@ func TestSetEffectiveAmpPubID(t *testing.T) { }, }, }, - urlQueryParams: testURLQueryParams, - expectedPubID: testPubID, + account: testPubID, + expectedPubID: testPubID, }, { description: "req.Site.Publisher present but ID set to empty string", @@ -1097,7 +1095,7 @@ func TestSetEffectiveAmpPubID(t *testing.T) { } for _, test := range testCases { - setEffectiveAmpPubID(test.req, test.urlQueryParams) + setEffectiveAmpPubID(test.req, test.account) if test.req.Site != nil { assert.Equal(t, test.expectedPubID, test.req.Site.Publisher.ID, "should return the expected Publisher ID for test case: %s", test.description) From 103a75440ae49f8e2da89778a388802316cccd1b Mon Sep 17 00:00:00 2001 From: Scott Kay Date: Fri, 8 Jan 2021 00:50:19 -0500 Subject: [PATCH 2/2] Added Tests --- amp/parse.go | 2 +- amp/parse_test.go | 168 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 169 insertions(+), 1 deletion(-) create mode 100644 amp/parse_test.go diff --git a/amp/parse.go b/amp/parse.go index bfe1f31ac03..9e0e019f953 100644 --- a/amp/parse.go +++ b/amp/parse.go @@ -31,7 +31,7 @@ type Size struct { Width uint64 } -// ParseParams parses the AMP request paramters from a http request. +// ParseParams parses the AMP paramters from a HTTP request. func ParseParams(httpRequest *http.Request) (Params, error) { query := httpRequest.URL.Query() diff --git a/amp/parse_test.go b/amp/parse_test.go new file mode 100644 index 00000000000..e2c82d71030 --- /dev/null +++ b/amp/parse_test.go @@ -0,0 +1,168 @@ +package amp + +import ( + "net/http" + "testing" + + "github.com/mxmCherry/openrtb" + "github.com/stretchr/testify/assert" +) + +func TestParseParams(t *testing.T) { + var expectedTimeout uint64 = 42 + + testCases := []struct { + description string + query string + expectedParams Params + expectedError string + }{ + { + description: "Empty", + query: "", + expectedError: "AMP requests require an AMP tag_id", + }, + { + description: "All Fields", + query: "tag_id=anyTagID&account=anyAccount&curl=anyCurl&consent_string=anyConsent&debug=1&__amp_source_origin=anyOrigin" + + "&slot=anySlot&timeout=42&h=1&w=2&oh=3&ow=4&ms=10x11,12x13", + expectedParams: Params{ + Account: "anyAccount", + CanonicalURL: "anyCurl", + Consent: "anyConsent", + Debug: true, + Origin: "anyOrigin", + Slot: "anySlot", + StoredRequestID: "anyTagID", + Timeout: &expectedTimeout, + Size: Size{ + Height: 1, + OverrideHeight: 3, + OverrideWidth: 4, + Width: 2, + Multisize: []openrtb.Format{ + {W: 10, H: 11}, {W: 12, H: 13}, + }, + }, + }, + }, + { + description: "Integer Values Ignored If Invalid", + query: "tag_id=anyTagID&h=invalid&w=invalid&oh=invalid&ow=invalid&ms=invalid", + expectedParams: Params{StoredRequestID: "anyTagID"}, + }, + { + description: "consent_string Preferred Over gdpr_consent", + query: "tag_id=anyTagID&consent_string=consent1&gdpr_consent=consent2", + expectedParams: Params{StoredRequestID: "anyTagID", Consent: "consent1"}, + }, + { + description: "consent_string Preferred Over gdpr_consent - Order Doesn't Matter", + query: "tag_id=anyTagID&gdpr_consent=consent2&consent_string=consent1", + expectedParams: Params{StoredRequestID: "anyTagID", Consent: "consent1"}, + }, + { + description: "Just gdpr_consent", + query: "tag_id=anyTagID&gdpr_consent=consent2", + expectedParams: Params{StoredRequestID: "anyTagID", Consent: "consent2"}, + }, + { + description: "Debug 0", + query: "tag_id=anyTagID&debug=0", + expectedParams: Params{StoredRequestID: "anyTagID", Debug: false}, + }, + { + description: "Debug Ignored If Invalid", + query: "tag_id=anyTagID&debug=invalid", + expectedParams: Params{StoredRequestID: "anyTagID", Debug: false}, + }, + } + + for _, test := range testCases { + httpRequest, err := http.NewRequest("GET", "http://any.url/anypage?"+test.query, nil) + assert.NoError(t, err, test.description+":request") + + params, err := ParseParams(httpRequest) + assert.Equal(t, test.expectedParams, params, test.description+":params") + if test.expectedError == "" { + assert.NoError(t, err, test.description+":err") + } else { + assert.EqualError(t, err, test.expectedError) + } + } +} + +func TestParseMultisize(t *testing.T) { + testCases := []struct { + description string + multisize string + expectedFormats []openrtb.Format + }{ + { + description: "Empty", + multisize: "", + expectedFormats: nil, + }, + { + description: "One", + multisize: "1x2", + expectedFormats: []openrtb.Format{{W: 1, H: 2}}, + }, + { + description: "Many", + multisize: "1x2,3x4", + expectedFormats: []openrtb.Format{{W: 1, H: 2}, {W: 3, H: 4}}, + }, + { + // Existing Behavior: The " 3" token in the second size is parsed as 0. + description: "Many With Space - Quirky Result", + multisize: "1x2, 3x4", + expectedFormats: []openrtb.Format{{W: 1, H: 2}, {W: 0, H: 4}}, + }, + { + description: "One - Zero Size - Ignored", + multisize: "0x0", + expectedFormats: nil, + }, + { + description: "Many - Zero Size - All Ignored", + multisize: "0x0,3x4", + expectedFormats: nil, + }, + { + description: "One - Extra Dimension - Ignored", + multisize: "1x2x3", + expectedFormats: nil, + }, + { + description: "Many - Extra Dimension - All Ignored", + multisize: "1x2x3,4x5", + expectedFormats: nil, + }, + { + description: "One - Invalid Values - Ignored", + multisize: "INVALIDxINVALID", + expectedFormats: nil, + }, + { + description: "Many - Invalid Values - All Ignored", + multisize: "1x2,INVALIDxINVALID", + expectedFormats: nil, + }, + { + description: "One - No Pair - Ignored", + multisize: "INVALID", + expectedFormats: nil, + }, + { + description: "Many - No Pair - All Ignored", + multisize: "1x2,INVALID", + expectedFormats: nil, + }, + } + + for _, test := range testCases { + result := parseMultisize(test.multisize) + assert.ElementsMatch(t, test.expectedFormats, result, test.description) + } +}