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

Jump to bottom

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 3 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
25 changes: 25 additions & 0 deletions endpoints/openrtb2/auction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1497,6 +1497,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 +1521,26 @@ func setAuctionTypeImplicitly(r *openrtb_ext.RequestWrapper) {
}
}

func setGPCImplicitly(httpReq *http.Request, r *openrtb_ext.RequestWrapper) error {
secGPC := httpReq.Header.Get("Sec-GPC")
przemkaczmarek marked this conversation as resolved.
Show resolved Hide resolved
fmt.Printf("Sec-GPC Header: %s\n", secGPC)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove print statement


if secGPC != "1" {
return nil
}

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

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

fmt.Printf("GPC set to: %s\n", *regExt.GetGPC())

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
44 changes: 44 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,50 @@ func TestValidateOrFillCookieDeprecation(t *testing.T) {
}
}

func TestSetGPCImplicitly(t *testing.T) {
testCases := []struct {
description string
headerValue string
przemkaczmarek marked this conversation as resolved.
Show resolved Hide resolved
expectedGPC string
expectError bool
}{
{
description: "Sec-GPC header set to '1', GPC should be set to '1'",
przemkaczmarek marked this conversation as resolved.
Show resolved Hide resolved
headerValue: "1",
expectedGPC: "1",
expectError: false,
},
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,
	},
},

}

for _, test := range testCases {
t.Run(test.description, func(t *testing.T) {
httpReq := &http.Request{
Header: http.Header{
"Sec-GPC": []string{test.headerValue},
przemkaczmarek marked this conversation as resolved.
Show resolved Hide resolved
},
}

r := &openrtb_ext.RequestWrapper{
BidRequest: &openrtb2.BidRequest{}, // what I should add here???
przemkaczmarek marked this conversation as resolved.
Show resolved Hide resolved
}

err := setGPCImplicitly(httpReq, r)
if (err != nil) != test.expectError {
t.Errorf("Unexpected error state: got %v, expected error: %v", err, test.expectError)
}
przemkaczmarek marked this conversation as resolved.
Show resolved Hide resolved

regExt, _ := r.GetRegExt()
gpcValue := regExt.GetGPC()

if gpcValue != nil && *gpcValue != test.expectedGPC {
t.Errorf("Expected GPC %v, but got %v", test.expectedGPC, *gpcValue)
} else if gpcValue == nil && test.expectedGPC != "" {
t.Errorf("Expected GPC to be set, but it was nil")
}
przemkaczmarek marked this conversation as resolved.
Show resolved Hide resolved
})
}
}

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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: GPC variable name should be gpc in accordance with golang best practices

}

func (re *RegExt) SetGPC(GPC *string) {
re.gpc = GPC
przemkaczmarek marked this conversation as resolved.
Show resolved Hide resolved
re.gdprDirty = true
}

func (re *RegExt) GetUSPrivacy() string {
uSPrivacy := re.usPrivacy
return uSPrivacy
Expand Down
Loading