Refactor SwitcherContextBase to use instance instead of reflection (#328)

* Refactor SwitcherContextBase to use instance instead of reflection

* Fix possible thread race when setting up context instance

* Fixes code smell

Author: petruki

README.md

@@ -61,7 +61,7 @@ You can also use environment variables using the standard notation ${VALUE:DEFAU
 
 ```
 #required
-switcher.context -> Feature class that extends SwitcherContext
+switcher.context -> Feature class that extends SwitcherContext/SwitcherContextBase
 switcher.url -> Switcher-API URL
 switcher.apikey -> Switcher-API key generated for the application/component
 switcher.component -> Application/component name

src/main/java/com/github/switcherapi/client/ContextBuilder.java

@@ -53,11 +53,11 @@ SwitcherProperties build() {
 	}
 
 	/**
-	 * @param contextLocation Feature class that extends SwitcherContext
+	 * @param context Feature class that extends SwitcherContext
 	 * @return ContextBuilder
 	 */
-	public ContextBuilder contextLocation(String contextLocation) {
-		switcherProperties.setValue(ContextKey.CONTEXT_LOCATION, contextLocation);
+	public ContextBuilder context(String context) {
+		switcherProperties.setValue(ContextKey.CONTEXT_LOCATION, context);
 		return this;
 	}
 

src/main/java/com/github/switcherapi/client/SwitcherConfig.java

@@ -2,7 +2,6 @@
 
 abstract class SwitcherConfig {
 
-	protected String contextLocation;
 	protected String url;
 	protected String apikey;
 	protected String domain;
@@ -29,9 +28,14 @@ abstract class SwitcherConfig {
 	 */
 	protected abstract void configureClient();
 
-	public void setContextLocation(String contextLocation) {
-		this.contextLocation = contextLocation;
-	}
+	/**
+	 * Initialize the Switcher Client using a context properties file.<br>
+	 * - Load context properties file {@link SwitcherContextBase#loadProperties(String)}<br>
+	 * - Initialize client {@link SwitcherContextBase#initializeClient()}<br>
+	 *
+	 * @param contextFile path to the context file
+	 */
+	protected abstract void configureClient(String contextFile);
 
 	public void setUrl(String url) {
 		this.url = url;

src/main/java/com/github/switcherapi/client/SwitcherContext.java

@@ -33,7 +33,9 @@ public abstract class SwitcherContext extends SwitcherContextBase {
 	 * After loading the properties, it will validate the arguments and load the Switchers in memory.
 	 */
 	public static void loadProperties() {
-		loadProperties("switcherapi");
+		SwitcherContextBase.contextBase = null;
+		SwitcherContextBase.loadProperties("switcherapi");
+		SwitcherContextBase.initializeClient();
 	}
 
 	/**
@@ -121,5 +123,5 @@ public static boolean contextBol(ContextKey contextKey) {
 	public static void configure(ContextBuilder builder) {
 		SwitcherContextBase.configure(builder);
 	}
-	
+
 }

src/main/java/com/github/switcherapi/client/SwitcherContextBase.java

@@ -65,7 +65,7 @@
  * // Initialize the Switcher Client using ContextBuilder
  * public void configureClient() {
  *  	Features.configure(ContextBuilder.builder()
- *   		.contextLocation("com.business.config.Features")
+ *   		.context("com.business.config.Features")
  *   		.apiKey("API_KEY")
  *   		.domain("Playground")
  *   		.component("switcher-playground")
@@ -91,15 +91,17 @@ public abstract class SwitcherContextBase extends SwitcherConfig {
 	private static ScheduledExecutorService scheduledExecutorService;
 	private static ExecutorService watcherExecutorService;
 	private static SnapshotWatcher watcherSnapshot;
+	protected static SwitcherContextBase contextBase;
 
 	static {
 		switcherProperties = new SwitcherPropertiesImpl();
 	}
 
 	@Override
 	protected void configureClient() {
+		setContextBase(this);
 		configure(ContextBuilder.builder(true)
-				.contextLocation(contextLocation)
+				.context(contextBase.getClass().getName())
 				.url(url)
 				.apiKey(apikey)
 				.domain(domain)
@@ -116,35 +118,47 @@ protected void configureClient() {
 				.truststorePath(truststore.getPath())
 				.truststorePassword(truststore.getPassword()));
 
+		switcherProperties.setValue(ContextKey.CONTEXT_LOCATION, contextBase.getClass().getName());
 		initializeClient();
 	}
-	
+
+	@Override
+	protected void configureClient(String contextFile) {
+		setContextBase(this);
+		loadProperties(contextFile);
+
+		switcherProperties.setValue(ContextKey.CONTEXT_LOCATION, contextBase.getClass().getName());
+		initializeClient();
+	}
+
+	private static synchronized void setContextBase(SwitcherContextBase contextBase) {
+		SwitcherContextBase.contextBase = contextBase;
+	}
+
 	/**
 	 * Load properties from the resources' folder, look up for a given context file name (without extension).<br>
-	 * After loading the properties, it will validate the arguments and load the Switchers in memory.
 	 * <p>
-	 * Use this method optionally if you want to load the settings from a customized file name.
+	 * Use this method optionally if you want to load the settings from properties file.<br>
 	 * </p>
 	 *
 	 * Features must inherit {@link SwitcherContextBase}
 	 * <pre>
-	 * // Load from resources/switcherapi-test.properties 
+	 * // Load from resources/switcherapi-test.properties
 	 * Features.loadProperties("switcherapi-test");
 	 * </pre>
 	 * @param contextFilename to load properties from
 	 */
 	public static void loadProperties(String contextFilename) {
 		try (InputStream input = SwitcherContextBase.class
 				.getClassLoader().getResourceAsStream(String.format("%s.properties", contextFilename))) {
-			
+
 			Properties prop = new Properties();
-            prop.load(input);
-            
-            switcherProperties.loadFromProperties(prop);
-    		initializeClient();
-        } catch (IOException io) {
-        	throw new SwitcherContextException(io.getMessage());
-        }
+			prop.load(input);
+
+			switcherProperties.loadFromProperties(prop);
+		} catch (IOException io) {
+			throw new SwitcherContextException(io.getMessage());
+		}
 	}
 
 	/**
@@ -201,17 +215,29 @@ private static void validateContext() throws SwitcherContextException {
 	 * It will ensure that only properly annotated Switchers can be used.
 	 */
 	private static void validateSwitcherKeys() {
-		try {
-			switcherKeys = new HashSet<>();
-			
-			final Class<?> clazz = Class.forName(contextStr(ContextKey.CONTEXT_LOCATION));
-			for (Field field : clazz.getFields()) {
-				if (field.isAnnotationPresent(SwitcherKey.class)) {
-					switcherKeys.add(field.getName());
-				}
+		if (Objects.nonNull(contextBase)) {
+			registerSwitcherKey(contextBase.getClass().getFields());
+		} else {
+			try {
+				final Class<?> clazz = Class.forName(contextStr(ContextKey.CONTEXT_LOCATION));
+				registerSwitcherKey(clazz.getFields());
+			} catch(ClassNotFoundException e){
+				throw new SwitcherContextException(e.getMessage());
+			}
+		}
+	}
+
+	/**
+	 * Register Switcher Keys based on the annotation {@link SwitcherKey}
+	 *
+	 * @param fields to be registered
+	 */
+	private static void registerSwitcherKey(Field[] fields) {
+		switcherKeys = new HashSet<>();
+		for (Field field : fields) {
+			if (field.isAnnotationPresent(SwitcherKey.class)) {
+				switcherKeys.add(field.getName());
 			}
-		} catch (ClassNotFoundException e) {
-			throw new SwitcherContextException(e.getMessage());
 		}
 	}
 

src/main/java/com/github/switcherapi/client/remote/ClientWS.java

@@ -53,7 +53,7 @@ public interface ClientWS {
 	 * @param token Access token
 	 * @return the execution based on the configured switcher
 	 */
-	CriteriaResponse executeCriteriaService(final Switcher switcher, final String token);
+	CriteriaResponse executeCriteria(final Switcher switcher, final String token);
 
 	/**
 	 * Returns the token to access all available endpoints

src/main/java/com/github/switcherapi/client/remote/ClientWSImpl.java

@@ -52,7 +52,7 @@ public static ClientWS build(SwitcherProperties switcherProperties, ExecutorServ
 	}
 
 	@Override
-	public CriteriaResponse executeCriteriaService(final Switcher switcher, final String token) {
+	public CriteriaResponse executeCriteria(final Switcher switcher, final String token) {
 		final String url = switcherProperties.getValue(ContextKey.URL);
 
 		try {
@@ -183,6 +183,7 @@ public SwitchersCheck checkSwitchers(Set<String> switchers, final String token)
 		try {
 			final HttpResponse<String> response = client.send(HttpRequest.newBuilder()
 					.uri(URI.create(String.format(CHECK_SWITCHERS, url)))
+					.timeout(Duration.ofMillis(timeoutMs))
 					.headers(HEADER_AUTHORIZATION, String.format(TOKEN_TEXT, token),
 							CONTENT_TYPE[0], CONTENT_TYPE[1])
 					.POST(HttpRequest.BodyPublishers.ofString(gson.toJson(new SwitchersCheck(switchers)))

src/main/java/com/github/switcherapi/client/service/remote/ClientRemoteService.java

@@ -47,7 +47,7 @@ public CriteriaResponse executeCriteria(final Switcher switcher)  {
 		try {
 			this.auth(tokenStatus);
 
-			return this.clientWs.executeCriteriaService(switcher,
+			return this.clientWs.executeCriteria(switcher,
 					Optional.of(this.authResponse).orElseGet(AuthResponse::new).getToken());
 		} catch (final SwitcherRemoteException e) {
 			if (tokenStatus != TokenStatus.SILENT) {

src/test/java/com/github/switcherapi/client/SwitcherConfigNativeTest.java

@@ -0,0 +1,52 @@
+package com.github.switcherapi.client;
+
+import com.github.switcherapi.SwitchersBase;
+import com.github.switcherapi.fixture.MockWebServerHelper;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+class SwitcherConfigNativeTest extends MockWebServerHelper {
+
+	@BeforeAll
+	static void setup() throws IOException {
+		MockWebServerHelper.setupMockServer();
+	}
+
+	@AfterAll
+	static void tearDown() throws IOException {
+		MockWebServerHelper.tearDownMockServer();
+	}
+
+	@Test
+	void shouldUseNativeContext() {
+		SwitchersBase context = buildSwitcherClientConfigMinimal(new SwitchersBase(), String.format("http://localhost:%s", mockBackEnd.getPort()));
+		context.configureClient();
+
+		givenResponse(generateMockAuth(10));
+		givenResponse(generateCriteriaResponse("true", false));
+
+		assertTrue(SwitchersBase.getSwitcher(SwitchersBase.USECASE11).isItOn());
+	}
+
+	@Test
+	void shouldUseNativeContextFromProperties() {
+		SwitchersBase context = new SwitchersBase();
+		context.configureClient("switcherapi-native");
+
+		assertTrue(SwitchersBase.getSwitcher(SwitchersBase.USECASE11).isItOn());
+	}
+
+	private <T extends SwitcherConfig> T buildSwitcherClientConfigMinimal(T classConfig, String url) {
+		classConfig.setUrl(url);
+		classConfig.setApikey("[API-KEY]");
+		classConfig.setDomain("domain");
+		classConfig.setComponent("component");
+		return classConfig;
+	}
+
+}

src/test/java/com/github/switcherapi/client/SwitcherConfigTest.java

@@ -48,7 +48,6 @@ private <T extends SwitcherConfig> T buildSwitcherClientConfig(T classConfig, St
 		truststore.setPath(null);
 		truststore.setPassword(null);
 
-		classConfig.setContextLocation(SwitchersBase.class.getName());
 		classConfig.setUrl("http://localhost:3000");
 		classConfig.setApikey("[API-KEY]");
 		classConfig.setComponent(component);
@@ -67,7 +66,6 @@ private <T extends SwitcherConfig> T buildSwitcherClientConfigMinimal(T classCon
 		SwitcherConfig.SnapshotConfig snapshot = new SwitcherConfig.SnapshotConfig();
 		snapshot.setLocation(SNAPSHOTS_LOCAL);
 
-		classConfig.setContextLocation(SwitchersBase.class.getName());
 		classConfig.setEnvironment("fixture1");
 		classConfig.setLocal(true);
 		classConfig.setSnapshot(snapshot);