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.

Sign up for GitHub

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

GPC: Set extension based on header #3895

Merged
merged 10 commits into from
Sep 25, 2024
Merged

GPC: Set extension based on header #3895

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8971a36
Add GPC extension handling to exchange.go and
przemkaczmarek Aug 31, 2024
58aab7c
example
przemkaczmarek Sep 4, 2024
b7692d7
example
przemkaczmarek Sep 4, 2024
a2b7cf1
fix bugs
przemkaczmarek Sep 5, 2024
c7fbfe1
fix comments
przemkaczmarek Sep 6, 2024
f713ff1
fix comments
przemkaczmarek Sep 11, 2024
5fba3db
fix regs_nil_and_header_not_set test
przemkaczmarek Sep 12, 2024
f8e7977
checking validation
przemkaczmarek Sep 12, 2024
b5acce3
fix comments
przemkaczmarek Sep 16, 2024
bff49e2
additional test in request_wrapper_test
przemkaczmarek Sep 17, 2024
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
28 changes: 28 additions & 0 deletions endpoints/openrtb2/auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ const observeBrowsingTopicsValue = "?1"

var (
dntKey string = http.CanonicalHeaderKey("DNT")
secGPCKey string = http.CanonicalHeaderKey("Sec-GPC")
dntDisabled int8 = 0
dntEnabled int8 = 1
notAmp int8 = 0
Expand Down Expand Up @@ -1497,6 +1498,11 @@ func (deps *endpointDeps) setFieldsImplicitly(httpReq *http.Request, r *openrtb_

setAuctionTypeImplicitly(r)

err := setGPCImplicitly(httpReq, r)
if err != nil {
return []error{err}
}

errs := setSecBrowsingTopicsImplicitly(httpReq, r, account)
return errs
}
Expand All @@ -1516,6 +1522,28 @@ func setAuctionTypeImplicitly(r *openrtb_ext.RequestWrapper) {
}
}

func setGPCImplicitly(httpReq *http.Request, r *openrtb_ext.RequestWrapper) error {
secGPC := httpReq.Header.Get(secGPCKey)

if secGPC != "1" {
return nil
}

regExt, err := r.GetRegExt()
if err != nil {
return err
}

if regExt.GetGPC() != nil {
return nil
}

gpc := "1"
regExt.SetGPC(&gpc)
Comment on lines +1540 to +1542
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest adding the following early return just before the set:

if regExt.GetGPC() != nil {
    return nil
}

This way we won't perform an unnecessary write which sets the dirty flag and causes extra work when rebuilding the request downstream.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @przemkaczmarek, here is a super nitpick to this function.
In the issue description the logic is straightforward:

1. If the incoming request already contains regs.ext.gpc, use that.
2. Otherwise, if there's an HTTP header for Sec-GPC with a value of "1" or 1, set regs.ext.gpc to "1".

The code here does the right thing, however it's a little confusing and difficult to read. I had to read it couple of times to understand it.
Here is a suggestion that I think traces better to the requirements and might be better for readability:

func setGPCImplicitly(httpReq *http.Request, r *openrtb_ext.RequestWrapper) error {

	regExt, err := r.GetRegExt()
	if err != nil {
		return err
	}

	if regExt.GetGPC() != nil {
		return nil
	}

	secGPC := httpReq.Header.Get(secGPCKey)

	if secGPC == "1" {
		gpc := "1"
		regExt.SetGPC(&gpc)
	}

	return nil
}

The unit test passes as is.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In my version of the code, I first check the value of secGPC, and if it doesn't meet the condition, the function ends immediately with return nil. In your version of the code, this happens later, which leads to unnecessary retrieval of regExt, even when it's not needed. I avoid unnecessary operations. I avoid performing costly operations (such as r.GetRegExt()) if the secGPC header does not have the value '1'. In the first version, you retrieve the regExt extension regardless of the secGPC value, which is less optimal

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I understand this. I am only concerned about the readability.
Also this is a minor performance optimization (but still an optimization!) so it's up to you to change it.
I approve this PR, and if you decide to change it - I'll re-approve it right away too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer to keep the code as is but thanks for the comment


return nil
}

// setSecBrowsingTopicsImplicitly updates user.data with data from request header 'Sec-Browsing-Topics'
func setSecBrowsingTopicsImplicitly(httpReq *http.Request, r *openrtb_ext.RequestWrapper, account *config.Account) []error {
secBrowsingTopics := httpReq.Header.Get(secBrowsingTopics)
Expand Down
169 changes: 169 additions & 0 deletions endpoints/openrtb2/auction_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5618,6 +5618,175 @@ func TestValidateOrFillCookieDeprecation(t *testing.T) {
}
}

func TestSetGPCImplicitly(t *testing.T) {
testCases := []struct {
description string
header string
regs *openrtb2.Regs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: move regs up a line so all of the expected fields are at the end of the struct definition. Please also do this in the actual test cases.

expectError bool
expectedRegs *openrtb2.Regs
}{
{
description: "regs_ext_gpc_not_set_and_header_is_1",
header: "1",
regs: &openrtb2.Regs{
Ext: []byte(`{}`),
},
expectError: false,
expectedRegs: &openrtb2.Regs{
Ext: []byte(`{"gpc":"1"}`),
},
},
{
description: "sec_gpc_header_not_set_gpc_should_not_be_modified",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest covering the following test cases:
regs_ext_gpc_not_set_and_header_not_set
regs_ext_gpc_not_set_and_header_is_1
regs_ext_gpc_not_set_and_header_not_1
regs_ext_gpc_is_1_and_header_is_1
regs_ext_gpc_is_1_and_header_not_1
regs_ext_other_data_and_header_is_1
regs_ext_nil_and_header_is_1
regs_nil_and_header_is_1

header: "",
regs: &openrtb2.Regs{
Ext: []byte(`{}`),
},
expectError: false,
expectedRegs: &openrtb2.Regs{
Ext: []byte(`{}`),
},
},
{
description: "sec_gpc_header_set_to_2_gpc_should_not_be_modified",
header: "2",
regs: &openrtb2.Regs{
Ext: []byte(`{}`),
},
expectError: false,
expectedRegs: &openrtb2.Regs{
Ext: []byte(`{}`),
},
},
{
description: "sec_gpc_header_set_to_1_and_regs_ext_contains_other_data",
header: "1",
regs: &openrtb2.Regs{
Ext: []byte(`{"some_other_field":"some_value"}`),
},
expectError: false,
expectedRegs: &openrtb2.Regs{
Ext: []byte(`{"some_other_field":"some_value","gpc":"1"}`),
},
},
{
description: "regs_ext_gpc_not_set_and_header_not_set",
header: "",
regs: &openrtb2.Regs{
Ext: []byte(`{}`),
},
expectError: false,
expectedRegs: &openrtb2.Regs{
Ext: []byte(`{}`),
},
},
{
description: "regs_ext_gpc_not_set_and_header_not_1",
header: "0",
regs: &openrtb2.Regs{
Ext: []byte(`{}`),
},
expectError: false,
expectedRegs: &openrtb2.Regs{
Ext: []byte(`{}`),
},
},
{
description: "regs_ext_gpc_is_1_and_header_is_1",
header: "1",
regs: &openrtb2.Regs{
Ext: []byte(`{"gpc":"1"}`),
},
expectError: false,
expectedRegs: &openrtb2.Regs{
Ext: []byte(`{"gpc":"1"}`),
},
},
{
description: "regs_ext_gpc_is_1_and_header_not_1",
header: "0",
regs: &openrtb2.Regs{
Ext: []byte(`{"gpc":"1"}`),
},
expectError: false,
expectedRegs: &openrtb2.Regs{
Ext: []byte(`{"gpc":"1"}`),
},
},
{
description: "regs_ext_other_data_and_header_is_1",
header: "1",
regs: &openrtb2.Regs{
Ext: []byte(`{"other":"value"}`),
},
expectError: false,
expectedRegs: &openrtb2.Regs{
Ext: []byte(`{"other":"value","gpc":"1"}`),
},
},
{
description: "regs_nil_and_header_is_1",
header: "1",
regs: nil,
expectError: false,
expectedRegs: &openrtb2.Regs{
Ext: []byte(`{"gpc":"1"}`),
},
},
Copy link
Collaborator

@bsardo bsardo Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we need to add these two test cases to ensure that we don't create a regs or regs.ext object if we're not writing to the gpc field:

{
	description: "regs_nil_and_header_not_set",
	header:      "",
	regs:        nil,
	expectError: false,
	expectedRegs: nil,
},
{
	description: "regs_ext_is_nil_and_header_not_set",
	header:      "",
	regs:        &openrtb2.Regs{
		Ext: nil,
	},
	expectError: false,
	expectedRegs: &openrtb2.Regs{
		Ext: nil,
	},
},

{
description: "regs_nil_and_header_not_set",
header: "",
regs: nil,
expectError: false,
expectedRegs: nil,
},
{
description: "regs_ext_is_nil_and_header_not_set",
header: "",
regs: &openrtb2.Regs{
Ext: nil,
},
expectError: false,
expectedRegs: &openrtb2.Regs{
Ext: nil,
},
},
}

for _, test := range testCases {
t.Run(test.description, func(t *testing.T) {
httpReq := &http.Request{
Header: http.Header{
http.CanonicalHeaderKey("Sec-GPC"): []string{test.header},
},
}

r := &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{
Regs: test.regs,
},
}

err := setGPCImplicitly(httpReq, r)

if test.expectError {
assert.Error(t, err)
} else {
assert.NoError(t, err)
}
assert.NoError(t, r.RebuildRequest())
if test.expectedRegs == nil {
assert.Nil(t, r.BidRequest.Regs)
} else if test.expectedRegs.Ext == nil {
assert.Nil(t, r.BidRequest.Regs.Ext)
} else {
assert.JSONEq(t, string(test.expectedRegs.Ext), string(r.BidRequest.Regs.Ext))
}
})
}
}

func TestValidateRequestCookieDeprecation(t *testing.T) {
testCases :=
[]struct {
Expand Down
38 changes: 37 additions & 1 deletion openrtb_ext/request_wrapper.go
View file
Open in desktop
Copy link
Collaborator

Choose a reason for hiding this comment

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

The code looks good. Please update openrtb_ext/request_wrapper_test.go with test coverage for all of your changes in this file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if you missed this during your last iteration but please add/update the wrapper tests to cover changes.

Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ const (
schainKey = "schain"
us_privacyKey = "us_privacy"
cdepKey = "cdep"
gpcKey = "gpc"
)

// LenImp returns the number of impressions without causing the creation of ImpWrapper objects.
Expand Down Expand Up @@ -1201,6 +1202,8 @@ type RegExt struct {
dsaDirty bool
gdpr *int8
gdprDirty bool
gpc *string
gpcDirty bool
usPrivacy string
usPrivacyDirty bool
}
Expand Down Expand Up @@ -1244,6 +1247,13 @@ func (re *RegExt) unmarshal(extJson json.RawMessage) error {
}
}

gpcJson, hasGPC := re.ext[gpcKey]
if hasGPC && gpcJson != nil {
if err := jsonutil.Unmarshal(gpcJson, &re.gpc); err != nil {
return err
}
}

return nil
}

Expand Down Expand Up @@ -1287,6 +1297,19 @@ func (re *RegExt) marshal() (json.RawMessage, error) {
re.usPrivacyDirty = false
}

if re.gpcDirty {
if re.gpc != nil {
rawjson, err := jsonutil.Marshal(re.gpc)
if err != nil {
return nil, err
}
re.ext[gpcKey] = rawjson
} else {
delete(re.ext, gpcKey)
}
re.gpcDirty = false
}

re.extDirty = false
if len(re.ext) == 0 {
return nil, nil
Expand All @@ -1295,7 +1318,7 @@ func (re *RegExt) marshal() (json.RawMessage, error) {
}

func (re *RegExt) Dirty() bool {
return re.extDirty || re.dsaDirty || re.gdprDirty || re.usPrivacyDirty
return re.extDirty || re.dsaDirty || re.gdprDirty || re.usPrivacyDirty || re.gpcDirty
}

func (re *RegExt) GetExt() map[string]json.RawMessage {
Expand Down Expand Up @@ -1337,6 +1360,19 @@ func (re *RegExt) SetGDPR(gdpr *int8) {
re.gdprDirty = true
}

func (re *RegExt) GetGPC() *string {
if re.gpc == nil {
return nil
}
gpc := *re.gpc
return &gpc
}

func (re *RegExt) SetGPC(gpc *string) {
re.gpc = gpc
re.gpcDirty = true
}

func (re *RegExt) GetUSPrivacy() string {
uSPrivacy := re.usPrivacy
return uSPrivacy
Expand Down
16 changes: 16 additions & 0 deletions openrtb_ext/request_wrapper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2194,6 +2194,7 @@ func TestRegExtUnmarshal(t *testing.T) {
extJson json.RawMessage
expectDSA *ExtRegsDSA
expectGDPR *int8
expectGPC *string
expectUSPrivacy string
expectError bool
}{
Expand Down Expand Up @@ -2253,6 +2254,21 @@ func TestRegExtUnmarshal(t *testing.T) {
expectGDPR: ptrutil.ToPtr[int8](0),
expectError: true,
},
// GPC
Copy link
Collaborator

Choose a reason for hiding this comment

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

These tests look good as they adequately cover your changes to the RegExt unmarshal function. However, there are still other request wrapper changes that require test coverage:

  1. Update TestRebuildRegExt, which will cover your RegExt marshal changes
  2. Add a new test TestRegExtGetGPCSetGPC that is similar to TestRegExtGetGDPRSetGDPR

{
name: "valid_gpc_json",
regExt: &RegExt{},
extJson: json.RawMessage(`{"gpc":"some_value"}`),
expectGPC: ptrutil.ToPtr("some_value"),
expectError: false,
},
{
name: "malformed_gpc_json",
regExt: &RegExt{},
extJson: json.RawMessage(`{"gpc":nill}`),
expectGPC: nil,
expectError: true,
},
// us_privacy
{
name: "valid_usprivacy_json",
Expand Down
Loading