Skip to content

Fix list type mapping #542

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.

Sign up for GitHub

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

Merged
austindrenski merged 1 commit into from
Jul 25, 2018
Merged

Fix list type mapping #542

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
99 changes: 60 additions & 39 deletions src/EFCore.PG/Storage/Internal/Mapping/NpgsqlListTypeMapping.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#region License

// The PostgreSQL License
//
// Copyright (C) 2016 The Npgsql Development Team
Expand All @@ -19,9 +20,11 @@
// AND FITNESS FOR A PARTICULAR PURPOSE. THE SOFTWARE PROVIDED HEREUNDER IS
// ON AN "AS IS" BASIS, AND THE NPGSQL DEVELOPMENT TEAM HAS NO OBLIGATIONS
// TO PROVIDE MAINTENANCE, SUPPORT, UPDATES, ENHANCEMENTS, OR MODIFICATIONS.

#endregion

using System;
using System.Collections;
using System.Text;
using Microsoft.EntityFrameworkCore.Storage;
using Microsoft.EntityFrameworkCore.Storage.ValueConversion;
Expand All @@ -32,53 +35,60 @@
namespace Npgsql.EntityFrameworkCore.PostgreSQL.Storage.Internal.Mapping
{
/// <summary>
/// Maps PostgreSQL arrays to .NET List{T}.
/// Maps PostgreSQL arrays to <see cref="List{T}"/>.
/// </summary>
public class NpgsqlListTypeMapping : RelationalTypeMapping
{
/// <summary>
/// The CLR type of the list items.
/// </summary>
public RelationalTypeMapping ElementMapping { get; }

/// <summary>
/// Creates the default array mapping (i.e. for the single-dimensional CLR array type)
/// Creates the default list mapping.
/// </summary>
public NpgsqlListTypeMapping(RelationalTypeMapping elementMapping, Type listType)
: this(elementMapping.StoreType + "[]", elementMapping, listType)
{}
: this(elementMapping.StoreType + "[]", elementMapping, listType) {}

/// <inheritdoc />
NpgsqlListTypeMapping(string storeType, RelationalTypeMapping elementMapping, Type listType)
: base(new RelationalTypeMappingParameters(
new CoreTypeMappingParameters(listType, null, CreateComparer(elementMapping, listType)), storeType
))
{
ElementMapping = elementMapping;
}
: base(
new RelationalTypeMappingParameters(
new CoreTypeMappingParameters(listType, null, CreateComparer(elementMapping, listType)), storeType))
=> ElementMapping = elementMapping;

/// <inheritdoc />
protected NpgsqlListTypeMapping(RelationalTypeMappingParameters parameters, RelationalTypeMapping elementMapping)
: base(parameters) {}
: base(parameters)
=> ElementMapping = elementMapping;

/// <inheritdoc />
public override RelationalTypeMapping Clone(string storeType, int? size)
=> new NpgsqlListTypeMapping(StoreType, ElementMapping, ClrType);

/// <inheritdoc />
public override CoreTypeMapping Clone(ValueConverter converter)
=> new NpgsqlListTypeMapping(Parameters.WithComposedConverter(converter), ElementMapping);

/// <inheritdoc />
protected override string GenerateNonNullSqlLiteral(object value)
{
// TODO: Duplicated from NpgsqlArrayTypeMapping
var arr = (Array)value;
var list = (IList)value;
Copy link
Copy Markdown
Member

@roji roji Jul 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@austindrenski I'm not too sure about this...

For one thing multidimensional arrays seem to implement List - but not List<T>. This is odd, and I suspect it to be .NET legacy behavior which was kept to maintain backwards compatibility.

At the very least, please add a specific check for multidimensional arrays, throwing NotSupportedException - it's an error to render a multidimensional .NET array as a one-dimensional PostgreSQL array...

(it would have been better to allow reviewing this non-cosmetic change before merging, but no big deal :)).

Copy link
Copy Markdown
Contributor Author

@austindrenski austindrenski Jul 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Admittedly, I wasn't thinking about multidimensional arrays here...which underscores your point about reviews—that's my bad.

Since we don't support jagged arrays, I assumed that List<T> would only be mappable to one-dimensional PostgreSQL arrays. (Since List<List<T>> is more or less the equivalent of T[][].)

Did you have something else in mind?

Copy link
Copy Markdown
Member

@roji roji Jul 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See some additional thoughts in #552 (comment).

As I wrote there, we basically need to separate between multidimensional arrays and jagged arrays. The latter really aren't supported by PostgreSQL and a clear, informative message should say so. I agree that List<List<T> is just another form of a jagged array and should throw in the same way.

There's no such thing as a multidimensional array with List<T>, so I don't think you need to think about that here in NpgsqlListTypeMapping (as opposed to NpgsqlArrayTypeMapping).

Aside from that, I'm simply not sure there's value in support non-generic List - I'm assuming part of you change was to support that. It's probably healthier to restrict support to generic List<T> only.

Hope this is clear, we're discussing several different things here...

Copy link
Copy Markdown
Contributor Author

@austindrenski austindrenski Jul 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I think we're on the same page. But I'm still a little lost on what you mean by the non-generic List:

Aside from that, I'm simply not sure there's value in support non-generic List - I'm assuming part of you change was to support that.

  1. My understanding is that we will only support List<T> where T is not a collection type.
  2. I cast the incoming parameter to IList so that I could access IList.Count for the loop below.
    • I'm using IList since the type mapper is non-generic, so there's no type parameter in scope for IList<T>/List<T>.

Copy link
Copy Markdown
Member

@roji roji Jul 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I wasn't very clear on that.

First, I'd rather we had a clear up-front check that GenerateNonNullSqlLiteral() gets the right kind of input, i.e. a generic List<T> where T corresponds to ElementMapping. Right now we throw with imprecise messages (multidimensional arrays, or just that the given object isn't a generic type).

Second, as I wrote in the other comment, I'd rather we had construction-time checks for the correctness of the mapping object itself, i.e. throw if we attempt to construct a mapping that would correspond to a jagged array (i.e. T[][], List<List<T>>).

After all these checks it seems fine to cast to IList as we've verified that the types are correct.

Copy link
Copy Markdown
Contributor Author

@austindrenski austindrenski Jul 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I'll put something together in the next few days.


if (arr.Rank != 1)
if (list.GetType().GenericTypeArguments[0] != ElementMapping.ClrType)
throw new NotSupportedException("Multidimensional array literals aren't supported");

var sb = new StringBuilder();
sb.Append("ARRAY[");
for (var i = 0; i < arr.Length; i++)
for (var i = 0; i < list.Count; i++)
{
sb.Append(ElementMapping.GenerateSqlLiteral(arr.GetValue(i)));
if (i < arr.Length - 1)
sb.Append(",");
if (i > 0)
sb.Append(',');

sb.Append(ElementMapping.GenerateSqlLiteral(list[i]));
}
sb.Append("]");

sb.Append(']');
return sb.ToString();
}

Expand All @@ -91,14 +101,13 @@ protected override string GenerateNonNullSqlLiteral(object value)
static ValueComparer CreateComparer(RelationalTypeMapping elementMapping, Type listType)
{
Debug.Assert(listType.IsGenericType && listType.GetGenericTypeDefinition() == typeof(List<>));

var elementType = listType.GetGenericArguments()[0];

// We use different comparer implementations based on whether we have a non-null element comparer,
// and if not, whether the element is IEquatable<TElem>

if (elementMapping.Comparer != null)
return (ValueComparer)Activator.CreateInstance(
typeof(SingleDimComparerWithComparer<>).MakeGenericType(elementType), elementMapping);
return (ValueComparer)Activator.CreateInstance(typeof(SingleDimComparerWithComparer<>).MakeGenericType(elementType), elementMapping);

if (typeof(IEquatable<>).MakeGenericType(elementType).IsAssignableFrom(elementType))
return (ValueComparer)Activator.CreateInstance(typeof(SingleDimComparerWithIEquatable<>).MakeGenericType(elementType));
Expand All @@ -110,10 +119,11 @@ static ValueComparer CreateComparer(RelationalTypeMapping elementMapping, Type l

class SingleDimComparerWithComparer<TElem> : ValueComparer<List<TElem>>
{
public SingleDimComparerWithComparer(RelationalTypeMapping elementMapping) : base(
(a, b) => Compare(a, b, (ValueComparer<TElem>)elementMapping.Comparer),
o => o.GetHashCode(), // TODO: Need to get hash code of elements...
source => Snapshot(source, (ValueComparer<TElem>)elementMapping.Comparer)) {}
public SingleDimComparerWithComparer(RelationalTypeMapping elementMapping)
: base(
(a, b) => Compare(a, b, (ValueComparer<TElem>)elementMapping.Comparer),
o => o.GetHashCode(), // TODO: Need to get hash code of elements...
source => Snapshot(source, (ValueComparer<TElem>)elementMapping.Comparer)) {}

public override Type Type => typeof(List<TElem>);

Expand All @@ -133,25 +143,27 @@ static bool Compare(List<TElem> a, List<TElem> b, ValueComparer<TElem> elementCo

static List<TElem> Snapshot(List<TElem> source, ValueComparer<TElem> elementComparer)
{
if (source == null)
if (source is null)
Copy link
Copy Markdown
Member

@roji roji Jul 27, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any specific reason to convert x == null to x is null? It makes sense if you use pattern matching (i.e. x is null y) but just for null checking? I'd stick to the current standard syntax rather than starting to change the entire codebase etc.

Copy link
Copy Markdown
Contributor Author

@austindrenski austindrenski Jul 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a fair point. I tend to make these changes out of habit more than anything else.

I'll revert this in an upcoming commit. (This type mapper still lacks type-casting for the array literals.)

return null;

var snapshot = new List<TElem>(source.Count);

// Note: the following currently boxes every element access because ValueComparer isn't really
// generic (see https://github.com/aspnet/EntityFrameworkCore/issues/11072)
foreach (var e in source)
snapshot.Add(elementComparer.Snapshot(e));

return snapshot;
}
}

class SingleDimComparerWithIEquatable<TElem> : ValueComparer<List<TElem>>
where TElem : IEquatable<TElem>
class SingleDimComparerWithIEquatable<TElem> : ValueComparer<List<TElem>> where TElem : IEquatable<TElem>
{
public SingleDimComparerWithIEquatable(): base(
(a, b) => Compare(a, b),
o => o.GetHashCode(), // TODO: Need to get hash code of elements...
source => DoSnapshot(source)) {}
public SingleDimComparerWithIEquatable()
: base(
(a, b) => Compare(a, b),
o => o.GetHashCode(), // TODO: Need to get hash code of elements...
source => DoSnapshot(source)) {}

public override Type Type => typeof(List<TElem>);

Expand All @@ -169,8 +181,10 @@ static bool Compare(List<TElem> a, List<TElem> b)
{
if (elem2 == null)
continue;

return false;
}

if (!elem1.Equals(elem2))
return false;
}
Expand All @@ -180,21 +194,25 @@ static bool Compare(List<TElem> a, List<TElem> b)

static List<TElem> DoSnapshot(List<TElem> source)
{
if (source == null)
if (source is null)
return null;

var snapshot = new List<TElem>(source.Count);

foreach (var e in source)
snapshot.Add(e);

return snapshot;
}
}

class SingleDimComparerWithEquals<TElem> : ValueComparer<List<TElem>>
{
public SingleDimComparerWithEquals() : base(
(a, b) => Compare(a, b),
o => o.GetHashCode(), // TODO: Need to get hash code of elements...
source => DoSnapshot(source)) {}
public SingleDimComparerWithEquals()
: base(
(a, b) => Compare(a, b),
o => o.GetHashCode(), // TODO: Need to get hash code of elements...
source => DoSnapshot(source)) {}

public override Type Type => typeof(List<TElem>);

Expand All @@ -215,6 +233,7 @@ static bool Compare(List<TElem> a, List<TElem> b)
continue;
return false;
}

if (!elem1.Equals(elem2))
return false;
}
Expand All @@ -224,14 +243,16 @@ static bool Compare(List<TElem> a, List<TElem> b)

static List<TElem> DoSnapshot(List<TElem> source)
{
if (source == null)
if (source is null)
return null;

var snapshot = new List<TElem>(source.Count);

// Note: the following currently boxes every element access because ValueComparer isn't really
// generic (see https://github.com/aspnet/EntityFrameworkCore/issues/11072)
foreach (var e in source)
snapshot.Add(e);

return snapshot;
}
}
Expand Down