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

Fix for parsing Guid and string in the same condition #200

Merged
merged 23 commits into from
Aug 27, 2018
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
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
8 changes: 7 additions & 1 deletion src/System.Linq.Dynamic.Core/Parser/ExpressionHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,13 @@ public static Expression OptimizeStringForEqualityIfPossible(string text, Type t

static MethodInfo GetStaticMethod(string methodName, Expression left, Expression right)
{
return left.Type.GetMethod(methodName, new[] { left.Type, right.Type });
var methodInfo = left.Type.GetMethod(methodName, new[] {left.Type, right.Type});
if (methodInfo == null)
{
methodInfo = right.Type.GetMethod(methodName, new[] {left.Type, right.Type});
}

return methodInfo;
}

static Expression GenerateStaticMethodCall(string methodName, Expression left, Expression right)
Expand Down
2 changes: 1 addition & 1 deletion src/System.Linq.Dynamic.Core/Parser/ExpressionParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ Expression ParseComparisonOperator()
Token op = _textParser.CurrentToken;
_textParser.NextToken();
Expression right = ParseShiftOperator();
bool isEquality = op.Id == TokenId.Equal || op.Id == TokenId.DoubleEqual || op.Id == TokenId.ExclamationEqual;
bool isEquality = op.Id == TokenId.Equal || op.Id == TokenId.DoubleEqual || op.Id == TokenId.ExclamationEqual || op.Id == TokenId.LessGreater;

if (isEquality && (!left.Type.GetTypeInfo().IsValueType && !right.Type.GetTypeInfo().IsValueType || left.Type == typeof(Guid) && right.Type == typeof(Guid)))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,13 @@ internal interface IEqualitySignatures : IRelationalSignatures

void F(Guid x, Guid y);
void F(Guid? x, Guid? y);
void F(Guid x, string y);
void F(Guid? x, string y);
void F(string x, Guid y);
void F(string x, Guid? y);

// Disabled 2 lines below according to unit test ParseLambda_With_Guid_Equals_Null
Copy link
Collaborator

Choose a reason for hiding this comment

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

Change comment to:

// Disabled 4 lines below because of : https://github.com/StefH/System.Linq.Dynamic.Core/pull/200
//void F(Guid x, string y);
//void F(Guid? x, string y);
//void F(string x, Guid y);
//void F(string x, Guid? y);

//void F(Guid x, string y);
//void F(Guid? x, string y);

// Disabled 2 lines below according to unit test ParseLambda_With_Null_Equals_Guid
//void F(string x, Guid y);
//void F(string x, Guid? y);
}
}
193 changes: 193 additions & 0 deletions test/System.Linq.Dynamic.Core.Tests/DynamicExpressionParserTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,57 @@ public class CustomClassWithStaticMethod
public static int GetAge(int x) => x;
}

public class CustomTextClass
{
public CustomTextClass(string origin)
{
Origin = origin;
}

public string Origin { get; }

public static implicit operator string(CustomTextClass customTextValue)
{
return customTextValue?.Origin;
}

public static implicit operator CustomTextClass(string origin)
{
return new CustomTextClass(origin);
}

public override string ToString()
{
return Origin;
}
}

public class TextHolder
{
public TextHolder(string name, CustomTextClass note)
{
Name = name;
Note = note;
}

public string Name { get; }

public CustomTextClass Note { get; }

public override string ToString()
{
return Name + " (" + Note + ")";
}
}

public static class StaticHelper
{
public static Guid? GetGuid(string name)
{
return Guid.NewGuid();
}
}

private class TestCustomTypeProvider : AbstractDynamicLinqCustomTypeProvider, IDynamicLinkCustomTypeProvider
{
private HashSet<Type> _customTypes;
Expand All @@ -50,6 +101,7 @@ public virtual HashSet<Type> GetCustomTypes()

_customTypes = new HashSet<Type>(FindTypesMarkedWithDynamicLinqTypeAttribute(new[] { GetType().GetTypeInfo().Assembly }));
_customTypes.Add(typeof(CustomClassWithStaticMethod));
_customTypes.Add(typeof(StaticHelper));
return _customTypes;
}
}
Expand Down Expand Up @@ -452,5 +504,146 @@ public void ParseLambda_With_InnerStringLiteral()
object result = del.DynamicInvoke(String.Empty);
Check.That(result).IsEqualTo(originalTrueValue);
}

[Fact]
public void ParseLambda_With_Guid_Equals_Null()
{
// Arrange
var user = new User();
Guid guidEmpty = Guid.Empty;
Guid someId = Guid.NewGuid();
string expressionText = $"iif(@0.Id == null, @0.Id == Guid.Parse(\"{someId}\"), Id == Id)";

// Act
var lambda = DynamicExpressionParser.ParseLambda(typeof(User), null, expressionText, user);
var boolLambda = lambda as Expression<Func<User, bool>>;
Assert.NotNull(boolLambda);

var del = lambda.Compile();
bool result = (bool) del.DynamicInvoke(user);

// Assert
Assert.True(result);
}

[Fact]
public void ParseLambda_With_Null_Equals_Guid()
{
// Arrange
var user = new User();
Guid guidEmpty = Guid.Empty;
Guid someId = Guid.NewGuid();
string expressionText = $"iif(null == @0.Id, @0.Id == Guid.Parse(\"{someId}\"), Id == Id)";

// Act
var lambda = DynamicExpressionParser.ParseLambda(typeof(User), null, expressionText, user);
var boolLambda = lambda as Expression<Func<User, bool>>;
Assert.NotNull(boolLambda);

var del = lambda.Compile();
bool result = (bool) del.DynamicInvoke(user);

// Assert
Assert.True(result);
}

[Fact]
public void ParseLambda_With_Guid_Equals_String()
{
// Arrange
Guid someId = Guid.NewGuid();
Guid anotherId = Guid.NewGuid();
var user = new User();
user.Id = someId;
Guid guidEmpty = Guid.Empty;
string expressionText = $"iif(@0.Id == \"{someId}\", Guid.Parse(\"{guidEmpty}\"), Guid.Parse(\"{anotherId}\"))";

// Act
var lambda = DynamicExpressionParser.ParseLambda(typeof(User), null, expressionText, user);
var guidLambda = lambda as Expression<Func<User, Guid>>;
Assert.NotNull(guidLambda);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not also check here on:

var boolLambda = lambda as Expression<Func<User, bool>>;
Assert.NotNull(boolLambda);

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I've added it:

var guidLambda = lambda as Expression<Func<User, Guid>>;
Assert.NotNull(guidLambda);

var del = lambda.Compile();
Guid result = (Guid) del.DynamicInvoke(user);

// Assert
Assert.Equal(guidEmpty, result);
}

[Fact]
public void ParseLambda_With_Concat_String_CustomType()
{
// Arrange
string name = "name1";
string note = "note1";
var textHolder = new TextHolder(name, note);
string expressionText = $"Name + \" (\" + Note + \")\"";
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to use $ here I think ? It's just a normal string.


// Act 1
var lambda = DynamicExpressionParser.ParseLambda(typeof(TextHolder), null, expressionText, textHolder);
var stringLambda = lambda as Expression<Func<TextHolder, string>>;

// Assert 1
Assert.NotNull(stringLambda);

// Act 2
var del = lambda.Compile();
string result = (string) del.DynamicInvoke(textHolder);

// Assert 2
Assert.Equal("name1 (note1)", result);
}

[Fact]
public void ParseLambda_With_Concat_CustomType_String()
{
// Arrange
string name = "name1";
string note = "note1";
var textHolder = new TextHolder(name, note);
string expressionText = $"Note + \" (\" + Name + \")\"";
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to use $ here I think ? It's just a normal string.


// Act 1
var lambda = DynamicExpressionParser.ParseLambda(typeof(TextHolder), null, expressionText, textHolder);
var stringLambda = lambda as Expression<Func<TextHolder, string>>;

// Assert 1
Assert.NotNull(stringLambda);

// Act 2
var del = lambda.Compile();
string result = (string) del.DynamicInvoke(textHolder);

// Assert 2
Assert.Equal("note1 (name1)", result);
}

[Fact]
public void ParseLambda_With_Less_Greater()
Copy link
Collaborator

Choose a reason for hiding this comment

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

rename to ParseLambda_Operator_Less_Greater_With_Guids ?

{
var config = new ParsingConfig
{
CustomTypeProvider = new TestCustomTypeProvider()
};

// Arrange
Guid someId = Guid.NewGuid();
Guid anotherId = Guid.NewGuid();
var user = new User();
user.Id = someId;
Guid guidEmpty = Guid.Empty;
string expressionText = $"iif(@0.Id == StaticHelper.GetGuid(\"name\"), Guid.Parse(\"{guidEmpty}\"), Guid.Parse(\"{anotherId}\"))";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why introduce the StaticHelper class here if you only want to test the <> operator?

Copy link
Contributor Author

@OlegNadymov OlegNadymov Aug 26, 2018

Choose a reason for hiding this comment

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

This expression with Guid constant instead of Guid method was worked correctly before these changes. Therefore for represent bug with method I've introduced StaticHelper with GetGuid method. Or do you mean why I didn't create this class like as local class?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought that the <> was not working at all. But I understand from your comment that this was only not working in specific flows?

In that case your test looks good.

Maybe you could add some more comments to explain this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The <> was working with constant only.
Your ExpressionParser has this condition (line 494):

else if ((constantExpr = right as ConstantExpression) != null && constantExpr.Value is string && (typeConverter = TypeConverterFactory.GetConverter(left.Type)) != null)
{
    right = Expression.Constant(typeConverter.ConvertFromInvariantString((string)constantExpr.Value), left.Type);
}

Therefore if right epxression is ConstantExpression then the <> will be working.
But for non-constant and for different types (e.g. Guid and Guid?) will be executed this code (line 531):

CheckAndPromoteOperands(isEquality ? typeof(IEqualitySignatures) : typeof(IRelationalSignatures), op.Text, ref left, ref right, op.Pos);

And before my changes the variable isEquality was false. Therefore into method CheckAndPromoteOperands passed argument type as typeof(IRelationalSignatures). But type typeof(IRelationalSignatures) doesn't have conversion between Guid and Guid?.

Moreover for the != this case was working because for the != the variable isEquality was true:

bool isEquality = op.Id == TokenId.Equal || op.Id == TokenId.DoubleEqual || op.Id == TokenId.ExclamationEqual;

TokenId.ExclamationEqual is equal to the !=.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, thanks for the detailed explanation.


// Act
var lambda = DynamicExpressionParser.ParseLambda(config, typeof(User), null, expressionText, user);
var guidLambda = lambda as Expression<Func<User, Guid>>;
Assert.NotNull(guidLambda);

var del = lambda.Compile();
Guid result = (Guid) del.DynamicInvoke(user);

// Assert
Assert.Equal(anotherId, result);
}
}
}