Prebid Server adapter for Telaria

[telariaEng]

Type of change

• New bidder adapter

Description of change

Prebid Server adapter for Telaria

• contact email: github@telaria.com
• official adapter submission

[guscarreon]

Hi, thank you for requesting us to incorporate your adapter. We appreciate your time and effort and we'd like to start the review right away. Before that, can we please make sure the code coverage is 90% to 95% please? Usually, adapters create json files to cover most scenarios in their code's logic.

</prebid/prebid-server/
▾ adapters/
  ▾ fooadapter/
    ▾ fooadaptertest/
      ▸ exemplary/     <-- tests to assert successful requests
      ▸ params/        <-- tests to assert valid params
      ▸ supplemental/  <-- tests to cover error scenarios or unsuccessful requests
      foo.go
      foo_test.go      <--  Runs `TestJsonSamples` that automatically reads json files from `exemplary/`, `params/`, and `supplemental/`
      params_test.go
      usersync.go
      usersync_test.go

Current coverage

ok  	github.com/prebid/prebid-server/adapters/telaria	0.007s	coverage: 4.7% of statements
Package has less than 30% code coverage. Run ./scripts/coverage.sh --html to see a detailed coverage report, and add tests to improve your coverage
The command "./validate.sh --nofmt --cov --race 10" exited with 1.

Please let us know if there are any questions

[telariaEng]

Hi, thank you for requesting us to incorporate your adapter. We appreciate your time and effort and we'd like to start the review right away. Before that, can we please make sure the code coverage is 90% to 95% please? Usually, adapters create json files to cover most scenarios in their code's logic.

</prebid/prebid-server/
▾ adapters/
  ▾ fooadapter/
    ▾ fooadaptertest/
      ▸ exemplary/     <-- tests to assert successful requests
      ▸ params/        <-- tests to assert valid params
      ▸ supplemental/  <-- tests to cover error scenarios or unsuccessful requests
      foo.go
      foo_test.go      <--  Runs `TestJsonSamples` that automatically reads json files from `exemplary/`, `params/`, and `supplemental/`
      params_test.go
      usersync.go
      usersync_test.go

Current coverage

ok  	github.com/prebid/prebid-server/adapters/telaria	0.007s	coverage: 4.7% of statements
Package has less than 30% code coverage. Run ./scripts/coverage.sh --html to see a detailed coverage report, and add tests to improve your coverage
The command "./validate.sh --nofmt --cov --race 10" exited with 1.

Please let us know if there are any questions

Thank you for your reply. We will update the test cases as soon as possible.

[telariaEng]

Hello @guscarreon I've updated the test cases and the coverage is at 90% now. Please let me know if there's anything else I should do.

[guscarreon]

Looks pretty solid. Left some minor comments before approving. Also, logic shown in red below inside function GetResponseBody(response *adapters.ResponseData) seems worth covering in the test cases. Can we?

[guscarreon]

Parameter seatCode is listed in static/bidder-params/telaria.json but seems to not be listed here. Do we want it in the ExtImpTelaria struct?

[telariaEng]

I added the seatCode param to the request ext object intentionally (we support multiple imp objects) and instead of adding a seat code under each Imp object I figured it will be better to just have it under request.ext. I can move it to the Ext object under impression if required.

[guscarreon]

The root level ext is not considered in our video endpoint. Therefore, both the SeatCode and the OriginalPublisherID params be put in the imp object extensions instead please? I know it is a little bit repetitive to put them in every Imp, but our system picks them up that way.

  {
    "$schema": "http://json-schema.org/draft-04/schema#",
    "title": "Telaria Adapter Params",
    "description": "A schema which validates params accepted by the Telaria adapter",
  
    "type": "object",
    "properties": {
      "adCode": {
        "type": "string",
        "description": "The Ad Unit Code."
      },
      "seatCode": {
        "type": "string",
        "description": "Your Seat Code."
-     }
+     },
+     "originalPublisherId": {
+       "type": "string",
+       "description": "A description"
+     }
    }
  }
static/bidder-params/telaria.json

  type ExtImpTelaria struct {
  	AdCode string `json:"adCode,omitempty"`
+ 	SeatCode string `json:"seatCode,omitempty"`
+ 	OriginalPublisherID string `json:"originalPublisherId"`
  }
openrtb_ext/imp_telaria.go

[guscarreon]

In addition. Lines 148 to 150 of adapters/telaria/telaria.go seem to make clear that SeatCode is a required parameter so you could also add a required parameters list in static/bidder-params/telaria.json if you want to.

148     if requestExtIncoming.Telaria == nil || requestExtIncoming.Telaria.SeatCode == "" {
149         return &errortypes.BadInput{Message: "Seat Code is required"}
150     }
adapters/telaria/telaria.go

Entirely optional:

  {
    "$schema": "http://json-schema.org/draft-04/schema#",
    "title": "Telaria Adapter Params",
    "description": "A schema which validates params accepted by the Telaria adapter",
  
    "type": "object",
    "properties": {
      "adCode": {
        "type": "string",
        "description": "The Ad Unit Code."
      },
      "seatCode": {
        "type": "string",
        "description": "Your Seat Code."
      }
-   }
+   },
+   "required": ["seatCode"]
  }
static/bidder-params/telaria.json

[guscarreon]

Do we need AdCode to be a required parameter? Based on the fact that we want to assign its value to request.Imp[i].TagID in case no error is thrown, should we mark adCode as required in the static/bidder-params/telaria.json file?

  1   {
  2     "$schema": "http://json-schema.org/draft-04/schema#",
  3     "title": "Telaria Adapter Params",
  4     "description": "A schema which validates params accepted by the Telaria adapter",
  5  
  6     "type": "object",
  7     "properties": {
  8       "adCode": {
  9         "type": "string",
 10         "description": "The Ad Unit Code."
 11       },
 12       "seatCode": {
 13         "type": "string",
 14         "description": "Your Seat Code."
 15 -     }
    +     },
    +     "required": ["adCode"]
 16     }
 17   }
static/bidder-params/telaria.json

[telariaEng]

adCode isn't required, only seat code is required. There is a default tagId that the pubs will have to set up on our system and if the ad code isn't passed our server uses the default value.

[guscarreon]

Can we please list the Telaria import after line 9? We try to keep all of the adapter imports in one block.

[telariaEng]

Will do.

[guscarreon]

Is there any posibility that requestExtIncoming.Telaria which is a ReqExtTelariaIn pointer be nil? Given our context, do you think necessary to check for nil first?

type ReqExtIn struct {
	openrtb_ext.ExtRequest
	Telaria *ReqExtTelariaIn `json:"telaria,omitempty"`
}

[telariaEng]

Yes thats a good point. I'm adding a nil check over there.

[guscarreon]

Both request.Site.Publisher and request.App.Publisher get checked for nil inside FetchPublisherId(request *openrtb.BidRequest) and checked again in this function. Is there a way to avoid the extra check in lines 178 to 192? I guess it translates to something like:

141   func (a *TelariaAdapter) FetchPublisherId(request *openrtb.BidRequest) (string) {
142 - 
    +     pubID := requestExtIncoming.Telaria.SeatCode
143       if request.Site != nil {
144           if request.Site.Publisher != nil {
145 -             if request.Site.Publisher.ID != "" {
146 -                 return request.Site.Publisher.ID
147 -             }
    +             request.Site.Publisher.ID = pubID
    +         } else {
    +             pubID = request.Site.Publisher.ID
148           }
149       }
150  
151       if request.App != nil {
152           if request.App.Publisher != nil {
153 -             if request.App.Publisher.ID != "" {
154 -                 return request.App.Publisher.ID
155 -             }
    +             request.App.Publisher.ID = pubID
    +         } else {
    +             pubID = request.App.Publisher.ID
156           }
157       }
158  
159 -     return ""
    +     return pubID
160   }
161  
162   // This method changes <site/app>.publisher.id to request.ext.telaria.seatCode
163   // And moves the publisher.id to request.ext.originalPublisherId
164   func (a *TelariaAdapter) PopulateRequestExtAndPubId(request *openrtb.BidRequest) error {
165       var requestExtIncoming ReqExtIn
166       if err := json.Unmarshal(request.Ext, &requestExtIncoming); err != nil {
167           return err
168       }
169  
170       if requestExtIncoming.Telaria.SeatCode == "" {
171           return &errortypes.BadInput{Message: "Seat Code is required"}
172       }
173  
    +     originalPublisherID := a.FetchPublisherId(request)
174 -     request.Ext, _ = json.Marshal(&BidExtOut{requestExtIncoming.ExtRequest, a.FetchPublisherId(request)})
    +     request.Ext, _ = json.Marshal(&BidExtOut{requestExtIncoming.ExtRequest, originalPublisherID})
175  
176       publisherObject := &openrtb.Publisher{ID: originalPublisherID}
177 -
178 -     if request.Site != nil {
179 -         if request.Site.Publisher != nil {
180 -             request.Site.Publisher.ID = requestExtIncoming.Telaria.SeatCode
181 -         } else {
182 -             request.Site.Publisher = publisherObject
183 -         }
184 -     }
185 -
186 -     if request.App != nil {
187 -         if request.App.Publisher != nil {
188 -             request.App.Publisher.ID = requestExtIncoming.Telaria.SeatCode
189 -         } else {
190 -             request.App.Publisher = publisherObject
191 -         }
192 -     }
193  
194       return nil
195   }

This is just a suggestion and the logic needs to be double checked.

[telariaEng]

Right, I've combined the nil checks and the seat code assignments.

[guscarreon]

Again, are we sure no json.Marshal error could get returned under every circumstance?

[telariaEng]

I will add an err check at these places.

[VeronikaSolovei9]

I accidentally remove myself from reviewers. I approve this PR, it seems like I have no permissions to do this (even with assignment)

[VeronikaSolovei9]

Approved

[telariaEng]

@VeronikaSolovei9 @guscarreon thank you so much for helping me with the adapter. It was fun learning go and contributing to this project :).

[VeronikaSolovei9]

@telariaEng thank you so much for your hardwork!

[guscarreon]

Looks pretty good. Some comments still left

[guscarreon]

Please get rid of unnecessary comment

[telariaEng]

Will do.

[guscarreon]

Thank you

[guscarreon]

If originalPublisherid is only found in static/bidder-params/telaria.json and has no "receptive" field in openrtb_ext/imp_telaria.go, it won't be unmarshaled and won't be of any use. Is that ok?

[telariaEng]

Yeah that is fine. It is being sent out in the Imp.Ext object to our ad server

[guscarreon]

Can we place telaria's import in line 24 anywere from line 10 to line 60 were the rest of the adapters are listed?

[telariaEng]

Done.

Approved by mistake

[guscarreon]

A couple more comments were left. Looks pretty good so far

[guscarreon]

@telariaEng thanks for the quick response. Can we please add one more, very simple, test file inside adapters/telaria called adapters/telaria/params_test.go? This test file will assert the validity of the parameters defined in static/bidder-params/telaria.json both the required and not required and should also assert the parameters come with the correct type. Said propsed file could look like this:

  1 package telaria
  2
  3 import (
  4     "encoding/json"
  5     "github.com/prebid/prebid-server/openrtb_ext"
  6     "testing"
  7 )
  8
  9 func TestValidParams(t *testing.T) {
 10     validator, err := openrtb_ext.NewBidderParamsValidator("../../static/bidder-params")
 11     if err != nil {
 12         t.Fatalf("Failed to fetch the json-schemas. %v", err)
 13     }
 14
 15     for _, validParam := range validParams {
 16         if err := validator.Validate(openrtb_ext.BidderTelaria, json.RawMessage(validParam)); err != nil {
 17             t.Errorf("Schema rejected tappx params: %s \n Error: %s", validParam, err)
 18         }
 19     }
 20 }
 21
 22 func TestInvalidParams(t *testing.T) {
 23     validator, err := openrtb_ext.NewBidderParamsValidator("../../static/bidder-params")
 24     if err != nil {
 25         t.Fatalf("Failed to fetch the json-schemas. %v", err)
 26     }
 27
 28     for _, invalidParam := range invalidParams {
 29         if err := validator.Validate(openrtb_ext.BidderTelaria, json.RawMessage(invalidParam)); err == nil {
 30             t.Errorf("Schema allowed unexpected params: %s", invalidParam)
 31         }
 32     }
 33 }
 34
 35 var validParams = []string{
 36     `{"adCode": "string", "seatCode": "string", "originalPublisherid": "string"}`,
 37 }
 38
 39 var invalidParams = []string{
 40     ``,
 41     `null`,
 42     `true`,
 43     `5`,
 44     `4.2`,
 45     `[]`,
 46     `{}`,
 47     `{"adCode": "string", "originalPublisherid": "string"}`,
 48     `{"adCode": "string", "seatCode": 5, "originalPublisherid": "string"}`,
 49 }
~
adapters/telaria/params_test.go

[telariaEng]

@telariaEng thanks for the quick response. Can we please add one more, very simple, test file inside adapters/telaria called adapters/telaria/params_test.go? This test file will assert the validity of the parameters defined in static/bidder-params/telaria.json both the required and not required and should also assert the parameters come with the correct type. Said propsed file could look like this:

  1 package telaria
  2
  3 import (
  4     "encoding/json"
  5     "github.com/prebid/prebid-server/openrtb_ext"
  6     "testing"
  7 )
  8
  9 func TestValidParams(t *testing.T) {
 10     validator, err := openrtb_ext.NewBidderParamsValidator("../../static/bidder-params")
 11     if err != nil {
 12         t.Fatalf("Failed to fetch the json-schemas. %v", err)
 13     }
 14
 15     for _, validParam := range validParams {
 16         if err := validator.Validate(openrtb_ext.BidderTelaria, json.RawMessage(validParam)); err != nil {
 17             t.Errorf("Schema rejected tappx params: %s \n Error: %s", validParam, err)
 18         }
 19     }
 20 }
 21
 22 func TestInvalidParams(t *testing.T) {
 23     validator, err := openrtb_ext.NewBidderParamsValidator("../../static/bidder-params")
 24     if err != nil {
 25         t.Fatalf("Failed to fetch the json-schemas. %v", err)
 26     }
 27
 28     for _, invalidParam := range invalidParams {
 29         if err := validator.Validate(openrtb_ext.BidderTelaria, json.RawMessage(invalidParam)); err == nil {
 30             t.Errorf("Schema allowed unexpected params: %s", invalidParam)
 31         }
 32     }
 33 }
 34
 35 var validParams = []string{
 36     `{"adCode": "string", "seatCode": "string", "originalPublisherid": "string"}`,
 37 }
 38
 39 var invalidParams = []string{
 40     ``,
 41     `null`,
 42     `true`,
 43     `5`,
 44     `4.2`,
 45     `[]`,
 46     `{}`,
 47     `{"adCode": "string", "originalPublisherid": "string"}`,
 48     `{"adCode": "string", "seatCode": 5, "originalPublisherid": "string"}`,
 49 }
~
adapters/telaria/params_test.go

Done :)

[guscarreon]

Look pretty good. Thank you @telariaEng for addressing all of our comments

[VeronikaSolovei9]

Hi @telariaEng, we continue our testing of PBS with Telaria adapter with parameters you gave me. We noticed it returns a bid with zero price only. Can you please configure it to return price greater than zero and if possible configure it to return more than one bids back (when we request more than one impression). Thank you!