OMS: Video support #4172

denis-s-oms · 2025-01-22T13:14:16Z

No description provided.

oms: changed required parameters

github-actions · 2025-01-22T13:14:57Z

Code coverage summary

Note:

Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
Coverage summary encompasses all commits leading up to the latest one, a858a3f

oms

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/oms/oms.go:32:	Builder		100.0%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:39:	MakeRequests	85.7%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:69:	MakeBids	94.7%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:112:	getBidType	100.0%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:121:	getBidVideo	100.0%
total:								(statements)	93.2%

denis-s-oms · 2025-01-23T12:49:18Z

Related docs PR: prebid/prebid.github.io#5830

ccorbo · 2025-01-30T18:46:10Z

adapters/oms/params_test.go

@@ -53,4 +56,5 @@ var invalidParams = []string{
 	`[]`,
 	`{}`,
 	`{"pid": "0"}`,
+	`{"publisherId": 9999}`,


suggestion: add test covering wrong type as well, publisherId as string

scr-oath · 2025-01-31T18:42:02Z

adapters/oms/oms.go

+	var publisherID string
+	if len(request.Imp[0].Ext) > 0 {
+		ext := pbsExt{}
+		err = json.Unmarshal(request.Imp[0].Ext, &ext)


suggestion: please use jsonutil - so the choice of fastest json implementation can be central

Suggested change

err = json.Unmarshal(request.Imp[0].Ext, &ext)

err = jsonutil.Unmarshal(request.Imp[0].Ext, &ext)

scr-oath · 2025-01-31T18:43:18Z

adapters/oms/oms.go

 	requestData := &adapters.RequestData{
 		Method: "POST",
-		Uri:    a.endpoint,
+		Uri:    a.endpoint + fmt.Sprintf("?publisherId=%v", publisherID),


suggestion: if you're already using sprintf, can you just include a.endpoint as an argument and %s where it goes?

Suggested change

Uri: a.endpoint + fmt.Sprintf("?publisherId=%v", publisherID),

Uri: fmt.Sprintf("%s?publisherId=%v", a.endpoint, publisherID),

scr-oath · 2025-01-31T18:45:49Z

adapters/oms/oms.go

@@ -72,12 +96,40 @@ func (a *adapter) MakeBids(request *openrtb2.BidRequest, requestData *adapters.R
 	}
 	for _, seatBid := range response.SeatBid {


suggestion/question: I'm curious about the type of SeatBid if it's an []SomeStruct then this should also use an index - otherwise, seatBid will do a struct copy (shallow) for each loop iteration.

If the type supports it, consider something like this

Suggested change

for _, seatBid := range response.SeatBid {

for sbIndex := range response.SeatBid {

seatBid := &response.SeatBid[sbIndex]

These lines are unchanged.

scr-oath · 2025-01-31T18:47:14Z

adapters/oms/oms.go

 			bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{
-				Bid:     &seatBid.Bid[i],
-				BidType: openrtb_ext.BidTypeBanner,
+				Bid:      &seatBid.Bid[i],


observation/curiosity: hmm… so this is a pointer to the copy - see previous comment, is that intended? If you move to seatBid to a pointer, it'll point to that same instance's bid - that's efficient - any worries about contention here? you're not making changes here, I think it's ok

Yes, these lines are unchanged.

scr-oath · 2025-01-31T18:47:59Z

adapters/oms/oms.go

 			})
 		}
 	}

 	return bidResponse, nil
 }
+
+func getBidType(markupType openrtb2.MarkupType) openrtb_ext.BidType {


observation: hmm… this seems like it could be general… is there some central place that can do this mapping?

What you mean by central place?

After checking other adapters I made a conclusion that it depends on how any of them want to process bid types (some of them return error if mapping isn't succeeded, some of them return default value). In our case we want to support backward compatibility that's why banner type is returned as default value.

@scr-oath are you asking if there is a helper function offered that can be used by any adapter? There isn't. We might be able to offer that at some point but we should keep in mind that not all adapters support the same media types and the default case will vary. Also some adapters will fall back to deriving the media type from the request imp if mtype is not in the response.

…anged uri builder

github-actions · 2025-02-02T12:08:12Z

Code coverage summary

Note:

Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
Coverage summary encompasses all commits leading up to the latest one, 40801cc

oms

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/oms/oms.go:32:	Builder		100.0%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:39:	MakeRequests	85.7%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:69:	MakeBids	94.7%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:113:	getBidType	100.0%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:122:	getBidVideo	100.0%
total:								(statements)	93.2%

scr-oath · 2025-02-06T17:01:54Z

adapters/oms/oms.go

 	requestData := &adapters.RequestData{
 		Method: "POST",
-		Uri:    a.endpoint,
+		Uri:    fmt.Sprintf("%s?publisherId=%v", a.endpoint, publisherID),


question: why do you need Itoa? Could you just use %d to format the number into the query param?

github-actions · 2025-02-09T07:54:38Z

Code coverage summary

Note:

Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
Coverage summary encompasses all commits leading up to the latest one, 85ec4eb

oms

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/oms/oms.go:31:	Builder		100.0%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:38:	MakeRequests	85.7%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:68:	MakeBids	94.7%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:112:	getBidType	100.0%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:121:	getBidVideo	100.0%
total:								(statements)	93.2%

bsardo · 2025-02-11T04:59:07Z

adapters/oms/params_test.go

@@ -42,6 +42,9 @@ func TestInvalidParams(t *testing.T) {
 var validParams = []string{
 	`{"pid": "12345"}`,
 	`{"pid": "123456"}`,
+	`{"publisherId": 12345}`,
+	`{"publisherId": 123456}`,
+	`{"publisherId": 12345,"pid": "12345"}`,


Please add a valid boundary test for publisherId:
{"publisherId": 10000}

bsardo · 2025-02-11T05:14:35Z

adapters/oms/oms.go

 			})
 		}
 	}

 	return bidResponse, nil
 }
+
+func getBidType(markupType openrtb2.MarkupType) openrtb_ext.BidType {


@scr-oath are you asking if there is a helper function offered that can be used by any adapter? There isn't. We might be able to offer that at some point but we should keep in mind that not all adapters support the same media types and the default case will vary. Also some adapters will fall back to deriving the media type from the request imp if mtype is not in the response.

bsardo · 2025-02-11T05:21:20Z

adapters/oms/oms.go

+		ext := pbsExt{}
+		err = jsonutil.Unmarshal(request.Imp[0].Ext, &ext)
+		if err != nil {
+			return nil, []error{err}


Please add a supplemental test case where the unmarshal fails. You can do this by simply setting your imp.ext to a type other than object.

…ere unmarshal fails

github-actions · 2025-02-11T08:47:43Z

Code coverage summary

Note:

Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
Coverage summary encompasses all commits leading up to the latest one, 9b589fe

oms

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/oms/oms.go:31:	Builder		100.0%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:38:	MakeRequests	92.9%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:68:	MakeBids	94.7%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:112:	getBidType	100.0%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:121:	getBidVideo	100.0%
total:								(statements)	95.5%

VeronikaSolovei9 · 2025-02-12T00:50:08Z

adapters/oms/oms.go

+type pbsExt struct {
+	Bidder genericPID `json:"bidder"`
+	Tid    string     `json:"tid"`
+}
+
+type genericPID struct {
+	Pid         string `json:"pid"`
+	PublisherID int    `json:"publisherId"`
+}
+


These new types are not needed here.
You will need to add new field to ExtImpOms, it already exists in the openrtb_ext directory:

type ExtImpOms struct { Pid string `json:"pid"` PublisherID int `json:"publisherId"` }

And then in MakeRequests follow a general approach to Unmarshal imp.ext:
example

github-actions · 2025-02-12T13:06:47Z

Code coverage summary

Note:

Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
Coverage summary encompasses all commits leading up to the latest one, 7ae703f

oms

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/oms/oms.go:21:	Builder		100.0%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:28:	MakeRequests	87.5%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:62:	MakeBids	94.7%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:106:	getBidType	100.0%
github.com/prebid/prebid-server/v3/adapters/oms/oms.go:115:	getBidVideo	100.0%
total:								(statements)	93.5%

VeronikaSolovei9

LGTM

oms: added video support

a858a3f

oms: changed required parameters

bsardo added the adapter label Jan 22, 2025

denis-s-oms mentioned this pull request Jan 23, 2025

OMS: added video support prebid/prebid.github.io#5830

Open

8 tasks

bsardo changed the title ~~OMS Adapter: Video support~~ OMS: Video support Jan 24, 2025

ccorbo reviewed Jan 30, 2025

View reviewed changes

scr-oath suggested changes Jan 31, 2025

View reviewed changes

oms: added tests for invalid params, changed json marshal package, ch…

40801cc

…anged uri builder

scr-oath reviewed Feb 6, 2025

View reviewed changes

changed publisherID formatting

85ec4eb

bsardo self-assigned this Feb 11, 2025

bsardo reviewed Feb 11, 2025

View reviewed changes

added valid boundary test for publisherId, added supplemental test wh…

9b589fe

…ere unmarshal fails

bsardo assigned VeronikaSolovei9 Feb 11, 2025

VeronikaSolovei9 reviewed Feb 12, 2025

View reviewed changes

oms: changed to general approach of unmarshalling ext

7ae703f

VeronikaSolovei9 approved these changes Feb 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OMS: Video support #4172

OMS: Video support #4172

denis-s-oms commented Jan 22, 2025

github-actions bot commented Jan 22, 2025

denis-s-oms commented Jan 23, 2025

ccorbo Jan 30, 2025

denis-s-oms Feb 2, 2025

scr-oath Jan 31, 2025

denis-s-oms Feb 2, 2025

scr-oath Jan 31, 2025

denis-s-oms Feb 2, 2025

scr-oath Jan 31, 2025

denis-s-oms Feb 2, 2025

scr-oath Jan 31, 2025

denis-s-oms Feb 2, 2025

scr-oath Jan 31, 2025

denis-s-oms Feb 2, 2025

denis-s-oms Feb 2, 2025

bsardo Feb 11, 2025

github-actions bot commented Feb 2, 2025

scr-oath Feb 6, 2025

denis-s-oms Feb 9, 2025

github-actions bot commented Feb 9, 2025

bsardo Feb 11, 2025

denis-s-oms Feb 11, 2025

bsardo Feb 11, 2025

bsardo Feb 11, 2025

denis-s-oms Feb 11, 2025

github-actions bot commented Feb 11, 2025

VeronikaSolovei9 Feb 12, 2025

denis-s-oms Feb 12, 2025

github-actions bot commented Feb 12, 2025

VeronikaSolovei9 left a comment

	err = json.Unmarshal(request.Imp[0].Ext, &ext)
	err = jsonutil.Unmarshal(request.Imp[0].Ext, &ext)

	Uri: a.endpoint + fmt.Sprintf("?publisherId=%v", publisherID),
	Uri: fmt.Sprintf("%s?publisherId=%v", a.endpoint, publisherID),

		@@ -72,12 +96,40 @@ func (a adapter) MakeBids(request openrtb2.BidRequest, requestData *adapters.R
		}
		for _, seatBid := range response.SeatBid {

	for _, seatBid := range response.SeatBid {
	for sbIndex := range response.SeatBid {
	seatBid := &response.SeatBid[sbIndex]

OMS: Video support #4172

Are you sure you want to change the base?

OMS: Video support #4172

Conversation

denis-s-oms commented Jan 22, 2025

github-actions bot commented Jan 22, 2025

Code coverage summary

oms

denis-s-oms commented Jan 23, 2025

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

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

github-actions bot commented Feb 2, 2025

Code coverage summary

oms

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 9, 2025

Code coverage summary

oms

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

github-actions bot commented Feb 11, 2025

Code coverage summary

oms

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 12, 2025

Code coverage summary

oms

VeronikaSolovei9 left a comment

Choose a reason for hiding this comment