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

DSA: Inject default in bid requests if not present #3540

Merged
merged 21 commits into from
Apr 9, 2024
Merged

DSA: Inject default in bid requests if not present #3540

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
23eca73
Add account config
bsardo Feb 24, 2024
9cddf2b
Add DSA writer to insert default DSA in requests
bsardo Feb 24, 2024
e0c174e
Use DSA writer to insert default into each bidder request
bsardo Feb 25, 2024
a424d16
Add DSA default JSON test with test framework update
bsardo Feb 25, 2024
3d0a754
Insert default into request before splitting into bidder requests
bsardo Feb 25, 2024
ea58098
Change DataToPub to a pointer
bsardo Feb 27, 2024
2da4088
Rename dsawriter.go to writer.go
bsardo Feb 27, 2024
1bc45e6
Update variable name
bsardo Feb 27, 2024
3828209
Address feedback
bsardo Feb 28, 2024
d2ec47e
Update host config to accept DSA default as stringified JSON and map …
bsardo Mar 1, 2024
94857a3
Update exchange JSON test to accept stringified JSON in account config
bsardo Mar 1, 2024
e202460
Convert UnpackDSADefault from receiver method to function for use in …
bsardo Mar 1, 2024
c6c0d6f
Update account fetching to validate default DSA and unpack to struct
bsardo Mar 1, 2024
2ee9652
Update DSA writer to use unpacked default
bsardo Mar 1, 2024
8c29cb1
Handle error returned from RebuildRequest
bsardo Apr 2, 2024
ff93f52
Merge branch 'master' into DSA-3424
bsardo Apr 2, 2024
5729ab7
Group related conditions in Write
bsardo Apr 3, 2024
bdfca44
Feedback: error msg, env vars, minor refactor
bsardo Apr 4, 2024
f216c82
Clone DSA default unpacked and set on request
bsardo Apr 4, 2024
bcf23f3
Add integration test where default is ignored due to DSA on request
bsardo Apr 4, 2024
59309eb
Return error from RebuildRequest call
bsardo Apr 8, 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
Prev Previous commit
Next Next commit
Group related conditions in Write
  • Loading branch information
@bsardo
bsardo committed Apr 3, 2024
commit 5729ab79855873c8db13ebe871743f29b61fb4f9
5 changes: 1 addition & 4 deletions dsa/writer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,7 @@ type Writer struct {
// Write sets the default DSA object on the request at regs.ext.dsa if it is
// defined in the account config and it is not already present on the request
func (dw Writer) Write(req *openrtb_ext.RequestWrapper) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if this is a stronger version of the function, but here's a version where we combine all of the conditions that result in us returning nil, rather than breaking them all up:

func (dw Writer) Write(req *openrtb_ext.RequestWrapper) error {
	if req == nil || getReqDSA(req) != nil || dw.Config == nil || dw.Config.DefaultUnpacked == nil ||
		(dw.Config.GDPROnly && !dw.GDPRInScope) {
		return nil
	}

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

	regExt.SetDSA(dw.Config.DefaultUnpacked)
	return nil
}

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.

I think this is really a matter of preference. I see your perspective that there are multiple conditionals that result in the same return value, though my tendency is to break complex conditionals into logical groupings because I think it is easier to digest and instills confidence in the intended behavior. I also think it is easier to scan code vertically. In that case, at a minimum the first two conditionals could be grouped as such: if req == nil || getReqDSA(req) != nil {.
@SyntaxNode do you feel strongly either way here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I'm thinking your latest update looks great

if req == nil {
return nil
}
if getReqDSA(req) != nil {
if req == nil || getReqDSA(req) != nil {
return nil
}
if dw.Config == nil || dw.Config.DefaultUnpacked == nil {
Expand Down
5 changes: 4 additions & 1 deletion dsa/writer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ func TestWrite(t *testing.T) {
expectRequest *openrtb_ext.RequestWrapper
}{
{
name: "request_nil",
name: "request_nil",
giveConfig: &config.AccountDSA{
DefaultUnpacked: defaultDSA,
},
giveRequest: nil,
expectRequest: nil,
},
Expand Down
Loading