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

Formalizing support for site.ext.amp #711

Merged
merged 6 commits into from
Oct 22, 2018
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
8 changes: 6 additions & 2 deletions docs/endpoints/openrtb2/auction.md
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,11 @@ The only exception here is the top-level `BidResponse`, because it's bidder-inde
`ext.{anyBidderCode}` and `ext.bidder` extensions are defined by bidders.
`ext.prebid` extensions are defined by Prebid Server.

Exceptions are made for DigiTrust and GDPR, so that we define `ext` according to the official recommendations.
Exceptions are made for extensions with "standard" recommendations:

- `request.user.ext.digitrust` -- To support Digitrust support
- `request.regs.ext.gdpr` and `request.user.ext.consent` -- To support GDPR
- `request.site.ext.amp` -- To identify AMP as the request source

#### Bid Adjustments

Expand Down Expand Up @@ -254,7 +258,7 @@ For example, a request may return this in `response.ext`
],
"rubicon": [
{
"code": 1,
"code": 1,
"message": "The request exceeded the timeout allocated"
}
]
Expand Down
29 changes: 24 additions & 5 deletions endpoints/openrtb2/amp_auction.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"strings"
"time"

"github.com/buger/jsonparser"
"github.com/golang/glog"
"github.com/julienschmidt/httprouter"
"github.com/mxmCherry/openrtb"
Expand Down Expand Up @@ -280,6 +281,11 @@ func (deps *endpointDeps) loadRequestJSONForAmp(httpRequest *http.Request) (req
return
}

if req.App != nil {
errs = []error{errors.New("request.app must not exist in AMP stored requests.")}
return
}

// Force HTTPS as AMP requires it, but pubs can forget to set it.
if req.Imp[0].Secure == nil {
secure := int8(1)
Expand All @@ -294,6 +300,9 @@ func (deps *endpointDeps) loadRequestJSONForAmp(httpRequest *http.Request) (req
}

func (deps *endpointDeps) overrideWithParams(httpRequest *http.Request, req *openrtb.BidRequest) {
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)
Expand All @@ -311,11 +320,7 @@ func (deps *endpointDeps) overrideWithParams(httpRequest *http.Request, req *ope

canonicalURL := httpRequest.FormValue("curl")
if canonicalURL != "" {
if req.Site == nil {
req.Site = &openrtb.Site{Page: canonicalURL}
} else {
req.Site.Page = canonicalURL
}
req.Site.Page = canonicalURL
// Fixes #683
if parsedURL, err := url.Parse(canonicalURL); err == nil {
domain := parsedURL.Host
Expand All @@ -326,6 +331,8 @@ func (deps *endpointDeps) overrideWithParams(httpRequest *http.Request, req *ope
}
}

setAmpExt(req.Site, "1")

slot := httpRequest.FormValue("slot")
if slot != "" {
req.Imp[0].TagID = slot
Expand Down Expand Up @@ -455,3 +462,15 @@ func defaultRequestExt(req *openrtb.BidRequest) (errs []error) {

return
}

func setAmpExt(site *openrtb.Site, value string) {
if len(site.Ext) > 0 {
if _, dataType, _, _ := jsonparser.Get(site.Ext, "amp"); dataType == jsonparser.NotExist {
if val, err := jsonparser.Set(site.Ext, []byte(value), "amp"); err == nil {
site.Ext = val
}
}
} else {
site.Ext = json.RawMessage(`{"amp":` + value + `}`)
}
}
26 changes: 23 additions & 3 deletions endpoints/openrtb2/amp_auction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,10 @@ func TestGoodAmpRequests(t *testing.T) {
goodRequests := map[string]json.RawMessage{
"1": json.RawMessage(validRequest(t, "aliased-buyeruids.json")),
"2": json.RawMessage(validRequest(t, "aliases.json")),
"3": json.RawMessage(validRequest(t, "app.json")),
"4": json.RawMessage(validRequest(t, "digitrust.json")),
"5": json.RawMessage(validRequest(t, "gdpr-no-consentstring.json")),
"6": json.RawMessage(validRequest(t, "gdpr.json")),
"7": json.RawMessage(validRequest(t, "site.json")),
"8": json.RawMessage(validRequest(t, "timeout.json")),
"9": json.RawMessage(validRequest(t, "user.json")),
}

Expand Down Expand Up @@ -99,6 +97,29 @@ func TestAMPPageInfo(t *testing.T) {
assert.Equal(t, "test.somepage.co.uk", exchange.lastRequest.Site.Domain)
}

func TestAMPSiteExt(t *testing.T) {
stored := map[string]json.RawMessage{
"1": json.RawMessage(validRequest(t, "site.json")),
}
theMetrics := pbsmetrics.NewMetrics(metrics.NewRegistry(), openrtb_ext.BidderList())
exchange := &mockAmpExchange{}
endpoint, _ := NewAmpEndpoint(exchange, newParamsValidator(t), &mockAmpStoredReqFetcher{stored}, &config.Configuration{MaxRequestSize: maxSize}, theMetrics, analyticsConf.NewPBSAnalytics(&config.Analytics{}))
request, err := http.NewRequest("GET", "/openrtb2/auction/amp?tag_id=1", nil)
if !assert.NoError(t, err) {
return
}
recorder := httptest.NewRecorder()
endpoint(recorder, request, nil)

if !assert.NotNil(t, exchange.lastRequest, "Endpoint responded with %d: %s", recorder.Code, recorder.Body.String()) {
return
}
if !assert.NotNil(t, exchange.lastRequest.Site) {
return
}
assert.JSONEq(t, `{"amp":1}`, string(exchange.lastRequest.Site.Ext))
}

// TestBadRequests makes sure we return 400's on bad requests.
func TestAmpBadRequests(t *testing.T) {
files := fetchFiles(t, "sample-requests/invalid-whole")
Expand Down Expand Up @@ -126,7 +147,6 @@ func TestAmpBadRequests(t *testing.T) {
// TestAmpDebug makes sure we get debug information back when requested
func TestAmpDebug(t *testing.T) {
requests := map[string]json.RawMessage{
"1": json.RawMessage(validRequest(t, "app.json")),
"2": json.RawMessage(validRequest(t, "site.json")),
}

Expand Down
15 changes: 14 additions & 1 deletion endpoints/openrtb2/auction.go
Original file line number Diff line number Diff line change
Expand Up @@ -681,9 +681,19 @@ func (deps *endpointDeps) validateAliases(aliases map[string]string) error {
}

func (deps *endpointDeps) validateSite(site *openrtb.Site) error {
if site != nil && site.ID == "" && site.Page == "" {
if site == nil {
return nil
}

if site.ID == "" && site.Page == "" {
return errors.New("request.site should include at least one of request.site.id or request.site.page.")
}
if len(site.Ext) > 0 {
var s openrtb_ext.ExtSite
if err := json.Unmarshal(site.Ext, &s); err != nil {
return err
}
}

return nil
}
Expand Down Expand Up @@ -787,6 +797,9 @@ func setSiteImplicitly(httpReq *http.Request, bidReq *openrtb.BidRequest) {
}
}
}
if bidReq.Site != nil {
setAmpExt(bidReq.Site, "0")
}
}

func setImpsImplicitly(httpReq *http.Request, imps []openrtb.Imp) {
Expand Down
43 changes: 43 additions & 0 deletions endpoints/openrtb2/auction_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,49 @@ func TestTimeoutParser(t *testing.T) {
}
}

func TestImplicitAMPNoExt(t *testing.T) {
httpReq, err := http.NewRequest("POST", "/openrtb2/auction", strings.NewReader(validRequest(t, "site.json")))
if !assert.NoError(t, err) {
return
}

bidReq := openrtb.BidRequest{
Site: &openrtb.Site{},
}
setSiteImplicitly(httpReq, &bidReq)
assert.JSONEq(t, `{"amp":0}`, string(bidReq.Site.Ext))
}

func TestImplicitAMPOtherExt(t *testing.T) {
httpReq, err := http.NewRequest("POST", "/openrtb2/auction", strings.NewReader(validRequest(t, "site.json")))
if !assert.NoError(t, err) {
return
}

bidReq := openrtb.BidRequest{
Site: &openrtb.Site{
Ext: json.RawMessage(`{"other":true}`),
},
}
setSiteImplicitly(httpReq, &bidReq)
assert.JSONEq(t, `{"amp":0,"other":true}`, string(bidReq.Site.Ext))
}

func TestExplicitAMP(t *testing.T) {
httpReq, err := http.NewRequest("POST", "/openrtb2/auction", strings.NewReader(validRequest(t, "site-amp.json")))
if !assert.NoError(t, err) {
return
}

bidReq := openrtb.BidRequest{
Site: &openrtb.Site{
Ext: json.RawMessage(`{"amp":1}`),
},
}
setSiteImplicitly(httpReq, &bidReq)
assert.JSONEq(t, `{"amp":1}`, string(bidReq.Site.Ext))
}

// TestContentType prevents #328
func TestContentType(t *testing.T) {
endpoint, _ := NewEndpoint(
Expand Down
47 changes: 47 additions & 0 deletions endpoints/openrtb2/sample-requests/invalid-whole/site-ext-amp.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
{
"message": "Invalid request: request.site.ext.amp must be either 1, 0, or undefined\n",
"requestPayload": {
"id": "some-request-id",
"site": {
"page": "test.somepage.com",
"ext": {
"amp": 2
}
},
"imp": [
{
"id": "my-imp-id",
"banner": {
"format": [
{
"w": 300,
"h": 600
}
]
},
"pmp": {
"deals": [
{
"id": "some-deal-id"
}
]
},
"ext": {
"appnexus": {
"placementId": 10433394
}
}
}
],
"ext": {
"prebid": {
"targeting": {
"pricegranularity": "low"
},
"cache": {
"bids": {}
}
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
{
"id": "some-request-id",
"site": {
"page": "test.somepage.com",
"ext": {
"amp": 1
}
},
"imp": [
{
"id": "my-imp-id",
"banner": {
"format": [
{
"w": 300,
"h": 600
}
]
},
"pmp": {
"deals": [
{
"id": "some-deal-id"
}
]
},
"ext": {
"appnexus": {
"placementId": 10433394
}
}
}
],
"ext": {
"prebid": {
"targeting": {
"pricegranularity": "low"
},
"cache": {
"bids": {}
}
}
}
}
34 changes: 34 additions & 0 deletions openrtb_ext/site.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package openrtb_ext

import (
"errors"

"github.com/buger/jsonparser"
)

// ExtSite defines the contract for bidrequest.site.ext
type ExtSite struct {
// AMP should be 1 if the request comes from an AMP page, and 0 if not.
AMP int8 `json:"amp"`
}

func (es *ExtSite) UnmarshalJSON(b []byte) error {
if len(b) == 0 {
return errors.New("request.site.ext must have some data in it")
}
if value, dataType, _, _ := jsonparser.Get(b, "amp"); dataType != jsonparser.NotExist && dataType != jsonparser.Number {
return errors.New(`request.site.ext.amp must be either 1, 0, or undefined`)
} else if len(value) != 1 {
return errors.New(`request.site.ext.amp must be either 1, 0, or undefined`)
} else {
switch value[0] {
case byte(48): // 0
es.AMP = 0
case byte(49): // 1
es.AMP = 1
default:
return errors.New(`request.site.ext.amp must be either 1, 0, or undefined`)
}
}
return nil
}
28 changes: 28 additions & 0 deletions openrtb_ext/site_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package openrtb_ext_test

import (
"encoding/json"
"testing"

"github.com/prebid/prebid-server/openrtb_ext"
"github.com/stretchr/testify/assert"
)

func TestInvalidSiteExt(t *testing.T) {
var s openrtb_ext.ExtSite
assert.EqualError(t, json.Unmarshal([]byte(`{"amp":-1}`), &s), "request.site.ext.amp must be either 1, 0, or undefined")
assert.EqualError(t, json.Unmarshal([]byte(`{"amp":2}`), &s), "request.site.ext.amp must be either 1, 0, or undefined")
assert.EqualError(t, json.Unmarshal([]byte(`{"amp":true}`), &s), "request.site.ext.amp must be either 1, 0, or undefined")
assert.EqualError(t, json.Unmarshal([]byte(`{"amp":null}`), &s), "request.site.ext.amp must be either 1, 0, or undefined")
assert.EqualError(t, json.Unmarshal([]byte(`{"amp":"1"}`), &s), "request.site.ext.amp must be either 1, 0, or undefined")
}

func TestValidSiteExt(t *testing.T) {
var s openrtb_ext.ExtSite
assert.NoError(t, json.Unmarshal([]byte(`{"amp":0}`), &s))
assert.EqualValues(t, 0, s.AMP)
assert.NoError(t, json.Unmarshal([]byte(`{"amp":1}`), &s))
assert.EqualValues(t, 1, s.AMP)
assert.NoError(t, json.Unmarshal([]byte(`{"amp": 1 }`), &s))
assert.EqualValues(t, 1, s.AMP)
}