diff --git a/src/main/java/org/prebid/server/floors/PriceFloorFetcher.java b/src/main/java/org/prebid/server/floors/PriceFloorFetcher.java index ea583544e7e..cfda46786e7 100644 --- a/src/main/java/org/prebid/server/floors/PriceFloorFetcher.java +++ b/src/main/java/org/prebid/server/floors/PriceFloorFetcher.java @@ -39,6 +39,8 @@ import java.util.Set; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.regex.Matcher; +import java.util.regex.Pattern; public class PriceFloorFetcher { @@ -46,6 +48,9 @@ public class PriceFloorFetcher { private static final int ACCOUNT_FETCH_TIMEOUT_MS = 5000; private static final int MAXIMUM_CACHE_SIZE = 300; + private static final int MIN_MAX_AGE_SEC_VALUE = 600; + private static final int MAX_AGE_SEC_VALUE = Integer.MAX_VALUE; + private static final Pattern CACHE_CONTROL_HEADER_PATTERN = Pattern.compile("^.*max-age=(\\d+).*$"); private final ApplicationSettings applicationSettings; private final Metrics metrics; @@ -196,18 +201,18 @@ private static int resolveMaxRules(Long accountMaxRules) { } private Long cacheTtlFromResponse(HttpClientResponse httpClientResponse, String fetchUrl) { - final String cacheMaxAge = httpClientResponse.getHeaders().get(HttpHeaders.CACHE_CONTROL); - - if (StringUtils.isNotBlank(cacheMaxAge) && cacheMaxAge.contains("max-age")) { - final String[] maxAgeRecord = cacheMaxAge.split("="); - if (maxAgeRecord.length == 2) { - try { - return Long.parseLong(maxAgeRecord[1]); - } catch (NumberFormatException e) { - logger.error(String.format("Can't parse Cache Control header '%s', fetch.url: '%s'", - cacheMaxAge, - fetchUrl)); - } + final String cacheControlValue = httpClientResponse.getHeaders().get(HttpHeaders.CACHE_CONTROL); + final Matcher cacheHeaderMatcher = StringUtils.isNotBlank(cacheControlValue) + ? CACHE_CONTROL_HEADER_PATTERN.matcher(cacheControlValue) + : null; + + if (cacheHeaderMatcher != null && cacheHeaderMatcher.matches()) { + try { + return Long.parseLong(cacheHeaderMatcher.group(1)); + } catch (NumberFormatException e) { + logger.error(String.format("Can't parse Cache Control header '%s', fetch.url: '%s'", + cacheControlValue, + fetchUrl)); } } @@ -230,9 +235,23 @@ private PriceFloorData updateCache(ResponseCacheInfo cacheInfo, return fetchContext.getRulesData(); } - private long resolveCacheTtl(ResponseCacheInfo cacheInfo, AccountPriceFloorsFetchConfig fetchConfig) { + private static long resolveCacheTtl(ResponseCacheInfo cacheInfo, AccountPriceFloorsFetchConfig fetchConfig) { + final Long headerCacheTtl = cacheInfo.getCacheTtl(); - return ObjectUtils.defaultIfNull(cacheInfo.getCacheTtl(), fetchConfig.getMaxAgeSec()); + return isValidMaxAge(headerCacheTtl, fetchConfig) ? headerCacheTtl : fetchConfig.getMaxAgeSec(); + } + + private static boolean isValidMaxAge(Long headerCacheTtl, AccountPriceFloorsFetchConfig fetchConfig) { + final Long periodSec = fetchConfig.getPeriodSec(); + final long minMaxAgeValue = periodSec != null + ? Math.max(MIN_MAX_AGE_SEC_VALUE, periodSec) + : MIN_MAX_AGE_SEC_VALUE; + + return headerCacheTtl != null && isInMaxAgeRange(headerCacheTtl, minMaxAgeValue); + } + + private static boolean isInMaxAgeRange(long number, long min) { + return Math.max(min, number) == Math.min(number, MAX_AGE_SEC_VALUE); } private Long createMaxAgeTimer(String accountId, long cacheTtl) { diff --git a/src/test/groovy/org/prebid/server/functional/tests/pricefloors/PriceFloorsFetchingSpec.groovy b/src/test/groovy/org/prebid/server/functional/tests/pricefloors/PriceFloorsFetchingSpec.groovy index 16fa742681f..29184f0464e 100644 --- a/src/test/groovy/org/prebid/server/functional/tests/pricefloors/PriceFloorsFetchingSpec.groovy +++ b/src/test/groovy/org/prebid/server/functional/tests/pricefloors/PriceFloorsFetchingSpec.groovy @@ -21,6 +21,7 @@ import static org.prebid.server.functional.model.pricefloors.Country.MULTIPLE import static org.prebid.server.functional.model.pricefloors.MediaType.BANNER import static org.prebid.server.functional.model.request.auction.DistributionChannel.APP import static org.prebid.server.functional.model.request.auction.FetchStatus.ERROR +import static org.prebid.server.functional.model.request.auction.FetchStatus.INPROGRESS import static org.prebid.server.functional.model.request.auction.FetchStatus.NONE import static org.prebid.server.functional.model.request.auction.FetchStatus.SUCCESS import static org.prebid.server.functional.model.request.auction.Location.FETCH @@ -1664,6 +1665,43 @@ class PriceFloorsFetchingSpec extends PriceFloorsBaseSpec { null | JPY } + def "PBS should not contain errors when the header has Cache-Control with directives"() { + given: "Test start time" + def startTime = Instant.now() + + and: "Default BidRequest" + def bidRequest = BidRequest.defaultBidRequest + + and: "Account with enabled fetch, fetch.url in the DB" + def accountId = bidRequest.site.publisher.id + def account = getAccountWithEnabledFetch(accountId) + accountDao.save(account) + + and: "Set Floors Provider response with header Cache-Control and floor value" + def header = ["Cache-Control": "no-cache, no-store, max-age=800, must-revalidate"] + def floorValue = PBSUtils.randomFloorValue + def floorsResponse = PriceFloorData.priceFloorData.tap { + modelGroups[0].values = [(rule): floorValue] + } + floorsProvider.setResponse(accountId, floorsResponse, header) + + and: "PBS cache rules" + cacheFloorsProviderRules(bidRequest, floorValue) + + when: "PBS processes auction request" + floorsPbsService.sendAuctionRequest(bidRequest) + + then: "PBS log should not contain error" + def logs = floorsPbsService.getLogsByTime(startTime) + def floorsLogs = getLogsByText(logs, basicFetchUrl) + assert floorsLogs.size() == 0 + + and: "Bidder request should contain floors data from floors provider" + def bidderRequest = bidder.getBidderRequests(bidRequest.id).last() + assert bidderRequest.ext?.prebid?.floors?.fetchStatus == SUCCESS + assert bidderRequest.ext?.prebid?.floors?.location == FETCH + } + static int convertKilobyteSizeToByte(int kilobyteSize) { kilobyteSize * 1024 } diff --git a/src/test/java/org/prebid/server/floors/PriceFloorFetcherTest.java b/src/test/java/org/prebid/server/floors/PriceFloorFetcherTest.java index 182ad127e54..1dfd4ec6d9c 100644 --- a/src/test/java/org/prebid/server/floors/PriceFloorFetcherTest.java +++ b/src/test/java/org/prebid/server/floors/PriceFloorFetcherTest.java @@ -98,7 +98,7 @@ public void fetchShouldReturnPriceFloorFetchedFromProviderAndCache() { assertThat(fetchResult.getFetchStatus()).isEqualTo(FetchStatus.inprogress); verify(httpClient).get("http://test.host.com", 1300, 10240); - verify(vertx).setTimer(eq(1700000L), any()); + verify(vertx).setTimer(eq(1200000L), any()); verify(vertx).setTimer(eq(1500000L), any()); verifyNoMoreInteractions(httpClient); @@ -120,7 +120,7 @@ public void fetchShouldReturnEmptyRulesAndInProgressStatusForTheFirstInvocation( // then assertThat(fetchResult.getRulesData()).isNull(); assertThat(fetchResult.getFetchStatus()).isEqualTo(FetchStatus.inprogress); - verify(vertx).setTimer(eq(1700000L), any()); + verify(vertx).setTimer(eq(1200000L), any()); } @Test @@ -135,7 +135,7 @@ public void fetchShouldReturnEmptyRulesAndInProgressStatusForTheFirstInvocationA // then assertThat(firstInvocationResult.getRulesData()).isNull(); assertThat(firstInvocationResult.getFetchStatus()).isEqualTo(FetchStatus.inprogress); - verify(vertx).setTimer(eq(1700000L), any()); + verify(vertx).setTimer(eq(1200000L), any()); final FetchResult secondInvocationResult = priceFloorFetcher.fetch(givenAccount(identity())); assertThat(secondInvocationResult.getRulesData()).isNull(); @@ -154,7 +154,7 @@ public void fetchShouldReturnEmptyRulesAndInProgressStatusForTheFirstInvocationA // then assertThat(firstInvocationResult.getRulesData()).isNull(); assertThat(firstInvocationResult.getFetchStatus()).isEqualTo(FetchStatus.inprogress); - verify(vertx).setTimer(eq(1700000L), any()); + verify(vertx).setTimer(eq(1200000L), any()); final FetchResult secondInvocationResult = priceFloorFetcher.fetch(givenAccount(identity())); assertThat(secondInvocationResult.getRulesData()).isNull(); @@ -167,14 +167,63 @@ public void fetchShouldCacheResponseForTimeFromResponseCacheControlHeader() { given(httpClient.get(anyString(), anyLong(), anyLong())) .willReturn(Future.succeededFuture( HttpClientResponse.of(200, MultiMap.caseInsensitiveMultiMap() - .add(HttpHeaders.CACHE_CONTROL, "max-age=700"), + .add(HttpHeaders.CACHE_CONTROL, "max-age=1700"), jacksonMapper.encodeToString(givenPriceFloorData())))); // when priceFloorFetcher.fetch(givenAccount(identity())); // then - verify(vertx).setTimer(eq(700000L), any()); + verify(vertx).setTimer(eq(1700000L), any()); + } + + @Test + public void fetchShouldNotCacheResponseFoWithTimeFromResponseCacheControlHeaderIfLessThanMinValue() { + // given + given(httpClient.get(anyString(), anyLong(), anyLong())) + .willReturn(Future.succeededFuture( + HttpClientResponse.of(200, MultiMap.caseInsensitiveMultiMap() + .add(HttpHeaders.CACHE_CONTROL, "max-age=500"), + jacksonMapper.encodeToString(givenPriceFloorData())))); + + // when + priceFloorFetcher.fetch(givenAccount(config -> config.periodSec(500L))); + + // then + verify(vertx).setTimer(eq(1500000L), any()); + } + + @Test + public void fetchShouldNotCacheResponseFoWithTimeFromResponseCacheControlHeaderIfLessThanPeriodSec() { + // given + given(httpClient.get(anyString(), anyLong(), anyLong())) + .willReturn(Future.succeededFuture( + HttpClientResponse.of(200, MultiMap.caseInsensitiveMultiMap() + .add(HttpHeaders.CACHE_CONTROL, "max-age=900"), + jacksonMapper.encodeToString(givenPriceFloorData())))); + + // when + priceFloorFetcher.fetch(givenAccount(identity())); + + // then + verify(vertx).setTimer(eq(1500000L), any()); + } + + @Test + public void fetchShouldCacheResponseForTimeFromResponseCacheControlHeaderToleratingOtherHeaderData() { + // given + given(httpClient.get(anyString(), anyLong(), anyLong())) + .willReturn(Future.succeededFuture( + HttpClientResponse.of(200, MultiMap.caseInsensitiveMultiMap() + .add(HttpHeaders.CACHE_CONTROL, + "no-cache, no-store, max-age=1700, must-revalidate"), + jacksonMapper.encodeToString(givenPriceFloorData())))); + + // when + priceFloorFetcher.fetch(givenAccount(identity())); + + // then + verify(vertx).setTimer(eq(1700000L), any()); } @Test @@ -192,7 +241,7 @@ public void fetchShouldTakePrecedenceForTestingPropertyToCacheResponse() { // then verify(vertx).setTimer(eq(1000L), any()); - verify(vertx).setTimer(eq(1700000L), any()); + verify(vertx).setTimer(eq(1200000L), any()); } @Test @@ -322,7 +371,7 @@ public void fetchShouldReturnEmptyRulesAndErrorStatusForSecondCallAndCreatePerio verify(httpClient).get(anyString(), anyLong(), anyLong()); assertThat(firstInvocationResult.getRulesData()).isNull(); assertThat(firstInvocationResult.getFetchStatus()).isEqualTo(FetchStatus.inprogress); - verify(vertx).setTimer(eq(1700000L), any()); + verify(vertx).setTimer(eq(1200000L), any()); verify(vertx).setTimer(eq(1500000L), any()); final FetchResult secondInvocationResult = priceFloorFetcher.fetch(givenAccount(identity())); assertThat(secondInvocationResult.getRulesData()).isNull(); @@ -344,7 +393,7 @@ public void fetchShouldReturnEmptyRulesWithErrorStatusAndCreatePeriodicTimerWhen verify(httpClient).get(anyString(), anyLong(), anyLong()); assertThat(firstInvocationResult.getRulesData()).isNull(); assertThat(firstInvocationResult.getFetchStatus()).isEqualTo(FetchStatus.inprogress); - verify(vertx).setTimer(eq(1700000L), any()); + verify(vertx).setTimer(eq(1200000L), any()); verify(vertx).setTimer(eq(1500000L), any()); final FetchResult secondInvocationResult = priceFloorFetcher.fetch(givenAccount(identity())); assertThat(secondInvocationResult.getRulesData()).isNull(); @@ -366,7 +415,7 @@ public void fetchShouldReturnEmptyRulesWithErrorStatusForSecondCallAndCreatePeri verify(httpClient).get(anyString(), anyLong(), anyLong()); assertThat(firstInvocationResult.getRulesData()).isNull(); assertThat(firstInvocationResult.getFetchStatus()).isEqualTo(FetchStatus.inprogress); - verify(vertx).setTimer(eq(1700000L), any()); + verify(vertx).setTimer(eq(1200000L), any()); verify(vertx).setTimer(eq(1500000L), any()); final FetchResult secondInvocationResult = priceFloorFetcher.fetch(givenAccount(identity())); assertThat(secondInvocationResult.getRulesData()).isNull(); @@ -388,7 +437,7 @@ public void fetchShouldReturnEmptyRulesWithErrorStatusForSecondCallAndCreatePeri verify(httpClient).get(anyString(), anyLong(), anyLong()); assertThat(firstInvocationResult.getRulesData()).isNull(); assertThat(firstInvocationResult.getFetchStatus()).isEqualTo(FetchStatus.inprogress); - verify(vertx).setTimer(eq(1700000L), any()); + verify(vertx).setTimer(eq(1200000L), any()); verify(vertx).setTimer(eq(1500000L), any()); final FetchResult secondInvocationResult = priceFloorFetcher.fetch(givenAccount(identity())); assertThat(secondInvocationResult.getRulesData()).isNull(); @@ -429,10 +478,10 @@ public void fetchShouldReturnNullAndCreatePeriodicTimerWhenResponseExceededRules .willReturn(Future.succeededFuture(HttpClientResponse.of(200, MultiMap.caseInsensitiveMultiMap(), jacksonMapper.encodeToString(PriceFloorData.builder() - .modelGroups(singletonList(PriceFloorModelGroup.builder() - .value("video", BigDecimal.ONE).value("banner", BigDecimal.TEN) - .build())) - .build())))); + .modelGroups(singletonList(PriceFloorModelGroup.builder() + .value("video", BigDecimal.ONE).value("banner", BigDecimal.TEN) + .build())) + .build())))); // when final FetchResult firstInvocationResult = @@ -442,7 +491,7 @@ public void fetchShouldReturnNullAndCreatePeriodicTimerWhenResponseExceededRules verify(httpClient).get(anyString(), anyLong(), anyLong()); assertThat(firstInvocationResult.getRulesData()).isNull(); assertThat(firstInvocationResult.getFetchStatus()).isEqualTo(FetchStatus.inprogress); - verify(vertx).setTimer(eq(1700000L), any()); + verify(vertx).setTimer(eq(1200000L), any()); verify(vertx).setTimer(eq(1500000L), any()); final FetchResult secondInvocationResult = priceFloorFetcher.fetch(givenAccount(identity())); assertThat(secondInvocationResult.getRulesData()).isNull(); @@ -472,7 +521,7 @@ private static AccountPriceFloorsFetchConfig givenFetchConfig( .maxFileSize(10L) .timeout(1300L) .maxAgeSec(1500L) - .periodSec(1700L)) + .periodSec(1200L)) .build(); }