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

OAuth2TokenProvider should allow a flexible systemTime #691

Closed
xuanswe opened this issue May 27, 2024 · 6 comments · Fixed by #693
Closed

OAuth2TokenProvider should allow a flexible systemTime #691

xuanswe opened this issue May 27, 2024 · 6 comments · Fixed by #693
Labels
enhancement New feature or request

Comments

@xuanswe
Copy link
Contributor

xuanswe commented May 27, 2024

A global fixed systemTime is not enough to write tests in practice at all.

I propose to change the current implementation

class OAuth2TokenProvider(
        private val keyProvider: KeyProvider = KeyProvider(),
        val systemTime: Instant? = null,
    )

to the backward compatible implemenation like below

typealias TimeProvider = () -> Instant?

class OAuth2TokenProvider(
  private val keyProvider: KeyProvider = KeyProvider(),
  private val timeProvider: TimeProvider,
) {
  val systemTime
    get() = timeProvider()

  @JvmOverloads
  constructor(
    keyProvider: KeyProvider = KeyProvider(),
    systemTime: Instant? = null,
  ) : this(keyProvider, { systemTime })
}

I am not sure if we need to adapt anywhere else (ex. OAuth2TokenProviderDeserializer) with this change or not.

@xuanswe
Copy link
Contributor Author

xuanswe commented May 28, 2024

@ybelMekk do you agree so that I can contribute a PR?

@xuanswe
Copy link
Contributor Author

xuanswe commented May 29, 2024

From my understanding, OAuth2TokenProvider is created only in 2 places:

  • OAuth2TokenProviderDeserializer

    Here, the purpose is to convert json string into OAuth2TokenProvider object.
    In this case, we only need static systemTime, so not relevant to the change.

  • OAuth2Config

    Here, it could be done programmatically, both static and dynamic systemTime are possible.

xuanswe added a commit to xuanswe/mock-oauth2-server that referenced this issue May 29, 2024
xuanswe added a commit to xuanswe/mock-oauth2-server that referenced this issue May 29, 2024
xuanswe added a commit to xuanswe/mock-oauth2-server that referenced this issue May 29, 2024
xuanswe added a commit to xuanswe/mock-oauth2-server that referenced this issue May 29, 2024
@xuanswe
Copy link
Contributor Author

xuanswe commented May 30, 2024

@ybelMekk PR created :)

@xuanswe
Copy link
Contributor Author

xuanswe commented Jun 16, 2024

Hi, could you please review the PR?

@ybelMekk ybelMekk added the enhancement New feature or request label Jun 17, 2024
@ybelMekk ybelMekk linked a pull request Jun 17, 2024 that will close this issue
ybelMekk added a commit to xuanswe/mock-oauth2-server that referenced this issue Jun 17, 2024
@ybelMekk
Copy link
Contributor

Could u please run :formatKotlin and then push the changes?

xuanswe added a commit to xuanswe/mock-oauth2-server that referenced this issue Jun 17, 2024
xuanswe added a commit to xuanswe/mock-oauth2-server that referenced this issue Jun 17, 2024
ybelMekk pushed a commit that referenced this issue Jun 17, 2024
* #691 `OAuth2TokenProvider` should allow dynamic `systemTime`

* #691 implement test for timeProvider
@xuanswe
Copy link
Contributor Author

xuanswe commented Jun 17, 2024

@ybelMekk Thanks for merging and releasing! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants