Add translation of string.Join overload used with List<string> parameter, fix #3105

[georg-jung]

Not sure if I got everything right about this, but I hope it fixes #3105 :)

[georg-jung]

While this fixes the List<string> case it also enables translation of other IEnumerable<string> cases. I'm not sure, which side effects that has and if this is reasonable.

[georg-jung]

Regarding the side effects, an interesting case might be:

When changing the query from #3105 like this

 var q = ctx.Blogs.Select(b => new
 {
     b.Name,
     q = b.TagsList,
     w = b.TagsArray,
     ListTagsJoined = string.Join(", ", b.TagsList), // the translation of this seems wrong
     ArrayTagsJoined = string.Join(", ", b.TagsArray),
     b.RatingsList,
     b.RatingsArray,
-    ListRatingsJoined = string.Join(", ", b.RatingsList),
+    ListRatingsJoined = string.Join(", ", b.RatingsList.Select(x => x == 1 ? "Bad" : x == 2 ? "Quite Bad" : "Okayish or Better")),
     ArrayRatingsJoined = string.Join(", ", b.RatingsArray),
 });

the translation works in 8.0.2 and is

SELECT b."Name", b."TagsList", b."TagsArray", array_to_string(b."TagsArray", ', ', ''), b."RatingsList", b."RatingsArray", b."Id", CASE
    WHEN r.value = 1 THEN 'Bad'
    WHEN r.value = 2 THEN 'Quite Bad'
    ELSE 'Okayish or Better'
END, r.ordinality, array_to_string(b."RatingsArray", ', ', '')
FROM "Blogs" AS b
LEFT JOIN LATERAL unnest(b."RatingsList") WITH ORDINALITY AS r(value) ON TRUE
ORDER BY b."Id"

The string.Join seems to be performed client side, as with List<string> properties in 8.0.2. The new ListRatingsJoined with Select would compile to the string.Join overload this PR adds. Thus, I'm not sure if adding the overload as in this PR affects this.

[roji]

While this fixes the List case it also enables translation of other IEnumerable cases. I'm not sure, which side effects that has and if this is reasonable.

That's probably something we don't want: array_to_string() (the PG function we translate to) requires a PG array, and there can be IEnumerables which aren't that (in fact, there can technically even be .NET arrays which aren't mapped to PG arrays - though that's more rare/edge-casey). So prevent this unwanted translation, it's enough to make sure that the type mapping on the array/enumerable SqlExpression argument is an NpgsqlArrayTypeMapping - can you please add that?

The string.Join seems to be performed client side, as with List properties in 8.0.2. The new ListRatingsJoined with Select would compile to the string.Join overload this PR adds. Thus, I'm not sure if adding the overload as in this PR affects this.

Makes sense - the fact that it client-evals (no array_to_string in the actual SQL) indeed means that it's not translated, which is a good thing (since that complex LINQ expression does not represent a PG array) - there's most likely an earlier translation failure that doesn't even go into the method translator. However, it's still safer to add an explicit check here as I've suggested above - makes sense?

[roji]

This actually seems mis-named (before your PR) - the array of int is a column, not a parameter. Can you please rename to String_Join_with_array_of_int_column (also the string variant)? Also,

[georg-jung]

Not sure if something after your

Also,

was cut off?

[roji]

No, nothing important.

[roji]

👍

[georg-jung]

Sorry for the delay, I think I'll be able to look into the NpgsqlArrayTypeMapping check this weekend.

[georg-jung]

Adding the NpgsqlArrayTypeMapping check seemed easy, writing a proper test for it does not :/

• When translating calls with arbitrary IEnumerable<string>s, e.g. with a iterator function, EF just materializes it as a list and the translation/resulting SQL seems fine.
• When using something from the DB, e.g. e.IntArray.Select(x => x.ToString()) the translation fails, which seems correct, but not due to the newly added check but rather because e.IntArray.Select(x => x.ToString()) can not be translated in this context in the first place.

I didn't yet manage to construct a query where something non-ArrayTypeMapped would be passed to the string.Join translator and the translation also doesn't fail even without the check.

Adding a test like this probably doesn't make sense then, because it doesn't actually test what it is designed to test.

public class ArrayListQueryTest : ArrayQueryTest<ArrayListQueryTest.ArrayListQueryFixture>
// ...

// This test doesn't include a List<T> as the name of this class suggests.
// The overload of string.Join that is used with a List<string> argument is declared with IEnumerable<string>.
// While we want to translate that if the argument is a List<string> or similar, we can't translate
// it for arbitrary IEnumerable<string>s. We thus need to ensure that the translation fails in
// such cases.
[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
public async Task String_Join_disallow_enumerable_of_string_parameter(bool async)
{
    await AssertTranslationFailed(() => AssertQuery(
        async,
        ss => ss.Set<ArrayEntity>()
            .Where(e => string.Join(", ", e.IntArray.Select(x => x.ToString())) == "3, 4"));
}

Any suggestions what to put there? Or just leave it as is and add no test?

Edit: When executing this test, actually something non-ArrayTypeMapped is passed to the string.Join translator. arguments[1].TypeMapping then. Thus the check might change the further translation process. The translation does however fail with or without the check, because the argument can not be translated. Thus, I'm not sure if adding this test would be a good or a bad thing, given the translation this test assumes to fail fails even if the check it is testing would not be present.

[roji]

You may be able to reproduce a failure by doing string.Join over a value-converted List<string> (e.g. one where a value converter converts it to a comma-separated string); since it would be mapped to a database text rather than to a text[], the translation shouldn't kick in.

Otherwise, I think it's not critical if you can't put together a test for it - it's more of safety/correctness thing at this point (even though it may become more important in the future).

[roji]

In any case, let me know when this is ready for a new review (the Github UI allows asking for another review in the Reviewers section).

[georg-jung]

Thanks, works! Without the check, I get Npgsql.PostgresException : 42883: Funktion array_to_string(text, unknown, unknown) existiert nicht, with the check the translation fails for a value converted List<string> as expected. To add a property to ArrayEntity I think I'd need to fix all the ArrayListQueryTest/ArrayArrayQueryTest AssertSqls which seems like a rather big change (in terms of lines of code). Or, add another entity DbSet just for this purpose which maybe isn't great design?

Is there a better way? Any preference how to proceed?

[roji]

There's some recent infra in EF for doing "ad-hoc" tests, where the test defines the model inside it - that would be ideal here, but is a bigger change.

So barring that, yeah, ideally adding a value-converted array/List to ArrayQueryTest would be the way to go - it could be useful for other tests as well. It shouldn't be too hard - the main thing would be to add the new column to all the existing baselines, which can be easily done via bulk updating of the baseilnes (link, by setting the EF_TEST_REWRITE_BASELINES env var to 1 and running the tests...

[georg-jung]

Ah, that's a really handy thing I didn't know about. I think this is ready now.

[roji]

One last comment, otherwise LGTM.

[roji]

Thanks for the quality work and responsive back-and-forths on this!

[roji]

Backported to 8.0.3 via f280bb7

[georg-jung]

Thanks for the quick turnarounds and on-point feedback 👍