IX Adapter feature dynamic account id support #4218
Conversation
|
Hi @mmoschovas Do you have a PR for PBS Go? If yes, please add the link here |
| public IxBidder(String endpointUrl, | ||
| String defaultAccountId, | ||
| PrebidVersionProvider prebidVersionProvider, | ||
| JacksonMapper mapper) { |
There was a problem hiding this comment.
please format the parameters list
| public IxBidder(String endpointUrl, | ||
| String defaultAccountId, | ||
| PrebidVersionProvider prebidVersionProvider, | ||
| JacksonMapper mapper) { |
There was a problem hiding this comment.
please add an empty line between lines 78 and 79 (according to the repo's code style)
| PrebidVersionProvider prebidVersionProvider, | ||
| JacksonMapper mapper) { | ||
| this.endpointUrl = HttpUtil.validateUrl(Objects.requireNonNull(endpointUrl)); | ||
| this.defaultAccountId = (defaultAccountId != null) ? defaultAccountId : ""; |
There was a problem hiding this comment.
StringUtils.defaultString(defaultAccountId)
| @NotBlank | ||
| private String endpoint; | ||
|
|
||
| private String defaultAccountId; |
There was a problem hiding this comment.
this property should be added only for the IX bidder, not for all bidders, please create a custom BidderConfigurationProperties for the IX bidder
please check the SmartadserverConfiguration for inspiration
| final String accountId = impExt.getAccountId(); | ||
| if (StringUtils.isNotEmpty(accountId)) { | ||
| accountIds.add(accountId); | ||
| } |
There was a problem hiding this comment.
looks like the set of accounts isn't really needed
please replace the final Set<String> accountIds = new HashSet<>(); with the String accountId = null and the following code
final String accountId = impExt.getAccountId();
if (StringUtils.isNotEmpty(accountId)) {
accountIds.add(accountId);
}with the
accountId = accountId == null ? impExt.getAccountId() : accountId;| final String resolvedAccountId = accountIds.isEmpty() | ||
| ? null | ||
| : accountIds.iterator().next(); |
There was a problem hiding this comment.
This code is not needed after implementing https://github.com/prebid/prebid-server-java/pull/4218/files#r2406121593
|
|
||
| private String resolveEndpointMacro(String endpoint, String accountId) { | ||
| final String resolvedAccountId = StringUtils.isNotEmpty(accountId) ? accountId : this.defaultAccountId; | ||
| return endpoint.replace("${IX_ACCOUNT_ID}", resolvedAccountId); |
There was a problem hiding this comment.
- the macro should have the following pattern
{{AccountId}} - please extract the macro name as a constant
- I suggest adding this macro to the IX bidder endpoint in the
ix.yaml, but since the whole endpoint is absent, it would be nice to provide the example of the endpoint with the macro provided - use the
HttpUtil.encode(resolvedAccountId)if the account is a query parameter
| private static ObjectNode givenImpExt(String siteId, String sid, String accountId) { | ||
| return mapper.valueToTree(ExtPrebid.of(null, ExtImpIx.of(siteId, null, sid, accountId))); |
There was a problem hiding this comment.
let's create a separate overloaded givenImpExt without an accountId in order not to pass nulls everywhere
| assertThat(result.getErrors()).isEmpty(); | ||
| assertThat(result.getValue()).hasSize(1); | ||
| assertThat(result.getValue().get(0).getUri()) | ||
| .doesNotContain("${IX_ACCOUNT_ID}") | ||
| .contains(accountId); |
There was a problem hiding this comment.
please use extracting approach as any other tests in the class
|
@mmoschovas any updates? |
3 participants
🔧 Type of changes
✨ What's the context?
What's the context for the changes?
Macro support for IX bid endpoint. Ability to pass accountId to PBS for IX to be autopopulated in endpoint to handle multiople account id scenarios
🧠 Rationale behind the change
Why did you choose to make these changes? Were there any trade-offs you had to consider?
See above