Using Custom TypeConverter that returns null causes dangerous/spurious CsvWriter behavior

[jzabroski]

Describe the bug
On CsvHelper 31.0.4:

When using a custom TypeConverter:

1. data can be completely omitted, causing columns to shift left one or more columns
2. data can be written out-of-order from the column headers

To Reproduce
This is not a 100% complete repro yet, but illustrative of the problem.
SodPosition is a gRPC object that I cannot concisely copy-paste. I plan to work on whittling this down to a simpler repro.
I've mangled the names a bit to abstract away what problem domain this is for.

    public class RejectedAmendment
    {
        public SodPosition SodPosition { get; set; }
        public string ValidationMessage { get; set; }

        public override string ToString()
        {
            return $"ValidationMessage: {ValidationMessage}\tSodPosition:{SodPosition}";
        }
    }

    public sealed class RejectedAmendmentClassMap : ClassMap<RejectedAmendment>
    {
        public RejectedAmendmentClassMap()
        {
            Map(x => x.SodPosition.S).Name("S");
            Map(x => x.SodPosition.F).Name("F");
            Map(x => x.SodPosition.P).Name("P");
            Map(x => x.SodPosition.PositionGroup).Name("PositionGroup");
            Map(x => x.SodPosition.AType).Name("AType");
            Map(x => x.SodPosition.OptionalQuantityCase, false).Name("Quantity").TypeConverter<OptionalQuantityOneofCaseTypeConverter>();
/*
// This works fine
.Convert(args =>
            {
                return (args.Value.SodPosition.GetQuantity() ?? 0).ToString(CultureInfo.InvariantCulture);
            });*/
            Map(x => x.SodPosition.C, false).Name("C");
            Map(x => x.SodPosition.PR, false).Name("PR");
            Map(x => x.SodPosition.FXR, false).Name("FXR");
            Map(x => x.ValidationMessage).Name("ValidationMessage");
        }
    }

    public class OptionalQuantityOneofCaseTypeConverter : ITypeConverter
    {
        public object ConvertFromString(string text, IReaderRow row, MemberMapData memberMapData)
        {
            throw new System.NotImplementedException();
        }

        public string ConvertToString(object value, IWriterRow row, MemberMapData memberMapData)
        {
            if (value is SodPosition sodPosition)
            {
                return (sodPosition.GetQuantity() ?? 0).ToString(CultureInfo.InvariantCulture);
            }

            return null;
        }
    }


protected string WriteCsv<T, TClassMap>(ICollection<T> data, string folder, string filename)
    where TClassMap : ClassMap<T>
{
    if (data == null) throw new ArgumentNullException(nameof(data));

    if (!data.Any())
    {
        throw new Exception($"data missing for {typeof(T)}");
    }

    if (string.IsNullOrWhiteSpace(filename)) throw new ArgumentNullException(nameof(filename));

    var path = GetFullPath(folder, filename);

    if (path == null) throw new ArgumentNullException(nameof(path));

    if (_fileSystem.File.Exists(path))
    {
        throw new Exception($"File with name [{filename}] already exists at [{path}].");
    }

    using (var writer = new StreamWriter(path))
    {
        using (var csvWriter = new CsvWriter(writer, GetCsvConfiguration()))
        {
            csvWriter.Context.RegisterClassMap<TClassMap>();
            csvWriter.WriteHeader<T>();
            csvWriter.NextRecord();

            foreach (var record in data)
            {
                csvWriter.WriteRecord(record);
                csvWriter.NextRecord();
            }
        }
    }

    _log.Debug($"Saved file to path [{path}]");
    return path;
}

Expected behavior
Data to be written to the correct columns.

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
I experimented with the useExistingMap overload of Map(x => x...) to see if that had any bearing. It had a slight bearing in that if I also re-arranged a few columns, I could get the problem to improve (columns stopped appearing out of order), but the FXR column would still not write to the file.

[fen89]

@jzabroski We experienced this bug as well. After we made sure all our custom converters return an empty string instead of null the problem with the column shifting disappeared.

Just as a reference, a simple one:

BEFORE:

public class TrimConverter : StringConverter
{
    public override string ConvertToString(object? value, IWriterRow row, MemberMapData memberMapData)
    {
        return value?.ToString()?.Trim();
    }
}

AFTER:

public class TrimConverter : StringConverter
{
    public override string ConvertToString(object? value, IWriterRow row, MemberMapData memberMapData)
    {
        return value?.ToString()?.Trim() ?? "";
    }
}

[jzabroski]

I updated the issue title to reflect your findings. Thank you.

[jzabroski]

@JoshClose Did you fix anything to address this recently?

@fen89 I have written a unit test to capture your findings, but can no longer reproduce the bug on CsvHelper 33.0.1. However, the below test also passes on 31.0.4, where I initially reported the bug.

Below is a nearly fully complete MRE - I don't supply the code to Fixture here, but it is an instance of AutoFixture's IFixture used to facilitate object scaffolding for testing.

        [Theory]
        [InlineData("Foo", "")]
        [InlineData("Foo", "Baz")]
        [InlineData("Foo", null)]
        [InlineData("", "")]
        [InlineData("", "Baz")]
        [InlineData("", null)]
        [InlineData(null, "")]
        [InlineData(null, "Baz")]
        [InlineData(null, null)]
        public void TestIntegrationWithTheRestOfCsvHelper(string firstColumnValue, string thirdColumnValue)
        {
            var rows = Fixture.Build<TestRow>()
                .With(x => x.NonNullValue, firstColumnValue)
                .With(x => x.NullableValue, new TestSubRow())
                .With(x => x.NonNullValue2, thirdColumnValue)
                .CreateMany()
                .ToList();

            var sb = new StringBuilder();
            using (var csvWriter =
                   new CsvWriter(new StringWriter(sb), CultureInfo.InvariantCulture, false))
            {
                csvWriter.Context.RegisterClassMap<TestRowClassMap>();
                csvWriter.WriteRecords(rows);
                
            }

            Assert.Equal($@"NonNullValue,NullValue,NonNullValue2
{firstColumnValue},,{thirdColumnValue}
{firstColumnValue},,{thirdColumnValue}
{firstColumnValue},,{thirdColumnValue}
", sb.ToString());
        }


        private class TestRow
        {
            public string NonNullValue { get; set; }
            public TestSubRow NullableValue { get; set; }
            public string NonNullValue2 { get; set; }
        }

        private class TestSubRow
        {
            public string NullValue { get; set; }
        }

        private class TestRowClassMap : ClassMap<TestRow>
        {
            public TestRowClassMap()
            {
                Map(x => x.NonNullValue);
                Map(x => x.NullableValue.NullValue).TypeConverter<TrimConverter>();
                Map(x => x.NonNullValue2);
            }
        }

// Taken from @fen89 reply above - this version returns null if the string property is null
public class TrimConverter : StringConverter
{
    public override string ConvertToString(object? value, IWriterRow row, MemberMapData memberMapData)
    {
        return value?.ToString()?.Trim();
    }
}

[jzabroski]

@JoshClose OK, this test fails consistently now, on 31.0.4-33.0.1 (current version on nuget.org). Other than the instance of IFixture from AutoFixture, it's self-contained repro. You can just add the AutoFixture nuget package and set:

public IFixture Fixture => new Fixture();

as a property

        public class RejectedAmendment
        {
            public SodPosition SodPosition { get; set; }
            public string ValidationMessage { get; set; }

            public override string ToString()
            {
                return $"ValidationMessage: {ValidationMessage}\tSodPosition:{SodPosition}";
            }
        }

        public sealed class RejectedAmendmentClassMap : ClassMap<RejectedAmendment>
        {
            public RejectedAmendmentClassMap()
            {
                Map(x => x.SodPosition.S).Name("S");
                Map(x => x.SodPosition.F).Name("F");
                Map(x => x.SodPosition.P).Name("P");
                Map(x => x.SodPosition.PositionGroup).Name("PositionGroup");
                Map(x => x.SodPosition.AType).Name("AType");
                Map(x => x.SodPosition.OptionalQuantityCase, false).Name("Quantity").TypeConverter<OptionalQuantityOneofCaseTypeConverter>();
                /*
                // This works fine
                .Convert(args =>
                            {
                                return (args.Value.SodPosition.Quantity ?? 0).ToString(CultureInfo.InvariantCulture);
                            });*/
                Map(x => x.SodPosition.C, false).Name("C");
                Map(x => x.SodPosition.PR, false).Name("PR");
                Map(x => x.SodPosition.FXR, false).Name("FXR");
                Map(x => x.ValidationMessage).Name("ValidationMessage");
            }
        }

        public class SodPosition
        {
            public string S { get; set; }
            public string F { get; set; }
            public string P { get; set; }
            public string PositionGroup { get; set; }
            public string AType { get; set; }
            public OptionalQuantityCase OptionalQuantityCase { get; set; }
            public string C { get; set; }
            public decimal PR { get; set; }
            public decimal FXR { get; set; }
            public decimal? Quantity { get; set; }
        }

        public enum OptionalQuantityCase
        {
            A,
            B,
            C
        }

        public class OptionalQuantityOneofCaseTypeConverter : ITypeConverter
        {
            public object ConvertFromString(string text, IReaderRow row, MemberMapData memberMapData)
            {
                throw new NotImplementedException();
            }

            public string ConvertToString(object value, IWriterRow row, MemberMapData memberMapData)
            {
                //if (value is SodPosition sodPosition)
                //{
                //    return (sodPosition.Quantity ?? 0).ToString(CultureInfo.InvariantCulture);
                //}

                return null;
            }
        }

        [Fact]
        public void TestCsvWriterWithDottedLambdaClassMap()
        {
            var rows = Fixture.Build<RejectedAmendment>()
                .With(x => x.SodPosition,
                    Fixture.Build<SodPosition>()
                        .With(y => y.S, "S")
                        .With(y => y.F, "F")
                        .With(y => y.P, "P")
                        .With(y => y.PositionGroup, "PositionGroup")
                        .With(y => y.AType, "AType")
                        .With(y => y.C, "C")
                        .With(y => y.PR, 1.0M)
                        .With(y => y.FXR, 2)
                        .With(y => y.OptionalQuantityCase)
                        .Create())
                .With(x => x.ValidationMessage, string.Empty)
                .CreateMany()
                .ToList();

            var results = WriteCsv<RejectedAmendment, RejectedAmendmentClassMap>(rows);

            Assert.Equal(@"S,F,P,PositionGroup,AType,Quantity,C,PR,FXR,ValidationMessage
S,F,P,PositionGroup,AType,,C,1.0,2.0,
", results);
        }

        protected string WriteCsv<T, TClassMap>(ICollection<T> data)
            where TClassMap : ClassMap<T>
        {
            if (data == null) throw new ArgumentNullException(nameof(data));

            if (!data.Any())
            {
                throw new Exception($"data missing for {typeof(T)}");
            }

            var sb = new StringBuilder();

            using (var writer = new StringWriter(sb))
            {
                using (var csvWriter = new CsvWriter(writer, new CsvConfiguration(CultureInfo.InvariantCulture)))
                {
                    csvWriter.Context.RegisterClassMap<TClassMap>();
                    csvWriter.WriteHeader<T>();
                    csvWriter.NextRecord();

                    foreach (var record in data)
                    {
                        csvWriter.WriteRecord(record);
                        csvWriter.NextRecord();
                    }
                }
            }

            return sb.ToString();
        }

[jzabroski]

I think the problem is that ITypeConverter seems to still allow nullable objects, so there is no way to warn the user that this is invalid:

The other unsolved problem is why is this only happening some of the time, and not all of the time, as my previous unit test demonstrates.

[jzabroski]

This is an even better view of what is happening, using sharplab.io:

[jzabroski]

@JoshClose This is as small a repro as I can figure out so far.

        public IFixture Fixture => new Fixture(); // AutoFixture

        public class RejectedAmendment
        {
            public SodPosition SodPosition { get; set; }
        }

        public sealed class RejectedAmendmentClassMap : ClassMap<RejectedAmendment>
        {
            public RejectedAmendmentClassMap()
            {
                Map(x => x.SodPosition.S).Name("S");
                Map(x => x.SodPosition.OptionalQuantityCase, false).Name("Quantity").TypeConverter<NullConverter>();
                /*
                // This works fine
                .Convert(args =>
                            {
                                return (args.Value.SodPosition.Quantity ?? 0).ToString(CultureInfo.InvariantCulture);
                            });*/
                Map(x => x.SodPosition.C, false).Name("C");
            }
        }

        public class SodPosition
        {
            public string S { get; set; }
            public int ConvertibleValue { get; set; }
            public string C { get; set; }
            public decimal? Quantity { get; set; }
        }

        public class NullConverter : ITypeConverter
        {
            public object ConvertFromString(string text, IReaderRow row, MemberMapData memberMapData)
            {
                throw new NotImplementedException();
            }

            public string ConvertToString(object value, IWriterRow row, MemberMapData memberMapData)
            {
                return null;
            }
        }

        [Fact]
        public void TestCsvWriterWithDottedLambdaClassMap()
        {
            var rows = Fixture.Build<RejectedAmendment>()
                .With(x => x.SodPosition,
                    Fixture.Build<SodPosition>()
                        .With(y => y.S, "S")
                        .With(y => y.C, "C")
                        .With(y => y.ConvertibleValue)
                        .Create())
                .CreateMany()
                .ToList();

            var results = WriteCsv<RejectedAmendment, RejectedAmendmentClassMap>(rows);

            Assert.Equal(@"S,Quantity,C
S,,C
S,,C
S,,C
", results);
        }

        protected string WriteCsv<T, TClassMap>(ICollection<T> data)
            where TClassMap : ClassMap<T>
        {
            var sb = new StringBuilder();

            using (var writer = new StringWriter(sb))
            {
                using (var csvWriter = new CsvWriter(writer, new CsvConfiguration(CultureInfo.InvariantCulture)))
                {
                    csvWriter.Context.RegisterClassMap<TClassMap>();
                    csvWriter.WriteHeader<T>();
                    csvWriter.NextRecord();

                    foreach (var record in data)
                    {
                        csvWriter.WriteRecord(record);
                        csvWriter.NextRecord();
                    }
                }
            }

            return sb.ToString();
        }