Refactor: Add Instant to TimeService interface and use TimeService in UaaTokenStore #2315
Conversation
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/185107803 The labels on this github issue will be updated when the story is started. |
strehle
changed the title
strehle
changed the title
server/src/test/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenStoreTests.java
Show resolved
Hide resolved
There was a problem hiding this comment.
I don't understand the reason why it's better to reuse another interface that is already used by the code.
IMO this is very much the opposite of what dependency injection needs to be: you want to inject what you need, not what is used elsewhere in the code.
Each class or file needs to create their own interfaces specifying what they need, what their dependencies are. Even if 2 classes end up having the exact same dependency at a give point in time, we should NOT reuse the interface, because it creates a binding between the 2 users. If some one class now requires the dependency to be more powerful, that same power will be given to the other class, which doesn't need or want it.
server/src/main/java/org/cloudfoundry/identity/uaa/util/TimeService.java
Show resolved
Hide resolved
server/src/test/java/org/cloudfoundry/identity/uaa/oauth/ApprovalServiceTest.java
Outdated
Show resolved
Hide resolved
server/src/test/java/org/cloudfoundry/identity/uaa/oauth/UaaTokenStoreTests.java
Show resolved
Hide resolved
| private static TimeService givenMockedTime() { | ||
| TimeServiceImpl timeService = mock(TimeServiceImpl.class); | ||
| doReturn(Instant.now()).when(timeService).getCurrentInstant(); | ||
| return timeService; |
There was a problem hiding this comment.
I don't understand the point of this method. It now just returns a normal TimeService that returns the actual current time.
Add TimeService to UaaTokenStore and replace all now usage to timeService object
Allows to change expiration time without changes in DB expiration.
Follow-up from discussion of #2291
Sonar results
https://sonarcloud.io/summary/new_code?id=cloudfoundry-identity-parent&pullRequest=2315