Enable structured types as config values #77

lukas-c-wilhelm · 2023-09-14T14:00:01Z

This commit enables structured types as config values. That is, something like

@BaiganConfig
public interface SomeComplexConfiguration {
    SomeConfigObject someConfig();
}

can now be deserialized, e.g. from a config file with content

[{ "alias": "some.complex.configuration.some.config", "defaultValue": {"config_key":"a value"}}]

Users of the library do not have to add any extra configuration to enable structured config types.

It works like this:

There is a class BaiganConfigClasses that provides a mapping of config keys to their types. It is provided as a bean that is constructed by the ContextAwareConfigurationMethodInvocationHandler when creating the proxies for the BaiganConfig interfaces.
The repository is no longer handling the deserialization. Instead, it only reads the data from the source and hands it to a bean of type ConfigurationParser.
The ConfigurationParser does the actual deserialization of the config values. It first deserializes the data to Configuration<JsonNode>, then deserializes the JsonNode config value to the correct class for each config key, using a bean of type BaiganConfigClasses to determine the right type.
The ConfigurationParser is passed to the repositories by the RepositoryFactory. Library users do not need to be aware of these details.
The repository builders accept an ObjectMapper instance that will be used for the deserialization of the configuration.

The change is backwards compatible with respect to config files. All old config files can be processes as before. This is ensured in End2End tests.

TODOs

Check test coverage for changes
End2End tests for other repositories
Update documentation

This commit enables structured types as config values. That is, something like ``` @BaiganConfig public interface SomeComplexConfiguration { SomeConfigObject someConfig(); } ``` can now be deserialized, e.g. from a config file with content ``` [{ "alias": "some.complex.configuration.some.config", "defaultValue": {"config_key":"a value"}}] ``` How it works is that the repositories have to be constructed with a class `BaiganConfigClasses`, a container for all interfaces annotated with `@BaiganConfig`. The repository then deserializes first to `JsonNode`, then deserializes this to the correct class for each config key. The `BaiganConfigClasses` is provided as a bean. It is constructed by the `ContextAwareConfigurationMethodInvocationHandler` when creating the proxies for the `BaiganConfig` interfaces. Beyond this bean, users of the library do not have to add any extra configuration to enable structured config types. They only have to make sure that the `ObjectMapper` can deserialize the classes correctly. To this end, the `ObjectMapper` can now be injected into the repositories. The change is backwards compatible with respect to config files. All old config files can be processes as before. This is covered in an End2End test. It is not generally backwards compatible regarding the code as deprecated constructors for the `S3ConfigurationRepository` were removed and all constructors for repositories now require `BaiganConfigClasses` to be passed.

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.

… inheritance The inheritance implied by the AbstractConfigurationRepository makes it harder to test its logic and that of the repositories that use it. This commit changes it to composition, turning the abstract class into a service that is used by the repositories. The EtcdConfigurationRepository did not actually use any of the functionality of the abstract class, so it does not have the new service as component. Also, this commit increases the test coverage.

lukas-c-wilhelm · 2023-09-29T13:57:55Z

@lukasniemeier-zalando do we want to add a component test for the GitConfigurationRepo or just remove it since it is marked deprecated anyway?

lukasniemeier-zalando

I like the change as it moves deserialization to the pre-caching layer, which I believe is a crucial change once we allow complex type (deserialization). It's a considerably large change that I expect us to iterate a bit on, we may have to communicate this to library users (and find an early adopter application for validation).

I am interested in the thoughts of other baigan maintainers (if there are any left 😢) regarding if one want to support complex types with baigan. I know that the original intention was to only support simple feature flags by design (true/false or 0/100).

src/main/java/org/zalando/baigan/service/EtcdConfigurationRepository.java

src/main/java/org/zalando/baigan/service/ConfigurationParser.java

src/main/java/org/zalando/baigan/service/FileSystemConfigurationRepository.java

lukasniemeier-zalando · 2023-10-04T13:12:25Z

do we want to add a component test for the GitConfigurationRepo or just remove it since it is marked deprecated anyway?

My vote goes for removing it (in a separate, maybe earlier) PR. We have discourage from usage for many years

lukas-c-wilhelm · 2023-10-05T13:30:54Z

do we want to add a component test for the GitConfigurationRepo or just remove it since it is marked deprecated anyway?

My vote goes for removing it (in a separate, maybe earlier) PR. We have discourage from usage for many years

Done in #78

lukas-c-wilhelm · 2023-10-05T17:08:39Z

I like the change as it moves deserialization to the pre-caching layer, which I believe is a crucial change once we allow complex type (deserialization). It's a considerably large change that I expect us to iterate a bit on, we may have to communicate this to library users (and find an early adopter application for validation).

I am interested in the thoughts of other baigan maintainers (if there are any left 😢) regarding if one want to support complex types with baigan. I know that the original intention was to only support simple feature flags by design (true/false or 0/100).

Good call out. I'd appreciate input by other maintainers as well. The reasoning is that I see Baigan applied not just as a feature flag library, but as a general library for business configuration, where each file contains the configuration for a particular use case. This is a lot nicer if structured configuration types are possible.

This commit adds a builder for the FileSystemConfigurationRepository to simplify future refactorings of the FileSystemConfigurationRepository and hiding potential future complexity of the construction of this class. This was triggered by the discussion #77 (comment)

kasmarian

Hi, first, I'd like to note that I love the change, and it's something that is truly useful to have supported by the library out of the box. In my team, we used complex configs for a long time, but we're applying workarounds. I've added questions related to the backward compatibility of such workaround with this feature release.

src/main/java/org/zalando/baigan/proxy/ConfigurationBeanDefinitionRegistrar.java

.../java/org/zalando/baigan/proxy/handler/ContextAwareConfigurationMethodInvocationHandler.java

src/main/java/org/zalando/baigan/service/ChainedConfigurationRepository.java

This commit adds support for handling generics in the retunr type of a config method. Instead of parsing `Map<UUID, List<SomeConfigObject>>` to a `Map<String, List<Map<...>>>`, it is now correctly processed to the expected type. This works by using `Type` instead of `Class<?>` in `BaiganConfigClasses` and actually using the `AnnotatedType` instead of the plain type when constructing the bean in `ConfigurationBeanRegistrar`.

Repositories no longer require a `BaiganConfigClasses` instance upon construction. Instead, this class is hidden by a `ConfigTypeProvider` and a `ConfigurationParser` bean is provided to a `ConfigurationRepository` by the latter implementing `ApplicationContextAware`.

lukasniemeier-zalando

Looks good to me, many thanks! In terms of open tasks I see the "documentation one", for which I feel a light refresh of the README can also be a dedicated PR (we also still have #78 (comment)). Besides that I guess it's just resolving the merge conflicts now, then we take it in.

lukasniemeier-zalando · 2023-11-14T22:31:12Z

src/main/java/org/zalando/baigan/service/ConfigurationParser.java

+    final ConfigTypeProvider configTypeProvider;
+
+    @Autowired
+    public ConfigurationParser(final ConfigTypeProvider configTypeProvider, @Qualifier("baiganObjectMapper") final Optional<ObjectMapper> objectMapper) {


Do we have a test (or shall we add one) that verifies that an application context offering a specifically qualified ObjectMapper bean is handing it to the ConfigurationParser (plus the negative variants of that validation)?

There is a test that does it, but since it does not special configuration of the object mapper, it does not actually validate it properly. I'll extend this to cover the case.

lukas-c-wilhelm · 2023-11-15T07:25:52Z

Looks good to me, many thanks! In terms of open tasks I see the "documentation one", for which I feel a light refresh of the README can also be a dedicated PR (we also still have #78 (comment)). Besides that I guess it's just resolving the merge conflicts now, then we take it in.

Can you also check the change proposal #82

The current PR makes using the ChainedConfigurationRepository error prone as you can construct the individual repos with the constructor, but if they are not created as beans, the ConfigurationParser is not actually set and they don't work. The PR linked above fixes this at the cost of exposing a RepositoryFactory bean.

While relatively silently accepting failed parsing of configuration is problematic, this is how the EtcdConfigurationRepository behaved in the past. So it makes sense to not change the behavior here as the main change of the whole PR is not concerned with this.

lukasniemeier-zalando · 2023-11-24T18:24:31Z

👍

lukas-c-wilhelm added 5 commits September 14, 2023 15:58

handle config without matching class; simplify tests

9b1756f

increase test coverage

de5bd8e

lukasniemeier-zalando reviewed Oct 4, 2023

View reviewed changes

lukas-c-wilhelm mentioned this pull request Oct 5, 2023

delete GitConfigurationRepository #78

Merged

lukas-c-wilhelm mentioned this pull request Oct 5, 2023

builder for FileSystemConfigurationRepository #79

Merged

kasmarian reviewed Oct 5, 2023

View reviewed changes

lukas-c-wilhelm added 2 commits October 6, 2023 08:49

fix to use generic return type instead of annotated return type

4c645ee

lukas-c-wilhelm mentioned this pull request Oct 6, 2023

improve test coverage #80

Merged

lukas-c-wilhelm added 4 commits October 9, 2023 18:55

Merge branch 'main' into typed-configs

63e35c0

ConfigurationRepository should not extend ApplicationContextAware

b1827b8

qualify ObjectMapper bean

6111e9c

lukas-c-wilhelm mentioned this pull request Oct 12, 2023

end-2-end test for EtcdConfigurationRepository #81

Merged

Merge branch 'test-for-etcd' into typed-configs

80a9cba

lukasniemeier-zalando approved these changes Nov 14, 2023

View reviewed changes

lukas-c-wilhelm mentioned this pull request Nov 19, 2023

enable chained config repo #82

Closed

lukas-c-wilhelm added 4 commits November 21, 2023 08:22

Merge branch 'main' into typed-configs

e31a25f

add ConfigurationParser to RepositoryFactory

a1958c6

configurable ObjectMapper; bring back Java doc

c9945e8

documentation

cfc1ef1

lukas-c-wilhelm added 3 commits November 21, 2023 10:20

simplify code

1b88cc3

javadoc

b5fe180

lukas-c-wilhelm marked this pull request as ready for review November 21, 2023 10:54

lukasniemeier-zalando merged commit 4c0735c into main Nov 24, 2023
2 checks passed

lukasniemeier-zalando deleted the typed-configs branch November 24, 2023 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable structured types as config values #77

Enable structured types as config values #77

lukas-c-wilhelm commented Sep 14, 2023 •

edited

Loading

lukas-c-wilhelm commented Sep 29, 2023

lukasniemeier-zalando left a comment

lukasniemeier-zalando commented Oct 4, 2023

lukas-c-wilhelm commented Oct 5, 2023

lukas-c-wilhelm commented Oct 5, 2023

kasmarian left a comment

lukasniemeier-zalando left a comment

lukasniemeier-zalando Nov 14, 2023

lukas-c-wilhelm Nov 15, 2023

lukas-c-wilhelm commented Nov 15, 2023

lukasniemeier-zalando commented Nov 24, 2023

Enable structured types as config values #77

Enable structured types as config values #77

Conversation

lukas-c-wilhelm commented Sep 14, 2023 • edited Loading

lukas-c-wilhelm commented Sep 29, 2023

lukasniemeier-zalando left a comment

Choose a reason for hiding this comment

lukasniemeier-zalando commented Oct 4, 2023

lukas-c-wilhelm commented Oct 5, 2023

lukas-c-wilhelm commented Oct 5, 2023

kasmarian left a comment

Choose a reason for hiding this comment

lukasniemeier-zalando left a comment

Choose a reason for hiding this comment

lukasniemeier-zalando Nov 14, 2023

Choose a reason for hiding this comment

lukas-c-wilhelm Nov 15, 2023

Choose a reason for hiding this comment

lukas-c-wilhelm commented Nov 15, 2023

lukasniemeier-zalando commented Nov 24, 2023

lukas-c-wilhelm commented Sep 14, 2023 •

edited

Loading