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

Price Floors: Update Cache-Control header toleration #1851

Merged
merged 6 commits into from
May 16, 2022
Merged

Price Floors: Update Cache-Control header toleration #1851

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
45ca3e9
Price Floors: Update Cache-Control header toleration
May 10, 2022
d084af6
Header ttl logic update
May 11, 2022
096a35f
Add header validations
May 12, 2022
a8aa6a6
Fixes after review
May 13, 2022
f281959
Tests: Cache-Control header parsing (#1859)
marki1an May 16, 2022
2ea888f
Merge branch 'master' into price-floors/max-age_fix
May 16, 2022
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
47 changes: 33 additions & 14 deletions src/main/java/org/prebid/server/floors/PriceFloorFetcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,18 @@
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 {

private static final Logger logger = LoggerFactory.getLogger(PriceFloorFetcher.class);

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;
Expand Down Expand Up @@ -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));
}
}

Expand All @@ -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) {
Expand Down
83 changes: 66 additions & 17 deletions src/test/java/org/prebid/server/floors/PriceFloorFetcherTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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 =
Expand All @@ -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();
Expand Down Expand Up @@ -472,7 +521,7 @@ private static AccountPriceFloorsFetchConfig givenFetchConfig(
.maxFileSize(10L)
.timeout(1300L)
.maxAgeSec(1500L)
.periodSec(1700L))
.periodSec(1200L))
.build();
}

Expand Down