Adding type to native markup for Prebid mobile #1081
Conversation
|
@SyntaxNode Thanks for feedback. I pushed an update |
exchange/bidder.go
Outdated
| if err != nil { | ||
| errs = append(errs, err) | ||
| } | ||
| bidResponse.Bids[i].Bid.AdM = string(markup) |
There was a problem hiding this comment.
If there were errors in the marshalling, do we still want to we still want to try overwriting the AdM with whatever json.Marshal() managed to spit out?
There was a problem hiding this comment.
Good point. markup very well may be nil, of which string(markup) would succeed and result in an empty string.
There was a problem hiding this comment.
Agreed. Updated
|
@anwzhang Correct. If |
| errs = append(errs, err) | ||
| } | ||
| var nativePayload nativeRequests.Request | ||
| if err := json.Unmarshal(json.RawMessage((*nativeImp).Request), &nativePayload); err != nil { |
There was a problem hiding this comment.
We have a potential panic here. If getNativeImpByImpID() fails to find the imp, then nativeImp is nil. Trying to access nil.Request will then panic, as you cannot dereference a nil. You should probably abort above if nativeImp is nil. I don't think we need to abort here on Unmarshal errors as the worst that should happen is we fail asset lookups and don't do anything.
| "impid": "my-imp-id", | ||
| "price": 10, | ||
| "adid": "1234", | ||
| "adm": "{\"assets\":[{\"id\":1,\"img\":{\"type\":3,\"url\":\"http://vcdn.adnxs.com/p/creative-image/f8/7f/0f/13/f87f0f13-230c-4f05-8087-db9216e393de.jpg\",\"w\":989,\"h\":742,\"ext\":{\"appnexus\":{\"prevent_crop\":0}}}},{\"title\":{\"text\":\"This is a Prebid Native Creative\"}},{\"id\":2,\"data\":{\"type\":1,\"value\":\"Prebid.org\"}},{\"id\":3,\"data\":{\"type\":2,\"value\":\"This is a Prebid Native Creative. There are many like it, but this one is mine.\"}}],\"link\":{\"url\":\"http://some-url.com\"},\"imptrackers\":[\"http://someimptracker.com\"],\"jstracker\":\"some-js-tracker\"}", |
There was a problem hiding this comment.
In the mock response, don't you want the adapter to return native markup without types being set, so you can verify the code does actually set the types?
There was a problem hiding this comment.
I misunderstood the exchange_test.go Now I moved my test to bidder_test.go
|
@hhhjort Pushed an update. Addressed all your feedback |
This PR implements #1041