EnumerableRetriever does not play together with NullValueRetriever

[dotnetnico]

SpecFlow Version

3.9.74

Which test runner are you using?

xUnit

Test Runner Version Number

3.9.74

.NET Implementation

.NET 6.0

Project Format of the SpecFlow project

Sdk-style project format

.feature.cs files are generated using

SpecFlow.Tools.MsBuild.Generation NuGet package

Test Execution Method

Visual Studio Test Explorer

SpecFlow Section in app.config or content of specflow.json

No response

Issue Description

It seems like the NullValueRetriever is ignored by the EnumerableRetriever when using nullable types in enumerables in tabular data.

In short I want to be able to write this:

Given some tabular data:
| StringValue    | NullableEnumerableValue |
| ArbitraryValue | 1, 2, <null>            |

But instead have to write this:

Given some tabular data:
| StringValue    | NullableEnumerableValue |
| ArbitraryValue | 1, 2,                   |

Steps to Reproduce

Given a feature step that contains tabular data with an enumerable with a nullable type:

Given some tabular data:
| StringValue    | NullableEnumerableValue |
| ArbitraryValue | 1, 2, <null>            |

The type to deserialize to:

public class ExampleClass
{
    public string StringValue { get; set; }
    
    public IList<int?> NullableEnumerableValue { get; set; } = new List<int?>;
}

Given there is a NullValueRetriever registered:

[BeforeTestRun]
public static void BeforeTestRun()
{
    Service.Instance.ValueRetrievers.Register(new NullValueRetriever("<null>"));
}

I expect the result of this to contain Enumerable<int?> { 1,2, null }

 var exampleData = table.CreateSet<ExampleClass>();

However, it doesn't. The actual result is Enumerable<int?> { 1,2, 0 }, Seems like the NullValueRetriever is ignored by the EnumerableRetriever.

I have to write this to get a null value:

Given some tabular data:
| StringValue    | NullableEnumerableValue |
| ArbitraryValue | 1, 2,                   |

Personally I feel that is not very readable.

Link to Repro Project

No response

[clrudolphi]

The EnumerableValueRetriever (which processes lists/collections) makes an assumption that every item in the list can be processed by the same underlying value retriever. So when the Value string has "1, <null>", the Enumerable retriever uses the Int retriever over and over as it is the retriever established for the first item in the list (subsequently, the NullValueRetriever is never searched for, and never invoked).

My assumption is that this was done for performance reasons so that the search for the underlying retriever would not be repeated needlessly (under most circumstances).

@SabotageAndi - would you consider a PR that removes the null coalescing assignment operator?

Code in question:

https://github.com/SpecFlowOSS/SpecFlow/blob/77d8ad7f17dd9b03251a91ed9e12758ab12fb545/TechTalk.SpecFlow/Assist/ValueRetrievers/EnumerableValueRetriever.cs#L37

      private IEnumerable GetItems(string[] strings, KeyValuePair<string, string> keyValuePair, Type targetType, Type itemType)
        {
            IValueRetriever retriever = null;
            foreach (var splitValue in strings)
            {
                var itemKeyValuePair = new KeyValuePair<string, string>(keyValuePair.Key, splitValue.Trim());
                retriever ??= GetValueRetriever(itemKeyValuePair, targetType, itemType);
                yield return retriever?.Retrieve(itemKeyValuePair, targetType, itemType);
            }
        }

[bollhals]

@SabotageAndi Maybe continue here the discussion instead of polluting the PR.

What's the definition of a ValueRetriever? Is the expectation that there is one for a type, or should there be multiple allowed? (If it is the latter, can you shed some light on some usecases?

Why am I asking? => If there should only be one, then the PR fixes it the wrong way. If there should be multiple, then the PR is fine. But I'm then questioning the reasoning behind the multiple ones. :)

[clrudolphi]

I don't have the background that you all do, so I'll defer to you on the original intent.

I can imagine the input being quite messy with special values that don't map/parse cleanly to values of the type system. This would imply that we should be open to the idea of having multiple value retrievers for any given Property Type.

One way, perhaps, to think of the NullValueRetriever, is as a special case of the more generic problem of recognizing special tokens in the enumerable and then replacing them with values that match the Property Type of the Enumerable. For example, a recognizer/s that replaces all instances of the string "Foo" with 0, and "Bar" with 1. The string values in the input list are supposed to make sense to the business, not necessarily perfectly align with the language's type system.

An alternative design, then, would be to restrict these components as strict recognizers, not full-fledged Value Retrievers. Place these recognizers up front in the pipeline and run them across all elements, substituting appropriate 'real' values, then run the built-in Value Retrievers against these values.

Another alternative that I believe would meet @bollhals's intent, is to design the NullValueRetriever as a wrapper instead of a full-fledged ValueRetriever. It would defer recognition of the special value until the time of retrieval, substitute in a different value, then delegate to the built-in retriever that is appropriate for that Type. The null-coalescing logic of the existing EnumerableValueRetriever would then be correct and could stay in place. In the example of the NullValueRetriever<Int?>, the NullValueRetriever CanRetrieve method would return true if the Property Type matched Int?. Thus, it would be used for every element in a List<Int?> regardless of whether the trigger value was the first item in the list or the last.

[SabotageAndi]

I don't think there is an actual definition of a ValurRetriever or @gasparnagy?
For me, it is a functionality that makes it easier to convert datatables to DTOs/POCOs. I wrote too much code that uses the Row objects directly. It is really boring code to write.

If a ValueRetriever calls another ValueRetriever is a case, that is fine for me. It's all about making it easier.

[SabotageAndi]

#2638 is merged

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.