feat: Replace Newtonsoft.Json with System.Text.Json

[sommmen]

Closes #61.

This PR aims to remove jsonnet and replace it with STJ.

JsonNet was removed from Core and Web projects, including .nuspec of the web project.
Only 2 providers now use JsonNet, Elasticsearch (via Nest.JsonNetSerializer) and MongoDb.
MongoDb's ref to JsonNet could also be removed, but it serializes [BsonElement("Properties")] so it looked to risky. Perhaps in the future we can reevaluate this.

One test failed; It_serializes_an_error_on_exception which is because there was a double serialization for the error value.
I tried to fix the code so the test would remain intact, but that was hard to do.

Then when i decided i had to change the code anyways, and found no text references to 'errorMessage' anywhere else, i decided to upgrade the error feature to use the RFC 7807 - Problem Details for HTTP APIs. this is an industry standard also used by net core. Easily implemented with help from the renowned Andrew Lock.

The test projects may still use JsonNet - i deemed that okay since its not a dependency shipped to the library consumer.
Tested with the sample app - it looks good to me. Feel free to edit or remove this PR, let me know what you think.

[sommmen]

@followynne @mo-esmp review request 🚀 (i thought github had a 'request review' button? well could not find it...)

[followynne]

Hey @sommmen ,you can add reviewers in the top right corner, I just did it for you - will review as soon as I have sime time 😉

[followynne]

Hey @sommmen,

checked your PRs, thanks a lot for the work - I really appreciate the goal of replacing Newtonsoft! 💯

From what I saw, we shouldn't expect any hard/strange serialization in the library thus we should be safe to do this first replacement (usually the issue with STJ is that it's strict and less "gentle" on dirty data, while Newtonsoft accepts anything you throw at it :D)

In a second moment, we can work on replacing it in Elastic and Mongo libs.

The only request I have before closing the PR, if you have the time to do it, is to replace NJ in the tests projects, on files

tests/Serilog.Ui.Common.Tests/DataSamples/PropertiesFaker.cs
tests/Serilog.Ui.Web.Tests/Utilities/InMemoryDataProvider/InMemoryDataProvider.cs
tests/Serilog.Ui.Common.Tests/DataSamples/LogModelFaker.cs
tests/Serilog.Ui.Web.Tests/Endpoints/SerilogUiEndpointsTest.cs

We won't touch it in Mongo test setup but I think it should be done on all the other parts 😄

[followynne]

As said in the main comment, I'd prefer to remove all the unnecessary Netwonsoft references in the test projects.

[sommmen]

Hey @sommmen,

checked your PRs, thanks a lot for the work - I really appreciate the goal of replacing Newtonsoft! 💯

From what I saw, we shouldn't expect any hard/strange serialization in the library thus we should be safe to do this first replacement (usually the issue with STJ is that it's strict and less "gentle" on dirty data, while Newtonsoft accepts anything you throw at it :D)

In a second moment, we can work on replacing it in Elastic and Mongo libs.

The only request I have before closing the PR, if you have the time to do it, is to replace NJ in the tests projects, on files

tests/Serilog.Ui.Common.Tests/DataSamples/PropertiesFaker.cs
tests/Serilog.Ui.Web.Tests/Utilities/InMemoryDataProvider/InMemoryDataProvider.cs
tests/Serilog.Ui.Common.Tests/DataSamples/LogModelFaker.cs
tests/Serilog.Ui.Web.Tests/Endpoints/SerilogUiEndpointsTest.cs

We won't touch it in Mongo test setup but I think it should be done on all the other parts 😄

Yeah I'll check it probs tomorrow and if I don't have the time Friday I usually have a spare hour or two. 👍

[sommmen]

Updated the PR.

Merged upstream.
Newtonsoft.Json is still required in the test projects due to ES/MongoDb requiring them, but i removed as much usings as i could.

LogModelFaker.cs The exception is now just stringified because STJ does not serialize an exception without a custom converter. This could be added or picked up from somewhere, but in theory no assumption should be made about the log model's exception field having a piece of json inside - so i decided the tests should also reflect this, and just using Exception.ToString() is the best expected value you'd see in the wild.

LogModelPropsCollector.cs has some calls to ElementAtOrDefault(0) but then nothing was done about possible null values, so i replaced it with ElementAt(0), failing fast and defensive programming and all that. Additionally i made it so it would run less loops by using ICollection (and .ToList()) instead of IEnumerable so the test should run ever slightly faster.

MongoDbLogModelFaker.cs Since the Exception property is no longer some json, i simply generate a new fake exception for the MongoDbLogModel.Exception. A serialization round-trip is not really possible due to things like MethodBase.

Properties.cs Changed the Properties.EventId property from object to the right EventId type because else STJ has a hard time (de)serializing. object could still be used but then you have issues with Newtonsoft because STJ does not imply anything for an object property type and instead returns its own JsonNode which Newtonsoft does not understand OOB. Alas, doing it like this seemed the best way to go about things and should still cover the test scenarios (as in we're testing what we should be testing).

[followynne]

@sommmen

looks good to me, I'm not 100% sure about the assumption that the Exception property could not contain JSON, since the user should be making the choice between JSON or XML (if I remember right), but from another point of view you're right in making them more generic for all possible entries.

Approved, I'll leave to @mo-esmp the last review :)

[sommmen]

@sommmen

looks good to me, I'm not 100% sure about the assumption that the Exception property could not contain JSON, since the user should be making the choice between JSON or XML (if I remember right), but from another point of view you're right in making them more generic for all possible entries.

Approved, I'll leave to @mo-esmp the last review :)

I think you're confused with the Properties field - which contains xml/json, assumptions can be made that this field contains something that can be parsed, the exceptions field contains just a plain string.

Like here's a sample from my production database. The Exception column is simply Exception.ToString() which is now also the case in the tests. It could be that MongoDb or ES pushes JSON to the Exception column, but since mssql does not, and the default way to store exceptions is to call .ToString() this is the best way.

@mo-esmp when you decide to merge this in, please also push a release since that would contain the fix to #108 which is not on nuget yet and i'd like that fix :).

Microsoft.Data.SqlClient.SqlException (0x80131904): Transaction (Process ID 124) was deadlocked on lock resources with another process and has been chosen as the deadlock victim. Rerun the transaction.
   at Microsoft.Data.SqlClient.SqlConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at Microsoft.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
   at Microsoft.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
   at Microsoft.Data.SqlClient.SqlDataReader.TryHasMoreRows(Boolean& moreRows)
   at Microsoft.Data.SqlClient.SqlDataReader.TryReadInternal(Boolean setTimeout, Boolean& more)
   at Microsoft.Data.SqlClient.SqlDataReader.ReadAsyncExecute(Task task, Object state)
   at Microsoft.Data.SqlClient.SqlDataReader.InvokeAsyncCall[T](SqlDataReaderBaseAsyncCallContext`1 context)
--- End of stack trace from previous location ---
   at Microsoft.EntityFrameworkCore.Query.Internal.SingleQueryingEnumerable`1.AsyncEnumerator.MoveNextAsync()
   at Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions.ToListAsync[TSource](IQueryable`1 source, CancellationToken cancellationToken)
   at Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions.ToListAsync[TSource](IQueryable`1 source, CancellationToken cancellationToken)
   at OPG.Web.Shared.Controllers.BaseController`1.GetCollectionResponseModel[TEntity,TViewModel](CollectionHttpRequestModel requestModel, IQueryable`1 queryable, Func`2 resultQueryableFunc, CancellationToken cancellationToken, ValueTuple`2[] defaultOrderBy) in [REDACTED]
   at OPG.Web.Shared.Controllers.BaseController`1.GetCollectionResponseModel[TEntity,TViewModel](IQueryable`1 queryable, CollectionHttpRequestModel requestModel, CancellationToken cancellationToken, ValueTuple`2[] defaultOrderBy) in [REDACTED]
   at OPG.Web.Shared.Controllers.BaseController`1.Ok[TEntity,TViewModel](IQueryable`1 queryable, CollectionHttpRequestModel requestModel, CancellationToken cancellationToken, ValueTuple`2[] defaultOrderBy) in [REDACTED]
   at OPG.WebApi.Controllers.OrderShipmentPackageController.Get(CollectionHttpRequestModel requestModel, CancellationToken cancellationToken) in [REDACTED]
   at Microsoft.AspNetCore.Mvc.Infrastructure.ActionMethodExecutor.TaskOfIActionResultExecutor.Execute(ActionContext actionContext, 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.<InvokeFilterPipelineAsync>g__Awaited|20_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)
   at Swashbuckle.AspNetCore.SwaggerUI.SwaggerUIMiddleware.Invoke(HttpContext httpContext)
   at Swashbuckle.AspNetCore.Swagger.SwaggerMiddleware.Invoke(HttpContext httpContext, ISwaggerProvider swaggerProvider)
   at Microsoft.AspNetCore.Localization.RequestLocalizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at Serilog.AspNetCore.RequestLoggingMiddleware.Invoke(HttpContext httpContext)
   at NewRelic.Providers.Wrapper.AspNetCore.WrapPipelineMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http.HttpProtocol.ProcessRequests[TContext](IHttpApplication`1 application)
ClientConnectionId:dbd9cd83-67bc-41e3-beaf-a20c60543af9
Error Number:1205,State:51,Class:13

[mo-esmp]

@followynne A serialized exception is not needed here? because the output of .ToString() is different with a serialized exception.

[mo-esmp]

@sommmen Thanks for the PR.
@followynne Would you please release a new package version 2.6.0 for Serilog.Ui.Web ?

[followynne]

@mo-esmp

Yep,I'll try to do it this weekend together with mssql 2.2.3, as soon as I can 👍