Merge pull request kroxylicious#608 from SamBarker/config_recfactor_r…

…ound2

add Contributes and ConfigurationDefinition

CHANGELOG.md

@@ -6,6 +6,7 @@ Please enumerate **all user-facing** changes using format `<githib issue/pr numb
 
 ## 0.3.0
 
+* [#608](https://github.com/kroxylicious/kroxylicious/pull/608): Improve the contributor API to allow it to express more properties about the configuration. This release deprecates `Contributor.getConfigType` in favour of `Contributor.getConfigDefinition`. It also removes the proliferation of ContributorManager classes by providing a single type which can handle all Contributors.
 * [#538](https://github.com/kroxylicious/kroxylicious/pull/538): Refactor FilterHandler and fix several bugs that would leave messages unflushed to client/broker.
 * [#531](https://github.com/kroxylicious/kroxylicious/pull/531): Simple Test Client now supports multi-RPC conversations with the server.
 * [#510](https://github.com/kroxylicious/kroxylicious/pull/510): Add multi-tenant kubernetes example
@@ -50,4 +51,4 @@ In the kroxylicious config, the brokerAddressPattern parameter for the PortPerBr
 present on the brokerAddressPattern parameter. Previously, it was accepted but would lead to a failure later.
 
 Kroxylicious configuration no longer requires a non empty `filters` list, users can leave it unset or configure in an empty
-list of filters and Kroxylicious will proxy to the cluster successfully.
+list of filters and Kroxylicious will proxy to the cluster successfully.

kroxylicious-additional-filters/kroxylicious-multitenant/src/test/java/io/kroxylicious/proxy/filter/multitenant/MultiTenantFilterContributorTest.java

@@ -12,6 +12,7 @@
 import io.kroxylicious.proxy.config.BaseConfig;
 import io.kroxylicious.proxy.filter.Filter;
 import io.kroxylicious.proxy.filter.FilterConstructContext;
+import io.kroxylicious.proxy.service.ConfigurationDefinition;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -24,11 +25,18 @@ void testGetConfigType() {
         assertThat(configType).isEqualTo(BaseConfig.class);
     }
 
+    @Test
+    void testGetConfigTypeViaConfigurationDefinition() {
+        MultiTenantFilterContributor contributor = new MultiTenantFilterContributor();
+        ConfigurationDefinition actualConfigurationDefinition = contributor.getConfigDefinition("MultiTenant");
+        assertThat(actualConfigurationDefinition).isNotNull().hasFieldOrPropertyWithValue("configurationType", BaseConfig.class);
+    }
+
     @Test
     void testGetInstance() {
         MultiTenantFilterContributor contributor = new MultiTenantFilterContributor();
         Filter filter = contributor.getInstance("MultiTenant", Mockito.mock(FilterConstructContext.class));
         assertThat(filter).isNotNull().isInstanceOf(MultiTenantTransformationFilter.class);
     }
 
-}
+}

kroxylicious-additional-filters/kroxylicious-schema-validation/src/test/java/io/kroxylicious/proxy/filter/schema/ProduceRequestValidationFilterContributorTest.java

@@ -16,6 +16,7 @@
 import io.kroxylicious.proxy.filter.FilterConstructContext;
 import io.kroxylicious.proxy.filter.schema.config.RecordValidationRule;
 import io.kroxylicious.proxy.filter.schema.config.ValidationConfig;
+import io.kroxylicious.proxy.service.ConfigurationDefinition;
 
 import static org.assertj.core.api.Assertions.assertThat;
 
@@ -28,6 +29,13 @@ void testGetConfigType() {
         assertThat(configType).isEqualTo(ValidationConfig.class);
     }
 
+    @Test
+    void testGetConfigTypeViaConfigurationDefinition() {
+        ProduceRequestValidationFilterContributor contributor = new ProduceRequestValidationFilterContributor();
+        ConfigurationDefinition actualConfigurationDefinition = contributor.getConfigDefinition("ProduceValidator");
+        assertThat(actualConfigurationDefinition).isNotNull().hasFieldOrPropertyWithValue("configurationType", ValidationConfig.class);
+    }
+
     @Test
     void testGetInstance() {
         ProduceRequestValidationFilterContributor contributor = new ProduceRequestValidationFilterContributor();

kroxylicious-api/src/main/java/io/kroxylicious/proxy/service/BaseContributor.java

@@ -20,35 +20,76 @@
  */
 public abstract class BaseContributor<T, S extends Context> implements Contributor<T, S> {
 
-    private final Map<String, InstanceBuilder<T, S>> shortNameToInstanceBuilder;
+    private final Map<String, ContributionDetails<T, S>> typeNameToInstanceBuilder;
 
     /**
      * Constructs and configures the contributor using the supplied {@code builder}.
      * @param builder builder
      */
     protected BaseContributor(BaseContributorBuilder<T, S> builder) {
-        shortNameToInstanceBuilder = builder.build();
+        typeNameToInstanceBuilder = builder.build();
     }
 
+    /**
+     * Allows consumers to identify if this contributor recognises the requested type.
+     * @param typeName The type name to test for.
+     * @return <code>true</code> if this Contributor provides the typeName.
+     */
+    @Override
+    public boolean contributes(String typeName) {
+        return typeNameToInstanceBuilder.containsKey(typeName);
+    }
+
+    /**
+     * Returns the details of configuration of the supplied typeName
+     * @param typeName the name of the type.
+     * @return the configuration definition for the typename
+     * @throws IllegalArgumentException if the type name is not known to this contributor
+     */
+    @Override
+    public ConfigurationDefinition getConfigDefinition(String typeName) {
+        if (typeNameToInstanceBuilder.containsKey(typeName)) {
+            return typeNameToInstanceBuilder.get(typeName).configurationDefinition();
+        }
+        else {
+            throw new IllegalArgumentException("No configuration definition registered for " + typeName);
+        }
+    }
+
+    /**
+     * @deprecated in favour of calling {@link #getConfigDefinition(String)}}
+     * @param typeName service short name
+     *
+     * @return {@code null} if the typename is not know to this contributor.
+     */
+    @SuppressWarnings("removal")
     @Override
-    public Class<? extends BaseConfig> getConfigType(String shortName) {
-        InstanceBuilder<T, S> instanceBuilder = shortNameToInstanceBuilder.get(shortName);
-        return instanceBuilder == null ? null : instanceBuilder.configClass;
+    @Deprecated(forRemoval = true, since = "0.3.0")
+    public Class<? extends BaseConfig> getConfigType(String typeName) {
+        if (typeNameToInstanceBuilder.containsKey(typeName)) {
+            return typeNameToInstanceBuilder.get(typeName).configurationDefinition().configurationType();
+        }
+        else {
+            return null;
+        }
     }
 
     @Override
-    public T getInstance(String shortName, S context) {
-        InstanceBuilder<T, S> instanceBuilder = shortNameToInstanceBuilder.get(shortName);
-        return instanceBuilder == null ? null : instanceBuilder.construct(context);
+    public T getInstance(String typeName, S context) {
+        if (typeNameToInstanceBuilder.containsKey(typeName)) {
+            InstanceBuilder<T, S> instanceBuilder = typeNameToInstanceBuilder.get(typeName).instanceBuilder();
+            return instanceBuilder == null ? null : instanceBuilder.construct(context);
+        }
+        else {
+            return null;
+        }
     }
 
     private static class InstanceBuilder<L, D extends Context> {
 
-        private final Class<? extends BaseConfig> configClass;
         private final Function<D, L> instanceFunction;
 
-        InstanceBuilder(Class<? extends BaseConfig> configClass, Function<D, L> instanceFunction) {
-            this.configClass = configClass;
+        InstanceBuilder(Function<D, L> instanceFunction) {
             this.instanceFunction = instanceFunction;
         }
 
@@ -57,7 +98,7 @@ L construct(D context) {
         }
 
         static <T extends BaseConfig, L, D extends Context> InstanceBuilder<L, D> builder(Class<T> configClass, BiFunction<D, T, L> instanceFunction) {
-            return new InstanceBuilder<>(configClass, context -> {
+            return new InstanceBuilder<>(context -> {
                 BaseConfig config = context.getConfig();
                 if (config == null) {
                     // tests pass in a null config, which some instance functions can tolerate
@@ -85,62 +126,63 @@ public static class BaseContributorBuilder<L, D extends Context> {
         private BaseContributorBuilder() {
         }
 
-        private final Map<String, InstanceBuilder<L, D>> shortNameToInstanceBuilder = new HashMap<>();
+        private final Map<String, ContributionDetails<L, D>> typeNameToInstanceBuilder = new HashMap<>();
 
         /**
          * Registers a factory function for the construction of a service instance.
          *
-         * @param shortName service short name
+         * @param typeName service short name
          * @param configClass concrete type of configuration required by the service
          * @param instanceFunction function that constructs the service instance
          * @return this
          * @param <T> the configuration concrete type
          */
-        public <T extends BaseConfig> BaseContributorBuilder<L, D> add(String shortName, Class<T> configClass, Function<T, L> instanceFunction) {
-            return add(shortName, configClass, (context, config) -> instanceFunction.apply(config));
+        public <T extends BaseConfig> BaseContributorBuilder<L, D> add(String typeName, Class<T> configClass, Function<T, L> instanceFunction) {
+            return add(typeName, configClass, (context, config) -> instanceFunction.apply(config));
         }
 
         /**
          * Registers a factory function for the construction of a service instance.
          *
-         * @param shortName service short name
+         * @param typeName service short name
          * @param configClass concrete type of configuration required by the service
          * @param instanceFunction function that constructs the service instance
          * @return this
          * @param <T> the configuration concrete type
          */
-        public <T extends BaseConfig> BaseContributorBuilder<L, D> add(String shortName, Class<T> configClass, BiFunction<D, T, L> instanceFunction) {
-            if (shortNameToInstanceBuilder.containsKey(shortName)) {
-                throw new IllegalArgumentException(shortName + " already registered");
+        public <T extends BaseConfig> BaseContributorBuilder<L, D> add(String typeName, Class<T> configClass, BiFunction<D, T, L> instanceFunction) {
+            if (typeNameToInstanceBuilder.containsKey(typeName)) {
+                throw new IllegalArgumentException(typeName + " already registered");
             }
-            shortNameToInstanceBuilder.put(shortName, InstanceBuilder.builder(configClass, instanceFunction));
+            typeNameToInstanceBuilder.put(typeName,
+                    new ContributionDetails<>(new ConfigurationDefinition(configClass), InstanceBuilder.builder(configClass, instanceFunction)));
             return this;
         }
 
         /**
          * Registers a factory function for the construction of a service instance.
          *
-         * @param shortName service short name
+         * @param typeName service short name
          * @param instanceFunction function that constructs the service instance
          * @return this
          */
-        public BaseContributorBuilder<L, D> add(String shortName, Supplier<L> instanceFunction) {
-            return add(shortName, BaseConfig.class, config -> instanceFunction.get());
+        public BaseContributorBuilder<L, D> add(String typeName, Supplier<L> instanceFunction) {
+            return add(typeName, BaseConfig.class, config -> instanceFunction.get());
         }
 
         /**
          * Registers a factory function for the construction of a service instance.
          *
-         * @param shortName service short name
+         * @param typeName service short name
          * @param instanceFunction function that constructs the service instance from a context
          * @return this
          */
-        public BaseContributorBuilder<L, D> add(String shortName, Function<D, L> instanceFunction) {
-            return add(shortName, BaseConfig.class, (context, config) -> instanceFunction.apply(context));
+        public BaseContributorBuilder<L, D> add(String typeName, Function<D, L> instanceFunction) {
+            return add(typeName, BaseConfig.class, (context, config) -> instanceFunction.apply(context));
         }
 
-        Map<String, InstanceBuilder<L, D>> build() {
-            return Map.copyOf(shortNameToInstanceBuilder);
+        Map<String, ContributionDetails<L, D>> build() {
+            return Map.copyOf(typeNameToInstanceBuilder);
         }
     }
 
@@ -153,4 +195,6 @@ Map<String, InstanceBuilder<L, D>> build() {
     public static <L, D extends Context> BaseContributorBuilder<L, D> builder() {
         return new BaseContributorBuilder<>();
     }
-}
+
+    protected record ContributionDetails<T, D extends Context>(ConfigurationDefinition configurationDefinition, InstanceBuilder<T, D> instanceBuilder) {}
+}

kroxylicious-api/src/main/java/io/kroxylicious/proxy/service/ConfigurationDefinition.java

@@ -0,0 +1,16 @@
+/*
+ * Copyright Kroxylicious Authors.
+ *
+ * Licensed under the Apache Software License version 2.0, available at http://www.apache.org/licenses/LICENSE-2.0
+ */
+
+package io.kroxylicious.proxy.service;
+
+import io.kroxylicious.proxy.config.BaseConfig;
+
+/**
+ * Defines the details of how Contributions interact with configuration for {@link BaseContributor}'s purposes.
+ *
+ * @param configurationType defines the expected class for configuration objects
+ */
+public record ConfigurationDefinition(Class<? extends BaseConfig> configurationType) {}

kroxylicious-api/src/main/java/io/kroxylicious/proxy/service/Contributor.java

@@ -16,19 +16,37 @@
 public interface Contributor<T, S extends Context> {
 
     /**
+     * Identifies if this Contributor instance can create instances for the provided short name.
+     * @param typeName The type name to test for.
+     * @return <code>true</code> if the <code>typeName</code> is supported by this contributor.
+     */
+    boolean contributes(String typeName);
+
+    /**
+     * @deprecated subsumed into {@link ConfigurationDefinition} and thus {@link Contributor#getConfigDefinition(String)}
+     *
      * Gets the concrete type of the configuration required by this service instance.
-     * @param shortName service short name
+     * @param typeName service short name
      *
      * @return class of a concrete type, or null if this contributor does not offer this short name.
      */
-    Class<? extends BaseConfig> getConfigType(String shortName);
+    @Deprecated(forRemoval = true, since = "0.3.0")
+    Class<? extends BaseConfig> getConfigType(String typeName);
+
+    /**
+     * Defines the configuration requirements of this contributor for the given short name.
+     * @param typeName the name of the type.
+     * @return the ConfigurationDefinition for the short name
+     */
+    ConfigurationDefinition getConfigDefinition(String typeName);
 
     /**
      * Creates an instance of the service.
      *
-     * @param shortName service short name
+     * @param typeName service short name
      * @param context   context containing service configuration which may be null if the service instance does not accept configuration.
      * @return the service instance, or null if this contributor does not offer this short name.
      */
-    T getInstance(String shortName, S context);
-}
+    T getInstance(String typeName, S context);
+
+}

kroxylicious-api/src/test/java/io/kroxylicious/proxy/service/BaseContributorTest.java

@@ -34,6 +34,16 @@ void testDefaultConfigClass() {
         assertThat(one).isEqualTo(BaseConfig.class);
     }
 
+    @Test
+    void testDefaultConfigClassFromConfigDefinition() {
+        BaseContributor.BaseContributorBuilder<Long, Context> builder = BaseContributor.builder();
+        builder.add("one", () -> 1L);
+        BaseContributor<Long, Context> baseContributor = new BaseContributor<>(builder) {
+        };
+        ConfigurationDefinition one = baseContributor.getConfigDefinition("one");
+        assertThat(one).hasFieldOrPropertyWithValue("configurationType", BaseConfig.class);
+    }
+
     @Test
     void testSupplier() {
         BaseContributor.BaseContributorBuilder<Long, Context> builder = BaseContributor.builder();
@@ -80,6 +90,16 @@ void testSpecifyingConfigType() {
         assertThat(configType).isEqualTo(LongConfig.class);
     }
 
+    @Test
+    void testSpecifyingConfigTypeViaConfigDefinition() {
+        BaseContributor.BaseContributorBuilder<Long, Context> builder = BaseContributor.builder();
+        builder.add("fromBaseConfig", LongConfig.class, baseConfig -> baseConfig.value);
+        BaseContributor<Long, Context> baseContributor = new BaseContributor<>(builder) {
+        };
+        ConfigurationDefinition actualConfigDefinition = baseContributor.getConfigDefinition("fromBaseConfig");
+        assertThat(actualConfigDefinition).hasFieldOrPropertyWithValue("configurationType", LongConfig.class);
+    }
+
     @Test
     void testSpecifyingConfigTypeInstance() {
         BaseContributor.BaseContributorBuilder<Long, Context> builder = BaseContributor.builder();
@@ -112,10 +132,36 @@ void testFailsIfConfigNotAssignableToSpecifiedType() {
         builder.add("fromBaseConfig", LongConfig.class, baseConfig -> baseConfig.value);
         BaseContributor<Long, Context> baseContributor = new BaseContributor<>(builder) {
         };
-        AnotherConfig incompatibleConfig = new AnotherConfig();
-        assertThatThrownBy(() -> {
-            baseContributor.getInstance("fromBaseConfig", wrap(incompatibleConfig));
-        }).isInstanceOf(IllegalArgumentException.class);
+        final Context wrappedContext = wrap(new AnotherConfig());
+
+        assertThatThrownBy(() -> baseContributor.getInstance("fromBaseConfig", wrappedContext)).isInstanceOf(IllegalArgumentException.class);
     }
 
-}
+    @Test
+    void shouldGetConfigDefinitionForKnownTypeName() {
+        // Given
+        BaseContributor.BaseContributorBuilder<Long, Context> builder = BaseContributor.builder();
+        builder.add("fromBaseConfig", LongConfig.class, baseConfig -> baseConfig.value);
+        BaseContributor<Long, Context> baseContributor = new BaseContributor<>(builder) {
+        };
+
+        // When
+        ConfigurationDefinition actual = baseContributor.getConfigDefinition("fromBaseConfig");
+
+        // Then
+        assertThat(actual).isNotNull().hasFieldOrPropertyWithValue("configurationType", LongConfig.class);
+    }
+
+    @Test
+    void shouldThrowForUnknownTypeNameConfigDefinition() {
+        // Given
+        BaseContributor.BaseContributorBuilder<Long, Context> builder = BaseContributor.builder();
+        BaseContributor<Long, Context> baseContributor = new BaseContributor<>(builder) {
+        };
+
+        // When
+        assertThatThrownBy(() -> baseContributor.getConfigDefinition("fromBaseConfig")).isInstanceOf(IllegalArgumentException.class);
+
+        // Then
+    }
+}