#240 config properties

[Emily-Jiang]

A proposal to group properties that share the same prefix into a single class or interface to introduce "type-safe" property access and also reduce the boiler-plate code to access those properties.

[Emily-Jiang]

@radcortez @OndroMih please comment

[Emily-Jiang]

Please give you +1 if you are happy with the current change. I'll then add TCKs. Otherwise, please provide feedback asap.

[Emily-Jiang]

@radcortez I've added some tcks. Please either leave any feedback or approve. Thanks!

[radcortez]

@Emily-Jiang how about if we also add this to the programatic API? It could work similar as the MP Rest Client Builder. For instance:

ConfigBuilder.register(Class) // Interface registration
Config.getConfigProperties(Class) // Interface retrieval

[Emily-Jiang]

@OndroMih @jbee please review

[jbee]

It would be nice if PRs that introduce a new semantic of feature gives a short summary in the description from users perspective.

[OndroMih]

@jbee, @Emily-Jiang, I've added a short description to this PR as @jbee suggested just to have something there @Emily-Jiang , feel free to change it as you wish.

[Emily-Jiang]

@Emily-Jiang how about if we also add this to the programatic API? It could work similar as the MP Rest Client Builder. For instance:

ConfigBuilder.register(Class) // Interface registration
Config.getConfigProperties(Class) // Interface retrieval

@radcortez I added Config.getConfigProperties(Class). I don't think we need to register the class with configprovider. Configprovider only needs to know config sources and converters. This config properties is pretty much a group of config properties. I don't see it fits in with ConfigProvider unless I don't quite get what you meant.

[radcortez]

The registration is to avoid to scan the entire classpath for classes annotated with @ConfigProperties. For CDI containers, this is usually fine, since scanning is already required for a lot of other stuff and you would just register a new annotation.

For non-CDI environments, I think manually registration would be better, or implementations will be required to scan the classpath (and may not have code to do it).

[Emily-Jiang]

@radcortez, for CDI, it is easy as the @ConfigProperties is steoretype and it easily find the bean classes.

In last week's hangout, @struberg @jbee and I discuss this in details. @struberg suggested not to add the api to getConfigProperties. We can use the existing getValue. As for the non-spi, you don't need to preactively process that class. I don't see the reason to register the class. Besides, the registering is done by developers. We can't guarantee whether it will be called.

[radcortez]

Well, I'm not sure if I agree, because we don't seem to be consistent between the CDI API vs non CDI API. Right now, the user experience is already very different and this will be another thing to add to the list.

Is there any reason why we shouldn't add getConfigProperties? My only reason to add it to keep a consistent API and allow you to achieve the same thing in non CDI environments. If there is a better way to provide that I'm happy to discuss it, but I don't think we should remove it from the programmatic API.

[jbee]

@radcortez The capability for programmatic usage isn't removed by removing the getConfigProperties method and using the getValue method instead. Its just another way of doing it that does it with a more narrow API.

[radcortez]

@radcortez The capability for programmatic usage isn't removed by removing the getConfigProperties method and using the getValue method instead. Its just another way of doing it that does it with a more narrow API.

It it a difference user experience, because you are able to create an interface with all your configs and use it directly with CDI. Without CDI you are unable to do it, and you would need to call .getValue for each config you want to retrieve.

Like @OndroMih suggested, we could provide a similar functionality as MP REST Client. This not only provide consistency between CDI / non CDI, but also consistency across MP Specs.

[jbee]

and you would need to call .getValue for each config you want to retrieve.

This is a misunderstanding. You will not need to do that.

You can do:

config.getValue("base.path", MyConfigPropertiesBean.class)

which is equivalent to injecting MyConfigPropertiesBean using CDI.

[radcortez]

Ok, I see, it was not very clear in the comment.

In that case, we probably want a getValue(ConfigProperties) and a getValue(ConfigProperties, String) since the prefix is optional.

Anyway, I think overloading getValue might be misleading.

[Emily-Jiang]

I'll add more commit to rectify the tcks with the new update. I'll tidy up the PR shortly.

[Emily-Jiang]

We have a few options to choose from, based on the conversation with @radcortez @OndroMih @jbee @struberg . Let me list as follows.

Option 1. Create one new annotation RegisterConfigProperties, use ConfigProperty and getValue for lookup and provide prefix

@RegisterConfigProperties
public class ServerDetails {
    public String host; 
    public int port;   
    private String endpoint; 
    public @ConfigProperty(name="oldServer.location")
    String location;  
    public String getEndpoint() {
        return endpoint;      
    }     
}

Use the following to look up ServerDetails with provided prefix.

@Inject @ConfigProperty("server") ServerDetails serverDetails;
or

Config config = ConfigProvider.getConfig();
ServerDetails serverDetails = config.getVale("server", ServerDetails.class);

Option 2. same as the previous option but the prefix is fixed. Create one new annotation RegisterConfigProperties with prefix, use ConfigProperty and getValue for lookup only

@RegisterConfigProperties(prefix="server")
public class ServerDetails {
    public String host; 
    public int port;   
    private String endpoint; 
    public @ConfigProperty(name="oldServer.location")
    String location;  
    public String getEndpoint() {
        return endpoint;      
    }     
}

Use the following to look up ServerDetails with provided prefix.

@Inject ServerDetails serverDetails;
or

Config config = ConfigProvider.getConfig();
ServerDetails serverDetails = config.getVale("", ServerDetails.class);

Option 3: Create 2 new annotation RegisterConfigProperties, ConfigProperties and add one more method getConfigProperties(prefix, ServerDetails.class) on Config for look up and provide prefix

@RegisterConfigProperties
public class ServerDetails {
    public String host; 
    public int port;   
    private String endpoint; 
    public @ConfigProperty(name="oldServer.location")
    String location;  
    public String getEndpoint() {
        return endpoint;      
    }     
}

Use the following to look up ServerDetails with provided prefix.

@Inject @ConfigProperties("server") ServerDetails serverDetails;
or

Config config = ConfigProvider.getConfig();
ServerDetails serverDetails = config.getConfigPropertiesValue("server", ServerDetails.class);

Option 4: Create 2 new annotation RegisterConfigProperties with supplied prefix and cannot be updated, ConfigProperties and add one more method getConfigProperties(prefix, ServerDetails.class) on Config for look up and provide prefix

@RegisterConfigProperties(prefix="server")
public class ServerDetails {
    public String host; 
    public int port;   
    private String endpoint; 
    public @ConfigProperty(name="oldServer.location")
    String location;  
    public String getEndpoint() {
        return endpoint;      
    }     
}

Use the following to look up ServerDetails.

@Inject ServerDetails serverDetails;
or

Config config = ConfigProvider.getConfig();
ServerDetails serverDetails = config.getConfigPropertiesValue(ServerDetails.class);

Option 2 and 4 are similar to option 1 and 3. Do we think in the lookup logic, we would supply a different suffix? I think it is unlikely. We can then choose either 2 or 4. Once we choose the option, we then decide the annotation names.

[Emily-Jiang]

Vote: Option 1

[Emily-Jiang]

Vote: Option 2

[Emily-Jiang]

Vote: Option 3

[Emily-Jiang]

Vote: Option 4

[radcortez]

For Option 3 and 4, you don't really need an additional annotation to perform injection. Each interface / class is unique, there is only one implementation, so it doesn't need a CDI qualifier.

For Option 1 and 2 (that reuses the current getValue method for a different purpose), can we have an updated javadoc on how it will work?

[OndroMih]

I don't think we need to choose between specifying a prefix only in @RegisterConfigProperties or when injecting. We can specify a default prefix in @RegisterConfigProperties and allow to overwrite it in the injection point as I suggested earlier in #565 (comment):

@RegisterConfigProperties(defaultPrefix = "server")
public class ServerDetails {
    public String host; 
    public int port;   
    private String endpoint; 
    public @ConfigProperty(name="oldServer.location")
    String location;  
    public String getEndpoint() {
        return endpoint;      
    }     
}

Thus I propose a new Option 5 which basically merges Option 1 and 2, a new Option 6 which merges Option 3 and 4, and, finally, a new Option 7, which merges everything altogether to avoid unexpected behavior as follows:

Option 5

With ServerDetails as above, the following looks up ServerDetails with the server prefix (the default one specified in @RegisterConfigProperties).

@Inject @ConfigProperties ServerDetails serverDetails;

or

ServerDetails serverDetails = config.getValue(ServerDetails.class);   
  /* a new method that accepts a single argument. I'm not a fan of forcing users to 
   * specify an empty string just to ignore it and use another default value instead */

The following looks up ServerDetails with the server2 prefix (ignores the default prefix):

@Inject @ConfigProperty( name = "server2") ServerDetails serverDetails;

or

ServerDetails serverDetails = config.getValue("server2", ServerDetails.class);

Option 6

This option would introduce a new @ConfigProperties annotation specific to injecting a group of properties, and an analogous new method in the Config interface. Actually 2 methods: one with 2 arguments (prefix, Class), and another one with a single Class argument to use the default prefix. I don't like passing an empty string to use the default prefix.

@emilyjiang's Options 3 and 4 suggest that this method is named getConfigPropertiesValue but I suggest to name it getValues. This is because the existing getValue method is analogous to the @ConfigPropety qualifier and it makes sense to me that getValues is analogous to the @ConfigProperties qualifier.

Then the following looks up ServerDetails with the server prefix (the default one specified in @RegisterConfigProperties).

@Inject @ConfigProperties ServerDetails serverDetails;

or

// a variant with a single argument
ServerDetails serverDetails = config.getValues(ServerDetails.class);   

The following looks looks up ServerDetails with the server2 prefix (ignores the default prefix):

@Inject @ConfigProperties( prefix = "server2") ServerDetails serverDetails;

or

ServerDetails serverDetails = config.getValues("server2", ServerDetails.class);

Option 7

This option is basically the Option 6 but also supports what's proposed in Option 5 for users that might find it more natural. The reason is to avoid that users get confused when trying to use @ConfigProperty or config.getValue() to inject a group of properties.

Then, retrieving ServerDetails with the server2 prefix would be possible using all of the following:

@Inject @ConfigProperties( prefix = "server2") ServerDetails serverDetails;

@Inject @ConfigProperty( name = "server2") ServerDetails serverDetails;

ServerDetails serverDetails = config.getValues("server2", ServerDetails.class);

ServerDetails serverDetails = config.getValue("server2", ServerDetails.class);

The usage of @ConfigProperties and config.getValues would be preferred as it's semantically closer to what the runtime should do.

[Emily-Jiang]

@radcortez I've added a few more tests to cover the exception conditions. Please check to see whether you have any further comments.

Thank you @Emily-Jiang . I've added a few more comments.

And we should probably revisit the topic class vs interface with @OndroMih feedback.

As mentioned earlier, I started with Interface idea and decided against.
Earlier today, I took a look at DeltaSpike config, I think my issue with JavaBean naming convention can be sorted if we force the method names match with config property name.

@ConfigProperties(prefix="server")
@ApplicationScoped
public interface Details {
    String host(); // the value of the configuration property server.host
    int port();   // the value of the configuration property server.port
    String endpoint(); //the value of the configuration property server.endpoint
    @ConfigProperty(name="old.location")
    String location(); //the value of the configuration property server.old.location 
}

I will do a commit to add this, so that we can support both interface and class. Thoughts?

[jbee]

I will do a commit to add this, so that we can support both interface and class. Thoughts?

I am looking at this independent of what might be done or standard elsewhere. From that perspective I find interfaces a mismatch. Methods suggest the values might change when they will not. To implement it a proxy or dynamic class generation hammer needs to be utilised potentially making dynamic computations for something that never changes. With CDI scope annotation present in your example I wonder if the bean would be a full CDI bean with all consequences meaning the methods could actually return different values due to some interceptor magic which I again would find a mismatch with the nature of data that never changes.

[Emily-Jiang]

I will do a commit to add this, so that we can support both interface and class. Thoughts?

I am looking at this independent of what might be done or standard elsewhere. From that perspective I find interfaces a mismatch. Methods suggest the values might change when they will not. To implement it a proxy or dynamic class generation hammer needs to be utilised potentially making dynamic computations for something that never changes. With CDI scope annotation present in your example I wonder if the bean would be a full CDI bean with all consequences meaning the methods could actually return different values due to some interceptor magic which I again would find a mismatch with the nature of data that never changes.

@jbee I don't think interface suggests the values might change though as there is no way to set the property in the interface. As for implementation, I think it is pretty much similar to Class implementation. You just need to create instances for it, similar to Rest Client. Having this said, since we might not easily reach consensus on the interface flavour, I would like solve all issues on the class flavour first and then start working on interface so that further comments can be added and discussed more thoroughly. By the way, @OndroMih is pretty much keen for interface support. @OndroMih can you please comment?

As for the CDI Bean, when the instance injection happens, the instance is pretty much baked. Invoking the method on the bean should give you the same value. If you inject a Provider<>, you will get a proxy.

[jbee]

@Emily-Jiang I try to make my point more clear: Interfaces don't suggest anything but methods do and interfaces must use methods. Its not an opinion but a fact of the Java language that methods exist to give access to a value that is bound to change. Therefore using interfaces suggests the information isn't information but behaviour which means it might change on you. If the information is indeed information there are features in the language to express that and I only suggest to use them.

[Emily-Jiang]

@Emily-Jiang I try to make my point more clear: Interfaces don't suggest anything but methods do and interfaces must use methods. Its not an opinion but a fact of the Java language that methods exist to give access to a value that is bound to change. Therefore using interfaces suggests the information isn't information but behaviour which means it might change on you. If the information is indeed information there are features in the language to express that and I only suggest to use them.

I am ok to start with class first. We can always consider interface later if enough people ask for it. One might be enough as they are similar. Spring Config chooses class while Deltaspike config choose interface flavour.

[OndroMih]

Hi @Emily-Jiang and all, the PR now looks good to me. I agree that using interfaces is a bit more cumbersome than classes and doesn't add much more value in this case. It's still true that the API proposed right now differs from the REST client API but I understand they serve a different purpose and there's no need to match the APIs exactly. I agree that we can add support for interfaces later but their value is mostly in providing support for values that change dynamically, in the same way as @Inject Instance supports now already. After reviewing all the conversation now I think that the additional complexity isn't worth it and we should just support classes.

I only have one comment about the requirement for zero-arg constructors. As I commented at the code level, I disagree with enforcing that the implemenation must raise an exception in case of zero-arg constructors. The spec should instead encourage using zero-arg constructors but not forbid them. Implementations may be able to support classes without zero-arg constructors and even build custom features around them, there's really no reason to limit the implementations by forcing to throw an exception.

[jbee]

This class should contain a zero-arg constructor. Otherwise, non-portable behaviour occurs.

I think this is a good compromise to communicate to the user what is expected of the class used to be portable. If a vendor supports other scenarios these are clearly communicated as non-portable.

[Emily-Jiang]

@jbee @OndroMih Thanks for your approval! Please have a double check to see whether the final one is good. @radcortez please check whether all of your comments are address. If yes, please approve this PR.

[Emily-Jiang]

@radcortez I think I have addressed all of your comments! Can you double check and approve as I would like to merge this PR today based on our last week's conversation?

[Emily-Jiang]

@jbee @radcortez further to our meeting conversation, I have added AnnotationLiteral support. The PR is complete. Please review and approve if you have no further comments! Thanks

[jbee]

Just added one comment for consideration.

[Emily-Jiang]

@radcortez can you do a final review as I think all of your requests were addressed? As mentioned in our hangout last week, I would like to merge this last week. It will be good if we can merge it today.

[radcortez]

Considering the last sentence of the javadoc:

If the prefix is null, this method is equivalent to {@linkplain #getConfigProperties(Class)}.

Shouldn't <T> T getConfigProperties(Class<T> configProperties); be implemented as a default method that calls getConfigProperties(Class<T> configProperties, null)?

[Emily-Jiang]

yes

[radcortez]

I think we need to define a few more mapping rules. For instance, a dash -, is a valid char to be part of the name of a configuration property. How do you map them to method names? Or should we consider that there is no convention to map these and you always have to rely on the annotation to set the name?

[jbee]

I am confused, isn't the mapping the other way around? The field name maps to a property, and as you can use all valid field names as property names this is fine. Or if you annotate the field and provide a string for name the name can be any string which should make any valid property name work.

[Emily-Jiang]

It is tricky for the mapping rules as when we might have to look up client.name, client_name, client$name based on some mapping rules. Which one to match first? It will be very messy. Since the config property always rely on names to specify property names, we use the same approach. I suddently thought of something in this area. Should we force prefix to contain all characters? At the moment, we added dot between prefix and the rest part. I think it is better not to add dot as there might be other separator such as _ used in the config properties such as client_name, client_age.
Thoughts? @radcortez @jbee If you agree, I will update the last bit.

[jbee]

I am happy either way. Implicit dot is more constrained but maybe a bit more intuitive, explicit is more flexible but also a bit less intuitive to me.

[Emily-Jiang]

I think it is nice to make prefix as the prefix without adding arbitratry separator. I have done the changes accordingly. Please double check again as I aim to merge this PR asap.

[radcortez]

Not sure if I agree with this. It may cause additional issues if we want to support nested / subnamespaces.

Still, this doesn't address the original comment. For instance if I have the following property:

foo-bar=baz

There is no way I can express that in a method name, unless I use the annotation to set the name. For instance Spring uses these rules for binding: https://docs.spring.io/spring-boot/docs/current/reference/html/spring-boot-features.html#boot-features-external-config-relaxed-binding

Meaning that a method with the name fooBar could bind to the property fooBar or foo-bar, or foo_bar or FOOBAR.

[Emily-Jiang]

I think for the prefix it allows for all characters. We don't need to transform it and keep it as exact. I prefer to use the exact prefix without adding the additional dot as the prefix might be ending with $ or - or _.

I was thinking not to transform the field but using `ConfigProperty(name="") to specify the exact name. As for environment variable mapping, we have defined the rules already, which is to transforming all illegal characters to _ and then try to search all uppercase.

Let's explore a bit more on the support of mapping. @radcortez I think one of the possible tranformation is to translate . or - character to _

foo-bar=baz
matches to the field foo_bar.

However, when we search for config properties, we need to state foo_bar can match foo_bar, foo-bar, foo.bar in this order. It is a kind of messy. I am not sure. Using ConfigProperty might be much cleaner. I think for the first version, we just support the exact match and then adding the relaxation rule based on feedback.

[radcortez]

It is fine by me, I just wanted to add the clarification saying that we only support exact match or @ConfigProperty name, so people don't expect any kind of mapping rules.

[Emily-Jiang]

ok thanks @radcortez ! I'll merge my PR now.

[radcortez]

@Emily-Jiang added some review comments to fix some typos.

I did approve it the PR, but I think we need to clarify this: #565 (comment)

/cc @jbee

[Emily-Jiang]

@jbee @radcortez I have done the final tidy up with prefix. Please cast your eyes over for the final time and then we are good to go. I appreciate your early repsonse. Thank you!