Replace collection length checks for compatibility

[jake-carpenter]

• Fixes: Compiler errors in C# client generated code when request has a List<T> as path parameter #4672
• Repro: https://github.com/jake-carpenter/nswag-4672-repro
• Cause: 961cb7b#diff-416f8759e9ce0cecba7578a64bf1fc4b9fee9325d331641eaf7b07d62e7455bb

Using the Length property assumes the collection will be a C# array, but that is based on the parameterArrayType setting for the C# client generator. By default, IEnumerable is used and the majority of other collection types won't accept Length.

For now, this fix utilizes the LINQ Count(), which theoretically should check for Length or Count properties on it's own before enumerating. This seems like it is still an improvement to the enumeration that was present before the linked changeset. I'm open to solving this a different way if there are ways to access the C# type during generation.

There are likely other templates where this is also a problem, but this fix is focused on path parameter generation.

[jake-carpenter]

Build failures seem to be based on new mac images with .NET 6 not installed, which is probably not something I should address with code here.

[MoeHamdan]

Hello

This works fine only if "Parameter Type" for "Generic Array Type" is set to IEnumerable, however if it set to ICollection it will fail. And by fail I mean it will generate Count() and ElementAt() which are not supported by ICollection

The only way I was able to fix this is by adding namespance System.Linq

[Khyalis]

I might be a bit late to the discussion.

What I do not understand about the change is, why for some cases (IsDateTimeArray , IsDateArray) that moved from .Length to .Count(), the indexed access remained valid, while for others, it needed to be replaced with .ElementAt().
Does this mean for these cases only types with an index access signature will ever be used?

Also, if targeting IEnumerable as the lowest common denominator (and not trying to differentiate between actual types / targeted frameworks), it might be worthwile to take into consideration that the for loop and its index variable may not be optimal.

• The main purpose of the index variable appears to be to differentiate between the first and subsequent loop iterations. Otherwise, the intention appears to be to access all collection elements in order.
• The loop condition (including the .Count()) call may be evaluated for every loop iteration, possibly causing multiple enumerations of the collection.
• Accessing the "Current" element using .ElementAt() may also need to iterate over all previous Elements

Using a foreach-loop would also work with IEnumerable would also serve to access all collection elements in order; it would only need to iterate over the collection once.
Although additional code might be needed in order to recognize the first element, such as

bool isAfterFirst = false;
foreach (var item in {{ parameter.VariableName }})
{
    if (isAfterFirst) urlBuilder_.Append(',');
    urlBuilder_.Append(ConvertToString(item, System.Globalization.CultureInfo.InvariantCulture));
    isAfterFirst = true;
}

(Working with the enumerator directly (e.g. in order to avoid an additional variable) may not be feasible, with regards to foreach's special treatment of enumerators for which implicit conversions to IDisposable exist, given being oblivious to the actual type in use.)

[lahma]

@Khyalis maybe you can propose a PR with foreach then? I think the isAfterFirst might also require postfixing with variable name to ensure no conflicts can happen.

[Khyalis]

@lahma You're right, depending on whether the parameter is optional, the template may be used outside an isolated block.
Instead of using individual variable names, I could imagine declaring nested blocks (if this does not violate a style guide).

{
   var variableName = ...;
}

(Ensuring a block in Client.Class.liquid might also work.)

I added #5149.