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 14 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
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_If_Guid_Null
//void F(Guid x, string y);
//void F(Guid? x, string y);

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

Choose a reason for hiding this comment

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

I would think that removing this code would actually break unit-tests? Or do you say that this logic was wrong from the beginning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion logic was wrong from the start.
It's like the issue #19
I've added unit test ParseLambda_With_Guid_Equals_String for demonstrate that Guid plus String works without these lines (at least in condition part).

//void F(string x, Guid y);
//void F(string x, Guid? y);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ namespace System.Linq.Dynamic.Core.Tests
{
public class DynamicExpressionParserTests
{


private class MyClass
{
public int Foo()
Expand Down Expand Up @@ -452,5 +454,39 @@ public void ParseLambda_With_InnerStringLiteral()
object result = del.DynamicInvoke(String.Empty);
Check.That(result).IsEqualTo(originalTrueValue);
}

[Fact]
public void ParseLambda_With_If_Guid_Null()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use // Arrange , // Act and // Assert flow (take a look at other tests).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was done!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this be Iif ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was done!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Better text like: Guid_Equals_Null ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was done!

{
var objContext = new User();
Copy link
Collaborator

Choose a reason for hiding this comment

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

user

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was done!

var guidEmpty = Guid.Empty;
Copy link
Collaborator

Choose a reason for hiding this comment

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

For clarity, I mostly / always use real definitions for simple types. So use Guid here.
Only for complex types, I use var.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was done!

var someId = Guid.NewGuid();
var expressionText = $"iif(@0.{nameof(objContext.Id)} == null, @0.{nameof(objContext.Id)} == Guid.Parse(\"{someId}\"), {nameof(objContext.Id)}={nameof(objContext.Id)})";
Copy link
Collaborator

Choose a reason for hiding this comment

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

string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was done!

Copy link
Collaborator

Choose a reason for hiding this comment

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

For readability I would just type the expressionText string as simple as possible, so just:

$"iif(@0.Id == null, @0.Id == Guid.Parse(\"{someId}\"), Id=Id)";

And actually I wonder if this is ok?
Wouldn't the last statement set a value?
Like:

$"iif(@0.Id == null, @0.Id == Guid.Parse(\"{someId}\"), Id)";

Copy link
Contributor Author

@OlegNadymov OlegNadymov Aug 24, 2018

Choose a reason for hiding this comment

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

No, it would not. This expression should have boolean result. So "Id == Id" is kind of true result. It's part of query for our ORM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's correct expression:
string expressionText = $"iif(@0.Id == null, @0.Id == Guid.Parse(\"{someId}\"), Id == Id)";


var lambda = System.Linq.Dynamic.Core.DynamicExpressionParser.ParseLambda(typeof(User), null, expressionText, objContext);
var boolLambda = lambda as Expression<Func<User, bool>>;
Check.That(boolLambda).IsNotEqualTo(null);

var del = lambda.Compile();
object result = del.DynamicInvoke(objContext);
Check.That(result).IsEqualTo(true);
}

[Fact]
public void ParseLambda_With_If_Null_Guid()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comments as defined on other method also apply here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was done!

{
var objContext = new User();
var guidEmpty = Guid.Empty;
var someId = Guid.NewGuid();
var expressionText = $"iif(null == @0.{nameof(objContext.Id)}, @0.{nameof(objContext.Id)} == Guid.Parse(\"{someId}\"), {nameof(objContext.Id)}={nameof(objContext.Id)})";

var lambda = System.Linq.Dynamic.Core.DynamicExpressionParser.ParseLambda(typeof(User), null, expressionText, objContext);
var boolLambda = lambda as Expression<Func<User, bool>>;
Check.That(boolLambda).IsNotEqualTo(null);

var del = lambda.Compile();
object result = del.DynamicInvoke(objContext);
Check.That(result).IsEqualTo(true);
}
}
}