Skip to content

Commit

Permalink
Fix account fetching from stored requests for auctions (#1355)
Browse files Browse the repository at this point in the history
  • Loading branch information
@rpanchyk
rpanchyk authored Jul 6, 2021
1 parent ea6f28f commit e008d1a
Show file tree
Hide file tree
Showing 8 changed files with 53 additions and 46 deletions.
2 changes: 1 addition & 1 deletion src/main/java/org/prebid/server/auction/requestfactory/AmpRequestFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ public Future<AuctionContext> fromRequest(RoutingContext routingContext, long st
.map(bidRequest -> ortb2RequestFactory.enrichAuctionContext(
initialAuctionContext, httpRequest, bidRequest, startTime)))

.compose(auctionContext -> ortb2RequestFactory.fetchAccount(auctionContext, false)
.compose(auctionContext -> ortb2RequestFactory.fetchAccount(auctionContext)
.map(auctionContext::with))

.compose(auctionContext -> updateBidRequest(auctionContext)
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/prebid/server/auction/requestfactory/AuctionRequestFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ public Future<AuctionContext> fromRequest(RoutingContext routingContext, long st
.enrichAuctionContext(initialAuctionContext, httpRequest, bidRequest, startTime)
.with(requestTypeMetric(bidRequest))))

.compose(auctionContext -> ortb2RequestFactory.fetchAccount(auctionContext, true)
.compose(auctionContext -> ortb2RequestFactory.fetchAccount(auctionContext)
.map(auctionContext::with))

.compose(auctionContext -> ortb2RequestFactory.executeRawAuctionRequestHooks(auctionContext)
Expand Down
10 changes: 9 additions & 1 deletion src/main/java/org/prebid/server/auction/requestfactory/Ortb2RequestFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,15 @@ public AuctionContext enrichAuctionContext(AuctionContext auctionContext,
.build();
}

public Future<Account> fetchAccount(AuctionContext auctionContext, boolean isLookupStoredRequest) {
public Future<Account> fetchAccountWithoutStoredRequestLookup(AuctionContext auctionContext) {
return fetchAccount(auctionContext, false);
}

public Future<Account> fetchAccount(AuctionContext auctionContext) {
return fetchAccount(auctionContext, true);
}

private Future<Account> fetchAccount(AuctionContext auctionContext, boolean isLookupStoredRequest) {
final BidRequest bidRequest = auctionContext.getBidRequest();
final Timeout timeout = auctionContext.getTimeout();
final HttpRequestContext httpRequest = auctionContext.getHttpRequest();
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/org/prebid/server/auction/requestfactory/VideoRequestFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ public Future<WithPodErrors<AuctionContext>> fromRequest(RoutingContext routingC
.map(bidRequestWithErrors -> ortb2RequestFactory.enrichAuctionContext(
initialAuctionContext, httpRequest, bidRequestWithErrors.getData(), startTime)))

.compose(auctionContext -> ortb2RequestFactory.fetchAccount(auctionContext, false)
.compose(auctionContext -> ortb2RequestFactory.fetchAccountWithoutStoredRequestLookup(auctionContext)
.map(auctionContext::with))

.compose(auctionContext -> privacyEnforcementService.contextFromBidRequest(auctionContext)
Expand Down
3 changes: 1 addition & 2 deletions src/test/java/org/prebid/server/auction/requestfactory/AmpRequestFactoryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.tuple;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.anyList;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.anyString;
Expand Down Expand Up @@ -1435,7 +1434,7 @@ private void givenBidRequest(
.httpRequest((HttpRequestContext) invocationOnMock.getArguments()[1])
.bidRequest((BidRequest) invocationOnMock.getArguments()[2])
.build());
given(ortb2RequestFactory.fetchAccount(any(), anyBoolean())).willReturn(Future.succeededFuture());
given(ortb2RequestFactory.fetchAccount(any())).willReturn(Future.succeededFuture());

given(ortb2ImplicitParametersResolver.resolve(any(), any(), any())).willAnswer(
answerWithFirstArgument());
Expand Down
5 changes: 2 additions & 3 deletions src/test/java/org/prebid/server/auction/requestfactory/AuctionRequestFactoryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@
import static org.apache.commons.lang3.StringUtils.EMPTY;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.BDDMockito.given;
Expand Down Expand Up @@ -461,7 +460,7 @@ public void shouldCallOrtbFieldsResolver() {
public void shouldReturnFailedFutureIfOrtb2RequestFactoryReturnedFailedFuture() {
// given
givenValidBidRequest(BidRequest.builder().build());
given(ortb2RequestFactory.fetchAccount(any(), anyBoolean())).willReturn(Future.failedFuture("error"));
given(ortb2RequestFactory.fetchAccount(any())).willReturn(Future.failedFuture("error"));

// when
final Future<?> future = target.fromRequest(routingContext, 0L);
Expand Down Expand Up @@ -603,7 +602,7 @@ private void givenAuctionContext(BidRequest bidRequest, Account account) {
.willReturn(defaultActionContext.toBuilder()
.bidRequest(bidRequest)
.build());
given(ortb2RequestFactory.fetchAccount(any(), anyBoolean())).willReturn(Future.succeededFuture(account));
given(ortb2RequestFactory.fetchAccount(any())).willReturn(Future.succeededFuture(account));
}

private void givenProcessStoredRequest(BidRequest bidRequest) {
Expand Down
67 changes: 35 additions & 32 deletions src/test/java/org/prebid/server/auction/requestfactory/Ortb2RequestFactoryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,13 +172,15 @@ public void fetchAccountShouldReturnFailedFutureIfAccountIsEnforcedAndIdIsNotPro
ipAddressHelper,
hookStageExecutor);

given(storedRequestProcessor.processStoredRequests(any(), any()))
.willReturn(Future.succeededFuture(givenBidRequest(identity())));

// when
final Future<?> future = target.fetchAccount(
AuctionContext.builder()
.httpRequest(httpRequest)
.bidRequest(defaultBidRequest)
.build(),
false);
.build());

// then
verify(applicationSettings, never()).getAccountById(any(), any());
Expand Down Expand Up @@ -218,8 +220,7 @@ public void fetchAccountShouldReturnFailedFutureIfAccountIsEnforcedAndFailedGetA
AuctionContext.builder()
.httpRequest(httpRequest)
.bidRequest(bidRequest)
.build(),
false);
.build());

// then
verify(applicationSettings).getAccountById(eq(accountId), any());
Expand Down Expand Up @@ -247,8 +248,7 @@ public void fetchAccountShouldReturnFailedFutureIfAccountIsInactive() {

// when
final Future<?> future = target.fetchAccount(
AuctionContext.builder().bidRequest(bidRequest).build(),
false);
AuctionContext.builder().bidRequest(bidRequest).build());

// then
assertThat(future.failed()).isTrue();
Expand All @@ -267,8 +267,7 @@ public void fetchAccountShouldReturnFailedFutureWhenAccountIdIsBlacklisted() {

// when
final Future<?> result = target.fetchAccount(
AuctionContext.builder().bidRequest(bidRequest).build(),
false);
AuctionContext.builder().bidRequest(bidRequest).build());

// then
assertThat(result.failed()).isTrue();
Expand All @@ -295,8 +294,7 @@ public void fetchAccountShouldReturnAccountWithAccountIdTakenFromPublisherExt()

// when
final Future<Account> result = target.fetchAccount(
AuctionContext.builder().bidRequest(bidRequest).build(),
false);
AuctionContext.builder().bidRequest(bidRequest).build());

// then
verify(applicationSettings).getAccountById(eq(parentAccount), any());
Expand All @@ -318,8 +316,7 @@ public void fetchAccountShouldReturnAccountWithAccountIdTakenFromPublisherIdWhen

// when
final Future<Account> result = target.fetchAccount(
AuctionContext.builder().bidRequest(bidRequest).build(),
false);
AuctionContext.builder().bidRequest(bidRequest).build());

// then
verify(applicationSettings).getAccountById(eq(accountId), any());
Expand All @@ -343,8 +340,7 @@ public void fetchAccountShouldReturnAccountWithAccountIdTakenFromPublisherIdWhen

// when
final Future<Account> result = target.fetchAccount(
AuctionContext.builder().bidRequest(bidRequest).build(),
false);
AuctionContext.builder().bidRequest(bidRequest).build());

// then
verify(applicationSettings).getAccountById(eq(accountId), any());
Expand All @@ -368,8 +364,7 @@ public void fetchAccountShouldReturnAccountWithAccountIdTakenFromPublisherIdWhen

// when
final Future<Account> result = target.fetchAccount(
AuctionContext.builder().bidRequest(bidRequest).build(),
false);
AuctionContext.builder().bidRequest(bidRequest).build());

// then
verify(applicationSettings).getAccountById(eq("accountId"), any());
Expand All @@ -392,8 +387,7 @@ public void fetchAccountShouldReturnAccountWithAccountIdTakenFromAppPublisherId(

// when
final Future<Account> result = target.fetchAccount(
AuctionContext.builder().bidRequest(bidRequest).build(),
false);
AuctionContext.builder().bidRequest(bidRequest).build());

// then
verify(applicationSettings).getAccountById(eq(accountId), any());
Expand All @@ -420,8 +414,7 @@ public void fetchAccountShouldReturnEmptyAccountIfNotFound() {
AuctionContext.builder()
.httpRequest(httpRequest)
.bidRequest(bidRequest)
.build(),
false);
.build());

// then
verify(applicationSettings).getAccountById(eq(parentAccount), any());
Expand All @@ -443,8 +436,7 @@ public void fetchAccountShouldReturnEmptyAccountIfExceptionOccurred() {

// when
final Future<Account> result = target.fetchAccount(
AuctionContext.builder().bidRequest(bidRequest).build(),
false);
AuctionContext.builder().bidRequest(bidRequest).build());

// then
verify(applicationSettings).getAccountById(eq(accountId), any());
Expand All @@ -457,6 +449,9 @@ public void fetchAccountShouldReturnEmptyAccountIfItIsMissingInRequest() {
// given
final BidRequest bidRequest = givenBidRequest(identity());

given(storedRequestProcessor.processStoredRequests(any(), any()))
.willReturn(Future.succeededFuture(bidRequest));

given(applicationSettings.getAccountById(any(), any()))
.willReturn(Future.failedFuture(new RuntimeException("error")));

Expand All @@ -465,8 +460,7 @@ public void fetchAccountShouldReturnEmptyAccountIfItIsMissingInRequest() {
AuctionContext.builder()
.httpRequest(httpRequest)
.bidRequest(bidRequest)
.build(),
false);
.build());

// then
verifyZeroInteractions(applicationSettings);
Expand Down Expand Up @@ -497,8 +491,7 @@ public void shouldFetchAccountFromStoredIfStoredLookupIsTrueAndAccountIsNotFound
AuctionContext.builder()
.httpRequest(httpRequest)
.bidRequest(receivedBidRequest)
.build(),
true);
.build());

// then
verify(storedRequestProcessor).processStoredRequests("", receivedBidRequest);
Expand All @@ -524,8 +517,7 @@ public void shouldFetchAccountFromStoredAndReturnFailedFutureWhenAccountIdIsBlac
AuctionContext.builder()
.httpRequest(httpRequest)
.bidRequest(receivedBidRequest)
.build(),
true);
.build());

// then
verify(storedRequestProcessor).processStoredRequests("", receivedBidRequest);
Expand Down Expand Up @@ -562,8 +554,7 @@ public void shouldFetchAccountFromStoredAndReturnFailedFutureIfValidIsEnforcedAn
AuctionContext.builder()
.httpRequest(httpRequest)
.bidRequest(receivedBidRequest)
.build(),
true);
.build());

// then
verify(storedRequestProcessor).processStoredRequests("", receivedBidRequest);
Expand All @@ -585,8 +576,7 @@ public void shouldFetchAccountFromStoredAndReturnEmptyAccountIfStoredLookupIsFai
AuctionContext.builder()
.httpRequest(httpRequest)
.bidRequest(receivedBidRequest)
.build(),
true);
.build());

// then
verify(storedRequestProcessor).processStoredRequests("", receivedBidRequest);
Expand All @@ -596,6 +586,19 @@ public void shouldFetchAccountFromStoredAndReturnEmptyAccountIfStoredLookupIsFai
assertThat(result.cause()).hasMessage("error");
}

@Test
public void fetchAccountWithoutStoredRequestLookupShouldNeverCallStoredProcessor() {
// when
target.fetchAccountWithoutStoredRequestLookup(
AuctionContext.builder()
.httpRequest(httpRequest)
.bidRequest(givenBidRequest(identity()))
.build());

// then
verifyZeroInteractions(storedRequestProcessor);
}

@Test
public void createAuctionContextShouldReturnExpectedAuctionContext() {
// when
Expand Down
8 changes: 3 additions & 5 deletions src/test/java/org/prebid/server/auction/requestfactory/VideoRequestFactoryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@
import static org.apache.commons.lang3.StringUtils.EMPTY;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.ArgumentMatchers.eq;
Expand Down Expand Up @@ -290,9 +289,8 @@ public void shouldReturnExpectedResultAndReturnErrors() throws JsonProcessingExc
verify(videoStoredRequestProcessor).processVideoRequest("", null, emptySet(), requestVideo);
verify(ortb2RequestFactory).createAuctionContext(any(), eq(MetricName.video));
verify(ortb2RequestFactory).enrichAuctionContext(any(), any(), eq(bidRequest), eq(0L));
verify(ortb2RequestFactory).fetchAccount(
eq(AuctionContext.builder().bidRequest(bidRequest).build()),
eq(false));
verify(ortb2RequestFactory).fetchAccountWithoutStoredRequestLookup(
eq(AuctionContext.builder().bidRequest(bidRequest).build()));
verify(ortb2RequestFactory).validateRequest(bidRequest);
verify(paramsResolver).resolve(eq(bidRequest), any(), eq(timeoutResolver));
verify(ortb2RequestFactory).enrichBidRequestWithAccountAndPrivacyData(
Expand Down Expand Up @@ -348,7 +346,7 @@ private void givenBidRequest(BidRequest bidRequest, List<PodError> podErrors) {
.willAnswer(invocationOnMock -> AuctionContext.builder()
.bidRequest((BidRequest) invocationOnMock.getArguments()[2])
.build());
given(ortb2RequestFactory.fetchAccount(any(), anyBoolean())).willReturn(Future.succeededFuture());
given(ortb2RequestFactory.fetchAccountWithoutStoredRequestLookup(any())).willReturn(Future.succeededFuture());

given(ortb2RequestFactory.validateRequest(any())).willAnswer(answerWithFirstArgument());
given(paramsResolver.resolve(any(), any(), any()))
Expand Down

0 comments on commit e008d1a

Please sign in to comment.