Skip to content
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.

Sign up for GitHub

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 Module: Bid response filter #12147

Merged
merged 9 commits into from
Oct 8, 2024
Merged

New Module: Bid response filter #12147

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ce5bcdb
initial commit
Aug 14, 2024
a89fcbd
update
Aug 21, 2024
ed8dda8
review changes
Aug 27, 2024
5697f4e
+ auction index
Aug 27, 2024
585bb46
unit tests to ortb converter
Sep 2, 2024
66bc55a
review changes
Sep 10, 2024
8366048
battr setting
Sep 11, 2024
f90c448
improvements
Sep 11, 2024
fb9458c
fix log message
dgirardi Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions libraries/ortbConverter/processors/default.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,13 @@ export const DEFAULT_PROCESSORS = {
if (bid.ext?.dsa) {
bidResponse.meta.dsa = bid.ext.dsa;
}
if (bid.cat) {
bidResponse.meta.secondaryCatIds = bid.cat;
bidResponse.meta.primaryCatId = bid.cat[0];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is a primary category also a secondary category?

}
if (bid.attr) {
bidResponse.meta.attr = bid.attr;
}
}
}
}
Expand Down
39 changes: 39 additions & 0 deletions modules/bidResponseFilter/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import { config } from '../../src/config.js';
import { getHook } from '../../src/hook.js';

export const MODULE_NAME = 'bidResponseFilter';
export const PUBLISHER_FILTER_REJECTION_REASON = 'PUBLISHER_FILTER_REJECTION_REASON';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these should be human readable

export const BID_CATEGORY_REJECTION_REASON = 'BID_CATEGORY_REJECTION_REASON';
export const BID_ADV_DOMAINS_REJECTION_REASON = 'BID_ADV_DOMAINS_REJECTION_REASON';
export const BID_ATTR_REJECTION_REASON = 'BID_ATTR_REJECTION_REASON';

function init() {
getHook('addBidResponse').before(addBidResponseHook);
};

export function addBidResponseHook(next, adUnitCode, bid, reject) {
const { bcat = [], badv = [], battr = [] } = config.getAnyConfig('ortb2') || {};
Copy link
Collaborator

@dgirardi dgirardi Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ortb2 can be different for each bid response. You can get a specific response's with auctionManager.index.getBidderRequest(bid)?.ortb2; however, responses can also not be associated with any request, in which case the best avialable ortb2 is the one associated with the auction (like you used in #11946)

It may be worth to add a getOrtb2 method to auctionManager.idex - similar to getMediaTypes, which also has a fallback for when there is no request.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that battr is not here - it's repeated for each media type in imp. It's not clear where the publisher should set it - candidates are ortb2Imp[mediaType].battr and mediaTypes[mediaType].battr

Assuming the first one should have priority, we could:

  • update ad unit validation for each media type to find battr from ortb2Imp[type].battr, falling back to mediaTypes[type].battr, and copy it to both places
  • add some unit test for ortbConverter banner, video and native to make sure the "right" battr wins
  • this should then look for battr from index.getBidRequest(bid).ortb2Imp[bid.mediaType].battr, falling back to index.getAdUnit(bid).ortb2Imp[bid.mediaType].battr when there is no request for the bid

(pending confirmation on the relative priority of ortb2Imp vs mediaTypes)

const moduleConfig = config.getConfig(MODULE_NAME);

const catConfig = moduleConfig?.cat || { enforce: true, block_unknown_cat: true };
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if I

setConfig({bidResponseFilter: {cat: {block_unknown_cat: false}}})

my expectation would be that bcat would still be enforced (e.g., enforce defaults to true), but I think this would disable it.

const advConfig = moduleConfig?.adv || { enforce: true, block_unknown_adomain: true };
Copy link
Collaborator

@dgirardi dgirardi Aug 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably out of scope, but I don't like these parameter names. attr: {block_uknown_attr} is redundant; and we do not use snake_case anywhere else. It's also inconsistent within itself (why cat/block_unknown_cat, but adv/block_unknown_adomain?)

If we weren't following PBS this would be something like adv: {enforce, blockUnknown}. Do we still prefer PBS's syntax @patmmccann ?

Copy link
Collaborator

@patmmccann patmmccann Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those are good reasons to abandon the hope of matching the syntax. oh well

const attrConfig = moduleConfig?.attr || { enforce: true, block_unknown_attr: true };

const { primaryCatId, secondaryCatIds = [], advertiserDomains = [], attr: metaAttr } = bid.meta || {};

// checking if bid fulfills ortb2 fields rules
if ((catConfig.enforce && bcat.some(category => [primaryCatId, ...secondaryCatIds].includes(category))) ||
(catConfig.block_unknown_cat && !primaryCatId)) {
reject(BID_CATEGORY_REJECTION_REASON);
} else if ((advConfig.enforce && badv.some(domain => advertiserDomains.includes(domain))) ||
(advConfig.block_unknown_adomain && !advertiserDomains.length)) {
reject(BID_ADV_DOMAINS_REJECTION_REASON);
} else if ((attrConfig.enforce && battr.includes(metaAttr)) ||
(attrConfig.block_unknown_attr && !metaAttr)) {
reject(BID_ATTR_REJECTION_REASON);
} else {
return next(adUnitCode, bid, reject);
}
}

init();
117 changes: 117 additions & 0 deletions test/spec/modules/bidResponseFilter_spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
import { BID_ADV_DOMAINS_REJECTION_REASON, BID_ATTR_REJECTION_REASON, BID_CATEGORY_REJECTION_REASON, MODULE_NAME, PUBLISHER_FILTER_REJECTION_REASON, addBidResponseHook } from '../../../modules/bidResponseFilter';
import { config } from '../../../src/config';

describe('bidResponseFilter', () => {
afterEach(() => {
config.resetConfig();
});

it('should pass the bid after successful ortb2 rules validation', () => {
const call = sinon.stub();
const bid = {
meta: {
advertiserDomains: ['domain1.com', 'domain2.com'],
primaryCatId: 'EXAMPLE-CAT-ID',
attr: 'attr'
}
};
config.setConfig({ortb2: {
badv: [], bcat: ['BANNED_CAT1', 'BANNED_CAT2'], battr: 'BANNED_ATTR'
}});

addBidResponseHook(call, 'adcode', bid, () => {});
sinon.assert.calledOnce(call);
});

it('should reject the bid after failed ortb2 cat rule validation', () => {
const reject = sinon.stub();
const call = sinon.stub();
const bid = {
meta: {
advertiserDomains: ['domain1.com', 'domain2.com'],
primaryCatId: 'BANNED_CAT1',
attr: 'attr'
}
};
config.setConfig({ortb2: {
badv: [], bcat: ['BANNED_CAT1', 'BANNED_CAT2'], battr: 'BANNED_ATTR'
}});

addBidResponseHook(call, 'adcode', bid, reject);
sinon.assert.calledWith(reject, BID_CATEGORY_REJECTION_REASON);
});

it('should reject the bid after failed ortb2 adv domains rule validation', () => {
const rejection = sinon.stub();
const call = sinon.stub();
const bid = {
meta: {
advertiserDomains: ['domain1.com', 'domain2.com'],
primaryCatId: 'VALID_CAT',
attr: 'attr'
}
};
config.setConfig({ortb2: {
badv: ['domain2.com'], bcat: ['BANNED_CAT1', 'BANNED_CAT2'], battr: 'BANNED_ATTR'
}});

addBidResponseHook(call, 'adcode', bid, rejection);
sinon.assert.calledWith(rejection, BID_ADV_DOMAINS_REJECTION_REASON);
});

it('should reject the bid after failed ortb2 attr rule validation', () => {
const reject = sinon.stub();
const call = sinon.stub();
const bid = {
meta: {
advertiserDomains: ['validdomain1.com', 'validdomain2.com'],
primaryCatId: 'VALID_CAT',
attr: 'BANNED_ATTR'
}
};
config.setConfig({ortb2: {
badv: ['domain2.com'], bcat: ['BANNED_CAT1', 'BANNED_CAT2'], battr: 'BANNED_ATTR'
}});

addBidResponseHook(call, 'adcode', bid, reject);
sinon.assert.calledWith(reject, BID_ATTR_REJECTION_REASON);
});

it('should omit the validation if the flag is set to false', () => {
const call = sinon.stub();
const bid = {
meta: {
advertiserDomains: ['validdomain1.com', 'validdomain2.com'],
primaryCatId: 'BANNED_CAT1',
attr: 'valid_attr'
}
};
config.setConfig({ortb2: {
badv: ['domain2.com'], bcat: ['BANNED_CAT1', 'BANNED_CAT2'], battr: 'BANNED_ATTR'
}});

config.setConfig({ [MODULE_NAME]: { cat: { enforce: false }} });

addBidResponseHook(call, 'adcode', bid, () => {});
sinon.assert.calledOnce(call);
});

it('should allow bid for unknown flag set to false', () => {
const call = sinon.stub();
const bid = {
meta: {
advertiserDomains: ['validdomain1.com', 'validdomain2.com'],
primaryCatId: undefined,
attr: 'valid_attr'
}
};
config.setConfig({ortb2: {
badv: ['domain2.com'], bcat: ['BANNED_CAT1', 'BANNED_CAT2'], battr: 'BANNED_ATTR'
}});

config.setConfig({ [MODULE_NAME]: { cat: { block_unknown_cat: false }} });

addBidResponseHook(call, 'adcode', bid, () => {});
sinon.assert.calledOnce(call);
});
})