Fix unexpected behavior for task updates regarding labels.

[AhmedZaki99]

Problem:

• According to Todoist Sync API, arguments other than the Id are not required for update requests, and if not given, would not affect or update the existing values.
• When we send an update request, using ItemsCommandService.UpdateAsync or similar methods, we expect that only the properties we set for the Item object will be updated, and the rest is ignored.
• That doesn't apply for the Item.Labels property, instead, we have to get the full Item body to ensure all labels are present in the Item.Labels collection, or all labels would be removed by the request if the property is left unchanged.

Reason:

The unexpected behavior explained above was a result of the Item.Labels property initialization in the Item constructor:

public Item(string content, ComplexId projectId)
{
    ...
    Labels = new Collection<string>();
}

And since any non-null property is sent as an argument with the sync command, ItemsCommandService.UpdateAsync sends a command to update the task with an empty collection of labels unintentionally based on the initialized value.

Fix:

The fix consists of two parts:

1. Remove the internal access modifier from the Item.Labels property setter to allow users from external assemblies to modify the initialized value, potentially to null, if needed.
2. Remove the Labels property initialization with an empty collection from the Item constructor, making it null by default.

The reason why this fix is split into two commits is because the second one potentially represents a braking change!

• In the first change, users were given the responsibility to set the Labels property to null whenever they wanted to update tasks without affecting the existing labels.
• In the second change, Labels became null by default, as users most likely would expect before updating, and when users need to create or update an Item with labels, they need to set the collection itself rather than adding values to it, or a NullReferenceException would be thrown.

So, any user of this library would need to make that change to avoid the NullReferenceException:
From

var task = new Item("new task");
task.Labels.Add("new label"); // Throws an exception

To

var task = new Item("new task");
task.Labels = new[] { "new label" };

[AhmedZaki99]

@olsh
What do you think? Should we split this PR into two, one for the workaround (first commit) and one for the breaking change (second commit), and leave the second PR to the next major release?

[olsh]

Hi @AhmedZaki99

Nice catch about the bug.

Well, I think that the current design (using the same class for both methods) is bad. I'd suggest creating a separate class (probably we should do this with inheritance or something) for the update method.
Yes, this will be a breaking change, but the API will be more clear. Also, the update object doesn't contain some properties of the add object like project_id, auto_reminder, etc.

[AhmedZaki99]

Well, I think that the current design (using the same class for both methods) is bad. I'd suggest creating a separate class (probably we should do this with inheritance or something) for the update method. Yes, this will be a breaking change, but the API will be more clear. Also, the update object doesn't contain some properties of the add object like project_id, auto_reminder, etc.

Ok, that's a good suggestion. I will push new commits soon with a proposed design for two separate Item models, one possibly inheriting the other.

[AhmedZaki99]

@olsh

Alright, here's a quick summary of what changed:

• Properties of the Item object that could be sent in Update requests are moved to a new base type called ItemBase.
• The rest of the properties in the Item object are split into two categories:
  i. Properties that could be sent in Create requests, hence they are settable by the user and have public setters.
  ii. Properties that are only present in objects returned from the Get request, hence they have internal setters.
• A couple of missing properties used in Create requests are also added (auto_reminder and auto_parse_labels), with public setters but nullable types because they are not present in objects sent by Get requests.
• The Labels property has now a public setter as we discussed at the beginning of this PR, but now its initialization with an empty collection is kept for the Item constructor, and it only defaults to null in the ItemBase objects, which excludes the Create/Add requests from being affected by breaking changes.
• The public constructor of the ItemBase object requires an itemId argument for the task to update.
• ItemsCommandService.UpdateAsync now uses an ItemBase object as an argument instead of an Item object.

The best part about inheritance is that ItemsCommandService.UpdateAsync could still accept Item objects passed to it, and it only uses properties present in the ItemBase base type, so not a single call to this method needed to be changed :)

However, right after I finished the changes, I stumbled across a problem that sent me back right from where we began :(

This PR started with the Labels property being set unintentionally to empty collection for the Update requests we send.
I accidentally found out that the same thing occurs with the DueDate and Duration properties.

This is because the DueDate and Duration types have the JsonProperty.NullValueHandling setting configured to NullValueHandling.Include, to allow us to remove due dates and durations with Update requests, by setting them to null.

So for example, if we only want to update the Content of a task that has a due date, the Update request we sent will also include a due_date argument with a null value, unless we made a Get request first to populate all properties and change the Content only.

Fix?

The fix this time was much harder than the Labels property. With Labels we only removed the empty collection initialization, but with DueDate, we need to include null values to be able to unset it, but we don't want to include them at the same time when they are not set by the user.

I almost spent my entire weekend on it but I finally reached a good fix. I will push commits with the proposed fix and explain it briefly in another comment.

[AhmedZaki99]

So, the idea of the fix for unexpected updates of the DueDate and Duration properties was to find a way to tell the Newtonsoft.Json serializer when to include null values and when to ignore them.

Newtonsoft.Json was very tricky to deal with, but after lots of thinking and playing around, I found that we can keep the current configuration of NullValueHandling set to Include, and use some value other than null to express that we want the property to be ignored.

For that purpose, I added a new interface (INonNullDefault) to express that the implementing type treats null as an included value, and has another non-null value to act as a default.

The interface has only one property declaration with the following comments:

/// <summary>
/// Represents a type that has a default value other than null.
/// </summary>
/// <remarks>
/// Non-null default values are typically used to be ignored during serialization
/// when null values should be included. 
/// </remarks>
internal interface INonNullDefault
{
    /// <summary>
    /// Gets a value indicating whether the current instance represents the type's default value.
    /// </summary>
    bool IsDefault { get; }
}

Using the information provided by that interface, the ConverterContractResolver.CreateProperty could now decide when to include a property of an implementing type, and when to ignore it:

var property = base.CreateProperty(member, memberSerialization);

. . .

if (typeof(INonNullDefault).IsAssignableFrom(property.PropertyType) && member is PropertyInfo propertyInfo)
{
    property.NullValueHandling = NullValueHandling.Include;
    property.ShouldSerialize = instance =>
    {
        var value = propertyInfo.GetValue(instance, null) as INonNullDefault;
        return value?.IsDefault != true; // Serialize null and non-default values (as long as IsDefault isn't true).
    };
}

return property;

And to implement the INonNullDefault interface, types like DueDate and Duration need to have a single instance accessible by a static field to represent the default value, as follows:

public class DueDate : INonNullDefault
{
    /// <summary>
    /// A <see cref="DueDate"/> instance that represents the non-null default value.
    /// </summary>
    internal static readonly DueDate Default = new DueDate();

    bool INonNullDefault.IsDefault => this == Default;

. . .

Finally, we update properties of type DueDate and Duration to be initialized by the default value instance:

[JsonProperty("due")]
public DueDate DueDate { get; set; } = DueDate.Default;

. . .

[JsonProperty("duration")]
public Duration Duration { get; set; } = Duration.Default;

And that makes each of these properties ignored by default in outgoing requests unless the user sets them to null or any other value, knowing that DueDate.Default is not accessible by external assemblies due to the internal access modifier.

So, what do you think, is there anything I might be missing? @olsh

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[AhmedZaki99]

Quick note.
I found a couple of issues when I used my fork and tested it a little:

• The Duration.Default static instance didn't have a value set to its Unit property, hence, the TimeValue property setter was throwing NotImplementedException for it. This problem was fixed by returning TimeSpan.Zero instead.
• The Newtonsoft.Json deserializer was populating the Default instance for properties like DueDate & Duration instead of setting a new instance. This problem was fixed by initializing these properties with the Default instance only in the public constructors, leaving the internal parameterless constructors used by the deserializer without property initialization to Default, so these properties default to null before deserialization.

[olsh]

Well, that approach is slightly better than the previous one. But a couple of things are concerning me.

1. The new default pattern has a value. This is confusing to see a random date when you debug the code. Also I don't quite understand how we will use this approach with other types like responsible_uid for example.

2. The Labels property has now a public setter as we discussed at the beginning of this PR, but now its initialization with an empty collection is kept for the Item constructor, and it only defaults to null in the ItemBase objects, which excludes the Create/Add requests from being affected by breaking changes.

This different behavior is confusing too. And it can potentially lead to a hard-to-spot bug. For instance when you used ItemBase somewhere and later decided to use Item. Actually the ItemBase is not a good name, the Base part is meaningless for an end user. The more I think about it, the more I find that Create and Update should be different classes without inheritance.

I suggest that we separate the two classes and then we have two approaches for the ItemUpdate class.

1. Use the monad pattern. https://github.com/vkhorikov/CSharpFunctionalExtensions?tab=readme-ov-file#make-nulls-explicit-with-the-maybe-type but this approach looks too complex for the simple library, in my opinion. But this is discussable.
2. The second approach is to use the builder pattern for the update class. Something like this

var updateItem = UpdateItemBuilder
                      .WithDuration(duration)
                      .WithPriority(2)
                      .WithEmptyResponsible()
                      .Build();

var updateItemFromExistingItem = 
                             UpdateItemBuilder
                                   // Copy all supported properties of the item to builder and return the UpdateItemBuilder instance
                                   .ToUpdateItemBuilder()
                                   .WithEmptyDueDate()
                                   .WithPriority(4);
                                   

Please let's discuss all the stuff before implementation. Or probably we can improve your solution somehow.

Thank you for your time. 👍🏻

[olsh]

One more solution to the problem.
We can actually just use the following implementation for UpdateItem class that doesn't require any patterns and has a pretty straightforward API for the user.

    public class UpdateItem
    {
        private Duration _duration;

        private bool _durationSpecified;

        public Duration Duration
        {
            get => _duration;
            set
            {
                _duration = value;
                _durationSpecified = true;
            }
        }

        public void UnsetDuration()
        {
            _durationSpecified = false;
            Duration = null;
        }
        
        // JSON NET ShouldSerialize pattern
        internal bool ShouldSerializeDuration()
        {
            return _durationSpecified;
        }
    }

Please let me know what you think.

BTW, one more disadvantage of INonNullDefault that it's slightly inconvenient to reset value of the property to default value.

updateItem.Duration = Duration.Default;

With this, user should know about the Default property and for other objects we need to have the Default value too, again, this will be more complex with embedded types like string.
With the ShouldSerialize approach all the necessary stuff is in one place and we can apply the pattern to any type in any class.

[AhmedZaki99]

Well, it's true that the Default value approach could lead to some confusion, I faced some counter-intuitive behavior when using it in my projects.
But NewotonSoft's ConverterContractResolver was limiting my options there. I thought about using the monad pattern for a while but didn't exactly know how to instruct NewtonSoft.Json to ignore or include properties in serialization based on the data provided by the generic class, not even after reading through the documentation. Maybe because I didn't have enough experience, or maybe I needed a little more reading, but that was the furthest I reached.

The other two solutions you suggested are also great, I thought as well about using boolean flags and property setters (the third solution) but was leaving that solution as a last approach because it contains lots of boilerplate, similar to MVVM property-changed notifiers.

I had the most ambition with the Maybe<T> pattern, with ideas to add implicit conversion from and to the T type. That conversion, theoretically, could reduce the complexity of usage.
I think we might figure out a way to configure serialization for it properly if we migrated to System.Text.Json, as discussed in #48, so, I might open another PR to make this migration first and then pick up where we left off here.

As for separating Item classes passed to Create and Update methods, I agree with you that would be much better as long as this PR would be published in the next major version.

I will notify you when I start working on the System.Text.Json PR in #48 comments to prevent duplicate work, then once merged we will continue our discussion.

Thank you for the great work 👍

[olsh]

I agree that the backing field approach has much boilerplate code. But on the other hand, the approach is most straightforward for end users and the API is basic. Maybe<T> introduces complexity for the end users. This is all about the tradeoff. Either we introduce more complexity for the end user or we have to write more boilerplate on our side.
Again, the library has a pretty small codebase and the boilerplate will be in a couple of places.

I think we might figure out a way to configure serialization for it properly if we migrated to System.Text.Json, as discussed in #48, so, I might open another PR to make this migration first and then pick up where we left off here.

That would be great!

[AhmedZaki99]

@olsh
Now we have migrated to System.Text.Json, let's continue on this thread.

Since our last discussion here, I've faced this same issue with a couple of other projects, where I needed a way to send null values in a POST request to unset properties in an external API.
So, I spent a while trying and testing the different approaches we talked about, but then I settled on an approach that sounded most suitable for me: using a collection to store unset properties.

The core of this approach is the following interface:

public interface IUnsetProperties
{
    [JsonIgnore]
    HashSet<PropertyInfo> UnsetProperties { get; }
}

This interface, once implemented by a model, can be used to hold information for properties you want to unset (i.e. send null values for them), and then in a JsonTypeInfoResolver we can make it possible to include these null properties.

It's fairly easy to implement by most of the models using a base class like this:

public class ModelBase : IUnsetProperties
{
    HashSet<PropertyInfo> IUnsetProperties.UnsetProperties { get; } = [];
}

And then in a JsonTypeInfoResolver or one of its Modifiers we add the following:

public static void IncludeUnsetProperties(JsonTypeInfo typeInfo)
{
    // There's no use for this contract if the default ignore condition is set to Never.
    // Also, this contract can't handle fields when they are included.
    if (typeInfo.Options.DefaultIgnoreCondition is JsonIgnoreCondition.Never || typeInfo.Options.IncludeFields)
    {
        return;
    }
    if (!typeof(IUnsetProperties).IsAssignableFrom(typeInfo.Type))
    {
        return;
    }
    foreach (var propertyInfo in typeInfo.Properties)
    {
        propertyInfo.ShouldSerialize = (obj, value) =>
        {
            if (value != null)
            {
                // If the property has no setter delegate, it's assumed that it's a readonly property.
                return !propertyInfo.Options.IgnoreReadOnlyProperties || propertyInfo.Set != null;
            }
            return ((IUnsetProperties)obj).UnsetProperties
                .Any(name => GetPropertyName(name, typeInfo.Options) == propertyInfo.Name);
        };
    }
}

private static string GetPropertyName(PropertyInfo property, JsonSerializerOptions options)
{
    var propertyNameAttribute = property.GetCustomAttribute<JsonPropertyNameAttribute>();

    return propertyNameAttribute?.Name
        ?? options.PropertyNamingPolicy?.ConvertName(property.Name)
        ?? property.Name;
}

Finally, to make it easy to use, we add this extension method:

public static class UnsetPropertiesExtensions
{
    public static void Unset<T, TProp>(this T entity, Expression<Func<T, TProp>> propertyExpression) where T : IUnsetProperties
    {
        var propertyInfo = (propertyExpression.Body as MemberExpression)?.Member as PropertyInfo
            ?? throw new ArgumentException($"Invalid property expression: ({propertyExpression})", nameof(propertyExpression));

        // Make sure that the property is null.
        propertyInfo.SetValue(entity, null);

        entity.UnsetProperties.Add(propertyInfo);
    }
}

And usage would be like this for example:

var itemInfo = await client.Items.GetAsync(item.Id);
itemInfo.Item.Unset(i => i.DueDate);

await client.Items.UpdateAsync(itemInfo.Item);

So, what do you think about this approach? I have been using it for a while and it worked just fine.

[olsh]

So, with this approach we can call the Unset method on any property, like Id for instance? Also, we can pass any expression to the Unset method and we get ArgumentException only in runtime if the Expression is invalid?

[AhmedZaki99]

@olsh
Calling Unset on the Id property will have the same effect - an Empty id would be sent to the API - but that's up to the user. This might indeed cause problems, but there's no clear reason why users will try to Unset IDs or similar properties.

As for the possibility of passing any expression without being notified before runtime, the same issue is present with Entity Framework methods, for example, the following method call is legit at compilation:

await _dbContext.Users.Include(user => user.ToString()).ToListAsync();

But then it's easy to catch these runtime errors as we provide helpful details in the exception:

throw new ArgumentException($"Invalid property expression: ({propertyExpression})", nameof(propertyExpression));

It's not the best approach I know, but I think it's better than other approaches we have thought of.

[olsh]

Well, I think this is pretty good solution. Let's implement this.
One more suggestion. Is it possible to add restriction for TProp argument here?

public static void Unset<T, TProp>(this T entity, Expression<Func<T, TProp>> propertyExpression) where T : IUnsetProperties

[AhmedZaki99]

Well, I think this is pretty good solution. Let's implement this. One more suggestion. Is it possible to add restriction for TProp argument here?

You are right, we would need that Unset method for reference type properties only. Here's how it would look like:

public static void Unset<T, TProp>(this T entity, Expression<Func<T, TProp>> propertyExpression)
    where T : IUnsetProperties
    where TProp : class

I will start working on the new commits now.

[olsh]

@AhmedZaki99
I mean, can we add an interface (marker) for properties that we want to unset and restrict the TProp argument by the interface?
Something like

where TProp : IUnsettableProperty

[AhmedZaki99]

@olsh
We can, but we pretty much want to be able to unset any type of property inside a class implementing IUnsetProperties.
For example, we might want to unset a string property, like Item.Description for instance, in cases where empty strings might not be desired (or allowed by the API).

We also might want to unset a property of type that we don't own.

[olsh]

Good point, OK, let’s implement this as you suggested.

[AhmedZaki99]

@olsh
Before we carry on with the Unset property implementation, please review the new Item model structure for update and add methods, based on your suggestion here:

This different behavior is confusing too. And it can potentially lead to a hard-to-spot bug. For instance when you used ItemBase somewhere and later decided to use Item. Actually the ItemBase is not a good name, the Base part is meaningless for an end user. The more I think about it, the more I find that Create and Update should be different classes without inheritance.

What I did is that I used two separate classes for the Create and Update methods, named AddItem and UpdateItem respectively.
The original Item model is kept for the Get method, but it now inherits from the UpdateItem model so that it would be possible to get a task, update a property, and send it to the Update method.

You can take a look at the changed tests to see how this new design could be used with the Add and Update methods.

[AhmedZaki99]

One last thing,
I have added an extra commit to wrap all test-cleanup methods inside a try\finally blocks to ensure they get executed, for example:

var client = TodoistClientFactory.Create(_outputHelper);

var item = ...
await client.Items.AddAsync(item);
try
{
    // Run tests here.
}
finally
{
    await client.Items.DeleteAsync(item.Id);
}

[olsh]

Also, let's fix SonarQube issues.

Thank you for the great job.

[AhmedZaki99]

@olsh
It's a little bit confusing for me to find a proper way to explain this new change, your opinion\contribution here would be very helpful :)

[olsh]

It's always a good idea to include a piece of code with comments to show how the Unset method works.
Something like

// create an item with due date
...

// unset the value and update the item
...

[AhmedZaki99]

This is what I added, what do you think?

For example, the following code will not have effect starting from v7 ❌

// This code won't have an effect.
var task = new UpdateItem("TASK_ID");
task.DueDate = null;

await client.Items.UpdateAsync(task);

However, using Unset will send a due property with a null value ✔️

// This code removes a task's due date.
var task = new UpdateItem("TASK_ID");
task.Unset(t => t.DueDate);

await client.Items.UpdateAsync(task);

[olsh]

Ouch, I missed that :)

I think there is no need to give an example for the previous version. We aren't going to support it anyway.
The example for the new version is pretty much self-explanatory. 👍🏻

[AhmedZaki99]

Ok, should I then remove the ## Migrating to v7 header and comments about migration, or leave it?

[olsh]

Yes, I think we can remove the comments too.

[AhmedZaki99]

Done 👍

[olsh]

Quality Gate failed

Failed conditions B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

I think we are ready to merge the PR when the analysis will be green.

[AhmedZaki99]

I think we are ready to merge the PR when the analysis will be green.

Unfortunately, I am not aware of other ways to check if TProp is a class OR Nullable<T> type other than using this code:

default(TProp) == null

SonarCloud complains about comparing TProp to null without having a constraint to ensure it's a class.
It thinks we missed the fact that value types should not be compared to null, but instead, compared to default when we want to check whether it has a value. However, we are not checking for values, we are checking default against null on purpose!

So, SonarCloud kind of missed the point here :)

[olsh]

I think we are ready to merge the PR when the analysis will be green.

Unfortunately, I am not aware of other ways to check if TProp is a class OR Nullable<T> type other than using this code:

default(TProp) == null

SonarCloud complains about comparing TProp to null without having a constraint to ensure it's a class. It thinks we missed the fact that value types should not be compared to null, but instead, compared to default when we want to check whether it has a value. However, we are not checking for values, we are checking default against null on purpose!

So, SonarCloud kind of missed the point here :)

Let's ignore this one and fix other issues.

[AhmedZaki99]

I think we are ready to merge the PR when the analysis will be green.

Unfortunately, I am not aware of other ways to check if TProp is a class OR Nullable<T> type other than using this code:

default(TProp) == null

SonarCloud complains about comparing TProp to null without having a constraint to ensure it's a class. It thinks we missed the fact that value types should not be compared to null, but instead, compared to default when we want to check whether it has a value. However, we are not checking for values, we are checking default against null on purpose!
So, SonarCloud kind of missed the point here :)

Let's ignore this one and fix other issues.

Done 👍

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
1 Accepted issue

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

[olsh]

@AhmedZaki99 thank you! 👍🏻