Improved fix for #285

1. Write SAME_BYTES to cacheMap when woven bytes are null
2. Fix TODO in SimpleCache::getAndInitialize, using Optional to help
   indicate cache hit for unwoven class
3. Improve test coverage (cache miss, cache hit for unwoven class)

Relates to #285.

Co-authored-by: Alexander Kriegisch <Alexander@Kriegisch.name>
Signed-off-by: KimmingLau <294001791@qq.com>
Signed-off-by: Alexander Kriegisch <Alexander@Kriegisch.name>

loadtime/src/main/java/org/aspectj/weaver/loadtime/Aj.java

@@ -20,6 +20,7 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Set;
 import java.util.StringTokenizer;
 
@@ -104,9 +105,9 @@ public class Aj implements ClassPreProcessor {
 			synchronized (classLoader) {
 
 				if (SimpleCacheFactory.isEnabled()) {
-					byte[] cacheBytes= laCache.getAndInitialize(className, bytes, classLoader, protectionDomain);
-					if (cacheBytes!=null){
-							return cacheBytes;
+					Optional<byte[]> cacheBytes = laCache.getAndInitialize(className, bytes, classLoader, protectionDomain);
+					if (cacheBytes != null){
+						return cacheBytes.orElse(null);
 					}
 				}
 

weaver/src/main/java/org/aspectj/weaver/tools/cache/SimpleCache.java

@@ -15,6 +15,7 @@ import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Optional;
 import java.util.zip.CRC32;
 
 import org.aspectj.weaver.Dump;
@@ -37,7 +38,7 @@ import org.aspectj.weaver.tools.TraceFactory;
 public class SimpleCache {
 
 	private static final String SAME_BYTES_STRING = "IDEM";
-	private static final byte[] SAME_BYTES = SAME_BYTES_STRING.getBytes();
+	static final byte[] SAME_BYTES = SAME_BYTES_STRING.getBytes();
 
 	private Map<String, byte[]> cacheMap;
 	private boolean enabled = false;
@@ -64,7 +65,7 @@ public class SimpleCache {
 		}
 	}
 
-	public byte[] getAndInitialize(String classname, byte[] bytes,
+	public Optional<byte[]> getAndInitialize(String classname, byte[] bytes,
 			ClassLoader loader, ProtectionDomain protectionDomain) {
 		if (!enabled) {
 			return null;
@@ -72,14 +73,12 @@ public class SimpleCache {
 		byte[] res = get(classname, bytes);
 
 		if (Arrays.equals(SAME_BYTES, res)) {
-			return null;
-		} else {
-			if (res != null) {
-				initializeClass(classname, res, loader, protectionDomain);
-			}
-			return res;
+			return Optional.empty();
+		} else if (res != null) {
+			initializeClass(classname, res, loader, protectionDomain);
+			return Optional.of(res);
 		}
-
+		return null;
 	}
 
 	private byte[] get(String classname, byte bytes[]) {

weaver/src/test/java/org/aspectj/weaver/tools/cache/SimpleClassCacheTest.java

@@ -12,60 +12,83 @@
 
 package org.aspectj.weaver.tools.cache;
 
-import java.io.File;
-
 import junit.framework.TestCase;
 
-/**
- */
-public class SimpleClassCacheTest extends TestCase {
-	byte[] FAKE_BYTES_V1 = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9};
-	byte[] FAKE_BYTES_V2 = {1, 1, 2, 3, 4, 5, 6, 7, 8, 9};
+import java.io.File;
 
-	byte[] FAKE_WOVEN_BYTES_V1 = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9,10};
-	byte[] FAKE_WOVEN_BYTES_V2 = {1, 1, 2, 3, 4, 5, 6, 7, 8, 9,10};
+public class SimpleClassCacheTest extends TestCase {
+	byte[] FAKE_BYTES_V1 = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 };
+	byte[] FAKE_BYTES_V2 = { 1, 1, 2, 3, 4, 5, 6, 7, 8, 9 };
 
+	byte[] FAKE_WOVEN_BYTES_V1 = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
+	byte[] FAKE_WOVEN_BYTES_V2 = { 1, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10 };
 
-	private SimpleCache createCache() throws Exception {
-		return new SimpleCache(System.getProperty("java.io.tmpdir"),true);
+	private SimpleCache createCache() {
+		return new SimpleCache(System.getProperty("java.io.tmpdir"), true);
 	}
 
-
-	public void testCache() throws Exception {
+	public void testCache() {
 		String classA = "com.generated.A";
 		SimpleCache cache = createCache();
-
 		cache.put(classA, FAKE_BYTES_V1, FAKE_WOVEN_BYTES_V1);
 
+		// Returned woven bytes are the original ones
+		byte[] result = cache.getAndInitialize(classA, FAKE_BYTES_V1, null, null).orElse(null);
+		assertNotNull(result);
+		for (int i = 0; i < result.length; i++)
+			assertEquals(
+				"Cached version byte[" + i + "] should be equal to the original woven class",
+				result[i], FAKE_WOVEN_BYTES_V1[i]
+			);
+
+		// Class is properly backed up
+		File f = new File(System.getProperty("java.io.tmpdir") + File.separator + "com.generated.A-1164760902");
+		assertTrue(
+			"Class should be backed up with CRC 1164760902",
+			f.exists()
+		);
+	}
+
+	public void testDifferentVersionCache() {
+		String classA = "com.generated.A";
+		SimpleCache cache = createCache();
+		cache.put(classA, FAKE_BYTES_V1, FAKE_WOVEN_BYTES_V1);
+		cache.put(classA, FAKE_BYTES_V2, FAKE_WOVEN_BYTES_V2);
 
-		// Test the returned woven bytes are the original one
-		byte result[] = cache.getAndInitialize(classA, FAKE_BYTES_V1, null, null);
-		for(int i = 0; i < result.length; i ++){
-			assertEquals("Cached version byte[" +i+"] should be equal to the original woven classe",result[i],FAKE_WOVEN_BYTES_V1[i]);
-		}
+		// Returned woven bytes are the original ones for v1
+		byte[] result = cache.getAndInitialize(classA, FAKE_BYTES_V1, null, null).orElse(null);
+		assertNotNull(result);
+		for (int i = 0; i < result.length; i++)
+			assertEquals(
+				"Cached version v1 byte[" + i + "] should be equal to the original woven class",
+				result[i], FAKE_WOVEN_BYTES_V1[i]
+			);
+
+		// Returned woven bytes are the original ones for v2
+		result = cache.getAndInitialize(classA, FAKE_BYTES_V2, null, null).orElse(null);
+		assertNotNull(result);
+		for (int i = 0; i < result.length; i++)
+			assertEquals(
+				"Cached version v2 byte[" + i + "] should be equal to the original woven class",
+				result[i], FAKE_WOVEN_BYTES_V2[i]
+			);
+	}
 
-		// Assure the class is properly backed up in the backing folder
-		File f = new File (System.getProperty("java.io.tmpdir") + File.separator + "com.generated.A-1164760902");
-		assertTrue("Class should be backed up to backing folder, with te CRC:1164760902 ",f.exists());
+	public void testCacheMiss() {
+		String classA = "com.generated.A";
+		SimpleCache cache = createCache();
 
+		// Woven bytes not found in cache
+		assertNull(cache.getAndInitialize(classA, FAKE_BYTES_V1, null, null));
 	}
 
-	public void testDifferentVersionCache() throws Exception {
+	public void testCacheHitUnwoven() {
 		String classA = "com.generated.A";
 		SimpleCache cache = createCache();
-		cache.put(classA, FAKE_BYTES_V1, FAKE_WOVEN_BYTES_V1);
-		cache.put(classA, FAKE_BYTES_V2, FAKE_WOVEN_BYTES_V2);
+		cache.put(classA, FAKE_BYTES_V1, SimpleCache.SAME_BYTES);
 
-		// Test the returned woven bytes are the original one for v1
-		byte result[] = cache.getAndInitialize(classA, FAKE_BYTES_V1, null, null);
-		for(int i = 0; i < result.length; i ++){
-			assertEquals("Cached version v1 byte[" +i+"] should be equal to the original woven classe",result[i],FAKE_WOVEN_BYTES_V1[i]);
-		}
-
-		// Test the returned woven bytes are the original one for v2
-		result = cache.getAndInitialize(classA, FAKE_BYTES_V2, null, null);
-		for(int i = 0; i < result.length; i ++){
-			assertEquals("Cached version v2 byte[" +i+"] should be equal to the original woven classe",result[i],FAKE_WOVEN_BYTES_V2[i]);
-		}
+		// Returned woven bytes are null, indicating an unwoven class
+		byte[] result = cache.getAndInitialize(classA, FAKE_BYTES_V1, null, null).orElse(null);
+		assertNull(result);
 	}
 }