Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for code generation for List<X> types #2402

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

yinzara
Copy link

@yinzara yinzara commented Jun 10, 2022

Fixes #2401

@yinzara
Copy link
Author

yinzara commented Jun 10, 2022

I also added a few additional tests to verify that array type columns are being generated properly as well as inserting default and HasData as I could not find any

@roji
Copy link
Member

roji commented Jun 10, 2022

This seems like it adds support for new[] { "1", "2" }.ToList(), but what we want is new List<string> { "1", "2" }, no?

@yinzara
Copy link
Author

yinzara commented Jun 10, 2022

lol I agree yes that's exactly what I did.

Let me fix that. I didn't realize there was a ListInit expresion function. I was having trouble finding it but:
https://docs.microsoft.com/en-us/dotnet/api/system.linq.expressions.expression.listinit?view=net-6.0

there it is :)

@yinzara
Copy link
Author

yinzara commented Jun 10, 2022

Looks like there's a problem with that. When I use the following code:

public override Expression GenerateCodeLiteral(object value)
    {
        var values = (System.Collections.IList)value;
        List<Expression> elements = new(values.Count);
        var generated = true;
        foreach (var element in values)
        {
            if (generated)
            {
                try
                {
                    elements.Add(ElementMapping.GenerateCodeLiteral(element)); // attempt to convert if required
                    continue;
                }
                catch (NotSupportedException)
                {
                    generated = false; // if we can't generate one element, we probably can't generate any
                }
            }
            elements.Add(Expression.Constant(element));
        }
        return Expression.ListInit(Expression.New(typeof(List<>).MakeGenericType(ElementMapping.ClrType)), elements);
    }

I get the error:

The literal expression 'new List`1() {Void Add(Int32)(1), Void Add(Int32)(2), Void Add(Int32)(3)}' for 'List<int>' cannot be parsed. Only simple constructor calls and factory methods are supported.

so it looks like we might have to stick with the array initializer calling ToList() on it.

@roji
Copy link
Member

roji commented Jun 11, 2022

IIRC this is why I originally implemented array literals but not List...

I think we should simply add support for this on the EF side - it shouldn't be too hard. Then we'd be able to generate the right thing here.

@yinzara
Copy link
Author

yinzara commented Jun 11, 2022

I certainly don't mind doing that either. If you have a hint of where you think it might be it would be exceedingly helpful.

@yinzara
Copy link
Author

yinzara commented Jun 13, 2022

Alrighty. I think what I've done should fix this issue. In fact, it might just fix it without the code in this PR since I've now added support for List literals in EF Core but I'm going to test locally and see what the outcome is.

@yinzara
Copy link
Author

yinzara commented Jun 13, 2022

Yup.... this PR isn't even necessary once the other PR to EFCore is merged.

All my test cases I wrote as part of this PR now pass without the code changes to NgpgsqlArrayTypeMapping or NgpgsqlArrayListTypeMapping if I change my local NuGet to point to the compiled source from the other PR.

Would you like to me to just close this PR completely or leave it open with just the tests pending the other PR being merged so that we have additional tests for these cases?

yinzara added a commit to yinzara/efcore that referenced this pull request Jun 13, 2022
@roji
Copy link
Member

roji commented Jun 13, 2022

Thanks @yinzara, yeah - having tests here would be a good thing. Once the support is merged EF-side, I'll sync here to use the latest version and we can merge tests.

yinzara added a commit to yinzara/efcore that referenced this pull request Jun 13, 2022
yinzara added a commit to yinzara/efcore that referenced this pull request Jun 13, 2022
yinzara added a commit to yinzara/efcore that referenced this pull request Jun 13, 2022
ghost pushed a commit to dotnet/efcore that referenced this pull request Jun 27, 2022
* Add support for CSharpHelper for List literals

Fixes #19274

Also relates to npgsql/efcore.pg#2402

* Fixes from review comments
@roji
Copy link
Member

roji commented Jul 9, 2022

@yinzara the main branch of EFCore.PG has been synced to the latest EF Core daily build, which contains your List support from dotnet/efcore#28212. So you can add tests now.

@roji
Copy link
Member

roji commented Aug 1, 2022

@yinzara are you planning to add the appropriate tests here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support HasData for List<X> type fields
2 participants