Skip to content

Commit

Permalink
Fixes the previous commit, unskips tests.
Browse files Browse the repository at this point in the history 
Something to do with `MemberAccessBindingExpressionVisitor`
happens under some specific circumstances that coincide with a
compiled query.

Added an `IsSafeToVisit` helper to `NpgsqlSqlTranslating...Visitor`
for an additional guard when handling arrays that could be
some type of replaced parameter.
  • Loading branch information
@austindrenski
austindrenski committed Jul 23, 2018
1 parent 2a5e41a commit a917e00
Show file tree
Hide file tree
Showing 7 changed files with 59 additions and 190 deletions.
50 changes: 0 additions & 50 deletions QueryBaseline.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4014,53 +4014,3 @@



System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions) :
AssertSql(
@"SELECT (o.""OrderDate"" + MAKE_INTERVAL(secs => 1)) AS ""OrderDate""
FROM ""Orders"" AS o
WHERE ((o.""OrderDate"" <> NULL) OR (o.""OrderDate"" IS NULL OR NULL IS NULL)) AND (o.""OrderDate"" IS NOT NULL OR NULL IS NOT NULL)");



System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions) :
AssertSql(
@"SELECT (o.""OrderDate"" + MAKE_INTERVAL(years => 1)) AS ""OrderDate""
FROM ""Orders"" AS o
WHERE ((o.""OrderDate"" <> NULL) OR (o.""OrderDate"" IS NULL OR NULL IS NULL)) AND (o.""OrderDate"" IS NOT NULL OR NULL IS NOT NULL)");



System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions) :
AssertSql(
@"SELECT (o.""OrderDate"" + MAKE_INTERVAL(mins => 1)) AS ""OrderDate""
FROM ""Orders"" AS o
WHERE ((o.""OrderDate"" <> NULL) OR (o.""OrderDate"" IS NULL OR NULL IS NULL)) AND (o.""OrderDate"" IS NOT NULL OR NULL IS NOT NULL)");



System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions) :
AssertSql(
@"@__years_0='2'
SELECT (o.""OrderDate"" + MAKE_INTERVAL(years => @__years_0)) AS ""OrderDate""
FROM ""Orders"" AS o
WHERE ((o.""OrderDate"" <> NULL) OR (o.""OrderDate"" IS NULL OR NULL IS NULL)) AND (o.""OrderDate"" IS NOT NULL OR NULL IS NOT NULL)");



System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions) :
AssertSql(
@"SELECT (o.""OrderDate"" + MAKE_INTERVAL(months => 1)) AS ""OrderDate""
FROM ""Orders"" AS o
WHERE ((o.""OrderDate"" <> NULL) OR (o.""OrderDate"" IS NULL OR NULL IS NULL)) AND (o.""OrderDate"" IS NOT NULL OR NULL IS NOT NULL)");



System.RuntimeMethodHandle.InvokeMethod(Object target, Object[] arguments, Signature sig, Boolean constructor, Boolean wrapExceptions) :
AssertSql(
@"SELECT (o.""OrderDate"" + MAKE_INTERVAL(hours => 1)) AS ""OrderDate""
FROM ""Orders"" AS o
WHERE ((o.""OrderDate"" <> NULL) OR (o.""OrderDate"" IS NULL OR NULL IS NULL)) AND (o.""OrderDate"" IS NOT NULL OR NULL IS NOT NULL)");



30 changes: 23 additions & 7 deletions src/EFCore.PG/Extensions/NpgsqlArrayExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@

#endregion

using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using JetBrains.Annotations;
using Npgsql.EntityFrameworkCore.PostgreSQL.Utilities;

// ReSharper disable once CheckNamespace
namespace Microsoft.EntityFrameworkCore
Expand All @@ -49,7 +50,7 @@ public static class NpgsqlArrayExtensions
/// This method is only intended for use via SQL translation as part of an EF Core LINQ query.
/// </exception>
public static string ArrayToString<T>([CanBeNull] this DbFunctions _, [NotNull] T[] array, [CanBeNull] string delimiter)
=> throw new ClientEvaluationNotSupportedException();
=> throw ClientEvaluationNotSupportedException();

/// <summary>
/// Concatenates elements using the supplied delimiter.
Expand All @@ -65,7 +66,7 @@ public static string ArrayToString<T>([CanBeNull] this DbFunctions _, [NotNull]
/// This method is only intended for use via SQL translation as part of an EF Core LINQ query.
/// </exception>
public static string ArrayToString<T>([CanBeNull] this DbFunctions _, [NotNull] List<T> list, [CanBeNull] string delimiter)
=> throw new ClientEvaluationNotSupportedException();
=> throw ClientEvaluationNotSupportedException();

/// <summary>
/// Concatenates elements using the supplied delimiter and the string representation for null elements.
Expand All @@ -82,7 +83,7 @@ public static string ArrayToString<T>([CanBeNull] this DbFunctions _, [NotNull]
/// This method is only intended for use via SQL translation as part of an EF Core LINQ query.
/// </exception>
public static string ArrayToString<T>([CanBeNull] this DbFunctions _, [NotNull] T[] array, [CanBeNull] string delimiter, [CanBeNull] string nullString)
=> throw new ClientEvaluationNotSupportedException();
=> throw ClientEvaluationNotSupportedException();

/// <summary>
/// Concatenates elements using the supplied delimiter and the string representation for null elements.
Expand All @@ -99,7 +100,7 @@ public static string ArrayToString<T>([CanBeNull] this DbFunctions _, [NotNull]
/// This method is only intended for use via SQL translation as part of an EF Core LINQ query.
/// </exception>
public static string ArrayToString<T>([CanBeNull] this DbFunctions _, [NotNull] List<T> list, [CanBeNull] string delimiter, [CanBeNull] string nullString)
=> throw new ClientEvaluationNotSupportedException();
=> throw ClientEvaluationNotSupportedException();

/// <summary>
/// Converts the input string into an array using the supplied delimiter and the string representation for null elements.
Expand All @@ -116,7 +117,7 @@ public static string ArrayToString<T>([CanBeNull] this DbFunctions _, [NotNull]
/// This method is only intended for use via SQL translation as part of an EF Core LINQ query.
/// </exception>
public static T[] StringToArray<T>([CanBeNull] this DbFunctions _, [NotNull] string input, [CanBeNull] string delimiter, [CanBeNull] string nullString)
=> throw new ClientEvaluationNotSupportedException();
=> throw ClientEvaluationNotSupportedException();

/// <summary>
/// Converts the input string into a <see cref="List{T}"/> using the supplied delimiter and the string representation for null elements.
Expand All @@ -133,6 +134,21 @@ public static T[] StringToArray<T>([CanBeNull] this DbFunctions _, [NotNull] str
/// This method is only intended for use via SQL translation as part of an EF Core LINQ query.
/// </exception>
public static List<T> StringToList<T>([CanBeNull] this DbFunctions _, [NotNull] string input, [CanBeNull] string delimiter, [CanBeNull] string nullString)
=> throw new ClientEvaluationNotSupportedException();
=> throw ClientEvaluationNotSupportedException();

#region Utilities

/// <summary>
/// Helper method to throw a <see cref="NotSupportedException"/> with the name of the throwing method.
/// </summary>
/// <param name="method">The method that throws the exception.</param>
/// <returns>
/// A <see cref="NotSupportedException"/>.
/// </returns>
[NotNull]
static NotSupportedException ClientEvaluationNotSupportedException([CallerMemberName] string method = default)
=> new NotSupportedException($"{method} is only intended for use via SQL translation as part of an EF Core LINQ query.");

#endregion
}
}
34 changes: 29 additions & 5 deletions src/EFCore.PG/Query/ExpressionVisitors/NpgsqlSqlTranslatingExpressionVisitor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
using Microsoft.EntityFrameworkCore.Query;
using Microsoft.EntityFrameworkCore.Query.Expressions;
using Microsoft.EntityFrameworkCore.Query.ExpressionVisitors;
using Microsoft.EntityFrameworkCore.Query.ExpressionVisitors.Internal;
using Npgsql.EntityFrameworkCore.PostgreSQL.Query.Expressions.Internal;
using Remotion.Linq.Clauses;
using Remotion.Linq.Clauses.Expressions;
Expand Down Expand Up @@ -80,24 +81,36 @@ public class NpgsqlSqlTranslatingExpressionVisitor : SqlTranslatingExpressionVis

#endregion

/// <summary>
/// The current query model visitor.
/// </summary>
[NotNull] readonly RelationalQueryModelVisitor _queryModelVisitor;

/// <summary>
/// The current query compilation context.
/// </summary>
[NotNull]
RelationalQueryCompilationContext Context => _queryModelVisitor.QueryCompilationContext;

/// <inheritdoc />
public NpgsqlSqlTranslatingExpressionVisitor(
[NotNull] SqlTranslatingExpressionVisitorDependencies dependencies,
[NotNull] RelationalQueryModelVisitor queryModelVisitor,
[CanBeNull] SelectExpression targetSelectExpression = null,
[CanBeNull] Expression topLevelPredicate = null,
bool inProjection = false)
: base(dependencies, queryModelVisitor, targetSelectExpression, topLevelPredicate, inProjection) {}
: base(dependencies, queryModelVisitor, targetSelectExpression, topLevelPredicate, inProjection)
=> _queryModelVisitor = queryModelVisitor;

#region Overrides

/// <inheritdoc />
protected override Expression VisitBinary(BinaryExpression expression)
=> expression.NodeType is ExpressionType.ArrayIndex
? Expression.MakeIndex(
=> expression.NodeType is ExpressionType.ArrayIndex &&
IsSafeToVisit(expression, Context)
? Expression.ArrayAccess(
Visit(expression.Left) ?? expression.Left,
indexer: null,
new[] { Visit(expression.Right) ?? expression.Right, })
Visit(expression.Right) ?? expression.Right)
: base.VisitBinary(expression);

/// <inheritdoc />
Expand Down Expand Up @@ -312,6 +325,17 @@ protected virtual Expression VisitArrayContains([NotNull] Expression array, [Not
/// </returns>
static bool IsArrayOrList([NotNull] Type type) => type.IsArray || type.IsGenericType && typeof(List<>) == type.GetGenericTypeDefinition();

/// <summary>
/// True if the expression is safe to visitat this stage.
/// </summary>
/// <param name="expression">The expression to check</param>
/// <param name="context">The context to use.</param>
/// <returns>
/// True to visit this expression; otherwise false.
/// </returns>
static bool IsSafeToVisit(BinaryExpression expression, RelationalQueryCompilationContext context)
=> MemberAccessBindingExpressionVisitor.GetPropertyPath(expression.Left, context, out _).Count != 0;

#endregion
}
}
1 change: 1 addition & 0 deletions src/EFCore.PG/Query/Expressions/Internal/PgFunctionExpression.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
using System.Diagnostics;
using System.Linq;
using System.Linq.Expressions;
using System.Text;
using JetBrains.Annotations;
using Npgsql.EntityFrameworkCore.PostgreSQL.Query.Sql.Internal;
using Npgsql.EntityFrameworkCore.PostgreSQL.Utilities;
Expand Down
48 changes: 0 additions & 48 deletions src/EFCore.PG/Utilities/ClientEvaluationNotSupported.cs
View file

This file was deleted.

44 changes: 5 additions & 39 deletions test/EFCore.PG.FunctionalTests/Query/ArrayQueryTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -947,73 +947,41 @@ public void List_Exists_equals_with_column_array_element_flipped()

#region Support

/// <summary>
/// Provides resources for unit tests.
/// </summary>
ArrayFixture Fixture { get; }

/// <summary>
/// Initializes resources for unit tests.
/// </summary>
/// <param name="fixture">The fixture of resources for testing.</param>
public ArrayQueryTest(ArrayFixture fixture)
{
Fixture = fixture;
Fixture.TestSqlLoggerFactory.Clear();
}

/// <summary>
/// Creates a new <see cref="ArrayContext"/>.
/// </summary>
/// <returns>
/// An <see cref="ArrayContext"/> for testing.
/// </returns>
ArrayContext CreateContext() => Fixture.CreateContext();

/// <summary>
/// Asserts that the SQL fragment appears in the logs.
/// </summary>
/// <param name="expected">The SQL statement or fragment to search for in the logs.</param>
void AssertContainsInSql(string expected)
=> Assert.Contains(expected, Fixture.TestSqlLoggerFactory.Sql);

/// <summary>
/// Asserts that the SQL fragment does not appear in the logs.
/// </summary>
/// <param name="expected">The SQL statement or fragment to search for in the logs.</param>
void AssertDoesNotContainInSql(string expected)
=> Assert.DoesNotContain(expected, Fixture.TestSqlLoggerFactory.Sql);

#endregion Support

#region Fixtures

/// <summary>
/// Represents a database suitable for testing operations with PostgreSQL arrays.
/// </summary>
public class ArrayContext : DbContext
{
public DbSet<SomeArrayEntity> SomeEntities { get; set; }
public ArrayContext(DbContextOptions options) : base(options) {}
protected override void OnModelCreating(ModelBuilder builder) {}
}

/// <summary>
/// Represents an entity suitable for testing operations with PostgreSQL arrays.
/// </summary>
public class SomeArrayEntity
{
public int Id { get; set; }
public int[] SomeArray { get; set; }
public List<int> SomeList { get; set; }
public int[,] SomeMatrix { get; set; }
public List<int> SomeList { get; set; }
public byte[] SomeBytea { get; set; }

// ReSharper disable once UnusedMember.Global
public string SomeString { get; set; }
}

/// <summary>
/// Represents a fixture suitable for testing operations with PostgreSQL arrays.
/// </summary>
public class ArrayFixture : IDisposable
{
readonly DbContextOptions _options;
Expand All @@ -1039,18 +1007,16 @@ public ArrayFixture()
Id = 1,
SomeArray = new[] { 3, 4 },
SomeBytea = new byte[] { 3, 4 },
SomeList = new List<int> { 3, 4 },
SomeMatrix = new[,] { { 5, 6 }, { 7, 8 } },
SomeString = "This_is_a_test"
SomeList = new List<int> { 3, 4 }
});
ctx.SomeEntities.Add(new SomeArrayEntity
{
Id = 2,
SomeArray = new[] { 5, 6, 7 },
SomeBytea = new byte[] { 5, 6, 7 },
SomeList = new List<int> { 5, 6, 7 },
SomeMatrix = new[,] { { 10, 11 }, { 12, 13 } },
SomeString = "this_is_a_test"
SomeList = new List<int> { 3, 4 }
});
ctx.SaveChanges();
}
Expand Down
Loading

0 comments on commit a917e00

Please sign in to comment.