Configuration mapper

[dmlloyd]

Allow mapping of configurations at an interface level. Fixes #279.

Features:

• Configuration properties are mapped to interface methods
• Multiple inheritance of configuration interfaces (only interfaces with at least one abstract method are considered to be configuration interfaces)
• Covariant overriding of configuration subgroups
• Map support:
  • Maps of subgroups or leaf types
  • Separate conversion of Map keys and values (map key types are not limited to String)
  • Arbitrary Map nesting depth
• Support for Optional leaf properties and subgroups
• Support for primitive property types
• "Annotation-free" in the common case (only use annotations to override things like the property name, conversion behavior, default values, etc.)
• Pluggable conversion via @ConvertWith, including groundwork for supporting fully generic conversion (see Arbitrary parameterized type as a config property? #40)
• Validation support (WIP)
• Global registration of default values on the configuration source builder
• Mapping of configuration property names to "skewered" names (Quarkus style)
• Implemented as a separate module (so feel free to reject the idea)

[dmlloyd]

/cc @geoand

[dmlloyd]

This is still a draft because it needs a lot more tests written and there are several known bugs still. But I wanted to open the PR as soon as I was sure the idea was feasible, so that the idea is out in the wild and can be reviewed.

[geoand]

Oh, nice!

[geoand]

I'll check it out more tomorrow

[geoand]

I love the idea! Looking forward to seeing more tests so as to see examples of it in action.

[radcortez]

Might be too much to review :)

But I'll give it a go.

[dmlloyd]

It's more important to review the API than the implementation at the moment. The implementation is full of bugs :)

[radcortez]

Hi @dmlloyd

Some quick notes:

• +1 to @CompareWith, @Default and @WithName. We can even move this to the main implementation.
• We probably need an annotation to set up the root in the interface
• +1 to Default Global values. We can move this to the main implementation.

I know this is controversial, but for validation, I would prefer if we could come up with some ideia in how to support the BVal annotations without the extra load. From a user experience perspective, it is confusing to use different annotations with the same semantical meaning. I think it would be interesting if we could provide a more coherent developer experience by reusing some APIs.

Or we end up with things like this:

    @Schema(
        required = true,
        minimum = "1",
        maximum = "10",
    )
    @NotNull
    @Min(1)
    @Max(10)
    private Integer something;

[radcortez]

I want to move forward with this, so I had another look.

I think we should simplify the entry API a bit and just expose a couple of methods in SmallRyeConfig:

• getConfigProperties(Class<T> klass)
• getConfigProperties(Class<T> klass, String prefix)

The Configuration Properties classes do not need to be registered in the builder. They are handled when the method is called. We can optionally support registration if they need to be eagerly loaded.

One detail that we need to discuss is the implementation. I'm not completely sure if we should do this with ASM:

• It is possible we will clash with ASM versions and our consumers. We may require to repack ASM. (speculation).
• The code is harder to maintain and to support multiple JDKs (MR JAR's)
• Other MP impls for similar features (RESTEasy Client) use simple JDK proxies (be consistent across all our impls?).

Or maybe if we want to use ASM, do it through Gizmo?

@kenfinnigan any thoughts as well from an Architecture perspective?

[kenfinnigan]

I would agree that depending on ASM directly could lead to dependency problems.

If we can use Gizmo to generate bytecode that can be used, I'd lean in that direction.

[radcortez]

Gizmo itself uses ASM as a dependency, so we may end up in the same situation, but it should be easier to maintain.

Should we not consider to use plain old JDK proxies?

[kenfinnigan]

Maybe, but Gizmo is a lot easier to work with.

It could use Proxies, but we'd need to replace that in Quarkus anyway, so might be easier just to use it from here

[radcortez]

Yes, I've seen what was done with RESTEasy and the REST Client. They also use the JDK Proxies.

@dmlloyd What do you think? Should we do it with Gizmo?

[dmlloyd]

I want to move forward with this, so I had another look.

I think we should simplify the entry API a bit and just expose a couple of methods in SmallRyeConfig:

• getConfigProperties(Class<T> klass)
• getConfigProperties(Class<T> klass, String prefix)

The Configuration Properties classes do not need to be registered in the builder. They are handled when the method is called. We can optionally support registration if they need to be eagerly loaded.

This makes it impossible to log configuration names which are not recognized, because in order to do so, the configuration space must be iterated with all known configuration items registered. It also makes it impractical to support maps, because to do so the whole configuration has to be iterated, so this would have to be done every time a new configuration object is read.

Default values are registered on the configuration because they are known by way of a config source. Otherwise, it's the config mapper itself which knows the complete set of configuration names, so once all the types are registered, the whole configuration can be iterated one time, the configuration objects can be eagerly created, and any unknown properties in a controlled namespace can be reported.

One detail that we need to discuss is the implementation. I'm not completely sure if we should do this with ASM:

• It is possible we will clash with ASM versions and our consumers. We may require to repack ASM. (speculation).

What kind of dependency problems are you thinking of? ASM is pretty solidly forwards-compatible in my experience. Users normally should use the newest version (normal Maven resolver rules).

TBH I think using ASM is probably much more stable in the long term than using Gizmo, at least for now.

• The code is harder to maintain and to support multiple JDKs (MR JAR's)

What do you mean by this?

• Other MP impls for similar features (RESTEasy Client) use simple JDK proxies (be consistent across all our impls?).

JDK proxies are considerably slower and use substantially more memory (and rely extensively on reflection), which is why I went to ASM.

[radcortez]

This makes it impossible to log configuration names which are not recognized, because in order to do so, the configuration space must be iterated with all known configuration items registered. It also makes it impractical to support maps, because to do so the whole configuration has to be iterated, so this would have to be done every time a new configuration object is read.

Is the idea to expose this feature in a completely new API ConfigMapping? I thought it was done that way, just to not change the main code while we were discussing. I think this should live behind the Config / SmallRyeConfig API. In that case, the registration should be optional, so we don't mandate for everyone to create a Provider.

Default values are registered on the configuration because they are known by way of a config source. Otherwise, it's the config mapper itself which knows the complete set of configuration names, so once all the types are registered, the whole configuration can be iterated one time, the configuration objects can be eagerly created, and any unknown properties in a controlled namespace can be reported.

This is ok. We could register "default" default values. Unless you are doing composition of Configs Properties, which would require pre registration, or a way to pass in the entire tree in the method call, I was looking to have a fairly simple API:

Config config = ConfigProvider.getConfig();
SmallRyeConfig smallRyeConfig = (SmallRyeConfig) config;
Server server = smallRyeConfig.getConfigProperties(Server.class, "server");

class Server {
  String hostName;
  int port;
}

So, the idea is that you should need to create any additional API's. We should be able to support inline simple cases like this in my opinion.

What kind of dependency problems are you thinking of? ASM is pretty solidly forwards-compatible in my experience. Users normally should use the newest version (normal Maven resolver rules).

Hopefully none. Was just putting that into perspective, for us to think about.

TBH I think using ASM is probably much more stable in the long term than using Gizmo, at least for now.

It was mostly to line up with Quarkus and couldn't hurt to have more projects using it and making it stable. Plus a little bit easier to use, so I think we should use it :)

JDK proxies are considerably slower and use substantially more memory (and rely extensively on reflection), which is why I went to ASM.

Yes, I understand. The comment was mostly about consistency. In MP Rest Client / RESTEasy, the client builder is very similar to this and it is implemented with JDK proxies.

[dmlloyd]

This makes it impossible to log configuration names which are not recognized, because in order to do so, the configuration space must be iterated with all known configuration items registered. It also makes it impractical to support maps, because to do so the whole configuration has to be iterated, so this would have to be done every time a new configuration object is read.

Is the idea to expose this feature in a completely new API ConfigMapping? I thought it was done that way, just to not change the main code while we were discussing. I think this should live behind the Config / SmallRyeConfig API. In that case, the registration should be optional, so we don't mandate for everyone to create a Provider.

Yes, the idea for the initial proposal was to make it separate. This serves several purposes:

• It allows a configuration mapping to be established after the configuration is read.
• By doing so, if errors in the mapped configuration are detected, the original config object remains usable.
• If the change is rejected, I can lift it and use it in my project directly.

Default values are registered on the configuration because they are known by way of a config source. Otherwise, it's the config mapper itself which knows the complete set of configuration names, so once all the types are registered, the whole configuration can be iterated one time, the configuration objects can be eagerly created, and any unknown properties in a controlled namespace can be reported.

This is ok. We could register "default" default values. Unless you are doing composition of Configs Properties, which would require pre registration, or a way to pass in the entire tree in the method call, I was looking to have a fairly simple API:

Config config = ConfigProvider.getConfig();
SmallRyeConfig smallRyeConfig = (SmallRyeConfig) config;
Server server = smallRyeConfig.getConfigProperties(Server.class, "server");

class Server {
  String hostName;
  int port;
}

I don't object in principle to merging the APIs, however my point above stands: if the types are registered on the config builder, then building the config could fail due to validation errors, and if so, the config would then not be available for other purposes. By using one or more separate mapping objects, the mapped config can be validated in stages or units without impacting the ability to construct the configuration itself. As a reminder, it is necessary to validate the configuration as you build it if we want to be able to process the configuration in a single pass.

Also, a note: the proposed API uses interfaces, not classes with fields.

TBH I think using ASM is probably much more stable in the long term than using Gizmo, at least for now.

It was mostly to line up with Quarkus and couldn't hurt to have more projects using it and making it stable. Plus a little bit easier to use, so I think we should use it :)

As someone who has done a lot of work with Gizmo and Quarkus, I would say the opposite: it's useful in some cases, but it generally produces larger bytecode, and also would mean introducing a new run time dependency (into Quarkus, where Gizmo is normally compile-time only, as well as everything else which would use SmallRye Config). I do not believe that doing so would substantially simplify this, and I'd rather not have the dependency from my project. If there was something even more minimal than ASM, I'd probably prefer that!

[radcortez]

This makes it impossible to log configuration names which are not recognized, because in order to do so, the configuration space must be iterated with all known configuration items registered. It also makes it impractical to support maps, because to do so the whole configuration has to be iterated, so this would have to be done every time a new configuration object is read.

Is the idea to expose this feature in a completely new API ConfigMapping? I thought it was done that way, just to not change the main code while we were discussing. I think this should live behind the Config / SmallRyeConfig API. In that case, the registration should be optional, so we don't mandate for everyone to create a Provider.

Yes, the idea for the initial proposal was to make it separate. This serves several purposes:

• It allows a configuration mapping to be established after the configuration is read.
• By doing so, if errors in the mapped configuration are detected, the original config object remains usable.
• If the change is rejected, I can lift it and use it in my project directly.

Don't get me wrong. I don't want to reject it, please check below.

Default values are registered on the configuration because they are known by way of a config source. Otherwise, it's the config mapper itself which knows the complete set of configuration names, so once all the types are registered, the whole configuration can be iterated one time, the configuration objects can be eagerly created, and any unknown properties in a controlled namespace can be reported.

This is ok. We could register "default" default values. Unless you are doing composition of Configs Properties, which would require pre registration, or a way to pass in the entire tree in the method call, I was looking to have a fairly simple API:

Config config = ConfigProvider.getConfig();
SmallRyeConfig smallRyeConfig = (SmallRyeConfig) config;
Server server = smallRyeConfig.getConfigProperties(Server.class, "server");

class Server {
  String hostName;
  int port;
}

I don't object in principle to merging the APIs, however my point above stands: if the types are registered on the config builder, then building the config could fail due to validation errors, and if so, the config would then not be available for other purposes. By using one or more separate mapping objects, the mapped config can be validated in stages or units without impacting the ability to construct the configuration itself. As a reminder, it is necessary to validate the configuration as you build it if we want to be able to process the configuration in a single pass.

I think we can come up with a way to achieve both. My point is that it would be confusing for the end user to have multiple APIs to deal with configuration. My proposal was to make registration optional, for cases you want it to be eager, but you may not even need that. For instance a SmallRyeConfig#getMappingProperties could wrap all the boilerplate and returning you the mapping instance. Of course, if you want to use the separate API you could do so, but at least there is a simple entry point as a starter to handle simple cases.

This kind of feature is algo being discussed in MP Config eclipse/microprofile-config#565. An entry point is required to expose the feature and naturally Config is the candidate, so we may need to support it in someway or another.

Also, a note: the proposed API uses interfaces, not classes with fields.

I know, but I was thinking that we should support both, since Quarkus does support class / interface style of Config Properties. Isn't the expectation to replace Quarkus implementation once this is done in SmallRye like we did with the other stuff?

TBH I think using ASM is probably much more stable in the long term than using Gizmo, at least for now.

It was mostly to line up with Quarkus and couldn't hurt to have more projects using it and making it stable. Plus a little bit easier to use, so I think we should use it :)

As someone who has done a lot of work with Gizmo and Quarkus, I would say the opposite: it's useful in some cases, but it generally produces larger bytecode, and also would mean introducing a new run time dependency (into Quarkus, where Gizmo is normally compile-time only, as well as everything else which would use SmallRye Config). I do not believe that doing so would substantially simplify this, and I'd rather not have the dependency from my project. If there was something even more minimal than ASM, I'd probably prefer that!

Ok sure. No worries, let's keep ASM.

[dmlloyd]

Default values are registered on the configuration because they are known by way of a config source. Otherwise, it's the config mapper itself which knows the complete set of configuration names, so once all the types are registered, the whole configuration can be iterated one time, the configuration objects can be eagerly created, and any unknown properties in a controlled namespace can be reported.

This is ok. We could register "default" default values. Unless you are doing composition of Configs Properties, which would require pre registration, or a way to pass in the entire tree in the method call, I was looking to have a fairly simple API:

Config config = ConfigProvider.getConfig();
SmallRyeConfig smallRyeConfig = (SmallRyeConfig) config;
Server server = smallRyeConfig.getConfigProperties(Server.class, "server");

class Server {
  String hostName;
  int port;
}

I don't object in principle to merging the APIs, however my point above stands: if the types are registered on the config builder, then building the config could fail due to validation errors, and if so, the config would then not be available for other purposes. By using one or more separate mapping objects, the mapped config can be validated in stages or units without impacting the ability to construct the configuration itself. As a reminder, it is necessary to validate the configuration as you build it if we want to be able to process the configuration in a single pass.

I think we can come up with a way to achieve both. My point is that it would be confusing for the end user to have multiple APIs to deal with configuration. My proposal was to make registration optional, for cases you want it to be eager, but you may not even need that. For instance a SmallRyeConfig#getMappingProperties could wrap all the boilerplate and returning you the mapping instance. Of course, if you want to use the separate API you could do so, but at least there is a simple entry point as a starter to handle simple cases.

This kind of feature is algo being discussed in MP Config eclipse/microprofile-config#565. An entry point is required to expose the feature and naturally Config is the candidate, so we may need to support it in someway or another.

In my opinion, standardizing the feature is not nearly as important as ensuring that we don't cut off use cases. Allowing the separation of a configuration into units or stages goes against the MP Config philosophy of having a single configuration per "application" (whatever that is), but is necessary for a program where one configuration can control multiple isolated systems independently.

I'm not opposed to adding the API to the core package but I think the mappings have to be created after the configuration is created to avoid the validation trap I mentioned, and I don't see how that can be done cleanly without a separate type.

Also, a note: the proposed API uses interfaces, not classes with fields.

I know, but I was thinking that we should support both, since Quarkus does support class / interface style of Config Properties. Isn't the expectation to replace Quarkus implementation once this is done in SmallRye like we did with the other stuff?

Yes and no. I consider Quarkus' use of fields to be a design error, which adds complexity and problems for little gain. I don't want to complicate this code any further to support both; I'd rather make a "clean cut" if possible, and put the burden of supporting both strategies on the Quarkus side where it can be phased out.

[radcortez]

In my opinion, standardizing the feature is not nearly as important as ensuring that we don't cut off use cases. Allowing the separation of a configuration into units or stages goes against the MP Config philosophy of having a single configuration per "application" (whatever that is), but is necessary for a program where one configuration can control multiple isolated systems independently.

I'm not opposed to adding the API to the core package but I think the mappings have to be created after the configuration is created to avoid the validation trap I mentioned, and I don't see how that can be done cleanly without a separate type.

That is fine, I'm not saying that we should drop the proposed type. In this case, the exposed API in SmallRyeConfig would just wrap ConfigMapping creation and output the instance directly. Would that work for you? I'll give it a try to see how that looks like.

Also, a note: the proposed API uses interfaces, not classes with fields.

I know, but I was thinking that we should support both, since Quarkus does support class / interface style of Config Properties. Isn't the expectation to replace Quarkus implementation once this is done in SmallRye like we did with the other stuff?

Yes and no. I consider Quarkus' use of fields to be a design error, which adds complexity and problems for little gain. I don't want to complicate this code any further to support both; I'd rather make a "clean cut" if possible, and put the burden of supporting both strategies on the Quarkus side where it can be phased out.

Ok, if required we can do a separate implementation anyway.

To conclude, and if you don't mind I'll pull this PR and try to integrate it implementation core. I'll try to keep the PR core as is and just expose it directly in SmallRyeConfig.

[radcortez]

Ah, it also needs the CDI part.

[radcortez]

@dmlloyd How about this?

radcortez@c50ad1a

Just a prototype.

[dmlloyd]

Creating the mapping on demand generally defeats the purpose of this implementation: there is normally going to be more than one config root, and unknown property validation cannot effectively trigger this way. Imagine you have two config root types in the same prefix. If you have a configuration covering both roots, and you load first root this way, all the properties for the second root will be reported as unknown, and when you load the second root, all the properties for the first root will be reported as unknown. In addition, conflicts between two roots are not detectable.

Either all the overlapping mappings need to be known at once, or else reporting of unrecognized properties should be abandoned.

[radcortez]

The idea was just to give a quick easy access to the API for simple cases. The builder itself to cover most complex case can be used directly, like you designed it, or we can just wrap the builder and expose it in SmallRyeConfig so we can cover the complex cases directly there as well.

[radcortez]

Ah I was also wondering about that, because I haven't seen it failed and found out that https://github.com/radcortez/smallrye-config/blob/c50ad1ad91748e6fbe807753382e758aa42303ac/implementation/src/main/java/io/smallrye/config/mapper/MappingContext.java#L198 is not implemented :D

[dmlloyd]

Yeah it's not completely finished yet.

[radcortez]

Do you want to fail if we don't have everything mapped? It might be too harsh. I see it going both ways, you may want to fail, or you may want to have it loosely mapped.

[dmlloyd]

We want to fail if a required property is missing. We want to warn if an unrecognized property name was given. If a looser mapping (no warnings) is desired, then the checking can be disabled for certain prefixes or name patterns.

[radcortez]

Moved to #333 .