Update to OpenRTB 2.6-202309 #3387

SyntaxNode · 2024-01-05T17:42:09Z

Updates Prebid Server to use the latest release of the Prebid OpenRTB library.

Change log: https://github.com/prebid/openrtb/releases/tag/v20.1.0

Request Reviews from Affected Adapters:

- 33across: @thomas-33across @curlyblueeagle
- beachfront: @muncha
- cadent_aperture_mx @EMXDigital
- eplanning @matimar @Nistenf @fndigrazia @chino117
- gumgum @cchang320 @susyt @gg-natalia
- huaweiads @wwwyyy @ahmetfaruk59 @clho40
- madvertise @notmani
- richaudience @richaudience
- smartadserver @tadam75 @tadam

adapters/33across/33across.go

adapters/beachfront/beachfront.go

SyntaxNode · 2024-01-05T17:47:25Z

adapters/cadent_aperture_mx/cadentaperturemx.go

@@ -147,7 +147,7 @@ func buildImpVideo(imp *openrtb2.Imp) error {
 		}
 	}

-	if imp.Video.H == 0 && imp.Video.W == 0 {
+	if (imp.Video.H == nil || *imp.Video.H == 0) && (imp.Video.W == nil || *imp.Video.W == 0) {


Added nil checks before accessing the W and H fields.

Cadent: Should we remove the checks for 0 value now that nil is checked? .. aka, are you ok to receive a 0 value if it's explicitly provided in the request?

adapters/eplanning/eplanning.go

SyntaxNode · 2024-01-05T17:52:22Z

adapters/gumgum/gumgum.go

@@ -221,7 +221,7 @@ func getMediaTypeForImpID(impID string, imps []openrtb2.Imp) openrtb_ext.BidType
 }

 func validateVideoParams(video *openrtb2.Video) (err error) {
-	if video.W == 0 || video.H == 0 || video.MinDuration == 0 || video.MaxDuration == 0 || video.Placement == 0 || video.Linearity == 0 {
+	if video.W == nil || *video.W == 0 || video.H == nil || *video.H == 0 || video.MinDuration == 0 || video.MaxDuration == 0 || video.Placement == 0 || video.Linearity == 0 {


Added nil checks before accessing the W and H fields.

Gumgum: Should a 0 value be considered an "invalid or missing" field now that it can represent nil if it's missing?

SyntaxNode · 2024-01-05T18:17:23Z

firstpartydata/first_party_data_test.go

-      "c": 3
-    }
-  }
-}


Removed unused variable.

SyntaxNode · 2024-01-05T18:18:11Z

openrtb_ext/response.go

@@ -103,9 +103,9 @@ const (
 )

 // NonBidObject is subset of Bid object with exact json signature
-// defined at https://github.com/prebid/openrtb/blob/v19.0.0/openrtb2/bid.go


Removed url, since it will be hard to keep current.

SyntaxNode · 2024-01-05T18:19:18Z

ortb/clone.go

Added new pointer fields to existing Clone functions for OpenRTB objects. Cleaned up the BidderReq clone to now involve the request wrapper (this file is just for openrtb objects) and to make it's partial support clear. I'm guessing it's partial because that all we've needed so far?

SyntaxNode · 2024-01-05T18:19:50Z

ortb/clone.go

-	if req.User != nil {
-		userCopy := CloneUser(req.User)
-		newReq.User = userCopy
-	}


Removed duplicate nil checks.

SyntaxNode · 2024-01-05T18:20:16Z

ortb/clone_test.go

-		assert.Equal(t, given, result, "equality")
-		assert.NotSame(t, given, result, "pointer")
-	})
-}


Replaced with ptrutil.

SyntaxNode · 2024-01-05T21:11:18Z

adapters/beachfront/beachfront.go

+			videoCopy := *imp.Video
+			videoCopy.W = ptrutil.ToPtr[int64](defaultVideoWidth)
+			videoCopy.H = ptrutil.ToPtr[int64](defaultVideoHeight)
+			imp.Video = &videoCopy


Beachfront: Fixed unprotected shared memory write. This code path previously had no test coverage.

SyntaxNode · 2024-01-05T21:11:43Z

adapters/beachfront/beachfronttest/supplemental/adm-video-no-h.json

+                "mimes": [
+                  "video/mp4"
+                ],
+                "w": 300


Beachfront: What behavior would you like if one of the w or h values are nil / missing?

SyntaxNode · 2024-01-05T21:20:23Z

adapters/cadent_aperture_mx/cadent_aperture_mxtest/supplemental/bad-imp-video-missing-w.json

+            5,
+            6
+          ],
+          "h": 480


Cadent: What should the behavior be if just one size dimension is missing? Previous behavior would set the other one to 0.

adapters/huaweiads/huaweiads.go

muncha · 2024-01-08T12:39:42Z

@SyntaxNode
I am no longer the maintainer for Beachfront's adapter, nor associated with them. An email to [email protected] should get to the right people to have a look at this and update their maintainer info. It was a pleasure working on this project. Rock on.

adapters/audienceNetwork/facebook.go

adapters/beachfront/beachfront.go

AlexBVolcy · 2024-01-09T19:38:35Z

openrtb_ext/convert_down.go

+func clear202309Fields(r *RequestWrapper) {
+	r.ACat = nil
+
+	for _, imp := range r.GetImp() {
+		if audio := imp.Audio; audio != nil {
+			audio.DurFloors = nil
+		}
+
+		if video := imp.Video; video != nil {
+			video.DurFloors = nil
+		}
+
+		if pmp := imp.PMP; pmp != nil {
+			for i := range pmp.Deals {
+				pmp.Deals[i].Guar = 0
+				pmp.Deals[i].MinCPMPerSec = 0
+				pmp.Deals[i].DurFloors = nil
+			}
+		}
+	}
+}


I think this function can be simplified slightly where we nil check the imp members directly instead of assigning them to a variable first.

func clear202309Fields(r *RequestWrapper) { r.ACat = nil for _, imp := range r.GetImp() { if imp.Audio != nil { imp.Audio.DurFloors = nil } if imp.Video != nil { imp.Video.DurFloors = nil } if imp.PMP != nil { for i := range imp.PMP.Deals { imp.PMP.Deals[i].Guar = 0 imp.PMP.Deals[i].MinCPMPerSec = 0 imp.PMP.Deals[i].DurFloors = nil } } } }

I had earlier versions of code written that way. I found that when the hierarchy gets deep the code is harder to follow. This is more evident in clear26Fields. It might also be more efficient since the variable is only resolved once and reused - although admittedly the compiler may correct for that on its own.

Do you find the way you suggested to be easier to read?

I do find my suggestion easier to read for me, but I have no issue keeping the function as it is.

adapters/huaweiads/huaweiads.go

SyntaxNode · 2024-01-19T18:21:32Z

Thank you 33across, beachfront, eplanning, and huaweiads for reviewing this PR.

I did not hear from cadent_aperture, eplanning, gumgum, madvertise, richaudience, or smartadserver after including GitHub mentions and contacting their support email address. We'll proceed with this PR using our best judgement for the non response adapters.

guscarreon

When greping github.com\/prebid\/openrtb\/v19 two adapters that are still referencing v19 come up. They were probably merged recently:

$  grep -RnH 'github.com\/prebid\/openrtb\/v19' *                                                                                                                                                                                                                                             1 ↵
adapters/zmaticoo/zmaticoo.go:6:        "github.com/prebid/openrtb/v19/openrtb2"
adapters/minutemedia/minutemedia.go:11: "github.com/prebid/openrtb/v19/openrtb2"

Can we please update?

guscarreon · 2024-01-23T21:11:33Z

adapters/richaudience/richaudience.go

-					seatBid.Bid[i].W = reqBid.Video.W
-					seatBid.Bid[i].H = reqBid.Video.H
+					seatBid.Bid[i].W = ptrutil.ValueOrDefault(reqBid.Video.W)
+					seatBid.Bid[i].H = ptrutil.ValueOrDefault(reqBid.Video.H)


Nitpick: Line 110 above indicates that nil and 0 values for W and H are filtered out. Should we simply dereference the pointer that we know is valid as a small optimization?

181 182 if bidType == "video" { 183 - seatBid.Bid[i].W = ptrutil.ValueOrDefault(reqBid.Video.W) + seatBid.Bid[i].W = *reqBid.Video.W 184 - seatBid.Bid[i].H = ptrutil.ValueOrDefault(reqBid.Video.H) + seatBid.Bid[i].H = *reqBid.Video.H 185 } 186 adapters/richaudience/richaudience.go

adapters/audienceNetwork/facebook.go

guscarreon · 2024-01-25T01:39:10Z

adapters/beachfront/beachfront.go

-			bids[i].H = xtrnal.Imp[i].Video.H
-			bids[i].W = xtrnal.Imp[i].Video.W
+			bids[i].H = ptrutil.ValueOrDefault(xtrnal.Imp[i].Video.H)
+			bids[i].W = ptrutil.ValueOrDefault(xtrnal.Imp[i].Video.W)


Function ValueOrDefault[T any](v *T) might return 0 because that's the default value of the int64 type, but lines 479 to 494 above imply that defaultVideoWidth and defaultVideoHeight are prefered as default values instead. Should we modify to default to those constants?

682 for i := 0; i < len(bids); i++ { 683 crid := extractNurlVideoCrid(bids[i].NURL) 684 685 bids[i].CrID = crid 686 bids[i].ImpID = xtrnal.Imp[i].ID 687 - bids[i].H = ptrutil.ValueOrDefault(xtrnal.Imp[i].Video.H) 688 - bids[i].W = ptrutil.ValueOrDefault(xtrnal.Imp[i].Video.W) + // I know there's no ternary operator in Go. Wish there was + bids[i].H = (xtrnal.Imp[i].Video.H != nil && *xtrnal.Imp[i].Video.H > 0) ? *xtrnal.Imp[i].Video.H : defaultVideoHeight + bids[i].W = (xtrnal.Imp[i].Video.W != nil && *xtrnal.Imp[i].Video.W > 0) ? *xtrnal.Imp[i].Video.W : defaultVideoWidth 689 bids[i].ID = fmt.Sprintf("%sNurlVideo", xtrnal.Imp[i].ID) 690 } 691 adapters/beachfront/beachfront.go

Or, do lines 479 to 494 modified above, guarantee that xtrnal.Imp[i].Video.H and xtrnal.Imp[i].Video.W are already always set to defaultVideoWidth and defaultVideoHeight respectively and we just simply need to dereference here as we did in the RichAudience adapter? I honestly don't know this adapter's code enough to be perfectly sure.

682 for i := 0; i < len(bids); i++ { 683 crid := extractNurlVideoCrid(bids[i].NURL) 684 685 bids[i].CrID = crid 686 bids[i].ImpID = xtrnal.Imp[i].ID 687 - bids[i].H = ptrutil.ValueOrDefault(xtrnal.Imp[i].Video.H) + bids[i].H = *xtrnal.Imp[i].Video.H 688 - bids[i].W = ptrutil.ValueOrDefault(xtrnal.Imp[i].Video.W) + bids[i].W = *xtrnal.Imp[i].Video.W 689 bids[i].ID = fmt.Sprintf("%sNurlVideo", xtrnal.Imp[i].ID) 690 } 691 adapters/beachfront/beachfront.go

Any thoughts from the adapter's owners?

This code is in MakeBids so, yes, the default value substitution should already have occurred during MakeRequests. The Beachfront has more logic than most, so if it's hard to determine if these values are guaranteed to not be nil here, perhaps a little extra protection isn't bad.

Beachfront has reviewed and approved the changes in this PR.

guscarreon · 2024-01-25T01:48:08Z

adapters/beachfront/beachfronttest/supplemental/adm-video-partial-size.json

+                  ],
+                  "cid": "277",
+                  "crid": "532",
+                  "w": 300,


Should we assume that the mock response will have a value of 100 instead? Or do we expect the minimum width to be 300?

I don't think it matters for the test, but I can update it for consistency.

guscarreon · 2024-01-25T03:35:19Z

adapters/huaweiads/huaweiads.go

-		request.Geo = geo
+		request.Geo = geo{
+			Lon:      float32(ptrutil.ValueOrDefault(openRTBRequest.Device.Geo.Lon)),
+			Lat:      float32(ptrutil.ValueOrDefault(openRTBRequest.Device.Geo.Lat)),


I realize this is beyond the scope of this PR but Geo.Lon and Geo.Lat point to float64 values and when casting them to float32 we risk wrapping the values around. Should we add additional logic to cap the values to the math.MaxFloat32 constant before assigning? I'm curious of @ahmetfaruk59 's thoughts on this.

The range for Lat is -90 to 90 and the range for Lon is -180 to 180. Neither approach the maximum size for float32, so the conversion from float64 to float32 may just reduce precision. If a value is so large that it may wrap around, it's invalid data.

guscarreon · 2024-01-25T04:12:09Z

endpoints/openrtb2/video_auction_test.go

-	assert.Equal(t, recorder.Code, 500, "Should catch error in request")
-	assert.Equal(t, "Critical error while running the video endpoint:  request missing required field: PodConfig.DurationRangeSec request missing required field: PodConfig.Pods", errorMessage, "Incorrect request validation message")
+	assert.Equal(t, 500, recorder.Code, "Should catch error in request")
+	assert.Equal(t, errorMessage, "Critical error while running the video endpoint:  request missing required field: PodConfig.DurationRangeSec request missing required field: PodConfig.Pods", "Incorrect request validation message")


The hardcoded string seems to be the expected value. Should we rollback the order of the params in this particular assert.Equal call?

Good catch. Reverted.

guscarreon · 2024-01-25T04:15:58Z

endpoints/openrtb2/video_auction_test.go


-	assert.Equal(t, videoReq.User, bidReq.User, "User is incorrect")
+	assert.Equal(t, bidReq.User, videoReq.User, "User is incorrect")


Given that the videoReq seems to have all the hardcoded values above, should we rollback to put its values as the expected params in these assert.Equal calls?

Yes, reverted.

guscarreon

In the github.com/prebid/openrtb/v20/openrtb2 repo, field req.imp[i].ClickBrowser was recently modified to be a pointer. Are we missing a ptrutil.Clone call in the ImpWrapper's Clone() definition?

1535   func (w *ImpWrapper) Clone() *ImpWrapper {
1536       if w == nil {
1537           return nil
1538       }
1539  
1540       clone := *w
1541       clone.impExt = w.impExt.Clone()
     +     clone.ClickBrowser = ptrutil.Clone(w.ClickBrowser)
1542  
1543       return &clone
1544   }
openrtb_ext/request_wrapper.go

guscarreon · 2024-01-25T18:05:34Z

Actually, I see a lot of references in the Imp struct, not being cloned by (w *ImpWrapper) Clone() *ImpWrapper method such as:

	Banner *Banner `json:"banner,omitempty"`
	Video *Video `json:"video,omitempty"`
	Audio *Audio `json:"audio,omitempty"`
	Native *Native `json:"native,omitempty"`
	PMP *PMP `json:"pmp,omitempty"`
	Qty *Qty `json:"qty,omitempty"`
	Refresh *Refresh `json:"refresh,omitempty"`

Do we need this? If so, does it belong in a separate PR?

SyntaxNode · 2024-01-25T20:57:16Z

Actually, I see a lot of references in the Imp struct, not being cloned by (w *ImpWrapper) Clone() *ImpWrapper method

That's ok. Similar to the RequestWrapper, the ImpWrapper is cloning it's own state which only includes the ext objects. Just as it's not expected for the RequestWrapper to clone the wrapped request, the ImpWrapper does not clone the wrapped impression.

guscarreon

LGTM

SyntaxNode added 4 commits January 5, 2024 12:33

Update To OpenRTB 2.6-202309

c90acbf

Merge branch 'master' into ortb26-202309

e052e0f

Factor Away getDimensionOrZero

8bd677f

standardize on ptrutil

afefa0e

SyntaxNode commented Jan 5, 2024

View reviewed changes

prebid deleted a comment from github-actions bot Jan 5, 2024

SyntaxNode added the do not port label Jan 5, 2024

SyntaxNode added 2 commits January 5, 2024 13:32

Add Down Conversions

10d4a30

test clear202309Fields

c8bfba7

prebid deleted a comment from github-actions bot Jan 5, 2024

SyntaxNode added 7 commits January 5, 2024 16:06

beachfront tests

ac1d847

tests cadent

4911442

tests eplanning

c6ca91e

tests gumgum

315ea64

tests richaudience

c5906bf

test cleanup

c578308

tests huawei

1df6b6f

SyntaxNode commented Jan 5, 2024

View reviewed changes

prebid deleted a comment from github-actions bot Jan 5, 2024

bsardo assigned guscarreon and AlexBVolcy Jan 8, 2024

AlexBVolcy reviewed Jan 9, 2024

View reviewed changes

beachfront feedback

dad8a79

ahmetfaruk59 approved these changes Jan 11, 2024

View reviewed changes

adapters/huaweiads/huaweiads.go Show resolved Hide resolved

adapters/huaweiads/huaweiads.go Show resolved Hide resolved

adapters/huaweiads/huaweiads.go Show resolved Hide resolved

adapters/huaweiads/huaweiads.go Show resolved Hide resolved

Merge branch 'master' into ortb26-202309

d709095

prebid deleted a comment from github-actions bot Jan 17, 2024

guscarreon reviewed Jan 23, 2024

View reviewed changes

SyntaxNode mentioned this pull request Jan 23, 2024

Skip coverage tests if more than two adapters were modified #3423

Merged

guscarreon reviewed Jan 23, 2024

View reviewed changes

SyntaxNode added 3 commits January 23, 2024 17:06

Merge branch 'master' into ortb26-202309

cb8adc8

Feedback

baebbc8

Remove Unneeded Nil Check

d398b1c

prebid deleted a comment from github-actions bot Jan 23, 2024

AlexBVolcy reviewed Jan 24, 2024

View reviewed changes

adapters/audienceNetwork/facebook.go Outdated Show resolved Hide resolved

Facebook Adapter Feedback

e823ace

prebid deleted a comment from github-actions bot Jan 24, 2024

guscarreon reviewed Jan 25, 2024

View reviewed changes

SyntaxNode added 3 commits January 25, 2024 10:25

Review Feedback

c9dcb5e

Merge branch 'master' into ortb26-202309

95964e1

Merge Conflicts

7c1589d

prebid deleted a comment from github-actions bot Jan 25, 2024

guscarreon reviewed Jan 25, 2024

View reviewed changes

guscarreon approved these changes Jan 29, 2024

View reviewed changes

AlexBVolcy approved these changes Jan 29, 2024

View reviewed changes

bsardo merged commit dd07431 into prebid:master Jan 29, 2024
7 checks passed

bsardo changed the title ~~Update To OpenRTB 2.6-202309~~ Update to OpenRTB 2.6-202309 Jan 29, 2024

SyntaxNode mentioned this pull request Jan 29, 2024

Fix OpenRTB 202309 Merge Conflict #3434

Merged

SyntaxNode deleted the ortb26-202309 branch January 31, 2024 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to OpenRTB 2.6-202309 #3387

Update to OpenRTB 2.6-202309 #3387

SyntaxNode commented Jan 5, 2024 •

edited

Loading

SyntaxNode Jan 5, 2024

SyntaxNode Jan 5, 2024

SyntaxNode Jan 5, 2024

SyntaxNode Jan 5, 2024

SyntaxNode Jan 5, 2024

SyntaxNode Jan 5, 2024

SyntaxNode Jan 5, 2024

SyntaxNode Jan 5, 2024

SyntaxNode Jan 5, 2024

SyntaxNode Jan 5, 2024

muncha commented Jan 8, 2024

AlexBVolcy Jan 9, 2024

SyntaxNode Jan 9, 2024

AlexBVolcy Jan 10, 2024

SyntaxNode commented Jan 19, 2024

guscarreon left a comment

guscarreon Jan 23, 2024

SyntaxNode Jan 23, 2024

guscarreon Jan 25, 2024 •

edited

Loading

SyntaxNode Jan 25, 2024 •

edited

Loading

guscarreon Jan 25, 2024

SyntaxNode Jan 25, 2024

guscarreon Jan 25, 2024

SyntaxNode Jan 25, 2024

guscarreon Jan 25, 2024

SyntaxNode Jan 25, 2024

guscarreon Jan 25, 2024

SyntaxNode Jan 25, 2024

guscarreon left a comment

guscarreon commented Jan 25, 2024

SyntaxNode commented Jan 25, 2024

guscarreon left a comment


		assert.Equal(t, videoReq.User, bidReq.User, "User is incorrect")
		assert.Equal(t, bidReq.User, videoReq.User, "User is incorrect")

Update to OpenRTB 2.6-202309 #3387

Update to OpenRTB 2.6-202309 #3387

Conversation

SyntaxNode commented Jan 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muncha commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SyntaxNode commented Jan 19, 2024

guscarreon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guscarreon Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

SyntaxNode Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guscarreon left a comment

Choose a reason for hiding this comment

guscarreon commented Jan 25, 2024

SyntaxNode commented Jan 25, 2024

guscarreon left a comment

Choose a reason for hiding this comment

SyntaxNode commented Jan 5, 2024 •

edited

Loading

guscarreon Jan 25, 2024 •

edited

Loading

SyntaxNode Jan 25, 2024 •

edited

Loading