-
-
Notifications
You must be signed in to change notification settings - Fork 187
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
Switch from Newtonsoft.Json to System.Text.Json #1160
Conversation
I'm using the DMS is allowing you to use any serializer other than It means you should be able to use your own serializer if the one used by default by DMS is not fitting your needs (for any reasons) That's why we have an example that uses a public class CustomMessagePackSerializer : ISerializer
{
public async Task<object> DeserializeAsync(Stream ms, Type type)
{
var val = await MessagePackSerializer.DeserializeAsync(type, ms, MessagePackSerializerOptions.Standard
.WithResolver(ContractlessStandardResolver.Instance));
return val;
}
public async Task<byte[]> SerializeAsync(object obj)
{
using var ms = new MemoryStream();
await MessagePackSerializer.SerializeAsync(ms, obj, MessagePackSerializerOptions.Standard
.WithResolver(ContractlessStandardResolver.Instance));
return ms.ToArray();
}
} And then use it in var webRemoteOrchestrator = new WebRemoteOrchestrator(serviceUri);
webRemoteOrchestrator.SerializerFactory = new CustomMessagePackSerializer();
var agent= new SyncAgent(clientProvider, webRemoteOrchestrator, options); Allowing the user to use its own serializer implies your custom serializer should be able to handle the attributes used in DMS (for now Now, that you know why we are using
First solution can work with Second solution will introduce a breaking change as I'm not sure A more complex solution would be to make the same kind of thing as What do you think ? |
Another thing to mention: We have a way to customize serialization from web client to web server using the We are also using one serializer called |
ZCS.DataContractResolver is nice. Their nuget package doesn't support netstandard 2.0 but not for any good reason. I opened an issue over there. But the whole thing is just 150 lines or so, and MIT licensed. We can pull in the file, with attribution, and all will be well. I have updated this PR accordingly. I like this approach best because I can minimally affect the non-serialization-related code in DMS. Pulling in this data contract resolver code also seems like a good idea, because it would allow me to more easily add some performance optimizations later. Their approach makes a lot of use of reflection, linq, etc. Of course, I could upstream some of these changes to them, if they turn out well. But even if that happens, and they add support, not having unnecessary nuget libraries reduces the attack surface with respect to supply chain attacks. The actual code simply being in this repo is the most secure. Alas, even after doing this the unit tests are still failing for an unknown reason. |
8ea52d9
to
a8e6371
Compare
I'm trying to see what happens, and it seems the For instance, here is a piece of JSON I've found, just by serializing a Before: {
"t": [
{
"n": "ProductCategory",
"s": "SalesLT",
"op": "SqlSyncProvider, Dotmim.Sync.SqlServer.SqlSyncProvider",
"c": [
{
"n": "ProductCategoryID",
"dt": "17",
"iu": true,
"iuc": true,
"ml": 12,
"odb": "NVarChar",
"oty": "nvarchar",
"db": 16
} After: {
"$id": "1",
"t": {
"$id": "2",
"$values": [
{
"$id": "3",
"n": "ProductCategory",
"s": "SalesLT",
"op": "SqlSyncProvider, Dotmim.Sync.SqlServer.SqlSyncProvider",
"c": {
"$id": "4",
"$values": [
{
"$id": "5",
"n": "ProductCategoryID",
"dt": "17",
"iu": true,
"iuc": true,
"ml": 12,
"odb": "NVarChar",
"oty": "nvarchar",
"db": 16
} Still not sure this is the bug we are hitting, but for sure we are not compatible with previous versions. I continue my research... |
Ok, removing Still searching :) |
I have make minor fixes in your code, removing That being said, while I'm was looking at public static (SyncTable schemaTable, int rowsCount, SyncRowState state) GetSchemaTableFromFile(string path)
{
if (!File.Exists(path))
return default;
string tableName = null, schemaName = null;
int rowsCount = 0;
SyncTable schemaTable = null;
SyncRowState state = SyncRowState.None;
string jsonString = File.ReadAllText(path);
using var doc = JsonDocument.Parse(jsonString);
var root = doc.RootElement; public IEnumerable<SyncRow> GetRowsFromFile(string path, SyncTable schemaTable)
{
if (!File.Exists(path))
yield break;
using var stream = File.OpenRead(path);
using var reader = new StreamReader(stream);
var root = serializer.Deserialize<JsonElement>(reader.ReadToEnd()); But the main purpose of The main purpose of It seems we can do the same following this doc: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/use-utf8jsonreader . For now I'm struggling on a locked file while reading a local file, still searching why ... |
Great job solving the test! Now that you fixed it, it feels like an obvious oversight to not remove ReferenceHandler.Preserve when switching to this new approach. But it probably would've taken me a long time. I switched from reading the whole file as json to reading from the UTF8 file stream directly as requested. That is clearly better, yes. |
I see the locked files. |
Don't work too much on the byte stream reader, I'm currently on it |
You can fix the locking issue by adding this.sw.Dispose() to the CloseFile() method in the LocalJsonSerializer. |
@Mimetis fixed the conflicts. Tests passed. You can feel free to update the PR with JsonExtensions functionality. Or merge and make a new PR. |
I don't see tests passing, unfortunately, yet |
You are right about that, but somehow github erroneously told me everything passed. Might need to update the pipelines to report test failures correctly. I will look at both the failure and try to improve the failure reporting. |
I'm on it ! |
Add more parsing Fix null parsing
Remove ConfigureAwait(false) in tests
Github is having some delays today. Was able to manually merge your branch, and hopefully the actions start later today. |
Ok, I guess we can merge it !! |
Benefits of System.Text.Json:
Faster than Newtonsoft, especially when using the source generators.
The binary size for the System.Text.Json dll is a bit smaller, and also trimmable.
Most people going forward will already be using System.Text.Json so people making apps for MAUI will have smaller app sizes by not having to bundle two serializers