Upconvert to rtb 2.6 (Some tests not working, need validation to support 2.6) #3914
Conversation
| assert.Equal(t, test.expectedRegExt.USPrivacy, *&result.Regs.USPrivacy, test.description+":USPrivacy") | ||
| assert.Equal(t, test.expectedRegExt.GDPR, *&result.Regs.GDPR, test.description+":GDPR") |
There was a problem hiding this comment.
Is the *& necessary? I thought Equal will only compare values not that they are the same memory location.
There was a problem hiding this comment.
I did not really consider that. The old code was the same, except looking to the unmarshalled ext from the result. I can try it without.
| @@ -533,6 +533,9 @@ func (deps *endpointDeps) parseRequest(httpRequest *http.Request, labels *metric | |||
| return | |||
| } | |||
|
|
|||
| // upgrade to 2.6 here | |||
| openrtb_ext.ConvertUpTo26(req) | |||
There was a problem hiding this comment.
Just noting that processStoredRequests does the following:
- generate request UUID
- merge in stored request
- merge in default request
- extract additional info from imp.ext.prebid (echo video attrs, passthrough)
- merge stored imps with incoming request imps
I think it is ok to perform these operations prior to the upconvert. All other request mutations are done after the upconvert.
endpoints/openrtb2/amp_auction.go
Outdated
| // upgrade to 2.6 here | ||
| openrtb_ext.ConvertUpTo26(reqWrapper) |
There was a problem hiding this comment.
Should this be moved into parseAmpRequest and be called right after the stored request is fetched but before we populate other fields like req.test, req.imp[x].secure, etc and perform validations?
There was a problem hiding this comment.
Discussed offline. At a minimum we will move the call to ConvertUpTo26 before ortb.SetDefaults but it could also be moved somewhere in loadRequestJSONForAmp. The question is how do we view the AMP params? Are they considered the incoming request similar to how they are treated on the auction endpoint? Perhaps it should be moved before the call to overrideWithParams?
There was a problem hiding this comment.
I will move it up to just after we create the request wrapper. Seems to be a good place for it. The functions afterwards only seem to hit non-upconverted fields.
| @@ -260,6 +260,8 @@ func (deps *endpointDeps) VideoAuctionEndpoint(w http.ResponseWriter, r *http.Re | |||
| // all code after this line should use the bidReqWrapper instead of bidReq directly | |||
| bidReqWrapper := &openrtb_ext.RequestWrapper{BidRequest: bidReq} | |||
|
|
|||
| openrtb_ext.ConvertUpTo26(bidReqWrapper) | |||
There was a problem hiding this comment.
There are a few fields being set on the request like id and test prior to the upconvert but that should be ok.
There was a problem hiding this comment.
Moving the conversion to the wrapper up would be a lot of work to make everything wrapper compatible. So as you said, it should work well here.
No description provided.