bug fix

cat-core/src/main/java/com/dianping/cat/abtest/ABTest.java

@@ -3,6 +3,8 @@ package com.dianping.cat.abtest;
 public interface ABTest {
 	public ABTestName getTestName();
 
+	public boolean isActive();
+
 	public boolean isDefaultGroup();
 
 	public boolean isGroupA();
@@ -16,5 +18,4 @@ public interface ABTest {
 	public boolean isGroupE();
 
 	public boolean isGroup(String name);
-	
 }

cat-core/src/main/java/com/dianping/cat/abtest/internal/DefaultABTest.java

 package com.dianping.cat.abtest.internal;
 
+import java.util.Date;
+
 import com.dianping.cat.abtest.ABTest;
 import com.dianping.cat.abtest.ABTestName;
 import com.dianping.cat.abtest.spi.ABTestContext;
+import com.dianping.cat.abtest.spi.ABTestEntity;
 import com.dianping.cat.abtest.spi.internal.ABTestContextManager;
 
 public class DefaultABTest implements ABTest {
@@ -15,15 +18,27 @@ public class DefaultABTest implements ABTest {
 		m_name = name;
 	}
 
+	private String getGroupName() {
+		ABTestContext ctx = m_contextManager.getContext(m_name);
+
+		return ctx.getGroupName();
+	}
+
 	@Override
 	public ABTestName getTestName() {
 		return m_name;
 	}
 
-	private String getGroupName() {
+	@Override
+	public boolean isActive() {
 		ABTestContext ctx = m_contextManager.getContext(m_name);
+		ABTestEntity entity = ctx.getEntity();
 
-		return ctx.getGroupName();
+		if (entity.isDisabled()) {
+			return false;
+		} else {
+			return entity.isEligible(new Date());
+		}
 	}
 
 	@Override
@@ -31,6 +46,11 @@ public class DefaultABTest implements ABTest {
 		return ABTestContext.DEFAULT_GROUP.equals(getGroupName());
 	}
 
+	@Override
+	public boolean isGroup(String name) {
+		return name.equals(getGroupName());
+	}
+
 	@Override
 	public boolean isGroupA() {
 		return "A".equals(getGroupName());
@@ -55,9 +75,4 @@ public class DefaultABTest implements ABTest {
 	public boolean isGroupE() {
 		return "E".equals(getGroupName());
 	}
-
-	@Override
-	public boolean isGroup(String name) {
-		return name.equals(getGroupName());
-	}
 }

cat-core/src/main/java/com/dianping/cat/abtest/spi/internal/ABTestCodec.java

+package com.dianping.cat.abtest.spi.internal;
+
+import java.util.Map;
+import java.util.Set;
+
+public interface ABTestCodec {
+	public String encode(Map<String, Map<String, String>> map);
+
+	public Map<String, Map<String, String>> decode(String value, Set<String> keys);
+}

cat-core/src/main/java/com/dianping/cat/abtest/spi/internal/DefaultABTestCodec.java

+package com.dianping.cat.abtest.spi.internal;
+
+import java.util.LinkedHashMap;
+import java.util.Map;
+import java.util.Set;
+
+public class DefaultABTestCodec implements ABTestCodec {
+
+	public Map<String, Map<String, String>> decode(String value, Set<String> keys) {
+		int len = value.length();
+		Map<String, Map<String, String>> map = new LinkedHashMap<String, Map<String, String>>();
+		StringBuilder key = new StringBuilder();
+		StringBuilder name = new StringBuilder();
+		StringBuilder val = new StringBuilder();
+		int part = 0;
+
+		for (int i = 0; i < len; i++) {
+			char ch = value.charAt(i);
+
+			switch (ch) {
+			case '&':
+				newCookielet(map, keys, key, name, val);
+
+				key.setLength(0);
+				name.setLength(0);
+				val.setLength(0);
+				part = 0;
+				break;
+			case '=':
+				if (part == 0) {
+					part = 1;
+				} else {
+					distribute(key, name, val, part, ch);
+				}
+				break;
+			case '|':
+				if (part == 2) {
+					newCookielet(map, keys, key, name, val);
+
+					name.setLength(0);
+					val.setLength(0);
+
+					part = 1;
+				} else {
+					distribute(key, name, val, part, ch);
+				}
+				break;
+			case ':':
+				if (part == 1) {
+					part = 2;
+				} else {
+					distribute(key, name, val, part, ch);
+				}
+				break;
+			default:
+				distribute(key, name, val, part, ch);
+
+				break;
+			}
+		}
+
+		newCookielet(map, keys, key, name, val);
+
+		return map;
+	}
+
+	private void distribute(StringBuilder key, StringBuilder name, StringBuilder val, int part, char ch) {
+		if (part == 0) {
+			key.append(ch);
+		} else if (part == 1) {
+			name.append(ch);
+		} else if (part == 2) {
+			val.append(ch);
+		}
+	}
+
+	private void newCookielet(Map<String, Map<String, String>> map, Set<String> keys, StringBuilder key,
+	      StringBuilder name, StringBuilder value) {
+		String k = key.toString();
+
+		if (keys == null || keys.contains(k)) {
+			String n = name.toString();
+			String v = value.toString();
+
+			if (k.length() > 0 && n.length() > 0) {
+				Map<String, String> cookielets = map.get(k);
+
+				if (cookielets == null) {
+					cookielets = new LinkedHashMap<String, String>();
+					map.put(k, cookielets);
+				}
+
+				cookielets.put(n, v);
+			}
+		}
+	}
+
+	@Override
+	public String encode(Map<String, Map<String, String>> map) {
+		StringBuilder sb = new StringBuilder(32);
+		boolean first = true;
+
+		for (Map.Entry<String, Map<String, String>> entry : map.entrySet()) {
+			String runId = entry.getKey();
+			Map<String, String> cookielets = entry.getValue();
+
+			if (first) {
+				first = false;
+			} else {
+				sb.append('&');
+			}
+
+			boolean first2 = true;
+
+			sb.append(runId).append('=');
+
+			for (Map.Entry<String, String> e : cookielets.entrySet()) {
+				if (first2) {
+					first2 = false;
+				} else {
+					sb.append('|');
+				}
+
+				sb.append(e.getKey()).append(':').append(e.getValue());
+			}
+		}
+
+		return sb.toString();
+	}
+}

cat-core/src/main/java/com/dianping/cat/abtest/spi/internal/DefaultABTestContext.java

 package com.dianping.cat.abtest.spi.internal;
 
 import java.util.Date;
+import java.util.LinkedHashMap;
 import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
@@ -24,12 +24,24 @@ public class DefaultABTestContext implements ABTestContext {
 	private HttpServletResponse m_response;
 
 	private ABTestGroupStrategy m_groupStrategy;
-	
+
 	private Map<String, String> m_cookielets;
 
 	public DefaultABTestContext(ABTestEntity entity) {
 		m_entity = entity;
-		m_cookielets = new ConcurrentHashMap<String, String>();
+	}
+
+	@Override
+	public String getCookielet(String name) {
+		if (m_cookielets != null) {
+			return m_cookielets.get(name);
+		} else {
+			return null;
+		}
+	}
+
+	public Map<String, String> getCookielets() {
+		return m_cookielets;
 	}
 
 	@Override
@@ -52,6 +64,19 @@ public class DefaultABTestContext implements ABTestContext {
 		return m_response;
 	}
 
+	@Override
+	public void setCookielet(String name, String value) {
+		if (m_cookielets == null) {
+			m_cookielets = new LinkedHashMap<String, String>();
+		}
+
+		if (value == null) {
+			m_cookielets.remove(name);
+		} else {
+			m_cookielets.put(name, value);
+		}
+	}
+
 	@Override
 	public void setGroupName(String groupName) {
 		m_groupName = groupName;
@@ -61,11 +86,12 @@ public class DefaultABTestContext implements ABTestContext {
 		m_groupStrategy = groupStrategy;
 	}
 
-	public void setup(HttpServletRequest request, HttpServletResponse response, Date timestamp) {
+	public void setup(HttpServletRequest request, HttpServletResponse response, Map<String, String> cookielets) {
 		m_request = request;
 		m_response = response;
+		m_cookielets = cookielets;
 
-		if (m_entity.isEligible(timestamp)) {
+		if (m_entity.isEligible(new Date())) {
 			Transaction t = Cat.newTransaction("GroupStrategy", m_entity.getGroupStrategyName());
 
 			try {
@@ -80,26 +106,4 @@ public class DefaultABTestContext implements ABTestContext {
 			}
 		}
 	}
-
-	@Override
-   public void setCookielet(String name, String value) {
-		if(value == null){
-			m_cookielets.remove(name);
-		}else{
-			m_cookielets.put(name, value);
-		}
-   }
-
-	@Override
-   public String getCookielet(String name) {
-	   return m_cookielets.get(name);
-   }
-
-	public Map<String, String> getCookielets() {
-   	return m_cookielets;
-   }
-
-	public void setCookielets(Map<String, String> cookielets) {
-   	m_cookielets = cookielets;
-   }
 }

cat-core/src/main/java/com/dianping/cat/abtest/spi/internal/DefaultABTestContextManager.java

 package com.dianping.cat.abtest.spi.internal;
 
-import java.util.Date;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -10,7 +9,6 @@ import javax.servlet.http.Cookie;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
-import org.unidal.helper.Splitters;
 import org.unidal.lookup.ContainerHolder;
 import org.unidal.lookup.annotation.Inject;
 
@@ -30,6 +28,9 @@ public class DefaultABTestContextManager extends ContainerHolder implements ABTe
 	@Inject
 	private MessageManager m_messageManager;
 
+	@Inject
+	private ABTestCodec m_cookieCodec;
+
 	private InheritableThreadLocal<Entry> m_threadLocal = new InheritableThreadLocal<Entry>() {
 		@Override
 		protected Entry initialValue() {
@@ -84,38 +85,15 @@ public class DefaultABTestContextManager extends ContainerHolder implements ABTe
 			return ctx;
 		}
 
-		private Map<String, String> getGroupsFromCookie(HttpServletRequest request) {
-			Map<String, String> map = new HashMap<String, String>();
-			Cookie[] cookies = request.getCookies();
-
-			if (cookies != null) {
-				for (Cookie cookie : cookies) {
-					if (ABTEST_COOKIE_NAME.equals(cookie.getName())) {
-						String value = cookie.getValue();
-						List<String> parts = Splitters.by('|').noEmptyItem().trim().split(value);
-
-						for (String part : parts) {
-							int pos = part.indexOf(':');
-
-							if (pos > 0) {
-								map.put(part.substring(0, pos), part.substring(pos + 1));
-							}
-						}
-					}
-				}
-			}
-
-			return map;
-		}
-		
-		private String getABTestCookie(HttpServletRequest request) {
+		private String getCookie(HttpServletRequest request, String name) {
 			Cookie[] cookies = request.getCookies();
-
 			String value = "";
+
 			if (cookies != null) {
 				for (Cookie cookie : cookies) {
-					if (ABTEST_COOKIE_NAME.equals(cookie.getName())) {
+					if (name.equals(cookie.getName())) {
 						value = cookie.getValue();
+						break;
 					}
 				}
 			}
@@ -123,43 +101,8 @@ public class DefaultABTestContextManager extends ContainerHolder implements ABTe
 			return value;
 		}
 
-		private String setGroupsToCookie(HttpServletRequest request, HttpServletResponse response,
-		      Map<String, String> result) {
-			StringBuilder sb = new StringBuilder(64);
-			boolean first = true;
-
-			for (Map.Entry<String, String> e : result.entrySet()) {
-				if (first) {
-					first = false;
-				} else {
-					sb.append('|');
-				}
-
-				sb.append(e.getKey()).append(':').append(e.getValue());
-			}
-
-			String value = sb.toString();
-			Cookie cookie = new Cookie(ABTEST_COOKIE_NAME, value);
-			String server = request.getServerName();
-
-			if (server.endsWith(".dianping.com")) {
-				cookie.setDomain(".dianping.com");
-			} else if (server.endsWith(".51ping.com")) {
-				cookie.setDomain(".51ping.com");
-			} else {
-				cookie.setDomain(server);
-			}
-
-			cookie.setMaxAge(30 * 24 * 60 * 60); // 30 days expiration
-			cookie.setPath("/");
-
-			response.addCookie(cookie);
-			return value;
-		}
-		
-		private String setABTestCookie(HttpServletRequest request, HttpServletResponse response,
-		      String value) {
-			Cookie cookie = new Cookie(ABTEST_COOKIE_NAME, value);
+		private void setCookie(HttpServletRequest request, HttpServletResponse response, String name, String value) {
+			Cookie cookie = new Cookie(name, value);
 			String server = request.getServerName();
 
 			if (server.endsWith(".dianping.com")) {
@@ -174,97 +117,35 @@ public class DefaultABTestContextManager extends ContainerHolder implements ABTe
 			cookie.setPath("/");
 
 			response.addCookie(cookie);
-			return value;
 		}
 
 		public void setup(HttpServletRequest request, HttpServletResponse response) {
 			List<ABTestEntity> activeEntities = m_entityManager.getEntityList();
 			Set<String> activeRuns = m_entityManager.getActiveRun();
-			Map<String,Map<String,String>> result = new HashMap<String,Map<String,String>>();
-			Map<String,Map<String,String>> map = decode(getABTestCookie(request));
-			
-			for (String id : activeRuns) {
-				if (map.containsKey(id)) {
-					result.put(id, map.get(id));
-				}
-			}
+			String value = getCookie(request, ABTEST_COOKIE_NAME);
+			Map<String, Map<String, String>> map = m_cookieCodec.decode(value, activeRuns);
 
-			for (int i = 0 ; i < activeEntities.size(); i++) {
+			for (int i = 0; i < activeEntities.size(); i++) {
 				ABTestEntity entity = activeEntities.get(i);
 				DefaultABTestContext ctx = (DefaultABTestContext) getContext(entity);
-				String key = String.valueOf(ctx.getEntity().getRun().getId());
+				String key = String.valueOf(entity.getRun().getId());
+				Map<String, String> cookielets = map.get(key);
 
-				ctx.setCookielets(map.get(key));
-				ctx.setup(request, response, new Date());
-				result.put(key, ctx.getCookielets());
-			}
-			
+				ctx.setup(request, response, cookielets);
 
-			String value = setABTestCookie(request, response, encode(result));
-			((DefaultMessageManager) m_messageManager).setMetricType(value);
-		}
+				Map<String, String> newCookielets = ctx.getCookielets();
 
-		public Map<String, Map<String, String>> decode(String cookieValue) {
-			Map<String, Map<String, String>> map = new HashMap<String, Map<String, String>>();
-			char[] chars = cookieValue.toCharArray();
-			int pos = 0;
-			String id = null, name = null, value = null;
-			Map<String, String> cookielets = new HashMap<String, String>();
-
-			for (int i = 0; i < chars.length; i++) {
-				if (chars[i] == '=') {
-					id = cookieValue.substring(pos, i);
-					pos = i + 1;
-				}else if (chars[i] == ':') {
-					name = cookieValue.substring(pos, i);
-					pos = i + 1;
-				}else if (chars[i] == '|') {
-					value = cookieValue.substring(pos, i);
-					cookielets.put(name, value);
-					pos = i + 1;
-				}else if (chars[i] == '&') {
-					value = cookieValue.substring(pos, i);
-					cookielets.put(name, value);
-					map.put(id, cookielets);
-					cookielets = new HashMap<String, String>();
-					pos = i + 1;
-				}
-				
-				if (i == chars.length - 1) {
-					value = cookieValue.substring(pos, i + 1);
-					cookielets.put(name, value);
-					map.put(id, cookielets);
-				}
-			}
-			return map;
-		}
-		
-		public String encode(Map<String,Map<String,String>> maps){
-			StringBuilder sb = new StringBuilder();
-			String[] keySet = maps.keySet().toArray(new String[0]);
-			
-			for(int i = 0 ; i < keySet.length ; i++){
-				sb.append(keySet[i]).append("=");
-				
-				Map<String, String> values = maps.get(keySet[i]);
-				if(values!= null){
-					String[] keySetofValues = values.keySet().toArray(new String[0]);
-					for(int j = 0 ; j < keySetofValues.length ; j++){
-						if(j != keySetofValues.length - 1){
-							sb.append(keySetofValues[j]).append(":").append(values.get(keySetofValues[j])).append("|");
-						}else{
-							sb.append(keySetofValues[j]).append(":").append(values.get(keySetofValues[j]));
-						}
-					}
-					
-					if(i != keySet.length - 1){
-						sb.append("&");
-					}
+				if (newCookielets != null) {
+					map.put(key, newCookielets);
+				} else {
+					map.remove(key);
 				}
-				
 			}
 
-			return sb.toString();
+			String newValue = m_cookieCodec.encode(map);
+
+			setCookie(request, response, ABTEST_COOKIE_NAME, newValue);
+			((DefaultMessageManager) m_messageManager).setMetricType(newValue);
 		}
 	}
 }

cat-core/src/main/java/com/dianping/cat/build/ABTestComponentConfigurator.java

@@ -8,8 +8,10 @@ import org.unidal.lookup.configuration.Component;
 
 import com.dianping.cat.abtest.repository.ABTestEntityRepository;
 import com.dianping.cat.abtest.repository.HttpABTestEntityRepository;
+import com.dianping.cat.abtest.spi.internal.ABTestCodec;
 import com.dianping.cat.abtest.spi.internal.ABTestContextManager;
 import com.dianping.cat.abtest.spi.internal.ABTestEntityManager;
+import com.dianping.cat.abtest.spi.internal.DefaultABTestCodec;
 import com.dianping.cat.abtest.spi.internal.DefaultABTestContextManager;
 import com.dianping.cat.abtest.spi.internal.DefaultABTestEntityManager;
 import com.dianping.cat.configuration.ClientConfigManager;
@@ -21,7 +23,8 @@ class ABTestComponentConfigurator extends AbstractResourceConfigurator {
 		List<Component> all = new ArrayList<Component>();
 
 		all.add(C(ABTestContextManager.class, DefaultABTestContextManager.class) //
-		      .req(ABTestEntityManager.class, MessageManager.class));
+		      .req(ABTestEntityManager.class, MessageManager.class, ABTestCodec.class));
+		all.add(C(ABTestCodec.class, DefaultABTestCodec.class));
 
 		all.add(C(ABTestEntityRepository.class, HttpABTestEntityRepository.class) //
 		      .req(ClientConfigManager.class).config(E("refreshTimeInSeconds").value("60")));

cat-core/src/main/resources/META-INF/plexus/components.xml

@@ -199,8 +199,15 @@
 				<requirement>
 					<role>com.dianping.cat.message.spi.MessageManager</role>
 				</requirement>
+				<requirement>
+					<role>com.dianping.cat.abtest.spi.internal.ABTestCodec</role>
+				</requirement>
 			</requirements>
 		</component>
+		<component>
+			<role>com.dianping.cat.abtest.spi.internal.ABTestCodec</role>
+			<implementation>com.dianping.cat.abtest.spi.internal.DefaultABTestCodec</implementation>
+		</component>
 		<component>
 			<role>com.dianping.cat.abtest.repository.ABTestEntityRepository</role>
 			<implementation>com.dianping.cat.abtest.repository.HttpABTestEntityRepository</implementation>

cat-core/src/test/java/com/dianping/cat/abtest/spi/internal/ABTestContextManagerTest.java

 package com.dianping.cat.abtest.spi.internal;
 
+import java.util.Arrays;
+import java.util.HashSet;
 import java.util.Map;
 
 import junit.framework.Assert;
 
 import org.junit.Test;
+import org.unidal.lookup.ComponentTestCase;
 
-public class ABTestContextManagerTest {
-	
+public class ABTestContextManagerTest extends ComponentTestCase {
 	@Test
-	public void testDecoding(){
-		DefaultABTestContextManager manager = new DefaultABTestContextManager();
-		DefaultABTestContextManager.Entry entry = manager.new Entry();
-		
-		Map<String, Map<String, String>> map = entry.decode("1=ab:A|cd:B&2=ab:A|cd:B");
-		Assert.assertEquals(map.toString(),"{2={ab=A, cd=B}, 1={ab=A, cd=B}}");
-		
-		Map<String, Map<String, String>> map2 = entry.decode("1=ab:|cd:B&2=ab:A|cd:B");
-		Assert.assertEquals(map2.toString(),"{2={ab=A, cd=B}, 1={ab=, cd=B}}");
-		
-		Map<String, Map<String, String>> map3 = entry.decode("1=ab:A|cd:B&2=ab:A|cd:");
-		Assert.assertEquals(map3.toString(),"{2={ab=A, cd=}, 1={ab=A, cd=B}}");
-		
-		Map<String, Map<String, String>> map4 = entry.decode("");
-		Assert.assertEquals(map4.toString(),"{}");
+	public void testCodec() throws Exception {
+		check("1=ab:A|cd:B&2=ab:A|cd:B", "1=ab:A|cd:B&2=ab:A|cd:B");
+		check("1=ab:|cd:B&2=ab:A|cd:B", "1=ab:|cd:B&2=ab:A|cd:B");
+		check("1=ab:A|cd:B&2=ab:A|cd:", "1=ab:A|cd:B&2=ab:A|cd:");
+		check("", "");
+
+		check("1=ab:A|cd:B&2=ab:A|cd:B", "1=ab:A|cd:B&2=ab:A|cd:B", "1", "2");
+		check("1=ab:A|cd:B&2=ab:A|cd:B", "1=ab:A|cd:B", "1");
 	}
-	
-	@Test
-	public void testEncoding(){
-		DefaultABTestContextManager manager = new DefaultABTestContextManager();
-		DefaultABTestContextManager.Entry entry = manager.new Entry();
-		
-		Map<String, Map<String, String>> map = entry.decode("1=ab:A|cd:B&2=ab:A|cd:B");
-		Assert.assertEquals(entry.encode(map),"2=ab:A|cd:B&1=ab:A|cd:B");
-		
-		Map<String, Map<String, String>> map2 = entry.decode("1=ab:|cd:B&2=ab:A|cd:B");
-		Assert.assertEquals(entry.encode(map2),"2=ab:A|cd:B&1=ab:|cd:B");
-		
-		Map<String, Map<String, String>> map3 = entry.decode("1=ab:A|cd:B&2=ab:A|cd:");
-		Assert.assertEquals(entry.encode(map3),"2=ab:A|cd:&1=ab:A|cd:B");
-		
-		Map<String, Map<String, String>> map4 = entry.decode("");
-		Assert.assertEquals(entry.encode(map4),"");
+
+	private void check(String source, String expected, String... keys) throws Exception {
+		ABTestCodec codec = lookup(ABTestCodec.class);
+		Map<String, Map<String, String>> map = codec.decode(source,
+		      keys.length == 0 ? null : new HashSet<String>(Arrays.asList(keys)));
+
+		Assert.assertEquals(expected, codec.encode(map));
 	}
 }