-
Notifications
You must be signed in to change notification settings - Fork 731
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
GPC: Set extension based on header #3895
Conversation
request_wrapper.go 2748
endpoints/openrtb2/auction.go
Outdated
dntEnabled int8 = 1 | ||
notAmp int8 = 0 | ||
dntKey string = http.CanonicalHeaderKey("DNT") | ||
secGPCHdrKey string = http.CanonicalHeaderKey("Sec-GPC") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: secGPCKey
suffices
endpoints/openrtb2/auction_test.go
Outdated
expectError bool | ||
description string | ||
headerValue string | ||
expectedGPC string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can remove this because expectedRegs
should contain the expected GPC value.
endpoints/openrtb2/auction.go
Outdated
@@ -1516,6 +1522,25 @@ func setAuctionTypeImplicitly(r *openrtb_ext.RequestWrapper) { | |||
} | |||
} | |||
|
|||
func setGPCImplicitly(httpReq *http.Request, r *openrtb_ext.RequestWrapper) error { | |||
secGPC := httpReq.Header.Get(secGPCHdrKey) | |||
fmt.Printf("Sec-GPC Header: %s\n", secGPC) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove print statement
header string | ||
expectedGPC string | ||
expectError bool | ||
regs *openrtb2.Regs |
There was a problem hiding this comment.
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.
openrtb_ext/request_wrapper.go
Outdated
GPC := *re.gpc | ||
return &GPC |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
||
gpc := "1" | ||
regExt.SetGPC(&gpc) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
}, | ||
}, | ||
{ | ||
description: "sec_gpc_header_not_set_gpc_should_not_be_modified", |
There was a problem hiding this comment.
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
expectedRegs: &openrtb2.Regs{ | ||
Ext: []byte(`{"gpc":"1"}`), | ||
}, | ||
}, |
There was a problem hiding this comment.
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,
},
},
endpoints/openrtb2/auction_test.go
Outdated
assert.NoError(t, err) | ||
} | ||
assert.NoError(t, r.RebuildRequest()) | ||
assert.JSONEq(t, string(test.expectedRegs.Ext), string(r.BidRequest.Regs.Ext)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to handle the two new test cases I requested you can modify this as such:
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))
}
There was a problem hiding this comment.
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.
@@ -2253,6 +2254,21 @@ func TestRegExtUnmarshal(t *testing.T) { | |||
expectGDPR: ptrutil.ToPtr[int8](0), | |||
expectError: true, | |||
}, | |||
// GPC |
There was a problem hiding this comment.
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:
- Update
TestRebuildRegExt
, which will cover yourRegExt
marshal
changes - Add a new test
TestRegExtGetGPCSetGPC
that is similar toTestRegExtGetGDPRSetGDPR
@bsardo Should I accept ? or Do You add another reviewer? |
We need two approvals to merge. I'll see if @VeronikaSolovei9 can look at it today. |
Implements #2748