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

Add Json support (WIP...) #791

Open
wants to merge 49 commits into
base: master
Choose a base branch
from
Open

Add Json support (WIP...) #791

wants to merge 49 commits into from

Conversation

StefH
Copy link
Collaborator

@StefH StefH commented Apr 8, 2024

No description provided.

@StefH StefH added the feature label Apr 8, 2024
@StefH StefH self-assigned this Apr 8, 2024
@StefH StefH mentioned this pull request Apr 8, 2024
@StefH StefH changed the title Add Json support Add Json support (WIP...) Apr 9, 2024
@StefH
Copy link
Collaborator Author

StefH commented Apr 11, 2024

@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?

@paule96
Copy link

paule96 commented Apr 15, 2024

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

@paule96
Copy link

paule96 commented Apr 23, 2024

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.

@StefH
Copy link
Collaborator Author

StefH commented Apr 23, 2024

@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 ?

@paule96
Copy link

paule96 commented Apr 23, 2024

@StefH this looks good :)

@StefH
Copy link
Collaborator Author

StefH commented Apr 23, 2024

1.4.0-preview-01 packages are added to MyGet

Info about MyGet: https://github.com/WireMock-Net/WireMock.Net/wiki/MyGet-preview-versions

@paule96
Copy link

paule96 commented Apr 24, 2024

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.

@paule96
Copy link

paule96 commented Apr 24, 2024

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

@StefH
Copy link
Collaborator Author

StefH commented Apr 25, 2024

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.

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?

@paule96
Copy link

paule96 commented Apr 25, 2024

@StefH that's correct I wasn't thinking about that use case. ✅

@StefH
Copy link
Collaborator Author

StefH commented Apr 25, 2024

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?

@paule96
Copy link

paule96 commented Apr 26, 2024

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

@paule96
Copy link

paule96 commented May 7, 2024

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

@StefH
Copy link
Collaborator Author

StefH commented May 7, 2024

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?

@paule96
Copy link

paule96 commented May 7, 2024

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

@paule96
Copy link

paule96 commented May 7, 2024

Sidenote: After working with the package for two weeks now, it feels pretty solid to me. 👍

@StefH
Copy link
Collaborator Author

StefH commented May 7, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants