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

Refactor AMP Param Parsing #1627

Merged
merged 3 commits into from
Jan 12, 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
110 changes: 110 additions & 0 deletions amp/parse.go
Original file line number Diff line number Diff line change
@@ -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 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")),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need it to be a pointer? Not saying it is wrong, I just noticed it wasn't a pointer before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using a pointer to distinguish if a timeout value was provided and valid versus not provided and/or invalid. Previously, the parse method was in-line with its usage and we checked for a parse error immediately. Now there's the need to communicate a parse error differently. Go doesn't have many constructs for doing so, sadly.

}
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
}
168 changes: 168 additions & 0 deletions amp/parse_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
Loading