Skip to content
This repository has been archived by the owner on May 16, 2023. It is now read-only.

Use configuration classes instead of @Value annotations #183

Closed
TilmannF opened this issue May 19, 2020 · 5 comments · Fixed by #238
Closed

Use configuration classes instead of @Value annotations #183

TilmannF opened this issue May 19, 2020 · 5 comments · Fixed by #238
Labels
community This item is well suited to be worked on by the community. enhancement New feature or request good first issue Good for newcomers

Comments

@TilmannF
Copy link

You're using @Value about a dozen times, which isn't considered best practice anymore.

To quote Thomas Uhrig (@tuhrig):

What will happen when you use this technique through your application? Well, whenever you need some property, you will inject it. You will inject it in services, controllers and components and you will properly inject the same property in different classes. That’s easy and absolutely what the mechanism is about.

However, this means nothing else than scattering your configuration about your whole application. Every class can pick a couple of properties to use. You will have no idea or control which class is using which properties. You will end-up with doing a full text search on your project to find out where a single key is used. You will have a lot of fun if you want to rename one of those keys or when you need to set each and every property for each and every class in your unit tests.
[...]
If you do so [use a configuration class], you will have a single point of responsibility to load and get your configuration from. As any other service, you can easily change the implementation. Maybe you don’t want to load your properties from a properties file, but from a database or web service!
(source)

While this issue wouldn't qualify as high priority right now, it'll probably help in the future.

@TilmannF TilmannF added the enhancement New feature or request label May 19, 2020
@christian-kirschnick christian-kirschnick added the good first issue Good for newcomers label May 19, 2020
@MALPI
Copy link

MALPI commented May 19, 2020

Hey there,

I'd like to contribute and work on this issue?

BTW: Great that you uses the good first issue label.

@jwedel
Copy link
Contributor

jwedel commented May 19, 2020

@MALPI You can have a look at the spring boot docs regarding the @ConfigurationProperties annotation:

The way to go would be to keep the property keys the same and use a separate class that binds the common key prefix (e.g. services.submission) and provides type safe config values. Then inject that into the class that currently uses @Value.

See here for an example:

@indyaah
Copy link
Contributor

indyaah commented May 21, 2020

@MALPI : Are you still planning to work on this? If you're not planning to I would be happy to take over.

@johanneseschrig johanneseschrig added the community This item is well suited to be worked on by the community. label May 21, 2020
indyaah added a commit to indyaah/cwa-server that referenced this issue May 21, 2020
indyaah added a commit to indyaah/cwa-server that referenced this issue May 21, 2020
@MALPI
Copy link

MALPI commented May 21, 2020 via email

indyaah added a commit to indyaah/cwa-server that referenced this issue May 21, 2020
@pithumke
Copy link
Member

I tried to use @ConfigurationProperties in an upcoming PR but didn't have any luck with it. I either ran into issues regarding default constructor visibility or the configuration property just didn't get autowired. Here's my attempt: 7d752e8. Unfortunately, I don't have the time right now to investigate this further, but I would be interested if somebody could please tell me what I was doing wrong.

indyaah added a commit to indyaah/cwa-server that referenced this issue May 21, 2020
christian-kirschnick added a commit to indyaah/cwa-server that referenced this issue May 22, 2020
pithumke added a commit to indyaah/cwa-server that referenced this issue May 23, 2020
johanneseschrig added a commit to indyaah/cwa-server that referenced this issue May 23, 2020
pithumke added a commit to indyaah/cwa-server that referenced this issue May 23, 2020
pithumke added a commit that referenced this issue May 23, 2020
* #183: Use configuration classes instead of @value annotations
* Externalized signature config
* Integrated object store config into distribution config

Co-authored-by: Anuj Patel <indyaah@users.noreply.github.com>
Co-authored-by: Pit Humke <pit.humke@sap.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community This item is well suited to be worked on by the community. enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants