Simplify ContextAwareConfigurationMethodInvocationHandler

As the repositories now return the type checking and constructor logic can now simplified. Primitive types would in principle work. However, if no config can be found, then trying to return null for a primitive return type of a configuration would cause a NullPointerException. To make sure users do not declare primitives as return types, the BaiganConfigClasses throws and exception if there are primitive return types, causing the starting of the ApplicationContext to fail.
zalando-incubator · Sep 22, 2023 · 58f3dce · 58f3dce
1 parent 11a19ca
commit 58f3dce
Show file tree

Hide file tree

Showing 6 changed files with 24 additions and 49 deletions.
diff --git a/README.md b/README.md
@@ -59,6 +59,8 @@ And the _@ConfigurationServiceScan_ annotation hints the Baigan registrar to loo
 	}
 ```
 
+**Note**: Primitives are not supported as return types.
+
 The above example code enables the application to inject _ExpressFeature_ spring bean into any other Spring bean:
 
 ```Java
@@ -118,7 +120,7 @@ A configuration object should conform to the following JSON Schema:
                     },
             		"conditionType": {
                         "description": "Type of condition to evaluate. This can be custom defined, with custom defined properties.",
-                        "type": "object",
+                        "type": "object"
                     }
                 }
             }

diff --git a/src/main/java/org/zalando/baigan/proxy/BaiganConfigClasses.java b/src/main/java/org/zalando/baigan/proxy/BaiganConfigClasses.java
@@ -15,6 +15,11 @@ public BaiganConfigClasses(Map<String, Class<?>> configTypesByKey) {
     }
 
     public void setConfigTypesByKey(Map<String, Class<?>> configTypesByKey) {
+        configTypesByKey.forEach((key, value) -> {
+            if (value.isPrimitive()) {
+                throw new IllegalArgumentException("Config " + key + " has an illegal return type " + value + ". Primitives are not supported as return type.");
+            }
+        });
         this.configTypesByKey = configTypesByKey;
     }
 

diff --git a/...va/org/zalando/baigan/proxy/handler/ContextAwareConfigurationMethodInvocationHandler.java b/...va/org/zalando/baigan/proxy/handler/ContextAwareConfigurationMethodInvocationHandler.java
@@ -1,7 +1,6 @@
 package org.zalando.baigan.proxy.handler;
 
 import com.google.common.base.Supplier;
-import com.google.common.primitives.Primitives;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.BeansException;
@@ -12,13 +11,10 @@
 import org.zalando.baigan.context.ContextProviderRetriever;
 import org.zalando.baigan.model.Configuration;
 import org.zalando.baigan.provider.ContextProvider;
-import org.zalando.baigan.proxy.ProxyUtils;
 import org.zalando.baigan.service.ConditionsProcessor;
 import org.zalando.baigan.service.ConfigurationRepository;
 
-import java.lang.reflect.Constructor;
 import java.lang.reflect.Method;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
@@ -60,48 +56,19 @@ public void setBeanFactory(final BeanFactory beanFactory) throws BeansException
     }
 
     @Override
-    protected Object handleInvocation(Object proxy, Method method,
-                                      Object[] args) throws Throwable {
+    protected Object handleInvocation(Object proxy, Method method, Object[] args) {
         final String key = createKey(getClass(proxy), method);
         final Object result = getConfig(key);
         if (result == null) {
             LOG.warn("Configuration not found for key: {}", key);
             return null;
         }
-
-        final Class<?> declaredReturnType = method.getReturnType();
-
-        try {
-
-            Constructor<?> constructor;
-            if (declaredReturnType.isInstance(result)) {
-                return result;
-            } else if (declaredReturnType.isPrimitive()) {
-                final Class<?> resultClass = result.getClass();
-                constructor = Primitives.wrap(declaredReturnType)
-                        .getDeclaredConstructor(resultClass);
-            } else if (declaredReturnType.isEnum()) {
-                for (Object t : Arrays
-                        .asList(declaredReturnType.getEnumConstants())) {
-                    if (result.toString().equalsIgnoreCase(t.toString())) {
-                        return t;
-                    }
-                }
-                LOG.warn("Unable to map [{}] to enum type [{}].", result, declaredReturnType.getName());
-                return null;
-            } else {
-                constructor = declaredReturnType
-                        .getDeclaredConstructor(result.getClass());
-            }
-            return constructor.newInstance(result);
-        } catch (Exception exception) {
-            LOG.warn(
-                    "Wrong or Incompatible configuration. Cannot find a constructor to create object of type "
-                            + declaredReturnType
-                            + " for value of the configuration key " + key,
-                    exception);
+        if (!method.getReturnType().isInstance(result)) {
+            LOG.error("Configuration repository returned object of wrong type. Expected: {}, actual: {}", method.getReturnType(), result.getClass());
+            return null;
         }
-        return null;
+
+        return result;
     }
 
     private Class<?> getClass(final Object proxy) {

diff --git a/src/main/java/org/zalando/baigan/service/AbstractConfigurationRepository.java b/src/main/java/org/zalando/baigan/service/AbstractConfigurationRepository.java
@@ -4,7 +4,6 @@
 import com.fasterxml.jackson.core.type.TypeReference;
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectMapper;
-import com.fasterxml.jackson.datatype.guava.GuavaModule;
 import org.zalando.baigan.model.Condition;
 import org.zalando.baigan.model.Configuration;
 import org.zalando.baigan.proxy.BaiganConfigClasses;
@@ -60,7 +59,9 @@ private <T> Configuration<T> deserializeConfig(Configuration<JsonNode> config, C
     }
 
     private Class<?> findClass(String alias) {
-        List<Class<?>> matchingClasses = baiganConfigClasses.getConfigTypesByKey().entrySet().stream().filter(entry -> alias.startsWith(entry.getKey())).map(Map.Entry::getValue).collect(toList());
+        List<Class<?>> matchingClasses = baiganConfigClasses.getConfigTypesByKey().entrySet().stream()
+                .filter(entry -> alias.equals(entry.getKey()))
+                .map(Map.Entry::getValue).collect(toList());
 
         if (matchingClasses.size() == 1) {
             return matchingClasses.get(0);

diff --git a/src/main/java/org/zalando/baigan/service/ChainedConfigurationRepository.java b/src/main/java/org/zalando/baigan/service/ChainedConfigurationRepository.java
@@ -51,6 +51,6 @@ public Optional<Configuration> get(@Nonnull String key) {
 
     @Override
     public void put(@Nonnull String key, @Nonnull String value) {
-
+        throw new UnsupportedOperationException("The ChainedConfigurationRepository doesn't allow any changes.");
     }
 }
diff --git a/src/test/java/org/zalando/baigan/context/MethodInvocationHandlerTest.java b/src/test/java/org/zalando/baigan/context/MethodInvocationHandlerTest.java
@@ -26,15 +26,15 @@ enum State {
 
 interface Base {
 
-    boolean isActive();
+    Boolean isActive();
 
 }
 
 interface Express extends Base {
 
     State stateDefault();
 
-    int maxDeliveryDays();
+    Integer maxDeliveryDays();
 }
 
 /**
@@ -48,7 +48,7 @@ public class MethodInvocationHandlerTest {
     public void testEnumValue() throws Throwable {
 
         final ConfigurationRepository repo = mock(ConfigurationRepository.class);
-        final Configuration<String> configuration = new Configuration<>("express.state.default", DESCRIPTION, of(), "SHIPPING");
+        final Configuration<State> configuration = new Configuration<>("express.state.default", DESCRIPTION, of(), State.SHIPPING);
         when(repo.get(anyString())).thenReturn(Optional.of(configuration));
 
         final ContextAwareConfigurationMethodInvocationHandler handler = createHandler(repo);
@@ -58,7 +58,7 @@ public void testEnumValue() throws Throwable {
     }
 
     @Test
-    public void testIllegalEnumValue() throws Throwable {
+    public void testConfigHasWrongType() throws Throwable {
 
         final ConfigurationRepository repo = mock(ConfigurationRepository.class);
         final Configuration<String> configuration = new Configuration<>("express.state.default", DESCRIPTION, of(), "42");
@@ -74,7 +74,7 @@ public void testIllegalEnumValue() throws Throwable {
     public void testPrimitiveType() throws Throwable {
 
         final ConfigurationRepository repo = mock(ConfigurationRepository.class);
-        final Configuration<String> configuration = new Configuration<>("express.max.delivery.days", DESCRIPTION, of(), "3");
+        final Configuration<Integer> configuration = new Configuration<>("express.max.delivery.days", DESCRIPTION, of(), 3);
         when(repo.get(anyString())).thenReturn(Optional.of(configuration));
 
         final ContextAwareConfigurationMethodInvocationHandler handler = createHandler(repo);
@@ -87,7 +87,7 @@ public void testPrimitiveType() throws Throwable {
     public void testInheritedToggle() throws Throwable {
 
         final ConfigurationRepository repo = mock(ConfigurationRepository.class);
-        final Configuration<String> configuration = new Configuration<>("express.is.active", DESCRIPTION, of(), Boolean.TRUE.toString());
+        final Configuration<Boolean> configuration = new Configuration<>("express.is.active", DESCRIPTION, of(), true);
         when(repo.get(anyString())).thenReturn(Optional.of(configuration));
 
         final ContextAwareConfigurationMethodInvocationHandler handler = createHandler(repo);