PBS adapter - fix logic for undefined endpoint URLs #7076
Conversation
| if (utils.isPlainObject(option[prop]) && (!option[prop].p1Consent || !option[prop].noP1Consent)) { | ||
| ['p1Consent', 'noP1Consent'].forEach((conUrl) => { | ||
| if (!option[prop][conUrl]) { | ||
| utils.logWarn(`s2sConfig.${prop}.${conUrl} was not defined in manually set s2sConfig. This may prevent PBS requests from executing in certain cases. Please define field manually or instead use the defaultVendor settings of host company (if avialable).`); |
There was a problem hiding this comment.
I've been encouraging folks to keep messages short because they don't minify.
How about something more terse like "s2sConfig.${prop}.${conUrl} not defined. PBS request will be skipped in some P1 scenarios."
There was a problem hiding this comment.
thanks, makes sense. updated.
| @@ -1107,6 +1114,8 @@ export function PrebidServer() { | |||
| requestJson, | |||
| { contentType: 'text/plain', withCredentials: true } | |||
| ); | |||
| } else { | |||
| utils.logError('Prebid Server request did not execute. Please ensure endpoint URL(s) are properly defined in your s2sConfig. Otherwise check for warnings/errors, likely noted earlier in console logs...'); | |||
There was a problem hiding this comment.
Again, with the goal of saving bytes in the package, how about something more terse like
"PBS request not made. Check endpoints."
|
updated messages and added unit test |
| @@ -1097,7 +1104,7 @@ export function PrebidServer() { | |||
| const request = OPEN_RTB_PROTOCOL.buildRequest(s2sBidRequest, bidRequests, validAdUnits, s2sBidRequest.s2sConfig, requestedBidders); | |||
| const requestJson = request && JSON.stringify(request); | |||
| utils.logInfo('BidRequest: ' + requestJson); | |||
| if (request && requestJson) { | |||
| if (request && requestJson && getMatchingConsentUrl(s2sBidRequest.s2sConfig.endpoint, gdprConsent)) { | |||
There was a problem hiding this comment.
Maybe put the URL into a variable. Makes refactoring easier as there's only one place to look for
| it('should block request if config did not define proper URL in endpoint object config', function() { | ||
| let badConfig = utils.deepClone(CONFIG); | ||
| badConfig.endpoint = { noP1Consent: 'https://prebid.adnxs.com/pbs/v1/openrtb2/auction' }; | ||
| config.setConfig({ s2sConfig: badConfig }); | ||
|
|
||
| let badCfgRequest = utils.deepClone(REQUEST); | ||
| badCfgRequest.s2sConfig = badConfig; | ||
|
|
||
| adapter.callBids(badCfgRequest, BID_REQUESTS, addBidResponse, done, ajax); | ||
|
|
||
| expect(server.requests.length).to.equal(0); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Maybe add two more test cases
- for
p1Consentonly configuration (which is probably the more common one) - for an empty object
| it('should block request if config did not define proper URL in endpoint object config', function() { | |
| let badConfig = utils.deepClone(CONFIG); | |
| badConfig.endpoint = { noP1Consent: 'https://prebid.adnxs.com/pbs/v1/openrtb2/auction' }; | |
| config.setConfig({ s2sConfig: badConfig }); | |
| let badCfgRequest = utils.deepClone(REQUEST); | |
| badCfgRequest.s2sConfig = badConfig; | |
| adapter.callBids(badCfgRequest, BID_REQUESTS, addBidResponse, done, ajax); | |
| expect(server.requests.length).to.equal(0); | |
| }); | |
| [ | |
| { p1Consent: 'https://prebid.adnxs.com/pbs/v1/openrtb2/auction' }, | |
| { noP1Consent: 'https://prebid.adnxs.com/pbs/v1/openrtb2/auction' }, | |
| { } | |
| ].forEach(endpoint => { | |
| it('should block request if config did not define proper URL in endpoint object config', function() { | |
| let badConfig = utils.deepClone(CONFIG); | |
| badConfig.endpoint = endpoint; | |
| config.setConfig({ s2sConfig: badConfig }); | |
| let badCfgRequest = utils.deepClone(REQUEST); | |
| badCfgRequest.s2sConfig = badConfig; | |
| adapter.callBids(badCfgRequest, BID_REQUESTS, addBidResponse, done, ajax); | |
| expect(server.requests.length).to.equal(0); | |
| }); | |
| }); |
There was a problem hiding this comment.
Do you mean add a test for when p1Consent is the only property defined and the request contains a no-consent scenario - to test that the request is blocked here as well?
There was a problem hiding this comment.
Ok thanks for confirming. I added some tests guessing that was the case. Please take a look when you have the chance (in addition to the other change you suggested).
|
Thanks for the fast fix @jsnellbaker ❤️ |
|
@bretg is this good to go? We have some publishers that have way to many calls to |
Type of change
Description of change
Fixes #7046
There was an issue present in s2sConfigs if the publisher defined the
endpoint(orsyncEndpoint) object config but didn't define bothp1ConsentandnoP1Consentfields. In this case, the field that wasundefinedwould still try to make a call to PBS with anundefinedURL for that type of user.This PR addresses this scenario by blocking the call to PBS entirely if a particular URL field was
undefinedfor that type of user. It will return an error for this scenario notifying what likely happened.Also added a warning when evaluating the s2sConfig if one of these fields were
undefinedwhen using the object config.TO NOTE:
Publishers still using the old
stringversion of theendpoint(orsyncEndpoint) fields or who relied on thedefaultVendorfeature to populate the endpoint URLs are unaffected by this scenario/fix. For the former, the single URL they define in the s2sConfig is still shared between consent/non-consent users. For the latter, the URLs are still read/assigned based on the configs defined in Prebid.js.