GPC: Set extension based on header

[przemkaczmarek]

Implements #2748

[bsardo]

Nitpick: secGPCKey suffices

[bsardo]

You can remove this because expectedRegs should contain the expected GPC value.

[bsardo]

Remove print statement

[bsardo]

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.

[bsardo]

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

[bsardo]

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

[bsardo]

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

[bsardo]

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.

[VeronikaSolovei9]

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?

[przemkaczmarek]

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

[VeronikaSolovei9]

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.

[przemkaczmarek]

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

[bsardo]

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

[bsardo]

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,
	},
},

[bsardo]

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))
}

[bsardo]

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

[bsardo]

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

[przemkaczmarek]

@bsardo Should I accept ? or Do You add another reviewer?

[bsardo]

@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.