Improved error handling for serialization errors

[JornWildt]

I just did a no-go thing and changed a string field to an integer in a custom entity. Obviously the deserialization of stored JSON failed instantly when showing the admin list of entities.

Now, it is easy for me to figure out what happened, but others might need a better error message than just "A server error happened".

In the admin log I see: "Current error context error is different to requested error." which is not really helpful - and then a stack trace:

   at Newtonsoft.Json.Serialization.JsonSerializerInternalBase.GetErrorContext(Object currentObject, Object member, String path, Exception error)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalBase.IsErrorHandled(Object currentObject, JsonContract contract, Object keyValue, IJsonLineInfo lineInfo, String path, Exception ex)
   at Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(JsonReader reader, Type objectType, Boolean checkAdditionalContent)
   at Newtonsoft.Json.JsonSerializer.DeserializeInternal(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonSerializer.Deserialize(JsonReader reader, Type objectType)
   at Newtonsoft.Json.JsonConvert.DeserializeObject(String value, Type type, JsonSerializerSettings settings)
   at Cofoundry.Domain.Data.Internal.DbUnstructuredDataSerializer.Deserialize(String serialized, Type type)
   at Cofoundry.Domain.Internal.CustomEntitySummaryMapper.MapAsync(ICollection`1 dbCustomEntities, IExecutionContext executionContext)
   at Cofoundry.Domain.Internal.SearchCustomEntitySummariesQueryHandler.ExecuteAsync(SearchCustomEntitySummariesQuery query, IExecutionContext executionContext)
   at Cofoundry.Domain.CQS.Internal.QueryExecutor.ExecuteQueryAsync[TQuery,TResult](TQuery query, IExecutionContext executionContext)
   at Cofoundry.Domain.CQS.Internal.QueryExecutor.ExecuteAsync[TResult](IQuery`1 query, IExecutionContext executionContext)
   at Cofoundry.Domain.CQS.Internal.QueryExecutor.ExecuteAsync[TResult](IQuery`1 query)
   at Cofoundry.Web.Admin.CustomEntityDefinitionsApiController.GetCustomEntities(String customEntityDefinitionCode, SearchCustomEntitySummariesQuery query)
   at lambda_method(Closure , Object )
   at Microsoft.Extensions.Internal.ObjectMethodExecutorAwaitable.Awaiter.GetResult()
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.TaskOfActionResultExecutor.Execute(IActionResultTypeMapper mapper, ObjectMethodExecutor executor, Object controller, Object[] arguments)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeActionMethodAsync>g__Awaited|12_0(ControllerActionInvoker invoker, ValueTask`1 actionResultValueTask)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeNextActionFilterAsync>g__Awaited|10_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Rethrow(ActionExecutedContextSealed context)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ControllerActionInvoker.<InvokeInnerFilterAsync>g__Awaited|13_0(ControllerActionInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResourceFilter>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.<InvokeFilterPipelineAsync>g__Awaited|19_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Logged|17_1(ResourceInvoker invoker)
   at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)
   at Cofoundry.Web.AutoUpdateMiddleware.Invoke(HttpContext cx)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at Cofoundry.Plugins.ErrorLogging.ErrorLoggingMiddleware.Invoke(HttpContext context, ILogger`1 logger, IErrorLoggingService errorLoggingService)

May I suggest you handle this error and tell the user there was a deserialization error - and perhaps add a description of possible reasons for the error?

[HeyJoel]

I thought we were handling errors in the JSON parsing, but I'll have to look into it. As you say, a custom error would be better.

It's difficult to know what we should do really, should we swallow the exception or throw it? I'd usually say throw it, as you don't want hidden parsing errors that could lead to lost data, but there could be situations where you'd want to ignore it or do a manual conversion, if for example you're updating existing data. I suppose in those situations you could always register your own custom JSON converter.

[JornWildt]

It's difficult to know what we should do really, should we swallow the exception or throw it?

I am also in favor of throwing (a better error message) for the same reasons. This is something that should not happen - and you need the programmer to fix it, not the CMS framework.

[HeyJoel]

I can't seem to replicate this in the test projects, but the default behavior is to assert the error if debugging, which I guess is how you somehow got a stack trace and error message (are you linking to the source?). In any case I don't think that is a good approach and instead we should just stick with the default behavior which is to log a warning. If anything, we should be improving the standard error logging module to capture warnings, but that's in the backlog anyway.

We could give the option to throw an error here, but that can get annoying pretty fast, especially when it's a deliberate action to change the type of the property and you're well aware of the implications.

The removal of the assertion will be in v0.10. Closing.