search

OctopusDeploy / Halibut

| Public | A secure communication stack for .NET using JSON-RPC over SSL.
Other
12 stars 44 forks source link

Add option for serializing the message envelope as generic #596

Closed Discolai closed 5 months ago

Discolai commented 6 months ago

Background

Moving a service response type to another namespace/assembly breaks backwards compatibility of s service. This is because Newtonsoft.Json is configured with TypeNameHandling.Auto and the response is serialized as MessageEnvelope<object> instead of MessageEnvelope<T>.

My team is currently handling this by forwarding to the correct type manually in a custom SerializationBinder, but this is prone to errors and introduces manual steps to our releases.

You might also consider making this the default behavior if this is true: Once ALL sources and targets are deserializing to MessageEnvelope<T>, (ReadBsonMessage) then this can be changed to T

Results

Users of the Halibut runtime can optionally specify that the message envelope will be serialized as a generic. Thus removing the $type from the response

Pre-requisites

Discolai commented 5 months ago

@LukeButters I am not sure if you are the person to tag, so feel free to bring in another reviewer.

This addition will improve our release experience significantly, so I would appreciate it if you could consider merging it

LukeButters commented 5 months ago

Could you add a test? It should be possible since you could put the old type in https://github.com/OctopusDeploy/Halibut/tree/main/source/Halibut.TestUtils.CompatBinary.Base and the new types within the normal test project https://github.com/OctopusDeploy/Halibut/tree/main/source/Halibut.Tests

Ideally the tests will show this fixes the issue.

Discolai commented 5 months ago

Could you add a test? It should be possible since you could put the old type in https://github.com/OctopusDeploy/Halibut/tree/main/source/Halibut.TestUtils.CompatBinary.Base and the new types within the normal test project https://github.com/OctopusDeploy/Halibut/tree/main/source/Halibut.Tests

Ideally the tests will show this fixes the issue.

I am sorry, but my initial testing was more of a proof of conecpt. And I didn't realize that ResponseMessage.Result was an object type. So this change does not fix the issue afterall. One would need to make ResponseMessage.Result generic as well.

And that seems like a big task which could easily break general backwards compatibility. So I will close this pr

LukeButters commented 5 months ago

Thanks for your PR @Discolai. While we didn’t get to a merge this time, we welcome PRs and suggestions. For significant changes, we recommend that you open an issue to discuss proposed changes to get alignment on the direction. :smile_cat:

If it helps Tentacle, which uses Halibut, had to deal with a change in namespace and assembly for some types. If I remember correctly this is done by converting to the old namespace/assembly on the way out and converts back to the new ones on the way in see here, here and here.