New Adapter: Ogury #4082
New Adapter: Ogury #4082
Conversation
* Accept all three params simultaneously * Fix bidder url
Sync upstream master
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Code coverage summaryNote:
oguryRefer here for heat map coverage report |
Code coverage summaryNote:
oguryRefer here for heat map coverage report |
|
Hello I have a question with this PR. |
always use imp.id for adunitcode, to map to imp.tagid
Code coverage summaryNote:
oguryRefer here for heat map coverage report |
Let's leave the discussion on the mentioned issue. |
* use jsonutil package instead standard json lib * don't allocate newImpExt, use impExt for hoisting bidder.params * rename to buildHeader * set IPv6 header * use mtype from response
Code coverage summaryNote:
oguryRefer here for heat map coverage report |
|
@SyntaxNode I pushed changes to address comments on PR |
|
Hello, would like to ask for your help with reviewing this PR. |
adapters/ogury/ogury.go
Outdated
|
|
||
| } | ||
|
|
||
| func filterValidImps(request *openrtb2.BidRequest) (validImps []openrtb2.Imp) { |
There was a problem hiding this comment.
You may want to return an array of the results of unmarshling the imp.ext's since you unmarshal them again in the calling function. It is true that you unmarshal imp.ext.bidder into different structs on the two passes, but you could either save the imp.ext unmarshal, or just use map[string]json.RawMessage target for both purposes, and then unmarshal the AssetKey and AdUnitID keys into strings as one last step. Unmarshalling JSON tends to be a CPU heavy process.
| "allOf": [ | ||
| { | ||
| "if": { "required": ["assetKey"] }, | ||
| "then": { "required": ["adUnitId"] } | ||
| }, | ||
| { | ||
| "if": { "required": ["adUnitId"] }, | ||
| "then": { "required": ["assetKey"] } | ||
| } | ||
| ] |
There was a problem hiding this comment.
This expression makes both of assetKey and adUnitId required or passes the imp to bidder if none of them are specified. Is this intentional?
Based on this code
if impExtOgury.AssetKey != "" && impExtOgury.AdUnitID != "" {
validImps = append(validImps, imp)
}it looks like you expect both assetKey and adUnitId to be specified. If so just replace this part of json file to:
"required": ["assetKey", "adUnitId"]And remove this check if impExtOgury.AssetKey != "" && impExtOgury.AdUnitID != "" { because it will be done in PBS core before passing the request to adapter.
The only thing you will need to check is
if request.Site != nil && request.Site.Publisher.ID != "" { return request.Imp }
it can be moved to the MakeRequests function and you can delete filterValidImps.
There was a problem hiding this comment.
This expression makes both of
assetKeyandadUnitIdrequired or passes the imp to bidder if none of them are specified. Is this intentional?
Yes it is intentional. We accept request that have assetKey and adUnitId or we can accept request that have just request.Site.Publisher.ID (in this case we don't need assetKey/adUnitId).
We have a preference for imp that have assetKey/adunitId so that's why we filter out only those that have them and if three are no imps that have them we accept the request if it has publisherID.
The only way I see to achieve this is like this, to check manually in code and filter.
The only thing that I see that we can improve here is what @hhhjort said in the comment above and that's to "unify" filtering so we do unmarshal only once.
adapters/ogury/ogury.go
Outdated
| func (a adapter) MakeRequests(request *openrtb2.BidRequest, requestInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) { | ||
| headers := buildHeaders(request) | ||
|
|
||
| request.Imp = filterValidImps(request) |
There was a problem hiding this comment.
I believe this function can be deleted, please my another comment in augury.json file
| } | ||
|
|
||
| var errors []error | ||
| for i, imp := range request.Imp { |
There was a problem hiding this comment.
Please consolidate this for loop with the for loop starting at the line 72.
| var impExt, impExtBidderHoist map[string]json.RawMessage | ||
| // extract ext | ||
| if err := jsonutil.Unmarshal(imp.Ext, &impExt); err != nil { | ||
| return nil, append(errors, &errortypes.BadInput{ | ||
| Message: "Bidder extension not provided or can't be unmarshalled", | ||
| }) | ||
| } | ||
| // find Ogury bidder params | ||
| if bidder, ok := impExt[openrtb_ext.PrebidExtBidderKey]; ok { | ||
| if err := jsonutil.Unmarshal(bidder, &impExtBidderHoist); err != nil { | ||
| return nil, append(errors, &errortypes.BadInput{ | ||
| Message: "Ogury bidder extension not provided or can't be unmarshalled", | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // extract every value from imp[].ext.bidder to imp[].ext | ||
| for key, value := range impExtBidderHoist { | ||
| impExt[key] = value | ||
| } | ||
|
|
||
| ext, err := jsonutil.Marshal(impExt) | ||
| if err != nil { | ||
| return nil, append(errors, &errortypes.BadInput{ | ||
| Message: "Error while marshaling Imp.Ext bidder exension", | ||
| }) | ||
| } | ||
| request.Imp[i].Ext = ext |
There was a problem hiding this comment.
Can you modify this to
var impExt adapters.ExtImpBidder
if err := jsonutil.Unmarshal(imp.Ext, &impExt); err != nil {
return ..
}
var impExtOgury openrtb_ext.ImpExtOgury
if err := jsonutil.Unmarshal(impExt.Bidder, &impExtOgury); err != nil {
return...
}this is a preferable way to handle imp.ext in PBS.
Or you can directly set imp.Ext = impExt.Bidder without unmarshalling to impExtOgury
There was a problem hiding this comment.
If we do imp.Ext = impExt.Bidder then we will lose all the other important fields that are in imp.Ext (here I'm mainly referring to gpid but there maybe some other that we will like to use in the future like tid).
If we do
var impExtOgury openrtb_ext.ImpExtOgury
if err := jsonutil.Unmarshal(impExt.Bidder, &impExtOgury); err != nil {
return...
}then we will lose all the other field besides what is in ImpExtOgury (we often experiment with some optional fields in the ImpExtOgury).
So the best way I see to handle this problem is like this, to unmarshal to map[string]json.RawMessage and hoist fields from impExtBidder to impExt. This looks like the most robust solution so we don't need to make any feature PRs if we want just to pass to or bidder some other field that in impExt.
consolidate all imp changed to one `for` loop
Code coverage summaryNote:
oguryRefer here for heat map coverage report |
|
Hello @hhhjort @VeronikaSolovei9 |
PR for the documentation prebid/prebid.github.io#5742