JsonConverter doesn't allow injection of parameters

[fbrum]

I wanted to create a converter for DateTimeOffset and having the date format being injected from configuration.
But I came to conclude that is not possible. I created a base converter

public abstract class ConvertNullableDateTime : JsonConverter<DateTimeOffset?>
{
     protected abstract string Format { get; }
     protected abstract TimeSpan Offset { get; }

     public override DateTimeOffset? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
     {
         return DateTime.Parse(reader.GetString());
     }
     public override void Write(Utf8JsonWriter writer, DateTimeOffset? value, JsonSerializerOptions options)
     {
         writer.WriteStringValue(value?.ToOffset(Offset).ToString(Format));
     }
}

And then I defined a specific converter:

public class ToNullableDateTimeOffset : ConvertNullableDateTime
{
    public ToNullableDateTimeOffset()
    {
        Format = String.Empty;
        Offset = new TimeSpan();
    }

    public ToNullableDateTimeOffset(string dateTimeFormat, TimeSpan offset)
    {
        Format = dateTimeFormat;
        Offset = offset;
    }

    protected override string Format { get; }
    protected override TimeSpan Offset { get; }
}

Added the serializer with the converter to the collection:

self.AddSingleton<ISerializer>( provider => 
{
    var timeSpan = new TimeSpan(clientOptions.Offset);
    SerializerSettings.Converters.Add(new ToNullableDateTimeOffset(clientOptions.DateTimeFormat!, timeSpan));

    return new JsonNetSerializer(provider.GetService<ILogger<JsonNetSerializer>>(), SerializerSettings);
});

I had to create a empty constructor because otherwise I would have a NullException while serializing.
And the constructor with the injected properties are not used. on the serialization.

I searched arround but was not able to find anything solution.
Could this be done for a future version?
Thanks

PS. Sorry but I'm not able to get the code formated correctly

[layomia]

@fbrum, can you share a small complete repro (e.g. a project, sanitized if necessary) that highlights the issue? Given this line where you add your converter to the global converters list, it is not clear to me why your specified options are not being honored.

SerializerSettings.Converters.Add(new ToNullableDateTimeOffset(clientOptions.DateTimeFormat!, timeSpan));

I had to create a empty constructor because otherwise I would have a NullException while serializing.

How is this NullReferenceException being experienced? Can you share a code sample and/or stack trace that highlights this error?

[fbrum]

Hi @layomia ,
Thanks for the response.

Just realized something!
My constructor with the options are not being ignored in fact... So the title of this ticket is wrong.
My problem is that I have several fields DateTimeOffset?, but that needs two different conversions.
So the above code (that for some reason wasn't able to format, sorry for that) is in fact correct.

The problem is that on the contract I will have something like this example:

public class Buyer
{
    [JsonPropertyName("buyerBirthDate")]
    [JsonConverter(typeof(NullableToDateTimeFormat))]
    public DateTimeOffset? DateOfBirth { get; set; }

    [JsonPropertyName("buyingDateTime")]
    [JsonConverter(typeof(NullableToDateTimeFormat))]
    public DateTimeOffset? BuyingDateTime { get; set; }
}

Now, if I don't specify the JsonConverter attribute there is no problem.
But since one only needs year-month-day and the other one a complete date format to the millisecond and offset, I'm forced to add that attribute.

Given that we will have the following: #399
The exception comes exacly on L208-L209

So, I thought that if the converter was already created and added to the Converters list, it would use that one and not instantiate a new one.
So that is why this is being ignored
SerializerSettings.Converters.Add(new ToNullableDateTimeOffset(clientOptions.DateTimeFormat!, timeSpan));

So I believe the suggestion here is to:
On GetConverterFromAttribute, load the convertert from the list is exists.

[layomia]

@fbrum

Given that we will have the following: #399
The exception comes exacly on L208-L209

Thanks, this issue has been fixed in 5.0 - #528

So, I thought that if the converter was already created and added to the Converters list, it would use that one and not instantiate a new one.
So that is why this is being ignored
SerializerSettings.Converters.Add(new ToNullableDateTimeOffset(clientOptions.DateTimeFormat!, timeSpan));

So I believe the suggestion here is to:
On GetConverterFromAttribute, load the convertert from the list is exists.

Choosing a converter from the Converters list over the converter specified directly on the attribute violates the established converter registration precedence implemented by the serializer.

Does the solution of adding a public parameterless constructor to the converter work for you? If I understand your scenario correctly, I think this would mean that the properties that the converter is placed would be serialized without custom formatting or a custom offset. Is this the desired behavior?

But since one only needs year-month-day and the other one a complete date format to the millisecond and offset, I'm forced to add that attribute.

Given the above Buyer class, do both properties need the converter attribute, or just one of them?

[fbrum]

Does the solution of adding a public parameterless constructor to the converter work for you? If I understand your scenario correctly, I think this would mean that the properties that the converter is placed would be serialized without custom formatting or a custom offset. Is this the desired behavior?

Correct. It does work but without the custom formatting. For now the solution is to leave it hard coded.

Given the above Buyer class, do both properties need the converter attribute, or just one of them?

Yes. The current scenario we have both cases.

Choosing a converter from the Converters list over the converter specified directly on the attribute violates the established converter registration precedence implemented by the serializer.

I understand, but my suggestion is to use the converter from attribute and when instantiating, instead of calling a default empty constructor, it could first see if there is an instance of the type in the attribute on the converters list.

[layomia]

@fbrum

I understand, but my suggestion is to use the converter from attribute and when instantiating, instead of calling a default empty constructor, it could first see if there is an instance of the type in the attribute on the converters list.

How would you describe the logical reasoning for doing this? Why is the converter attribute on the property if it should lose out to a converter on the Converters list ("global" converter)? In your experience, do you have many scenarios where this is useful? It seems that this request is covering the scenario where there may or may not be a global converter for the property's type, and if present, it is preferred over a local converter on the property. This seems very niche. I wonder why the workaround you landed on is not sufficient.

Perhaps one reason for using a global converter over a converter on a property is to support overriding the (de)serialization behavior for a property on non-owned type. This is something we haven't received much feedback about.

FWIW - another tool available if you need to pass state to a parameterized ctor of a converter on a property is to pass the type of a JsonConverterFactory on the JsonConverterAttribute. Not sure if this helps your specific scenario since you may not have the required clientOptions and timeSpan.

public class Buyer
{
    [JsonPropertyName("buyerBirthDate")]
    [JsonConverter(typeof(NullableDateTimeConverterFactory))]
    public DateTimeOffset? DateOfBirth { get; set; }
}

public class NullableDateTimeConverterFactory : JsonConverterFactory
{
    public override bool CanConvert(typeToConvert) => typeToConvert == typeof(DateTimeOffset?);

    public override JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions options) =>
        new ToNullableDateTimeOffset(...);
}

[fbrum]

How would you describe the logical reasoning for doing this? Why is the converter attribute on the property if it should lose out to a converter on the Converters list ("global" converter)?

I see your point about the hierarchy. But still the possibility of dependency injection for a attribute converter presides.
I didn't do any work around since it is simply not possible.

It is an interesting case but maybe not so important.
The converters list is limited to converting base on type. On this specific scenario we have same type with a need of different conversions.
On the other hand a converter per attribute is only instantiated with a default empty constructor. Not making it possible to do dependency injection.

I didn't look into it, but maybe the converter per type could look into the property name or attribute to decide the conversion.
It's not so elegant though.

So yes, the reasoning for an attribute converter to do a look up on the Converters list is to enable Dependency injection.
But the converter defined per attribute will always override a converter per type.

[eiriktsarpalis]

I see your point about the hierarchy. But still the possibility of dependency injection for a attribute converter presides.
I didn't do any work around since it is simply not possible.

A common approach I use is to define a derived class that hardcodes constructor parameters, for example

public class CamelCaseStringEnumConverter : JsonStringEnumConverter
{
    public CamelCaseStringEnumConverter() : base(namingPolicy: JsonNamingPolicy.CamelCase)
    {
    }
}

public class MyPoco
{
    [JsonConverter(typeof(JsonStringEnumConverter))]
    public Enum PropA { get; }

    [JsonConverter(typeof(CamelCaseStringEnumConverter))] // change parameters for specific property
    public Enum PropB { get; }

    public enum Enum { A, B, C }
}

If your DI needs are scoped to property definitions, then this approach should suffice. If instead you're scoping it per serialization, it might make sense to take a look at the proposals discussed in #34773.