Open StefH opened 6 months ago
@StefH is anything holding this PR back? I reviewed already the new changes :)
Thanks for reviewing. I have another question: did you review all the public extensions methods, are they correct and does is the return value ok?
@StefH I did now find a bit of time to play around with the actual solution. So far it works just find. (just the aggregate function confused me a bit, but it was RTFM) Find just some compile errors for the First / Single methods and create a PR on your working branch.
After playing with it a bit more I can't find issue with the System.Text.Json
implementation. So far I tested now the following functions:
Where
Select
Single
First
FirstOrDefault
Aggregate
Any
All
Last
@StefH Is there a possibility to get this branch as a preview like Nuget package? That would make it easier to integrate it into my main solution. Then the feedback loop would be more direct. Now I can only test with a playground project.
@paule96 I can upload the NuGets to https://www.myget.org/F/system-linq-dynamic-core/api/v3/index.json, is that enough? Or do you need the normal NuGet ?
@StefH this looks good :)
1.4.0-preview-01 packages are added to MyGet
Info about MyGet: https://github.com/WireMock-Net/WireMock.Net/wiki/MyGet-preview-versions
Disclaimer: All code refers to the
System.Text.Json
@StefH playing on a real world example already showed maybe a design flaw. But I'm not sure. So currently if you have a code like that:
json.Select("e => e.Manage")
The return type will be an JsonDocument
. But logically it should be an JsonElement
because it will be always a subelement of the original document. But also it will double the implementation complexity of this feature because I guess it would affect all extension methods.
So it's something that I would accept as a customer, but it feels also a bit weird.
Another finding is, that I don't know if the current implementation allows the validation of the expression in any way.
I understand from the docs a validation should be possible with that code:
ParameterExpression y = Expression.Parameter(typeof(JsonDocument), "y");
LambdaExpression e = DynamicExpressionParser.ParseLambda(new ParameterExpression[] { y }, null, "y => y.test");
Disclaimer: All code refers to the
System.Text.Json
@StefH playing on a real world example already showed maybe a design flaw. But I'm not sure. So currently if you have a code like that:
json.Select("e => e.Manage")
The return type will be an
JsonDocument
. But logically it should be anJsonElement
because it will be always a subelement of the original document. But also it will double the implementation complexity of this feature because I guess it would affect all extension methods.So it's something that I would accept as a customer, but it feels also a bit weird.
But when using Select, you can also create new objects, like
json.Select("new(Manage as Abc, Phone)")
In this case the result is a document which contains 0 or more JsonElements with an Abc and a Phone.
Correct? Or do I miss something?
@StefH that's correct I wasn't thinking about that use case. ✅
Another finding is, that I don't know if the current implementation allows the validation of the expression in any way.
I understand from the docs a validation should be possible with that code:
ParameterExpression y = Expression.Parameter(typeof(JsonDocument), "y"); LambdaExpression e = DynamicExpressionParser.ParseLambda(new ParameterExpression[] { y }, null, "y => y.test");
Can you please elaborate on this?
I was guessing that you can do with the snippet above a basic syntax validation like:
ParameterExpression y = Expression.Parameter(typeof(JsonDocument), "y");
_ = DynamicExpressionParser.ParseLambda(new ParameterExpression[] { y }, null, "y => y.test"); // No Error
_ = DynamicExpressionParser.ParseLambda(new ParameterExpression[] { y }, null, "y => y.test xy y.lol"); // Error
_ = DynamicExpressionParser.ParseLambda(new ParameterExpression[] { y }, null, "y => y.test != y.lol"); // No Error
And so on.
But currently I guess no matter what you provide as a parameter and lambda expression you will get the following error:
Unhandled exception. No property or field 'test' exists in type 'JsonDocument' (at index 7)
at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseMemberAccess(Type type, Expression expression, String id) in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 1889
at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParsePrimary() in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 814
at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseUnary() in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 801
at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseArithmetic() in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 746
at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseAdditive() in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 713
at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseShiftOperator() in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 689
at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseComparisonOperator() in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 477
at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseLogicalAndOrOperator() in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 409
at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseIn() in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 328
at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseAndOperator() in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 311
at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseOrOperator() in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 293
at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseLambdaOperator() in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 271
at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseNullCoalescingOperator() in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 258
at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseConditionalOperator() in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 242
at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseLambdaOperator() in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 278
at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseNullCoalescingOperator() in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 258
at System.Linq.Dynamic.Core.Parser.ExpressionParser.ParseConditionalOperator() in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 242
at System.Linq.Dynamic.Core.Parser.ExpressionParser.Parse(Type resultType, Boolean createParameterCtor) in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\Parser\ExpressionParser.cs:line 157
at System.Linq.Dynamic.Core.DynamicExpressionParser.ParseLambda(Type delegateType, ParsingConfig parsingConfig, Boolean createParameterCtor, ParameterExpression[] parameters, Type resultType, String expression, Object[] values) in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\DynamicExpressionParser.cs:line 120
at System.Linq.Dynamic.Core.DynamicExpressionParser.ParseLambda(ParsingConfig parsingConfig, Boolean createParameterCtor, ParameterExpression[] parameters, Type resultType, String expression, Object[] values) in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\DynamicExpressionParser.cs:line 98
at System.Linq.Dynamic.Core.DynamicExpressionParser.ParseLambda(ParameterExpression[] parameters, Type resultType, String expression, Object[] values) in D:\labs\System.Linq.Dynamic.Core\src\System.Linq.Dynamic.Core\DynamicExpressionParser.cs:line 391
at ConsoleApp_net6._0.Program.Json() in D:\labs\System.Linq.Dynamic.Core\src-console\ConsoleApp_net6.0\Program.cs:line 88
at ConsoleApp_net6._0.Program.Main(String[] args) in D:\labs\System.Linq.Dynamic.Core\src-console\ConsoleApp_net6.0\Program.cs:line 28
@StefH any opinions on the last comment? Maybe it's not needed for the first version of this package. But I know your community not good enough. For me, it would be just a nice to have. But definitely not needed for version one.
If you last comment is related to:
DynamicExpressionParser.ParseLambda(new ParameterExpression[] { y }, null, "y => y.test xy y.lol"); // Error
For that scenario, to my idea that expression ("y => y.test xy y.lol"
) is not a valid expression. Or do I miss something?
@StefH no not really.
every of the following lines throws the exception above:
ParameterExpression y = Expression.Parameter(typeof(JsonDocument), "y");
_ = DynamicExpressionParser.ParseLambda(new ParameterExpression[] { y }, null, "y => y.test");
_ = DynamicExpressionParser.ParseLambda(new ParameterExpression[] { y }, null, "y => y.test xy y.lol");
_ = DynamicExpressionParser.ParseLambda(new ParameterExpression[] { y }, null, "y => y.test != y.lol");
But I don't know if this is expected right now or if it should work. As mentioned for my code I really don't need it.
Sidenote: After working with the package for two weeks now, it feels pretty solid to me. 👍
@StefH no not really.
every of the following lines throws the exception above:
ParameterExpression y = Expression.Parameter(typeof(JsonDocument), "y"); _ = DynamicExpressionParser.ParseLambda(new ParameterExpression[] { y }, null, "y => y.test"); _ = DynamicExpressionParser.ParseLambda(new ParameterExpression[] { y }, null, "y => y.test xy y.lol"); _ = DynamicExpressionParser.ParseLambda(new ParameterExpression[] { y }, null, "y => y.test != y.lol");
But I don't know if this is expected right now or if it should work. As mentioned for my code I really don't need it.
Now I understand.
This cannot work because the JsonDocument does not have a test
property.
Only when a JsonDocument is converted to an anonymous class, then this class will have the test
property defined.
A possible solution would be to create a new SystemTextJsonDynamicExpressionParser.ParseLambda , however this will not be in scope for now.
@StefH what is the plan to merge this branch to master? what are the open points? Can I help at any?
@paule96
I noticed that the ThenBy
does not work correct, I need to remove that logic.
Issues
18 New issues
0 Accepted issues
Measures
0 Security Hotspots
0.0% Coverage on New Code
0.9% Duplication on New Code
@StefH is anything holding this PR back? I reviewed already the new changes :)