-
Notifications
You must be signed in to change notification settings - Fork 760
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New Adapter: Aax #2219
New Adapter: Aax #2219
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you re-name this pull request to: "New Adapter: Aax"
Just to follow naming conventions with how we handle new adapters PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
for i := range sb.Bid { | ||
bidType, err := getMediaTypeForImp(sb.Bid[i].ImpID, internalRequest.Imp) | ||
if err != nil { | ||
errs = append(errs, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a test case for this error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, Thanks.
adapters/aax/aax.go
Outdated
|
||
bidResponse := adapters.NewBidderResponse() | ||
|
||
for _, sb := range bidResp.SeatBid { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: could you rename sb
to be seatBid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
adapters/aax/aax.go
Outdated
}, nil | ||
} | ||
|
||
func getMediaTypeForImp(impID string, imps []openrtb2.Imp) (openrtb_ext.BidType, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this PR with a team. There is one thing for you to consider. I case if imp has multi format this function may return incorrect bid type. We recommend to extract mediaType from response. Here is the example for reference:
prebid-server/adapters/algorix/algorix.go
Line 190 in 74d2199
func getBidType(bid openrtb2.Bid, imps []openrtb2.Imp) (openrtb_ext.BidType, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than this this PR loos good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to second what @VeronikaSolovei9 stated above. This PR also looks good to me, but just want to bring up this update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, Thanks !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not really what is needed. You need to pass bid openrtb2.Bid
and take media type from bidExtention.MediaType
if you support this. If you modified this function to verify impression had multi format and throw error - then this should be done in MakeRequests function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted, Let me fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused, did you change it? I'm looking for something like this:
type aaxResponseBidExt struct { // Aax specific bid ext if yu know the format and return bid type in bid.ext
MediaType string `json:"mediaType"`
}
//pass bid itself to the function, should be seatBid.Bid[i]
func getMediaTypeForImp(bid openrtb2.Bid, imps []openrtb2.Imp) (openrtb_ext.BidType, error) {
var bidExt aaxResponseBidExt
err := json.Unmarshal(bid.Ext, &bidExt) //unmarshall bid ext to struct and extract bid type from it
if err == nil {
switch bidExt.MediaType {
case "banner":
return openrtb_ext.BidTypeBanner, nil
case "native":
return openrtb_ext.BidTypeNative, nil
case "video":
return openrtb_ext.BidTypeVideo, nil
}
}
// if type was not returned till this moment - extract it from imp, you have this part already
var mediaType openrtb_ext.BidType
var typeCnt = 0
for _, imp := range imps {
if imp.ID == bid.ImpID { //bid has imp id in it, you don't need to pass it to function separately
if imp.Banner != nil {
typeCnt += 1
mediaType = openrtb_ext.BidTypeBanner
}
if imp.Native != nil {
typeCnt += 1
mediaType = openrtb_ext.BidTypeNative
}
if imp.Video != nil {
typeCnt += 1
mediaType = openrtb_ext.BidTypeVideo
}
}
}
if typeCnt == 1 {
return mediaType, nil
}
return mediaType, fmt.Errorf("unable to fetch mediaType in multi-format: %s", bid.ImpID)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No No, Have not pushed anything yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I have pushed the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, it looks good now, thank you for the update!
static/bidder-info/aax.yaml
Outdated
userSync: | ||
redirect: | ||
url: https://c.aaxads.com/aacxc.php?fv=1&wbsh=psa&ryvlg=setstatuscode&bidder=aax&gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&redirect={{.RedirectURL}} | ||
userMacro: ${UUID} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I test this user sync url, I get a 200 response instead of a redirect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that the case with every bidder as well ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope. The expectation is for the user sync endpoint to apply the appropriate privacy policies, decode the redirect url from url encoded, replace the user macro with the user's id, and then perform a 302 redirect to the url. PBS will be the target of the redirect and update the user sync cookie.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SyntaxNode Can you post any ref. of bidder adapter .yaml where we can see this behaviour ? Because when I am hitting /cookie_sync
endpoint I am getting redirect URL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@product-aax Certainly. You can look at appnexus.yaml
and visx.yaml
for fully compliant examples. For appnexus.yaml
:
This is what they define in as their user sync:
redirect:
url: "https://ib.adnxs.com/getuid?{{.RedirectURL}}"
userMacro: "$UID"
Here is an example of the generated url by the cookie sync endpoint. This example comes from the AppNexus Prebid Server host.
https://ib.adnxs.com/getuid?https%3A%2F%2Fprebid.adnxs.com%2Fpbs%2Fv1%2Fsetuid%3Fbidder%3Dadnxs%26gdpr%3D0%26gdpr_consent%3D%26f%3Di%26uid%3D%24UID
The redirect url looks like the following when it is url decoded:
https://prebid.adnxs.com/pbs/v1/setuid?bidder=adnxs&gdpr=0&gdpr_consent=&f=i&uid=$UID
The AppNexus user sync endpoint will decode the redirect url, replace the $UID
macro with a the user's value from the AppNexus cookie and respond with the following redirect:
https://ib.adnxs.com/prebid/setuid?bidder=visx&gdpr=0&gdpr_consent=&uid=c8b707fe-bb40-4862-a12c-c0603bfaeb0f
The redirect endpoint (which is usually the same as the PBS host address, but not in this case) will then update the Prebid Server User Sync Cookie. For more information, check out our new adapter doc and our cookie sync spec.
It is common for adapters to use an allow list for the redirect domain name to protect against abuse, although that's not a requirement we have for Prebid Server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok @SyntaxNode, I got it. So see, at out end we have domain whitelisting, so that was the reason when you tried hitting the endpoint you got 200 and not 302 because that redirect url domain was not in our whitelist. I tried hitting the endpoint with rubicon redirect url and it worked. You can now test it with appnexus as well since we have added it in our list. Basically we are providing redirect link to selective domains who are in aax list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok. That makes sense and is a common approach. Could you please:
- Document that behavior in Adding Aax bidder adapter doc prebid.github.io#3696
- Consider commenting out the cookie sync by default in the yaml file so hosts would not get a broken cookie sync out of the box. You can add a comment instructing hosts to contact you to be added to the allow list. Something like:
userSync:
supports:
- redirect
# Aax support user syncing, but requires your host to be added to our allow list, please
# contact us directly at the email address in this file to be added.
#
#redirect:
# url: https://c.aaxads.com/aacxc.php?fv=1&wbsh=psa&ryvlg=setstatuscode&bidder=aax&gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&redirect={{.RedirectURL}}
# userMacro: ${UUID}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SyntaxNode, We have removed the whitelisting service from our end. I think we are good to go. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to test user sync URL and it returns just http 200, no redirects. On the screenshot you provided earlier I see the redirect URL is not encoded, it's uses as is. @SyntaxNode is this a valid case?
My test URL looks like this:
https://c.aaxads.com/aacxc.php?fv=1&wbsh=psa&ryvlg=setstatuscode&bidder=aax&gdpr=&gdpr_consent=&us_privacy=&redirect=https%3A%2F%2Fprebid.adnxs.com%2Fpbs%2Fv1%2Fsetuid%3Fbidder%3Daax%26gdpr%3D%26gdpr_consent%3D%26f%3Di%26uid%3D%24%7BUUID%7D
Other than this code looks good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point @VeronikaSolovei9. I can confirm it works perfectly if the redirect url is not encoded, but it always will be encoded for security. @product-aax looks like you need to support url encoded redirects.
Hi @VeronikaSolovei9, Can we merge this now ? |
Added a comment in User sync discussion |
I think this code looks good and I'm almost ready to approve, but just want to ensure the user sync discussion is resolved. |
Sorry for the inconvenience, while updating whitelisting service at our end we updated userMacro and forgot to push change in this PR. Now user sync issue is resolved. Thanks for pointing out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I used this URL:
https://c.aaxads.com/aacxc.php?fv=1&wbsh=psa&ryvlg=setstatuscode&bidder=aax&gdpr=&gdpr_consent=&us_privacy=&redirect=https%3A%2F%2Fprebid.adnxs.com%2Fpbs%2Fv1%2Fsetuid%3Fbidder%3Daax%26gdpr%3D%26gdpr_consent%3D%26f%3Di%26uid%3D%3Cvsid%3E
Prebid returns 400 but I think this is because this is a new adapter.
capabilities: | ||
app: | ||
mediaTypes: | ||
- banner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that in the docs PR you have mentioned support only for banner
and native
but here you've video
as well. Can you please confirm if it's the docs PR that needs to be updated or does this PR needs an update to remove video
from this list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mansinahar We have added video media type in doc as well. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Adds Aax Bidder Adapter.
Vendor ID: 720
Maintainer Email: product@aax.media