GraphQL + OpenID Error after update to newest version

[MikeKry]

Hi, after updating OC, there is a failure when generating token by /connect/token endpoint.

System.InvalidCastException: Failed to convert parameter value from a DateTimeOffset to a DateTime.
---> System.InvalidCastException: Object must implement IConvertible.
at System.Convert.ChangeType(Object value, Type conversionType, IFormatProvider provider)
at Microsoft.Data.SqlClient.SqlParameter.CoerceValue(Object value, MetaType destinationType, Boolean& coercedToDataFeed, Boolean& typeChanged, Boolean allowStreaming)
--- End of inner exception stack trace ---
at Microsoft.Data.SqlClient.SqlCommand.<>c.b__188_0(Task1 result) at System.Threading.Tasks.ContinuationResultTaskFromResultTask2.InnerInvoke()
at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location where exception was thrown ---
at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)
--- End of stack trace from previous location where exception was thrown ---
at Dapper.SqlMapper.ExecuteWrappedReaderImplAsync(IDbConnection cnn, CommandDefinition command, CommandBehavior commandBehavior) in /_/Dapper/SqlMapper.Async.cs:line 1135
at YesSql.Commands.BatchCommand.ExecuteAsync(DbConnection connection, DbTransaction transaction, ISqlDialect dialect, ILogger logger)
at YesSql.Session.FlushAsync()
at YesSql.Session.CommitAsync()
at OrchardCore.OpenId.YesSql.Stores.OpenIdTokenStore1.CreateAsync(TToken token, CancellationToken cancellationToken) at OpenIddict.Core.OpenIddictTokenManager1.CreateAsync(TToken token, CancellationToken cancellationToken)
at OpenIddict.Core.OpenIddictTokenManager1.CreateAsync(OpenIddictTokenDescriptor descriptor, CancellationToken cancellationToken) at OpenIddict.Core.OpenIddictTokenManager1.OpenIddict.Abstractions.IOpenIddictTokenManager.CreateAsync(OpenIddictTokenDescriptor descriptor, CancellationToken cancellationToken)
at OpenIddict.Server.OpenIddictServerHandlers.CreateAccessTokenEntry.HandleAsync(ProcessSignInContext context)
at OpenIddict.Server.OpenIddictServerDispatcher.DispatchAsync[TContext](TContext context)
at OpenIddict.Server.OpenIddictServerDispatcher.DispatchAsync[TContext](TContext context)
at OpenIddict.Server.AspNetCore.OpenIddictServerAspNetCoreHandler.SignInAsync(ClaimsPrincipal user, AuthenticationProperties properties)
at Microsoft.AspNetCore.Authentication.AuthenticationService.SignInAsync(HttpContext context, String scheme, ClaimsPrincipal principal, AuthenticationProperties properties)
at Microsoft.AspNetCore.Mvc.SignInResult.ExecuteResultAsync(ActionContext context)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|29_0[TFilter,TFilterAsync](ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResultExecutedContextSealed context)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.ResultNext[TFilter,TFilterAsync](State& next, Scope& scope, Object& state, Boolean& isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|27_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|24_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|19_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
at Microsoft.AspNetCore.Routing.EndpointMiddleware.g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
at SixLabors.ImageSharp.Web.Middleware.ImageSharpMiddleware.Invoke(HttpContext context)
at OrchardCore.Apis.GraphQL.GraphQLMiddleware.Invoke(HttpContext context, IAuthorizationService authorizationService, IAuthenticationService authenticationService, ISchemaFactory schemaService)
at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
at Microsoft.AspNetCore.Localization.RequestLocalizationMiddleware.Invoke(HttpContext context)
at OrchardCore.Diagnostics.DiagnosticsStartupFilter.<>c__DisplayClass3_0.<b__1>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
at Microsoft.AspNetCore.Diagnostics.StatusCodePagesMiddleware.Invoke(HttpContext context)
at OrchardCore.ContentPreview.PreviewStartupFilter.<>c.<b__1_1>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
at OrchardCore.Modules.ModularTenantRouterMiddleware.Invoke(HttpContext httpContext)
at OrchardCore.Environment.Shell.Scope.ShellScope.UsingAsync(Func`2 execute, Boolean activateShell)
at OrchardCore.Modules.ModularTenantContainerMiddleware.Invoke(HttpContext httpContext)
at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

[deanmarcussen]

Yes it's already been reported on YesSql sebastienros/yessql#325

[Skrypt]

DatetimeOffset is not supported with all DB providers. I think we save the values as string.

[sfmskywalker]

Actually, it was working fine before. I've been using DateTimeOffset for months with YesSql. It broke when updating to latest versions. It looks like it's broken with the re-implementation of the batching thing.

[Skrypt]

Yes maybe because DateTimeOffset was converted to a string before. I don't know I'd need to check in details.

[Skrypt]

Actually it makes no sense to convert a DatetimeOffset to a Datetime because it will miss the zone part. Unless it is saved as UTC but even then if the intent was to save a specific zone/offset on the DatetimeOffset to remember it, you will lose it.

It's usefull when you want to create a calendar event for example that will be occuring in a specific TimeZone and that you don't want to change it's display by TimeZone because it's happening in that TimeZone only.

[sfmskywalker]

Hence the exception? Somewhere the mapping to the Parameters of the Batching Command thinks that DateTimeOffset are to be mapped to DbType.DateTime rather than DbType.DateTimeOffset.

[Skrypt]

Yeah, but SQLite for example doesn't have a DatetimeOffset db type. So, it needs to be converted to a string to be consistent between DB providers.

[Skrypt]

Maybe this should be moved to use a db colum for the offset to keep the Datetime part saved as UTC.
DatetimeOffset db type is a SQL Server only thing.

[sfmskywalker]

Converting to a string makes sense for Sqlite. Sounds easier than having to deal with an extra column.

[kevinchalet]

FWIW, we also had to move back to DateTime in the OpenIddict EF 6/EF Core stores to work around issues in the PostgreSQL/Oracle MySQL providers (e.g openiddict/openiddict-core#1097 or openiddict/openiddict-core#1115). It was not a big deal as we only needed UTC dates support anyway, but it was definitely a breaking change.

In this case, we could do a similar thing in the YesSql stores as long as we use DateTime.SpecifyKind(value, DateTimeKind.Utc) to ensure there's no round-tripping issue (e.g https://github.com/openiddict/openiddict-core/blob/dev/src/OpenIddict.EntityFrameworkCore/Stores/OpenIddictEntityFrameworkCoreTokenStore.cs#L449), but I'm not sure how well OC migrations will handle this change.

[MikeKry]

I have found another issue, possibly related to yessql?

Error> The connection does not support MultipleActiveResultSets.

GraphQL.ExecutionError: Error trying to resolve contentItems. ---> System.InvalidOperationException: The connection does not support MultipleActiveResultSets. at Microsoft.Data.SqlClient.SqlInternalConnectionTds.ValidateConnectionForExecute(SqlCommand command) at Microsoft.Data.SqlClient.SqlInternalTransaction.Rollback() at Microsoft.Data.SqlClient.SqlTransaction.Dispose(Boolean disposing) at YesSql.Session.ReleaseTransaction() at YesSql.Services.DefaultQuery.Query`1.ListImpl() at OrchardCore.ContentManagement.GraphQL.DataLoaderExtensions.LoadPublishedContentItems(IEnumerable`1 contentItemIds, ISession session) at GraphQL.DataLoader.BatchDataLoader`2.FetchAsync(CancellationToken cancellationToken) at GraphQL.DataLoader.DataLoaderBase`1.DispatchAsync(CancellationToken cancellationToken) at GraphQL.DataLoader.BatchDataLoader`2.LoadAsync(TKey key) at GraphQL.DataLoader.BatchDataLoader`2.LoadAsync(TKey key) at GraphQL.DataLoader.BatchDataLoader`2.LoadAsync(TKey key) at GraphQL.Execution.ExecutionStrategy.ExecuteNodeAsync(ExecutionContext context, ExecutionNode node) at GraphQL.Execution.ExecutionStrategy.ExecuteNodeAsync(ExecutionContext context, ExecutionNode node) at GraphQL.Execution.ExecutionStrategy.ExecuteNodeAsync(ExecutionContext context, ExecutionNode node) at GraphQL.Execution.ExecutionStrategy.ExecuteNodeAsync(ExecutionContext context, ExecutionNode node) at GraphQL.Execution.ExecutionStrategy.ExecuteNodeAsync(ExecutionContext context, ExecutionNode node) at GraphQL.DataLoader.BatchDataLoader`2.LoadAsync(TKey key) at GraphQL.Execution.ExecutionStrategy.ExecuteNodeAsync(ExecutionContext context, ExecutionNode node)

Not sure, if this is related or if it is just new feature, because it works with modified connection string

[jtkech]

@MikeKry

About your MARS issue, most of the time is when there are some non awaited async call, or when doing explicitly some parallel execution => e.g. parallel yesSql queries on the scoped session, which is not good.

So i suspect that GraphQL.DataLoader.BatchDataLoader2.LoadAsync` makes some parallel yesSql queries

Did you configure graphQL to do this (or maybe this is how we configure it)?

Hmm i already saw some related issues, and as i remember @carlwoodhouse added some locking and / or disabled this option. Are you using the last dev branch?

[jtkech]

@Skrypt @deanmarcussen @sfmskywalker @kevinchalet @MikeKry Just for infos

Working on indexes, see #8245, we already saw a similar issue related to a "missing" DbType for indexes, it was an enum type, see #8222 for the workaround waiting for a fix on yesSql side.

@sfmskywalker it was for the WorkflowStatus enum type

@kevinchalet i think it can be fixed on yesSql side by adding the missing DateTimeOffset type, but not sure it's good to add this DbType. Meanwhile I will open a PR to suggest the same kind of workaround but for the DateTimeOffset, and as you say, assuming that we only need UTC dates support.

For now, i'm not able to configure openId so that the related tables are populated for testing ;)

So, @MikeKry would be good if you can try the PR i will open

[kevinchalet]

@jtkech if we decide moving to DateTime is the right thing to do, I think we should also update the entities to use it, for consistency (it would be a bit weird to have properties of the same name with a different type: DateTimeOffset in the entities and DateTime in the indexes). What do you think?

[jtkech]

Exactly, i thought about it but "je n'ai pas osé" ;)

I opened a PR, for now it's just a workaround

But what is better, to keep DateTimeOffset if it is added to the DbTypes on YesSql side?

Update: Hmm, but for now i will also update the entities in my workaround

[kevinchalet]

If we're fine adopting a breaking change at this point, moving to DateTime shouldn't be a problem and may help a bit with compat' as it has better support outside the SQL Server world.

[jtkech]

Okay, thanks

[Skrypt]

We have an issue with people requiring to use a DateTime with a specific offset somewhere which I'd need to find. Maybe we need a content part then for this special use case instead of using a field. This could be a Content Part that has a Datetime + numeric field for storing the offset. Then on the DB side no more headaches if we never allow to use a DatetimeOffset dbtype.

Though nothing prevents someone to want to use this kind of DbType. If a DateTimeOffset is required for a field then it should be stored as a char/string "I think".

[kevinchalet]

This could be a Content Part that has a Datetime + numeric field for storing the offset. Then on the DB side no more headaches if we never allow to use a DatetimeOffset dbtype.

FWIW, when I need to deal with user-"local" dates, I personally prefer storing the time zone identifier/name rather than the offset, which is much safer when dealing with time zone changes (e.g last month, Europe/Volgograd changed from +4 hours to +3).

[Skrypt]

Exactly what I explained to @jtkech last night while we we're looking at his PR. The offset can change over time so the TimeZone identifier is more adequate at all times. That's what NodaTime exist for. DateTimeOffset should always only used in code to manipulate DateTimes on runtime but never to be persisted against a DB or else. If you look at NodaTime there is also specific types that replaces the DateTimeOffset.

Here is the NodaTime BCL conversions :

https://nodatime.org/2.3.x/userguide/bcl-conversions

We did use that for the IClock implementation in OC if you want to see some examples.

So here we should be using ZonedDateTime internally for example.

[kevinchalet]

Yeah, NodaTime is great! And thanks to its JSON.NET integration, you could directly use things like ZonedDateTime in your YesSql entities (for indexes, well... it won't be as easy) 😃