search

ActiveCampaign / postmark-dotnet

A .NET library for the Postmark API
http://developer.postmarkapp.com/
Other
50 stars 46 forks source link

Compatibility with System.Text.Json enumerable converter #125

Closed OperatorOverload closed 8 months ago

OperatorOverload commented 10 months ago

This update makes the necessary changes to enable compatibility with the System.Text.Json enumerable converter that is the .NET pipeline default in .NET Core 3.1 and later. In .NET Core 3.1, the default project templates and pipeline was adjusted to remove the requirement for Newtonsoft.Json and make the new integrated System.Text.Json library the default. This ensures that the webhook model can be used out of the box with current ASP.NET default project templates and settings.

Note that the change retains compatibility with the older Newtonsoft.Json binder as well by fully qualifying both references, so adding support here is not a breaking change.

MariuszTrybus commented 8 months ago

Thank you for your contribution but I can't merge it because it doesn't provide System.Text.Json support for all model classes.

OperatorOverload commented 8 months ago

Understood @MariuszTrybus, thank you for the feedback. I've been using the webhook successfully with the modified code in production for ~2 months now, so I don't know that the other ones are required, but they certainly could be by code that we're not exercising today.

Based on that, I've stepped through and added the System.Text.Json converter to all relevant String Enum types across the project. Would you mind taking another look now?

MariuszTrybus commented 8 months ago

@OperatorOverload some of the attributes have incorrect argument set that doesn't match the property value. I can fix it myself but I also want to spend some time testing if it works correctly.

OperatorOverload commented 8 months ago

@MariuszTrybus thanks for catching that, got a little copy/paste happy. Latest commit should have all of these fixed.

MariuszTrybus commented 8 months ago

@OperatorOverload Thank you! I will merge it but I won't release it just yet as there are some other issues with System.Text.Json serialisation of the models.

OperatorOverload commented 8 months ago

Thanks @MariuszTrybus. Can you share any more details on the other issues you're seeing? We've got this code in production now catching webhooks, but we're having to maintain a private build to do it, and I'd love to get back on an official release. I'm happy to see if I can help resolve the issues.

MariuszTrybus commented 8 months ago

@OperatorOverload I've just merged #129 and released v4.8.0. I think webhooks were not affected, it was mainly about the OnDeserialized handlers and some type mismatches in models.