Fix get in evictable cache, as it did not update last accessed timest…

…amp (helidon-io#7422)

security/providers/common/src/main/java/io/helidon/security/providers/common/EvictableCacheImpl.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2021 Oracle and/or its affiliates.
+ * Copyright (c) 2018, 2023 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -13,6 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 package io.helidon.security.providers.common;
 
 import java.util.Optional;
@@ -29,15 +30,16 @@
  * Default implementation of {@link EvictableCache}.
  */
 class EvictableCacheImpl<K, V> implements EvictableCache<K, V> {
+    /**
+     * An implementation that does no caching.
+     */
+    static final EvictableCache<?, ?> NO_CACHE = new EvictableCache<>() { };
+
     /**
      * Number of threads in the scheduled thread pool to evict records.
      */
     private static final int EVICT_THREAD_COUNT = 1;
     private static final ScheduledThreadPoolExecutor EXECUTOR;
-    /**
-     * An implementation that does no caching.
-     */
-    static final EvictableCache<?, ?> NO_CACHE = new EvictableCache() { };
 
     static {
         ThreadFactory jf = new ThreadFactory() {
@@ -79,7 +81,7 @@ public Thread newThread(Runnable r) {
     @Override
     public Optional<V> remove(K key) {
         CacheRecord<K, V> removed = cacheMap.remove(key);
-        if (null == removed) {
+        if (removed == null) {
             return Optional.empty();
         }
 
@@ -88,7 +90,10 @@ public Optional<V> remove(K key) {
 
     @Override
     public Optional<V> get(K key) {
-        return getRecord(key).flatMap(this::validate).map(CacheRecord::getValue);
+        return getRecord(key)
+                .flatMap(this::validate)
+                .map(this::accessed)
+                .map(CacheRecord::getValue);
     }
 
     @Override
@@ -113,7 +118,7 @@ public void close() {
 
     void evict() {
         cacheMap.forEachKey(evictParallelismThreshold, key -> cacheMap.compute(key, (key1, cacheRecord) -> {
-            if ((null == cacheRecord) || evictor.apply(cacheRecord.getKey(), cacheRecord.getValue())) {
+            if ((cacheRecord == null) || evictor.apply(cacheRecord.getKey(), cacheRecord.getValue())) {
                 return null;
             } else {
                 if (cacheRecord.isValid(cacheTimeoutNanos, overallTimeoutNanos)) {
@@ -125,6 +130,11 @@ void evict() {
         }));
     }
 
+    private CacheRecord<K, V> accessed(CacheRecord<K, V> record) {
+        record.accessed();
+        return record;
+    }
+
     private Optional<CacheRecord<K, V>> validate(CacheRecord<K, V> record) {
         if (record.isValid(cacheTimeoutNanos, overallTimeoutNanos) && !evictor.apply(record.getKey(), record.getValue())) {
             return Optional.of(record);
@@ -135,9 +145,8 @@ private Optional<CacheRecord<K, V>> validate(CacheRecord<K, V> record) {
 
     private Optional<V> doComputeValue(K key, Supplier<Optional<V>> valueSupplier) {
         CacheRecord<K, V> record = cacheMap.compute(key, (s, cacheRecord) -> {
-            if ((null != cacheRecord) && cacheRecord.isValid(cacheTimeoutNanos, overallTimeoutNanos)) {
-                cacheRecord.accessed();
-                return cacheRecord;
+            if ((cacheRecord != null) && cacheRecord.isValid(cacheTimeoutNanos, overallTimeoutNanos)) {
+                return accessed(cacheRecord);
             }
 
             if (cacheMap.size() >= cacheMaxSize) {
@@ -149,7 +158,7 @@ private Optional<V> doComputeValue(K key, Supplier<Optional<V>> valueSupplier) {
                     .orElse(null);
         });
 
-        if (null == record) {
+        if (record == null) {
             return Optional.empty();
         } else {
             return Optional.of(record.value);

security/providers/common/src/test/java/io/helidon/security/providers/common/EvictableCacheTest.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2018, 2021 Oracle and/or its affiliates.
+ * Copyright (c) 2018, 2023 Oracle and/or its affiliates.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -63,6 +63,27 @@ void testEviction() throws InterruptedException {
         cache.close();
     }
 
+    @Test
+    void testGetUpdatesAccessed() throws InterruptedException {
+        EvictableCache<String, String> cache = EvictableCache.<String, String>builder()
+                .evictSchedule(100, 100, TimeUnit.MILLISECONDS)
+                .timeout(50, TimeUnit.MILLISECONDS)
+                .build();
+
+        assertThat(cache.computeValue("one", () -> Optional.of("1")), is(Optional.of("1")));
+
+        for (int i = 0; i < 10; i++) {
+            assertThat(cache.get("one"), is(Optional.of("1")));
+            // this should amount to more than 200 millis, which would time out if the record is not marked as accessed
+            Thread.sleep(20);
+        }
+        // now let's make sure it times out
+        TimeUnit.MILLISECONDS.sleep(200);
+        assertThat(cache.get("one"), is(EMPTY));
+
+        cache.close();
+    }
+
     @Test
     void testOverallTimeout() throws InterruptedException {
         EvictableCache<String, String> cache = EvictableCache.<String, String>builder()