Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 25 additions & 2 deletions src/main/java/org/prebid/server/bidder/ix/IxBidder.java
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,16 @@ public class IxBidder implements Bidder<BidRequest> {
private static final String PBS_VERSION_UNKNOWN = "unknown";

private final String endpointUrl;
private final String defaultAccountId;
private final PrebidVersionProvider prebidVersionProvider;
private final JacksonMapper mapper;

public IxBidder(String endpointUrl, PrebidVersionProvider prebidVersionProvider, JacksonMapper mapper) {
public IxBidder(String endpointUrl,
String defaultAccountId,
PrebidVersionProvider prebidVersionProvider,
JacksonMapper mapper) {
Comment on lines +75 to +78
Copy link
Collaborator

Choose a reason for hiding this comment

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

please format the parameters list

Comment on lines +75 to +78
Copy link
Collaborator

Choose a reason for hiding this comment

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

please add an empty line between lines 78 and 79 (according to the repo's code style)

this.endpointUrl = HttpUtil.validateUrl(Objects.requireNonNull(endpointUrl));
this.defaultAccountId = (defaultAccountId != null) ? defaultAccountId : "";
Copy link
Collaborator

Choose a reason for hiding this comment

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

StringUtils.defaultString(defaultAccountId)

this.prebidVersionProvider = Objects.requireNonNull(prebidVersionProvider);
this.mapper = Objects.requireNonNull(mapper);
}
Expand All @@ -82,6 +87,7 @@ public Result<List<HttpRequest<BidRequest>>> makeHttpRequests(BidRequest bidRequ
final Set<String> siteIds = new HashSet<>();
final List<Imp> imps = new ArrayList<>();
final List<BidderError> errors = new ArrayList<>();
final Set<String> accountIds = new HashSet<>();

for (Imp imp : bidRequest.getImp()) {
try {
Expand All @@ -91,6 +97,11 @@ public Result<List<HttpRequest<BidRequest>>> makeHttpRequests(BidRequest bidRequ
siteIds.add(siteId);
}

final String accountId = impExt.getAccountId();
if (StringUtils.isNotEmpty(accountId)) {
accountIds.add(accountId);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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;


imps.add(modifyImp(imp, impExt));
} catch (PreBidException e) {
errors.add(BidderError.badInput(e.getMessage()));
Expand All @@ -102,12 +113,24 @@ public Result<List<HttpRequest<BidRequest>>> makeHttpRequests(BidRequest bidRequ
}

final BidRequest modifiedBidRequest = modifyBidRequest(bidRequest, imps, siteIds);

final String resolvedAccountId = accountIds.isEmpty()
? null
: accountIds.iterator().next();
Comment on lines +117 to +119
Copy link
Collaborator

Choose a reason for hiding this comment

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


final String resolvedEndpoint = resolveEndpointMacro(endpointUrl, resolvedAccountId);

final List<HttpRequest<BidRequest>> httpRequests = Collections.singletonList(
BidderUtil.defaultRequest(modifiedBidRequest, endpointUrl, mapper));
BidderUtil.defaultRequest(modifiedBidRequest, resolvedEndpoint, mapper));

return Result.of(httpRequests, errors);
}

private String resolveEndpointMacro(String endpoint, String accountId) {
final String resolvedAccountId = StringUtils.isNotEmpty(accountId) ? accountId : this.defaultAccountId;
return endpoint.replace("${IX_ACCOUNT_ID}", resolvedAccountId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. the macro should have the following pattern {{AccountId}}
  2. please extract the macro name as a constant
  3. 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
  4. use the HttpUtil.encode(resolvedAccountId) if the account is a query parameter

}

private ExtImpIx parseImpExt(Imp imp) {
try {
return mapper.mapper().convertValue(imp.getExt(), IX_EXT_TYPE_REFERENCE).getBidder();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,8 @@ public class ExtImpIx {
List<Integer> size;

String sid;

@JsonProperty("accountId")
@JsonAlias({"accountid", "ixAccountId"})
String accountId;
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,11 @@ BidderDeps ixBidderDeps(BidderConfigurationProperties ixConfigurationProperties,
return BidderDepsAssembler.forBidder(BIDDER_NAME)
.withConfig(ixConfigurationProperties)
.usersyncerCreator(UsersyncerCreator.create(externalUrl))
.bidderCreator(config -> new IxBidder(config.getEndpoint(), prebidVersionProvider, mapper))
.bidderCreator(config -> new IxBidder(
config.getEndpoint(),
config.getDefaultAccountId(),
prebidVersionProvider,
mapper))
.assemble();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ public class BidderConfigurationProperties {
@NotBlank
private String endpoint;

private String defaultAccountId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


private Boolean pbsEnforcesCcpa;

private Boolean modifyingVastXmlAllowed;
Expand Down
70 changes: 53 additions & 17 deletions src/test/java/org/prebid/server/bidder/ix/IxBidderTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@
@ExtendWith(MockitoExtension.class)
public class IxBidderTest extends VertxTest {

private static final String ENDPOINT_URL = "http://exchange.org/";
private static final String ENDPOINT_URL = "http://exchange.org/?s=${IX_ACCOUNT_ID}";
private static final String DEFAULT_ACCOUNT_ID = "123456";
private static final String SITE_ID = "site id";

@Mock(strictness = LENIENT)
Expand All @@ -77,14 +78,14 @@ public class IxBidderTest extends VertxTest {

@BeforeEach
public void setUp() {
target = new IxBidder(ENDPOINT_URL, prebidVersionProvider, jacksonMapper);
target = new IxBidder(ENDPOINT_URL, DEFAULT_ACCOUNT_ID, prebidVersionProvider, jacksonMapper);
given(prebidVersionProvider.getNameVersionRecord()).willReturn(null);
}

@Test
public void creationShouldFailOnInvalidEndpointUrl() {
assertThatIllegalArgumentException().isThrownBy(
() -> new IxBidder("invalid_url", prebidVersionProvider, jacksonMapper));
() -> new IxBidder("invalid_url", DEFAULT_ACCOUNT_ID, prebidVersionProvider, jacksonMapper));
}

@Test
Expand All @@ -107,7 +108,7 @@ public void makeHttpRequestsShouldReturnErrorIfImpExtCouldNotBeParsed() {
public void makeHttpRequestsShouldModifyImpExtWithSid() {
// given
final BidRequest bidRequest = givenBidRequest(
impBuilder -> impBuilder.ext(givenImpExt(null, "sid")));
impBuilder -> impBuilder.ext(givenImpExt(null, "sid", null)));

// when
final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest);
Expand All @@ -128,7 +129,7 @@ public void makeHttpRequestsShouldSetImpBannerFormatsToFormatWithWidthAndHeightI
final BidRequest bidRequest = givenBidRequest(
impBuilder -> impBuilder
.banner(Banner.builder().w(1).h(2).build())
.ext(givenImpExt(null, null)));
.ext(givenImpExt(null, null, null)));

// when
final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest);
Expand All @@ -154,7 +155,7 @@ public void makeHttpRequestsShouldSetImpBannerWidthAndHeightIfTheyAreAbsentAndBa
final BidRequest bidRequest = givenBidRequest(
impBuilder -> impBuilder
.banner(Banner.builder().format(singletonList(Format.builder().w(1).h(2).build())).build())
.ext(givenImpExt(null, null)));
.ext(givenImpExt(null, null, null)));

// when
final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest);
Expand Down Expand Up @@ -220,8 +221,8 @@ public void makeHttpRequestsShouldReturnIxDiagWithPbsVersion() {
public void makeHttpRequestsShouldReturnIxDiagWithMultipleSiteIdsWhenMultipleImpExtSiteIdPresent() {
// given
final BidRequest bidRequest = givenBidRequest(
impBuilder -> impBuilder.ext(givenImpExt("site1", null)),
impBuilder -> impBuilder.ext(givenImpExt("site2", null)));
impBuilder -> impBuilder.ext(givenImpExt("site1", null, null)),
impBuilder -> impBuilder.ext(givenImpExt("site2", null, null)));

// when
final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest);
Expand Down Expand Up @@ -255,8 +256,8 @@ public void makeHttpRequestsShouldPassThroughProvidedIxDiagFields() {
"pbjsv1",
givenIxDiag)),
List.of(
impBuilder -> impBuilder.ext(givenImpExt("site1", null)),
impBuilder -> impBuilder.ext(givenImpExt("site2", null))));
impBuilder -> impBuilder.ext(givenImpExt("site1", null, null)),
impBuilder -> impBuilder.ext(givenImpExt("site2", null, null))));

// when
final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest);
Expand Down Expand Up @@ -284,7 +285,7 @@ public void makeHttpRequestsShouldModifyRequestSiteWithPublisherAndSetIdWhenImpE
// given
final BidRequest bidRequest = givenBidRequest(
bidRequestBuilder -> bidRequestBuilder.site(Site.builder().build()),
singletonList(impBuilder -> impBuilder.ext(givenImpExt("site1", null))));
singletonList(impBuilder -> impBuilder.ext(givenImpExt("site1", null, null))));

// when
final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest);
Expand All @@ -304,7 +305,7 @@ public void makeHttpRequestsShouldModifyRequestSiteWithPublisherAndSetIdWhenImpE
public void makeHttpRequestsShouldNotCreateRequestSiteWhenImpExtSiteIdPresentAndSiteIsAbsent() {
// given
final BidRequest bidRequest = givenBidRequest(
impBuilder -> impBuilder.ext(givenImpExt("site1", null)));
impBuilder -> impBuilder.ext(givenImpExt("site1", null, null)));

// when
final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest);
Expand All @@ -322,7 +323,7 @@ public void makeHttpRequestsShouldModifyRequestAppWithPublisherAndSetIdWhenImpEx
// given
final BidRequest bidRequest = givenBidRequest(
bidRequestBuilder -> bidRequestBuilder.app(App.builder().build()),
singletonList(impBuilder -> impBuilder.ext(givenImpExt("site1", null))));
singletonList(impBuilder -> impBuilder.ext(givenImpExt("site1", null, null))));

// when
final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest);
Expand All @@ -342,7 +343,7 @@ public void makeHttpRequestsShouldModifyRequestAppWithPublisherAndSetIdWhenImpEx
public void makeHttpRequestsShouldNotCreateRequestAppWhenImpExtSiteIdPresentAndSiteIsAbsent() {
// given
final BidRequest bidRequest = givenBidRequest(
impBuilder -> impBuilder.ext(givenImpExt("site1", null)));
impBuilder -> impBuilder.ext(givenImpExt("site1", null, null)));

// when
final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest);
Expand All @@ -355,6 +356,41 @@ public void makeHttpRequestsShouldNotCreateRequestAppWhenImpExtSiteIdPresentAndS
.containsOnlyNulls();
}

@Test
public void makeHttpRequestsShouldReplaceAccountIdMacroInEndpointUrlWithEXTAccountId() {
// given
final String accountId = "account123";
final BidRequest bidRequest = givenBidRequest(
impBuilder -> impBuilder.ext(givenImpExt("site1", "sid", accountId)));

// when
final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest);

// then
assertThat(result.getErrors()).isEmpty();
assertThat(result.getValue()).hasSize(1);
assertThat(result.getValue().get(0).getUri())
.doesNotContain("${IX_ACCOUNT_ID}")
.contains(accountId);
Comment on lines +370 to +374
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use extracting approach as any other tests in the class

}

@Test
public void makeHttpRequestsShouldReplaceAccountIdMacroInEndpointUrlWithDefaultAccountId() {
// given
final BidRequest bidRequest = givenBidRequest(
impBuilder -> impBuilder.ext(givenImpExt("site1", "sid", null)));

// when
final Result<List<HttpRequest<BidRequest>>> result = target.makeHttpRequests(bidRequest);

// then
assertThat(result.getErrors()).isEmpty();
assertThat(result.getValue()).hasSize(1);
assertThat(result.getValue().get(0).getUri())
.doesNotContain("${IX_ACCOUNT_ID}")
.contains(DEFAULT_ACCOUNT_ID);
}

@Test
public void makeBidderResponseShouldReturnErrorIfResponseBodyCouldNotBeParsed() {
// given
Expand Down Expand Up @@ -937,8 +973,8 @@ private static ExtRequest givenExtRequestWithIxDiag(String pbjsv, ObjectNode ixD
return extRequest;
}

private static ObjectNode givenImpExt(String siteId, String sid) {
return mapper.valueToTree(ExtPrebid.of(null, ExtImpIx.of(siteId, null, sid)));
private static ObjectNode givenImpExt(String siteId, String sid, String accountId) {
return mapper.valueToTree(ExtPrebid.of(null, ExtImpIx.of(siteId, null, sid, accountId)));
Comment on lines +976 to +977
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's create a separate overloaded givenImpExt without an accountId in order not to pass nulls everywhere

}

private static BidRequest givenBidRequest(UnaryOperator<Imp.ImpBuilder>... impCustomizers) {
Expand All @@ -961,7 +997,7 @@ private static Imp givenImp(Function<Imp.ImpBuilder, Imp.ImpBuilder> impCustomiz
return impCustomizer.apply(Imp.builder()
.id("123")
.banner(Banner.builder().w(1).h(1).build())
.ext(mapper.valueToTree(ExtPrebid.of(null, ExtImpIx.of(SITE_ID, null, null)))))
.ext(mapper.valueToTree(ExtPrebid.of(null, ExtImpIx.of(SITE_ID, null, null, null)))))
.build();
}

Expand Down
Loading