Fix for parsing Guid and string in the same condition by OlegNadymov · Pull Request #200 · zzzprojects/System.Linq.Dynamic.Core

OlegNadymov · 2018-08-23T12:34:09Z

Plus fix for parsing expression with custom types with implicit convertions

…ments

codecov · 2018-08-23T12:40:24Z

Codecov Report

Merging #200 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #200      +/-   ##
==========================================
+ Coverage   82.53%   82.55%   +0.02%     
==========================================
  Files          37       37              
  Lines        3676     3681       +5     
  Branches      503      504       +1     
==========================================
+ Hits         3034     3039       +5     
- Misses        498      502       +4     
+ Partials      144      140       -4

Impacted Files	Coverage Δ
...ystem.Linq.Dynamic.Core/Parser/ExpressionParser.cs	`78.22% <100%> (+0.16%)`	⬆️
...ystem.Linq.Dynamic.Core/Parser/ExpressionHelper.cs	`77.69% <100%> (+0.89%)`	⬆️
...namic.Core/Parser/SupportedMethods/MethodFinder.cs	`89.86% <0%> (-2.71%)`	⬇️
src/System.Linq.Dynamic.Core/Parser/TypeHelper.cs	`87.26% <0%> (-0.48%)`	⬇️
...c/System.Linq.Dynamic.Core/Tokenizer/TextParser.cs	`98.03% <0%> (+0.98%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b31d36f...dc9e24d. Read the comment docs.

OlegNadymov · 2018-08-23T12:44:41Z

Hi @StefH !
Could you show me more information about failing unit test and code issues.
All unit test were passed on my local machine.

StefH · 2018-08-23T16:23:50Z

Hello @OlegNadymov ,

Thanks for this PR, I'll check and review in some time.

1] Unit tests are fine (see https://ci.appveyor.com/project/StefH/system-linq-dynamic-core/build/1.0.8.643)

Total tests: 143. Passed: 143. Failed: 0. Skipped: 0.
Test Run Successful.
Test execution time: 57.6756 Seconds
Build success

2] CodeFactor complains, therefore you see a red cross there.
Review and fix the issues from CodeFactor, see this page: https://www.codefactor.io/repository/github/stefh/system.linq.dynamic.core/pull/200

StefH · 2018-08-23T16:27:24Z

        }
+
+        [Fact]
+        public void ParseLambda_With_If_Guid_Null()


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

It was done!

StefH · 2018-08-23T16:27:36Z

+        [Fact]
+        public void ParseLambda_With_If_Guid_Null()
+        {
+            var objContext = new User();


It was done!

StefH · 2018-08-23T16:28:32Z

+        public void ParseLambda_With_If_Guid_Null()
+        {
+            var objContext = new User();
+            var guidEmpty = Guid.Empty;


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

It was done!

StefH · 2018-08-23T16:28:57Z

+            var objContext = new User();
+            var guidEmpty = Guid.Empty;
+            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)})";


It was done!

StefH · 2018-08-23T16:30:17Z

        }
+
+        [Fact]
+        public void ParseLambda_With_If_Guid_Null()


Shouldn't this be Iif ?

It was done!

StefH · 2018-08-23T16:37:31Z

+            var objContext = new User();
+            var guidEmpty = Guid.Empty;
+            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)})";


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)";

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.

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

StefH · 2018-08-23T16:39:01Z

        }
+
+        [Fact]
+        public void ParseLambda_With_If_Guid_Null()


Better text like: Guid_Equals_Null ?

It was done!

StefH · 2018-08-23T16:39:30Z

+        }
+
+        [Fact]
+        public void ParseLambda_With_If_Null_Guid()


Same comments as defined on other method also apply here.

It was done!

StefH · 2018-08-23T16:42:39Z

+        //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


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

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).

OlegNadymov · 2018-08-24T07:05:18Z

@StefH,
I can't open the link https://www.codefactor.io/repository/github/stefh/system.linq.dynamic.core/pull/200.
I see the error:
"Access is denied. You do not have permission to access /repository/github//pull/200."

…/System.Linq.Dynamic.Core into pull-requests-branch

OlegNadymov · 2018-08-24T07:41:10Z

@StefH all changes according to your review were done!

StefH · 2018-08-24T09:00:06Z

CodeFactor found multiple issues last seen at 6213fd1:

An opening curly bracket must not be followed by a blank line.

test\System.Linq.Dynamic.Core.Tests\DynamicExpressionParserTests.cs:13

The code must not contain multiple blank lines in a row.

test\System.Linq.Dynamic.Core.Tests\DynamicExpressionParserTests.cs:15

StefH · 2018-08-24T09:00:51Z

Small issues:

Please use // Arrange , // Act and // Assert flow (take a look at other tests). What I mean is that you add comment lines to make easy to read. See for example this: https://github.com/StefH/System.Linq.Dynamic.Core/blob/master/test/System.Linq.Dynamic.Core.Tests/DynamicExpressionParserTests.cs#L60
For code-factor, see my post above. I'll check why you need to login to see these comments...

StefH · 2018-08-24T09:28:25Z

+            string expressionText = $"iif(@0.Id == \"{someId}\", Guid.Parse(\"{guidEmpty}\"), Guid.Parse(\"{anotherId}\"))";
+
+            var lambda = DynamicExpressionParser.ParseLambda(typeof(User), null, expressionText, user);
+


Why not also check here on:

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

?

Yes. I've added it:

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

…acket

OlegNadymov · 2018-08-24T10:06:50Z

Ok! Now I got that you mean about // Arrange , // Act and // Assert.
I've added these comments. But I'm not sure about something like this:

            // Act
            var lambda = DynamicExpressionParser.ParseLambda(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(guidEmpty, result);

As you can see here the first Assert.NotNull is located inside Act. Is it ok?

OlegNadymov · 2018-08-24T11:19:49Z

@StefH Do I need mention you in my each comment? Or you get notifications about new comments anyway.

StefH · 2018-08-24T12:29:34Z

@OlegNadymov No need to mention me in the comments.

StefH · 2018-08-24T12:31:11Z

As you can see here the first Assert.NotNull is located inside Act. Is it ok?

for now it's ok.
What I sometimes do is use an // Act 1 and // Assert 1 and // Act 2 and // Assert 2
Or just move all acts and asserts together.

OlegNadymov · 2018-08-24T13:05:59Z

Ok! I'll do it like you said with several Act1, Act2 in next PRs

OlegNadymov · 2018-08-25T10:27:05Z

@StefH, what about this PR?

StefH · 2018-08-25T14:34:25Z

I will merge this weekend and create new nuget.

OlegNadymov · 2018-08-25T17:10:38Z

Ok! Then I'll add one more change for this PR.

OlegNadymov · 2018-08-25T18:37:15Z

And one more change for LessGreater parsing.

StefH · 2018-08-25T18:48:56Z

Maybe it's best to revert the last changes you did for LessGreater (unit test fail?). And create a new PR for this.

OlegNadymov · 2018-08-25T19:10:10Z

I've fixed unit test. It was problem with custom static type

OlegNadymov · 2018-08-25T19:13:54Z

Do you want that I create new PR? Or I can leave it here. In case with new PR, I need to create new branch for new PR because the current branch is using for this open PR. Do I understand correctly?

StefH · 2018-08-25T19:27:14Z

OK, keep in same branch. I'll check the new code in some time...

OlegNadymov · 2018-08-25T19:29:14Z

OK! Thank you! I'm looking forward to reviewing my changes. :)

StefH

I've added some comments, please take a look.

And I think I need to restructure the test class a bit in the future to make clear what's tested.

StefH · 2018-08-25T19:26:34Z

+            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}\"))";


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

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?

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?

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 !=.

OK, thanks for the detailed explanation.

StefH · 2018-08-26T07:21:06Z

-        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


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);

StefH · 2018-08-26T07:22:36Z

+            string name = "name1";
+            string note = "note1";
+            var textHolder = new TextHolder(name, note);
+            string expressionText = $"Name + \" (\" + Note + \")\"";


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

StefH · 2018-08-26T07:23:06Z

+            string name = "name1";
+            string note = "note1";
+            var textHolder = new TextHolder(name, note);
+            string expressionText = $"Note + \" (\" + Name + \")\"";


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

StefH · 2018-08-26T07:24:51Z

+        }
+
+        [Fact]
+        public void ParseLambda_With_Less_Greater()


rename to ParseLambda_Operator_Less_Greater_With_Guids ?

OlegNadymov · 2018-08-26T11:00:30Z

All review's changes were done.

StefH

OK

OlegNadymov · 2018-08-27T07:05:09Z

Thank you for merge! And please nuget.

StefH · 2018-08-27T10:26:02Z

1.0.8.17 is added to NuGet

OlegNadymov · 2018-08-27T11:56:29Z

@StefH Thanks!

Oleg Nadymov and others added 14 commits August 2, 2018 00:33

Fix for ParseLambda with itType and resultType: correct order of argu…

eca21da

…ments

Add references for missing classes

4b7d15a

Fix for missing classes: UserState and UserProfileDetails

cd4b420

Small update on unit-test (no need to check for DoesNotThrow)

c7d29f4

add comment to appveyor

e6ddce6

Only run SonarScanner on master branch

9a5e5e3

APPVEYOR_REPO_BRANCH

02bda33

APPVEYOR_PULL_REQUEST_NUMBER

e77c116

No working unit test for inner double quotes

d7d432b

Fix for inner double quotes

2c840f8

Merge branch 'master' into pull-requests-branch

8590347

Using {}

e13c9d1

Fix for parsing Guid and string in the same condition

6213fd1

Merge branch 'master' into pull-requests-branch

4d6a4d5

StefH self-requested a review August 23, 2018 16:21

StefH requested changes Aug 23, 2018

View reviewed changes

Oleg Nadymov added 2 commits August 24, 2018 10:35

FIxes after review

43765a3

Merge branch 'pull-requests-branch' of https://github.com/OlegNadymov…

10bb5bd

…/System.Linq.Dynamic.Core into pull-requests-branch

StefH reviewed Aug 24, 2018

View reviewed changes

Fixes after review: just add comments and remove empty lines after br…

a9c93f2

…acket

Fix for parsing expression with custom types with implicit convertions

cc90915

Oleg Nadymov added 3 commits August 25, 2018 20:13

Use "string" instead of "var"

ea0f3bd

Remove empty line

1290d20

Fix for LessGreater (<>) parsing.

53c73ac

Fix for unit test

d143a0b

StefH requested changes Aug 26, 2018

View reviewed changes

Changes according to review

dc9e24d

StefH approved these changes Aug 27, 2018

View reviewed changes

StefH merged commit b78629f into zzzprojects:master Aug 27, 2018

		string expressionText = $"iif(@0.Id == \"{someId}\", Guid.Parse(\"{guidEmpty}\"), Guid.Parse(\"{anotherId}\"))";

		var lambda = DynamicExpressionParser.ParseLambda(typeof(User), null, expressionText, user);

Uh oh!

Conversation

OlegNadymov commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

OlegNadymov commented Aug 23, 2018

Uh oh!

StefH commented Aug 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlegNadymov Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlegNadymov commented Aug 24, 2018

Uh oh!

OlegNadymov commented Aug 24, 2018

Uh oh!

StefH commented Aug 24, 2018

An opening curly bracket must not be followed by a blank line.

The code must not contain multiple blank lines in a row.

Uh oh!

StefH commented Aug 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OlegNadymov commented Aug 24, 2018 • edited by StefH Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OlegNadymov commented Aug 24, 2018

Uh oh!

StefH commented Aug 24, 2018

Uh oh!

StefH commented Aug 24, 2018

Uh oh!

OlegNadymov commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

OlegNadymov commented Aug 23, 2018 •

edited

Loading

codecov bot commented Aug 23, 2018 •

edited

Loading

StefH commented Aug 23, 2018 •

edited

Loading

OlegNadymov Aug 24, 2018 •

edited

Loading

OlegNadymov commented Aug 24, 2018 •

edited by StefH

Loading

OlegNadymov commented Aug 24, 2018 •

edited

Loading

OlegNadymov Aug 26, 2018 •

edited

Loading