Skip to content

TaskOrchestrator re-serializes message payload using Newtonsoft.Json. #796

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

Closed
ulvesked opened this issue Sep 20, 2022 · 11 comments
Closed

Comments

@ulvesked
Copy link

I get a JsonException when passing complex types from Orchestrator to Activity. The payload received in the TaskActivity is not compatible with System.Text.Json.

After investigation, it appears that the orchestrator re-serializes all messages using Newtonsoft.Json, with TypeNameHandling = TypeNameHandling.Objects. This results in a JSON payload that is not parseable using System.Text.Json (which is default for TaskActivity payload)

The class that does the conversion:
https://github.com/Azure/durabletask/blob/main/src/DurableTask.Core/Serializing/JsonDataConverter.cs

Here is a sample project to demonstrate the issue: https://github.com/ulvesked/DurablePocoPayloadSample
And a corresponding SO question: https://stackoverflow.com/questions/73776121/json-serialization-fails-with-functioninputconverterexception-for-complex-types

My original payload:

{
    "Name": "Test",
    "Items": [{
            "Id": "4b42d183-02c8-4108-b274-cde71f792832",
            "Image": "xH5hV...",
            "Thumbnail": "v8Tc3..."
       }, {
            "Id": "4eb034f6-eed0-4ad4-942d-c9bf20e465be",
            "Image": "wAj...",
            "Thumbnail": "juSiE",
        }
    ]
}

The payload received in TaskActivity:

{
    "Name": "Test",
    "Items": [{
            "$type": "DurablePocoPayloadSample.TestPayloadItem, DurablePocoPayloadSample",
            "Id": "4b42d183-02c8-4108-b274-cde71f792832",
            "Image": {
                "$type": "System.Byte[], System.Private.CoreLib",
                "$value": "xH5hV..."
            },
            "Thumbnail": {
                "$type": "System.Byte[], System.Private.CoreLib",
                "$value": "v8Tc3..."
            }
        }, {
            "$type": "DurablePocoPayloadSample.TestPayloadItem, DurablePocoPayloadSample",
            "Id": "4eb034f6-eed0-4ad4-942d-c9bf20e465be",
            "Image": {
                "$type": "System.Byte[], System.Private.CoreLib",
                "$value": "wAj..."
            },
            "Thumbnail": {
                "$type": "System.Byte[], System.Private.CoreLib",
                "$value": "juSiE..."
            }
        }
    ]
}
@ulvesked
Copy link
Author

Probably related to microsoft/durabletask-dotnet#36

@cgillum
Copy link
Member

cgillum commented Sep 20, 2022

You should be able to replace the built-in DataConverter (which uses Newtonsoft.Json) with your own serializer based on System.Text.Json. That's effectively what was done for the issue you linked to. There are a few different places in the code where you can customize this.

@jviau
Copy link
Collaborator

jviau commented Sep 20, 2022

@ulvesked are you using durabletask-dotnet? If so, this issue will be addressed in the next preview.

If you are not using durabletask-dotnet, then the default is Newtonsoft.Json. If you are intending to replace the default converter, you can look at my fix in how I set the converter during orchestrations: microsoft/durabletask-dotnet#45, specifically the changes to TaskOrchestratorShim where the converters on OrchestrationContext are assigned to.

@plamber
Copy link

plamber commented Sep 21, 2022

Hi @jviau,
I am having the same issue and haven't figured out how I can replace the Newtonsoft.Json serializer with my custom System.Text.Json converter.

Do you have a code snippet that shows how to do this?

Thank you,

@ulvesked
Copy link
Author

Thanks for your comments. I have worked around the issue by using my own ObjectSerializer.

services.Configure<WorkerOptions>(opts => opts.Serializer = new MyNewtonsoftJsonObjectSerializer());

https://gist.github.com/ulvesked/44a6fc30d8fd622671343100eef8f1ae

I will keep it like this until the next preview, which hopefully has implemented the same serializer across the pipeline as @jviau wrote.

@jviau
Copy link
Collaborator

jviau commented Sep 21, 2022

@plamber to fully replace the converter in DurableTask, there are a several locations you need to perform that:

  1. In the constructor of TaskHubClient
  2. Property on AsyncTaskActivity<TInput, TResult>
    • It is a protected set, so you can derive another interface ISettableDataConverter which has a method or property to set the data converter. Make sure all your activities implement this, then add a middleware to the DTFx pipeline to retrieve the current activity, attempt a cast to this, and set it.
  3. Property on TaskOrchestration<TResult, TInput, TEvent, TStatus>
    • Again a protected set, so use the same steps as AsyncTaskActivity in point 2
  4. Properties on OrchestrationContext (MessageDataConverter and ErrorDataConverter).
    • You might be able to also set this in middleware, can't remember if you have access to this object there. Otherwise, you would need to derive your own base classes that implement the various TaskOrchestration types, and add a new step as part of its run method to set these properties on the context before calling the derived implementation.

@jviau jviau closed this as completed Nov 8, 2022
@jviau
Copy link
Collaborator

jviau commented Nov 8, 2022

Closing as this is possible

@RonenGlants
Copy link

RonenGlants commented Dec 18, 2022

Hey, I wanted to share that I used the method described above by @jviau (thank you)
And started to see exceptions from the System.Text.Json based data converter at the likes of:
"System.NotSupportedException: 'Serialization and deserialization of 'System.IntPtr' instances are not supported"
This happened because System.Text.Json - can't serialize exceptions
To avoid this unexpected noise I used ErrorPropagationMode.UseFailureDetails when configuring the task hub.

@franklixuefei
Copy link
Member

Hey, I wanted to share that I used the method described above by @jviau (thank you) And started to see exceptions from the System.Text.Json based data converter at the likes of: "System.NotSupportedException: 'Serialization and deserialization of 'System.IntPtr' instances are not supported" This happened because System.Text.Json - can't serialize exceptions To avoid this unexpected noise I used ErrorPropagationMode.UseFailureDetails when configuring the task hub.

Hi Ronen, I wonder if you could share some sample code that performs the replacement? I can't wrap my head around it yet. Thanks a lot!

@twsouthwick
Copy link
Contributor

twsouthwick commented Sep 11, 2023

@plamber to fully replace the converter in DurableTask, there are a several locations you need to perform that:

  1. In the constructor of TaskHubClient

  2. Property on AsyncTaskActivity<TInput, TResult>

    • It is a protected set, so you can derive another interface ISettableDataConverter which has a method or property to set the data converter. Make sure all your activities implement this, then add a middleware to the DTFx pipeline to retrieve the current activity, attempt a cast to this, and set it.
  3. Property on TaskOrchestration<TResult, TInput, TEvent, TStatus>

    • Again a protected set, so use the same steps as AsyncTaskActivity in point 2
  4. Properties on OrchestrationContext (MessageDataConverter and ErrorDataConverter).

    • You might be able to also set this in middleware, can't remember if you have access to this object there. Otherwise, you would need to derive your own base classes that implement the various TaskOrchestration types, and add a new step as part of its run method to set these properties on the context before calling the derived implementation.

Yes, this is possible, but that seems like a lot of steps to reimplement. I've opened a PR to flow the property bag through to the task/activity layer, which allows for these serializers to become properties tracked in the dispatch context (see #963). If this change gets merged, you'll be able to replace them in middleware with the following:

    public static class TestExtensions
    {
        public static void UseSerializer(this DispatchMiddlewareContext context, DataConverter converter)
        {
            if (converter is not JsonDataConverter json)
            {
                json = new JsonDataConverterProxy(converter);
            }

            context.SetProperty<DataConverter>(converter);
            context.SetProperty<JsonDataConverter>(json);
        }

        private sealed class JsonDataConverterProxy : JsonDataConverter
        {
            private readonly DataConverter _converter;

            public JsonDataConverterProxy(DataConverter converter)
            {
                _converter = converter;
            }

            public override object Deserialize(string data, Type objectType)
                => _converter.Deserialize(data, objectType);

            public override string Serialize(object value)
                => _converter.Serialize(value);

            public override string Serialize(object value, bool formatted)
                => _converter.Serialize(value, formatted);
        }
    }

NOTE: I haven't figured out how to just flow it from TaskHubClient, but that would be ideal, so we just need to set it in one place. I can't think of a great scenario where I'd want different serializers for different parts of the process

@plamber
Copy link

plamber commented Sep 12, 2023

Thank you @twsouthwick. This is highly appreciated. I am looking forward for the merged PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants