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

Use TimeProvider #2069

Closed
1 task done
trejjam opened this issue May 9, 2024 · 3 comments · Fixed by #2071
Closed
1 task done

Use TimeProvider #2069

trejjam opened this issue May 9, 2024 · 3 comments · Fixed by #2071
Assignees
Milestone

Comments

@trejjam
Copy link
Contributor

trejjam commented May 9, 2024

Confirm you've already contributed to this project or that you sponsor it

  • I confirm I'm a sponsor or a contributor

Describe the solution you'd like

Hello, I want to replace DateTimeOffset.UtcNow with TimeProvider.GetUtcNow(). TimeProvider will be passed from the service container.

Why?
I am running integration tests in which I use TimeProvider to control what Now means. Unfortunately, Openiddict does not use it, and it fails this attempt.

Does it sound like something you would like?


See https://www.nuget.org/packages/Microsoft.Bcl.TimeProvider/8.0.1#readme-body-tab

Additional context

No response

@trejjam
Copy link
Contributor Author

trejjam commented May 9, 2024

I am willing to prepare PR. I just need green light from you :)

@kevinchalet
Copy link
Member

kevinchalet commented May 9, 2024

Hey @trejjam,

Thanks for your offer! It's something I considered too, so 👍🏻

A few notes, in no particular order:

  • TimeProvider can technically be used on .NET Standard 2.0/.NET Framework 4.6.2+ thanks to the package you mentioned, but since the Microsoft.Extensions/Microsoft.Bcl packages are tied to the .NET release matching their package version (i.e they have the same support policy), we'll want to use TimeProvider only on .NET 8.0+ and keep using DateTime.UtcNow/DateTimeOffset.UtcNow for the other TFMs. For that, we'll need to add a SUPPORTS_TIME_PROVIDER compilation constant in Directory.Build.targets. You can take a look at what's been done for SUPPORTS_HTTP_CLIENT_RESILIENCE for some inspiration 😃

  • Retrieving TimeProvider directly from DI would work, but I'd prefer if we attached it to OpenIddict*Options and had an OpenIddict*Configuration class implementing IPostConfigureOptions<OpenIddict*Options> to resolve a default, DI-provided instance when no provider has been explicitly added (it's the pattern used in ASP.NET Core for the authentication handlers: https://github.com/dotnet/aspnetcore/blob/e8eeea60605f323014ab86ea34c2bd87ca8e84d7/src/Security/Authentication/Core/src/AuthenticationBuilder.cs#L141). DateTimeOffset.UtcNow is used in multiple projects, so we'll need to update a few options classes 😄

  • DateTime.UtcNow is used in the EF 6, EF Core and MongoDB stores, but since it's used for a LINQ query that is translated to SQL, we'll want to keep using it to ensure it's correctly mapped to GETUTCDATE().

@trejjam
Copy link
Contributor Author

trejjam commented May 10, 2024

I bumped an idea for a structure: https://github.com/openiddict/openiddict-core/pull/2071/files
Can you take a look?

@kevinchalet kevinchalet added this to the 5.6.0 milestone May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants