Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable isolation of config properties into a separate POJO #240

Closed
vsmid opened this issue Sep 6, 2017 · 23 comments · Fixed by #565
Closed

Enable isolation of config properties into a separate POJO #240

vsmid opened this issue Sep 6, 2017 · 23 comments · Fixed by #565
Assignees
Labels
area: CDI Issue related to the CDI area impl. proposal ⚙️ An issue or PR which proposes an implementation of use case(s) (link use case(s) in comments) use case 💡 An issue which illustrates a desired use case for the specification
Milestone

Comments

@vsmid
Copy link
Contributor

vsmid commented Sep 6, 2017

Hi,

i think it would be a nice feature to add another annotation e.g. @ConfigProperties which would allow injecting a group of properties with the same prefix into a separate POJO. This would reduce the amount of @Inject and @ConfigProperty annotations and would allow better usage of the same set of properties across multiple classes.

microprofile-config.properties:

person.data.firstName=John
person.data.lastName=Smith

Example class:

@ConfigProperties(prefix = "person.data")
public class PersonData {
  private String firstName;
  private String lastName;
  ...
}

Referencing PersonData:

public class Person {
  @Inject
  private PersonData personData;
}
@OndroMih
Copy link
Contributor

OndroMih commented Sep 9, 2017

This isn't a new idea but it's good that you raised an issue for it because it's not being tracked right now. I agree something like this would be convenient. In fact, I already proposed it with a very old PR #42 - the @ConfigRegistry is essentially the same thing as @ConfigProperties in the proposal here.

There was also a discussion on the google group somewhere. I think the agreement was that we skipped it for config version 1.0 and reevaluate later.

@struberg
Copy link
Contributor

Hi @vsmid, welcome to the MicroProfile party!
Yes, this has indeed already been discussed.
Is up for inclusion in future versions.

@vsmid
Copy link
Contributor Author

vsmid commented Sep 10, 2017

Thanks :-) Yeah, i did not see it, will look better next time.

@OndroMih
Copy link
Contributor

This makes even more sense if the suggested REST type-safe client is finalized: eclipse/microprofile-rest-client#12

Adding a type-safe access to config values proposed in this issue would be very similar to the REST type-safe client and would make MicroProfile API easier to use and compact.

@Emily-Jiang
Copy link
Member

Sounds sensible. We also discussed this prefix for staging, e.g. test. or live. etc. This might be related but not the same. We can discuss on this week's hangout.

@derekm
Copy link

derekm commented Oct 13, 2017

Typesafe Config has an interesting approach to this, although annotations as suggested by @vsmid & @OndrejM would make it nicer: https://github.com/typesafehub/config#configbeanfactory

@rmannibucau
Copy link

+1 to have it asap. Only difference i see is to use interfaces instead of pojos since it should stay anemic.

DeltaSpike has it and all apps I developed since a few years got a configuration consistency enhancement from it.

Not sure it does worth waiting since technically it is easy and the gain immediate.

@OndroMih
Copy link
Contributor

@rmannibucau What do you mean that with interfaces it would by anemic?

Interfaces make sense to me but an object would do the same job.

In fact, with objects this functionality can be achieved already, except the common prefix:

@RequestScoped
public class PersonData {
  @Inject @ConfigProperty(name = "person.data.firstName")
  public String firstName;
  @Inject @ConfigProperty(name = "person.data.lastName")
  public String lastName;
  ...
}

So this functionality is just a sugar that saves some typing.

In fact, we already specify that the name of the property is generated from the class name and the field name. If we specify that if @ConfigProperties defines a prefix, the prefix would be used instead of class name, then it would work like this:

@RequestScoped
@ConfigProperties(prefix = "person.data")
public class PersonData {
  @Inject @ConfigProperty
  public String firstName;
  @Inject @ConfigProperty
  public String lastName;
  ...
}

As a second step, we can make it work also with an interface:

@RequestScoped
@ConfigProperties(prefix = "person.data")
public interface PersonData {
  String firstName();
  String lastName();
  ...
}

But it's much more complicated to specify

  • a proxy has to be generated with a the scope defined on the interface (or just with @Dependent scope to keep things simpler)
  • what about getters? Should they be converted or not, e.g. getFirstName() would result in "firstName" or "getFirstName" as the property name suffix?

I think the approach with just adding new annotation @ConfigProperties is much simpler and can be done quickly, while the approach with interfaces is more complex and requires a longer discussion

@rmannibucau
Copy link

Issue with a pojo is you cant do much and actually just stay on the existing features whereas a proxy enables to:

  1. Support scopes (which is key here otherwise inconsistencies are trivial to create).
  2. Supports additional features (interceptors, caching, reloading etc...)

Getters are then not needed at all and you just used methods, trivial model and efficient :)

@aguibert
Copy link

aguibert commented Jul 9, 2018

Big +1 to this proposal. In my experience with MP-Config it is very helpful to have all of the configurable settings in one central location so I don't need to grep my application for @ConfigProperty when I'm trying to figure out what properties I need to set in order to deploy my application.

When I used MP-Config in one of my applications, once I got up to >5 config properties I naturally consolidated them all into a single class like this:
https://github.com/liberty-bikes/liberty-bikes/blob/master/auth-service/src/main/java/org/libertybikes/auth/service/ConfigBean.java

@thatsIch
Copy link
Contributor

I think the issue #334 created by @hwellmann deserves some attention.

@ApplicationScoped
@Configuration(prefix = "com.example.myapp.client.")
public interface MyConfig {

    @ConfigProperty(name = "url")
    String url();

    @ConfigProperty(name = "timeout", defaultValue = "30000")
    long timeout();
}

As a first step, a simpler approach might be possible

public interface MyConfig {
    
    @ConfigProperty(name = "com.example.myapp.client.url")
    String url();
 
    @ConfigProperty(name = "com.example.myapp.client.timeout", defaultValue = "30000")
    long timeout();
}

This could enable us to be more flexible in the future. For example, you could define the paths as in JAX-RS

@Path("root")
public class Resource {
    @Path("sub")
    ...
}

Not sure if

@ConfigProperty(name = "com.example.myapp.client")
public interface MyConfig {
    
    @ConfigProperty(name = "url")
    String url();
 
    @ConfigProperty(name = "timeout", defaultValue = "30000")
    long timeout();
}

is semantically correct, but being consistent in the API is a major factor to consider.

@rmannibucau
Copy link

+1, this is what we implemented in geronimo.
Only side note is the cdi scope can be hardcoded cause this feature only makes sense if sources are lockable/or alike, not the bean being injected.
For sources being lockable like there are 2 main options:

  1. Add an optional lock notion in the api
  2. Request the impl to wrap the source and refresh it each N ms caching the values in between. Here a refresh method is needed instead of a lock.

@kenfinnigan
Copy link
Contributor

@radcortez would be worth doing a comparison of what's proposed here vs what was already done in Quarkus

@radcortez
Copy link
Contributor

It is similar. It is actually very well documented in Quarkus:
https://quarkus.io/guides/config#using-configproperties

@rmannibucau
Copy link

AFAIK quarkus didnt solve the isolation/bulk.

Side note: geronimo config didnt merge the bean config mapper (as opposed to quarkus) to only keep interface support cause bean/class support is built in with cdi already. Surely something to consider, so just a global defaults+support is needed.

@Emily-Jiang
Copy link
Member

Let's get this done in MP 2.0.

@Emily-Jiang Emily-Jiang added this to the MP Config 2.0 milestone Jan 30, 2020
@vsmid
Copy link
Contributor Author

vsmid commented Jan 30, 2020

The thing with interfaces is interesting. Class approach has some nice advantages, e.g. natural integration with serialization libraries. My opinion is that this feature should enable property value injection to any instance marked as viable by adding annotation with group prefix or something like that. This is currently possible but it produces unnecessary large amount of annotations. Something like this should be possible:

user.name=John
user.address.street=Peckham Street
@ConfigProps(prefix="user")
class UserData {
  String name;
  Address address;
  ...
}

class Address {
  String street;
  ...
}

@rmannibucau
Copy link

The group annotation - note that it can be configproperty today and default value be a global json - is not a marker for this feature IMHO. The default prefix can be the qualified name of the type.
Now the class support does not bring anything in practise I think:

  1. You mention a better support of mapper so it means you will not use mpconfig so no issue
  2. Class are already config friendly as soon as you get the injection target (can be a not managed bean) or it is a cdi bean
  3. Classes can have logic and derive values from the config, it means a mutation of config can not impact the class as expected. Interfaces enable to reload all chnaged properties of the type at once and still provide a consistent view.

Hope it makes sense

@dmlloyd
Copy link
Contributor

dmlloyd commented Jan 31, 2020

Serialization of these config groupings is a bit questionable. Are you serializing the configuration as it was on the source process, or are you serializing a handle to a grouping of configuration properties that reflects the current process configuration? What is the use case for either possible approach?

@dmlloyd dmlloyd added area: CDI Issue related to the CDI area impl. proposal ⚙️ An issue or PR which proposes an implementation of use case(s) (link use case(s) in comments) use case 💡 An issue which illustrates a desired use case for the specification and removed mpconfig-later labels Mar 4, 2020
@Emily-Jiang Emily-Jiang self-assigned this May 18, 2020
Emily-Jiang added a commit to Emily-Jiang/microprofile-config that referenced this issue May 19, 2020
Signed-off-by: Emily Jiang <[email protected]>
Emily-Jiang added a commit to Emily-Jiang/microprofile-config that referenced this issue May 26, 2020
Emily-Jiang added a commit to Emily-Jiang/microprofile-config that referenced this issue May 26, 2020
Emily-Jiang added a commit to Emily-Jiang/microprofile-config that referenced this issue May 26, 2020
Emily-Jiang added a commit to Emily-Jiang/microprofile-config that referenced this issue May 26, 2020
Emily-Jiang added a commit to Emily-Jiang/microprofile-config that referenced this issue May 26, 2020
Emily-Jiang added a commit to Emily-Jiang/microprofile-config that referenced this issue Jun 3, 2020
Emily-Jiang added a commit to Emily-Jiang/microprofile-config that referenced this issue Jun 16, 2020
Emily-Jiang added a commit that referenced this issue Jul 16, 2020
* 240 config properties

Signed-off-by: Emily Jiang <[email protected]>
@aguibert
Copy link

hi @Emily-Jiang, was this issue closed because it was implemented somewhere? Its not clear what the resolution is

@rmannibucau
Copy link

hi @Emily-Jiang , seems the solution is partial and only strictly equivalent to what is in 1.0 (add @Inject on all injections and it is 1-1 so this does not bring any new feature, just makes the API more verbose/complex):

  1. misses the mapping of list of object and map of objects (can be a limitation it must be json friendly, so key being a string then value reusing object mapping and that list uses indexed based impl list.0=a, list.1=b and map uses same notation with key/value pairs: map.0.key=x, map.0.value=b etc...)
  2. misses the locking associated to this object instantiation (how we ensure we read from sources a consistent set of properties per object)

@Emily-Jiang
Copy link
Member

@rmannibucau this issue is a kind of shortcut to 1.0 without bringing any new features. However, for the 2nd point, it might cope better as the endpoints will be resolved at the same time, so a best effort consistent set might be achieved. As for your first point, please go ahead to raise a separate issue, we can expand the ConfigProperties to do the mapping.

@Emily-Jiang
Copy link
Member

Need to reopen this for further update based on some feedback and discussion on the google group mailinglist.

@Emily-Jiang Emily-Jiang reopened this Sep 7, 2020
@radcortez radcortez linked a pull request Sep 21, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: CDI Issue related to the CDI area impl. proposal ⚙️ An issue or PR which proposes an implementation of use case(s) (link use case(s) in comments) use case 💡 An issue which illustrates a desired use case for the specification
Projects
None yet
Development

Successfully merging a pull request may close this issue.