#183: Use configuration classes instead of @value annotations
#238
Conversation
…tations - Introduced configuration classes for abstracting away config.
indyaah
requested review from
christian-kirschnick,
johanneseschrig,
michael-burwig,
MKusber,
ole-lilienthal and
pithumke
as code owners
| this.diagnosisKeyService = diagnosisKeyService; | ||
| this.retentionDays = distributionServiceConfig.getRetentionDays(); |
There was a problem hiding this comment.
Wasnt sure if this would be an acceptable pattern but started with it thinking this would be least intrusive change to service/controller layer.
Would love to see/explore better design.
(Applies to most changes)
| } | ||
|
|
||
| public Integer getMaxNumberOfKeys() { | ||
| return payload.getMaxNumberOfKeys(); |
There was a problem hiding this comment.
One more design decision that needs critical review/thought.
Idea was to abstract away complexities (additional classes/getters) in configuration structure and only expose single contract outside.
Applies to DistributionServiceConfig as well.
There was a problem hiding this comment.
When abstracting the payload away, you will lose the information that getMaxNumberOfKeys refers to the payload. Then you would need to rename this to getMaxNumberOfPayloadKeys. Not sure if this is worth it. But if there is some redundant more complex operations done on the properties across the code, then this could be moved to this class.
Maybe @ConstructorBinding should be used to prevent in incomplete object that could result in NPEs (e.g. in `return payload.getMaxNumberOfKeys();).
Also, @ConfigurationProperties can be annotated with @Validated which will actually crash the app at start-up time with a proper readable error message in case non-optional properties are missing:
|
Hi @indyaah, |
|
@pithumke : Definitely, will incorporate 240 changes once it's merged. Thanks for the references. |
…ated in test context. - Fixed checkstyle violations
This reverts commit 4359bc0.
|
|
||
| @Value("${services.submission.payload.max-number-of-keys}") | ||
| private Integer maxNumberOfKeys; | ||
| @Autowired |
There was a problem hiding this comment.
As mentioned before, @Autowired is not required at constructors when there is only one constructor.
stevesap
added
the
community
| // N = fakeDelayMovingAverageSamples. | ||
| private Double initialFakeDelayMilliseconds; | ||
| private Double fakeDelayMovingAverageSamples; | ||
| private Integer retentionDays; |
There was a problem hiding this comment.
I would suggest to use java.time.Duration instead of int. Spring Boot should convert the input automatically. Using Duration in DiagnosisKey would make the implementation easier. Something like
return Duration.between(Instant.ofEpochMilli(rollingStartNumber), Instant.now()) .compareTo(durationToRetain) <= 0;
There was a problem hiding this comment.
Hey @elch78,
thanks for the feedback! Yes I guess you are right and we should use domain specific types where ever possible. However, please understand that the purpose of this PR is to "only" remove the @Value annotations and not to much more refactoring above that. Since this PR is already clocking in at almost 1000 LOC delta, I kindly ask you to create an issue for this so we can pick it up in a separate (more manageable) PR. Thanks! :)
|
Closes #207 |
|
Big thank you to all community contributors, you rock! 🚀 ⭐ ❤️ |
No description provided.