Merge pull request #31 from brogowski/main

Include parameters in cache key
DeguStudios · Dec 12, 2021 · 12a6a27 · 12a6a27
2 parents feba268 + e5226c7
commit 12a6a27
Show file tree

Hide file tree

Showing 5 changed files with 60 additions and 28 deletions.
diff --git a/...os/bitbucket/repository/validators/IdempotentlyCachedDotnetFormatRefValidatorWrapper.java b/...os/bitbucket/repository/validators/IdempotentlyCachedDotnetFormatRefValidatorWrapper.java
@@ -5,6 +5,7 @@
 import com.degustudios.dotnetformat.DotnetFormatCommandResult;
 import com.degustudios.executors.IdempotentExecutor;
 import com.degustudios.executors.IdempotentExecutorBuilder;
+import org.apache.commons.lang3.StringUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -44,7 +45,11 @@ public DotnetFormatCommandResult validate(RepositoryRef ref, List<String> params
         }
     }
 
-    private static String mapToKey(RepositoryRef x) {
-        return x.getRepository().getId() + "/" + x.getLatestCommit();
+    private static String mapToKey(RepositoryRef x, List<String> params) {
+        return x.getRepository().getId()
+                + "/"
+                + x.getLatestCommit()
+                + "/"
+                + StringUtils.join(params, ' ');
     }
 }
diff --git a/src/main/java/com/degustudios/executors/IdempotentExecutor.java b/src/main/java/com/degustudios/executors/IdempotentExecutor.java
@@ -16,10 +16,10 @@ public class IdempotentExecutor<T,R> {
     private final ConcurrentHashMap<String, LazyInitializer<Future<R>>> cache = new ConcurrentHashMap<>();
     private final ExecutorService executor = Executors.newCachedThreadPool();
     private final BiFunction<T, List<String>, R> executeFunc;
-    private final Function<T, String> mapToKeyFunc;
+    private final BiFunction<T, List<String>, String> mapToKeyFunc;
     private final Function<R, Boolean> shouldCacheFunc;
 
-    public IdempotentExecutor(BiFunction<T, List<String>, R> executeFunc, Function<T, String> mapToKeyFunc, Function<R, Boolean> shouldCacheFunc) {
+    public IdempotentExecutor(BiFunction<T, List<String>, R> executeFunc, BiFunction<T, List<String>, String> mapToKeyFunc, Function<R, Boolean> shouldCacheFunc) {
         this.executeFunc = executeFunc;
         this.mapToKeyFunc = mapToKeyFunc;
         this.shouldCacheFunc = shouldCacheFunc;
@@ -33,7 +33,7 @@ public Future<R> execute(T param1, List<String> param2) throws ConcurrentExcepti
     private LazyInitializer<Future<R>> getEarliestScheduledLazyTaskFor(T param1, List<String> param2) {
         LazyInitializer<Future<R>> justScheduledLazyTask = wrapWithLazy((() -> scheduleForExecution(param1, param2)));
         LazyInitializer<Future<R>> earlierScheduledLazyTask = cache.putIfAbsent(
-                mapToKeyFunc.apply(param1),
+                mapToKeyFunc.apply(param1, param2),
                 justScheduledLazyTask);
 
         if (earlierScheduledLazyTask != null) {
@@ -56,7 +56,7 @@ private Future<R> scheduleForExecution(T param1, List<String> param2) {
         return executor.submit(() -> {
             R result = executeFunc.apply(param1, param2);
             if (!Boolean.TRUE.equals(shouldCacheFunc.apply(result))) {
-                cache.remove(mapToKeyFunc.apply(param1));
+                cache.remove(mapToKeyFunc.apply(param1, param2));
             }
             return result;
         });

diff --git a/src/main/java/com/degustudios/executors/IdempotentExecutorBuilder.java b/src/main/java/com/degustudios/executors/IdempotentExecutorBuilder.java
@@ -10,7 +10,7 @@
 public class IdempotentExecutorBuilder {
     public <T,R> IdempotentExecutor<T,R> build(
             BiFunction<T, List<String>, R> executeFunc,
-            Function<T, String> mapToKeyFunc,
+            BiFunction<T, List<String>, String> mapToKeyFunc,
             Function<R, Boolean> shouldCacheFunc) {
         return new IdempotentExecutor<>(executeFunc, mapToKeyFunc, shouldCacheFunc);
     }

diff --git a/...itbucket/repository/validators/IdempotentlyCachedDotnetFormatRefValidatorWrapperTest.java b/...itbucket/repository/validators/IdempotentlyCachedDotnetFormatRefValidatorWrapperTest.java
@@ -7,6 +7,7 @@
 import com.degustudios.dotnetformat.DotnetFormatCommandResult;
 import com.degustudios.executors.IdempotentExecutor;
 import com.degustudios.executors.IdempotentExecutorBuilder;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.concurrent.ConcurrentException;
 import org.junit.Before;
 import org.junit.Test;
@@ -19,9 +20,8 @@
 import java.util.Arrays;
 import java.util.List;
 import java.util.concurrent.CompletableFuture;
-import java.util.function.Function;
 import java.util.function.BiFunction;
-import java.util.stream.Collectors;
+import java.util.function.Function;
 
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.CoreMatchers.notNullValue;
@@ -46,7 +46,7 @@ public class IdempotentlyCachedDotnetFormatRefValidatorWrapperTest {
     @Captor
     private ArgumentCaptor<BiFunction<RepositoryRef, List<String>, DotnetFormatCommandResult>> scheduleFuncCaptor;
     @Captor
-    private ArgumentCaptor<Function<RepositoryRef, String>> keyMapFuncCaptor;
+    private ArgumentCaptor<BiFunction<RepositoryRef, List<String>, String>> keyMapFuncCaptor;
     @Captor
     private ArgumentCaptor<Function<DotnetFormatCommandResult, Boolean>> shouldCacheFuncCaptor;
     @Mock
@@ -57,7 +57,7 @@ public class IdempotentlyCachedDotnetFormatRefValidatorWrapperTest {
     @Before
     public void initialize(){
         when(executorBuilder.<RepositoryRef, DotnetFormatCommandResult>build(any(), any(), any())).thenReturn(executor);
-        params = Arrays.asList(new String[]{"--check"});
+        params = Arrays.asList("--check", "-v");
     }
 
     @Test
@@ -73,7 +73,7 @@ public void schedulesDotnetFormatValidation() throws ConcurrentException {
     }
 
     @Test
-    public void correctlyMapsRefToKey() throws ConcurrentException {
+    public void correctlyMapsRefAndParamsToKey() throws ConcurrentException {
         String commitId = "19873";
         int repositoryId = 124;
         when(ref.getLatestCommit()).thenReturn(commitId);
@@ -84,7 +84,9 @@ public void correctlyMapsRefToKey() throws ConcurrentException {
         runValidatorWrapper();
 
         verify(executorBuilder).build(any(), keyMapFuncCaptor.capture(), any());
-        assertThat(keyMapFuncCaptor.getValue().apply(ref), is(repositoryId + "/" + commitId));
+        assertThat(
+                keyMapFuncCaptor.getValue().apply(ref, params),
+                is(repositoryId + "/" + commitId + '/' + StringUtils.join(params, ' ')));
     }
 
     @Test
@@ -118,8 +120,6 @@ public void doesNotCacheExecutedCorrectlyUnableToLocateDotNetCliExitCodeDotNetFo
         assertDoesNotCacheExecutedCorrectlyDotNetFormatExecutions(UnableToLocateDotNetCliExitCode);
     }
 
-
-
     @Test
     public void doesCacheExecutedCorrectlyZeroExitCodeDotNetFormatExecutions() throws ConcurrentException {
         assertDoesCacheExecutedCorrectlyDotNetFormatExecutions(0);

diff --git a/src/test/java/ut/com/degustudios/executors/IdempotentExecutorTest.java b/src/test/java/ut/com/degustudios/executors/IdempotentExecutorTest.java
@@ -1,6 +1,7 @@
 package ut.com.degustudios.executors;
 
 import com.degustudios.executors.IdempotentExecutor;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.lang3.concurrent.ConcurrentException;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -10,6 +11,7 @@
 
 import java.util.Arrays;
 import java.util.Collections;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
@@ -20,13 +22,16 @@
 import java.util.stream.Collectors;
 
 import static org.awaitility.Awaitility.await;
+import static org.hamcrest.CoreMatchers.hasItem;
 import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.assertThat;
 
 @RunWith(MockitoJUnitRunner.class)
 public class IdempotentExecutorTest {
     private static final int TIMEOUT_IN_MS = 1000;
     private static final Logger logger = LoggerFactory.getLogger(IdempotentExecutorTest.class);
+    private static final List<String> params = Arrays.asList("--mockParameter", "--otherParameter");
+    private static final String input = "INPUT PARAMETER";
 
     @Test
     public void executeReturnsFutureValueFromFunction() throws ExecutionException, InterruptedException {
@@ -35,9 +40,19 @@ public void executeReturnsFutureValueFromFunction() throws ExecutionException, I
     }
 
     @Test
-    public void executePassesParameterToFunction() throws ExecutionException, InterruptedException {
-        String returnValue = "TEST";
-        assertThat(tryExecute((String x, List<String> y) -> x, returnValue).get(), is(returnValue));
+    public void executePassesInputParametersToKeyMapFunction() throws ExecutionException, InterruptedException {
+        String expectedKey = input + StringUtils.join(params);
+        List<String> mapFuncCalls = new LinkedList<>();
+
+        tryExecute(
+                (String x, List<String> y) -> x,
+                (String x, List<String> y) -> {
+                    mapFuncCalls.add(x + StringUtils.join(y));
+                    return "STUB";
+                }).get();
+
+        assertThat(mapFuncCalls.stream().distinct().count(), is(1L));
+        assertThat(mapFuncCalls, hasItem(expectedKey));
     }
 
     @Test
@@ -94,31 +109,43 @@ public void willExecuteTheSameParametersAgainIfCacheFunctionReturnsFalse() {
         assertThat(invocationCounter.get(), is(2));
     }
 
+    private <T,R> IdempotentExecutor<T,R> getDefaultKeyCacheAllExecutor(
+            BiFunction<T, List<String>, R> executeFunc,
+            BiFunction<T, List<String>, String> mapKeyFunc) {
+        return new IdempotentExecutor<>(executeFunc, mapKeyFunc, r -> true);
+    }
+
     private <T,R> IdempotentExecutor<T,R> getDefaultKeyCacheAllExecutor(BiFunction<T, List<String>, R> executeFunc) {
-        return new IdempotentExecutor<>(executeFunc, Object::toString, r -> true);
+        return new IdempotentExecutor<>(executeFunc, IdempotentExecutorTest::defaultKeyMapper, r -> true);
     }
 
     private <T,R> IdempotentExecutor<T,R> getDefaultKeyCacheNoneExecutor(BiFunction<T, List<String>, R> executeFunc) {
-        return new IdempotentExecutor<>(executeFunc, Object::toString, r -> false);
+        return new IdempotentExecutor<>(executeFunc, IdempotentExecutorTest::defaultKeyMapper, r -> false);
+    }
+
+    private static <T> String defaultKeyMapper (T input, List<String> params) {
+        return input.toString() + StringUtils.join(params);
     }
 
-    private <T> Future<String> tryExecute(BiFunction<T,List<String>,String> executeFunc, String x) {
-        return tryExecute(getDefaultKeyCacheAllExecutor(executeFunc), x);
+    private Future<String> tryExecute(
+            BiFunction<String,List<String>,String> executeFunc,
+            BiFunction<String,List<String>,String> mapFunc) {
+        return tryExecute(getDefaultKeyCacheAllExecutor(executeFunc, mapFunc));
     }
 
-    private <T> Future<String> tryExecute(BiFunction<T,List<String>,String> executeFunc) {
+    private Future<String> tryExecute(BiFunction<String,List<String>,String> executeFunc) {
         return tryExecute(getDefaultKeyCacheAllExecutor(executeFunc));
     }
 
-    private Future<String> tryExecute(IdempotentExecutor executor) {
-        return tryExecute(executor, "STUB");
+    private <R> Future<R> tryExecute(IdempotentExecutor<String,R> executor) {
+        return tryExecute(executor, input);
     }
 
-    private <V> Future<V> tryExecute(IdempotentExecutor executor, V x) {
+    private <T, R> Future<R> tryExecute(IdempotentExecutor<T, R> executor, T x) {
         try {
-            return executor.execute(x, Collections.singletonList("--mockParameter"));
+            return executor.execute(x, params);
         } catch (ConcurrentException e) {
-            logger.error("Exception for conurent excception for exectur: {}", executor, e);
+            logger.error("Exception for concurrent exception for executor: {}", executor, e);
         }
         return null;
     }