kontent-ai / delivery-sdk-net

Kontent.ai Delivery .NET SDK
https://www.nuget.org/packages/Kontent.Ai.Delivery
MIT License
32 stars 41 forks source link

IDateTimeContent Does not deseralize from Redis property #378

Closed shgreig-mtm closed 10 months ago

shgreig-mtm commented 11 months ago

Brief bug description

We are using redis caching, when serlizing and deserlizing a IDateTimeContent property it will not serlize the properties. We beleive this is because the DateTimeContent is a sealed class Other properties such as IRichTextContent are not sealed and work as expected

Repro steps

Using ToBSON() on an object using IDateTImeContent won't serlize the the IDateTImeContent property correctly

Expected behavior

Date and timezone properties are seralized correctly

Test environment

dzmitryk-kontent-ai commented 11 months ago

Hi @shgreig-mtm , thank you for bringing the defect to my attention. I will look into this use-case and let you know if I can fix it.

dzmitryk-kontent-ai commented 11 months ago

Hi @shgreig-mtm. I tried the further scenario on my personal small project and I didn't manage to reproduce your problem. I have the project which contains the ContentType with DateTime element and a ContentItem of this ContentType: image

I generated a strong-typed model from this project so that DateTime element should be converted to IDateTypeContent type using DateTimeContentConverter:

using Kontent.Ai.Delivery.Abstractions;
namespace Test_DateTimeContent.Model
{
    public partial class Type1
    {
        public const string Codename = "type1";
        public const string DatetimeCodename = "datetime";

        public IDateTimeContent Datetime { get; set; }
        public IContentItemSystemAttributes System { get; set; }
    }
}

Then I created a small application in .NET 7 which gets this ContentItem using delivery-sdk-net, serializes an Item from the response to BSON and deserializes it back to the object of my local class. It's impossible to use DateTimeContent type from SDK as it's internal class. Also it's impossible to use model class (Type1) as Datetime and System properties are defined using interfaces.

It works fine for me.

You can find my code below

public class TestDateTimeContent
    {
        private readonly IDeliveryClient _deliveryClient;

        public TestDateTimeContent()
        {
            var customTypeProvider = new CustomTypeProvider();

            _deliveryClient = DeliveryClientBuilder
                .WithOptions(builder => builder
                    .WithProjectId("08256e53-a9ed-01af-880e-3407637e7a5f")
                    .UseProductionApi()
                    .Build())
                .WithTypeProvider(customTypeProvider)
                .Build();
        }

        public async Task Run()
        {
            var response = await _deliveryClient.GetItemAsync<Type1>("item1");
            var bsonString = ToBson(response.Item);
            var deserialized = FromBson<TypeWithDateTime>(bsonString);
        }

        public static string ToBson<T>(T value)
        {
            using (var ms = new MemoryStream())
            using (var datawriter = new BsonDataWriter(ms))
            {
                var serializer  = new JsonSerializer();
                serializer.Serialize(datawriter, value);
                return Convert.ToBase64String(ms.ToArray());
            }
        }

        public static T FromBson<T>(string base64data)
        {
            byte[] data = Convert.FromBase64String(base64data);

            using (MemoryStream ms = new MemoryStream(data))
            using (BsonDataReader reader = new BsonDataReader(ms))
            {
                JsonSerializer serializer = new JsonSerializer();
                return serializer.Deserialize<T>(reader);
            }
        }

        private class TypeWithDateTime
        {
            public DateTimeContent Datetime { get; set; }
            public ContentItemSystemAttributes System { get; set; }
        }

        private class DateTimeContent: IDateTimeContent
        {
            public DateTime? Value { get; set; }

            public string DisplayTimezone { get; set; }
        }

        private class ContentItemSystemAttributes : IContentItemSystemAttributes
        {
            public string Language { get; set; }

            [JsonProperty("sitemap_locations")]
            public IList<string> SitemapLocation { get; set; }

            public string Type { get; set; }

            public string Collection { get; set; }

            [JsonProperty("workflow_step")]
            public string WorkflowStep { get; set; }

            [JsonProperty("last_modified")]
            public DateTime LastModified { get; set; }

            public string Codename { get; set; }

            public string Id { get; set; }

            public string Name { get; set; }
        }
    }

So, it doesn't seem that sealed DateTimeContent class causes some problems when I serialize response to BSON format. Probably, I missed something important from your use-case. Please, feel free to provide more information so I could help you.

johnhydemtm365 commented 11 months ago

Hi, Stuart is off today. Just to correct the above issue, its not Serialising that is the issue, but De Serialising because DateTimeContent is a sealed class and as such cannot be used when deserialising. Is there a reason what this class is sealed, unlike the others like IRichTextContent?

Looking at the code above, and I can see how you have it working. You are specifically telling it to deserialise into the object of your choice. This would not work in a dynamic environment, where the caching is abstracted away, the code would have to know that its a special date type, to be able to deserialise it into the non sealed class.

We are already using '.FromBson' where T is the content type, retreived from the delivery API.

I have pulled down the SDK and can see you are using converters for the Datetime and also the Richtext, however because DateTime is sealed it will never work outside of the SDK?

Is there a way to tell it not to use the Sealed class, dynamically?

Thoughts? Thanks John

dzmitryk-kontent-ai commented 11 months ago

Hi @johnhydemtm365 , thank you for the clarification of the problem. I guess, there wasn't any specific reason to have this class sealed other than we didn't expect this class to be inherited, as we considered it as internal class for SDK. I think, it shouldn't be a problem to remove sealed modifier as it makes problem in your case. I will discuss it with colleagues, just to be sure that I don't miss something important

johnhydemtm365 commented 11 months ago

Hi @dzmitryk-kontent-ai awesome, if it could be set the same as RichTextContent then all should be good.

If not then, then we will need to look at other options as this is an issue in production at the moment. To get around the issue we have had to revert to memory caching and one instance of the server... which is not ideal for production workloads.

dzmitryk-kontent-ai commented 11 months ago

Hi @johnhydemtm365 , @shgreig-mtm . I believe I have resolved the issue with serialization and deserialization of IDateTimeContent to and from BSON. The problem did not lie in the class being sealed, but rather in the lack of knowledge on how to deserialize into the DateTimeContent object. After adding the necessary JsonProperty attributes, the deserialization process started functioning correctly. I have thoroughly tested this scenario, and I am confident that the problem has been successfully resolved.

Furthermore, I discovered that DateTime values were not being serialized and deserialized accurately. Even though the DateTime values initially had the property Kind=UTC, they were being deserialized with Kind=Local. However, I have rectified this issue, ensuring that DateTime values remain absolutely identical, including the Kind property, after serialization and deserialization.

If you wish, you can see the pull request. I'm currently awaiting feedback from my colleague during the code review process, and I anticipate releasing the new version of the SDK by the end of the week.

shgreig-mtm commented 11 months ago

@dzmitryk-kontent-ai Thank you! I look forward to seeing it deployed soon

dzmitryk-kontent-ai commented 11 months ago

Hi @johnhydemtm365 , @shgreig-mtm . The fix is released with v. 17.7.0. Please, let me know, whether it solved your problem

Simply007 commented 10 months ago

Hi!

This issue has gone quiet. 👻 It’s been a while since the last update here.

If we missed anything on this issue or if you want to keep it open, please reply here.

Thanks for being a part of the Kontent.ai community!

johnhydemtm365 commented 10 months ago

Hi all, I believe we have deployed the fix and all is good.

Many thanks for your help in this.

J

On Wed, 16 Aug 2023 at 12:29, Ondřej Chrastina @.***> wrote:

Hi!

This issue has gone quiet. 👻 It’s been a while since the last update here.

If we missed anything on this issue or if you want to keep it open, please reply here.

Thanks for being a part of the Kontent.ai community!

— Reply to this email directly, view it on GitHub https://github.com/kontent-ai/delivery-sdk-net/issues/378#issuecomment-1680430737, or unsubscribe https://github.com/notifications/unsubscribe-auth/AH2XYN76CAKOOHF22GKND6LXVSVILANCNFSM6AAAAAA2VMEC7A . You are receiving this because you were mentioned.Message ID: @.***>

--

John Hyde Technical Operations Manager T: +44 (0) 2380 215 399 | W: themtmagency.com https://themtmagency.com/?utm_source=mtm-email-footer

Facebook https://www.facebook.com/TheMTMAgency/ | Linkedin https://www.linkedin.com/company/mtmagency/ | Twitter https://twitter.com/themtmagency?lang=en | Sign up to our Newsletter https://confirmsubscription.com/h/y/1AE240E0C3EBC9A7 The MTM Agency – 2nd Floor, The Quay, 30 Channel Way Southampton, SO14 3TG

Rated by our clients via The Drum Recommends as a Top 10 agency in the UK