microsoft / GraphEngine

Microsoft Graph Engine
http://www.graphengine.io/
MIT License
2.19k stars 328 forks source link

Bug in FanoutSearch with net6 #359

Open alenas opened 1 year ago

alenas commented 1 year ago

File: src\Modules\LIKQ\FanoutSearch\QueryLanguage\ExpressionBuilder.cs

line 22 should be like this (with a type), otherwise it errors out and some unit tests do not pass: private static readonly MethodInfo s_string_contains = typeof(String).GetMethod("Contains", new Type[] { typeof(String) });

TaviTruman commented 1 year ago

@alenas - I am looking at this right now; what Test Case failed for you?

alenas commented 1 year ago

Most in FanoutSearch.UnitTest.JSONDSLTest (and 2). I ran from Visual Studio compiled with .net 6.0. And I see there are more JSON issues when using Newtonsoft version 13...

TaviTruman commented 1 year ago

image

TaviTruman commented 1 year ago

I found the actual bug! image

image

the and_pred has a value of null and the fails in the call: return Expression.Condition(GenerateBooleanPredicateExpression(pred_object, icell), action_const, noaction_const);

in method: GenerateConditionalPredicateExpression

alenas commented 1 year ago

I changed language version to 10 on FanoutSearch (as I left only .net 6 compilation) and I that's why that line is broken for me. but seems like your and_pred is null for similar reasons, I will see how it works in my project.

TaviTruman commented 1 year ago

Here is the offending outer code block:

image

The call to FanoutSearchDescriptor fails: FanoutSearchDescriptor fanoutSearch_desc = new FanoutSearchDescriptor(queryPath, queryObject);

Here is the JsonQuery content that fails:

image

These LIKQ JsonQuery statements are slightly different:

image

The first one fails while the second one works. It looks like the results field is the first statement is expecting a Conditional while the second one is not.

TaviTruman commented 1 year ago

Something is not right with the JSON parsing logic. I don't use JsonDSL but do use LambdaDSL and KnowledgeGraph LIKQ dialects and have not had any problems with the .NET 6 version.

alenas commented 1 year ago

I actually changed few other files as well: src\Modules\LIKQ\FanoutSearch\Descriptors\FanoutSearchDescriptor.cs Line 141 was: JObject jobj = JsonConvert.DeserializeObject(m_origin_query); changed to: var jobj = JObject.Parse(m_origin_query);

file: src\Modules\LIKQ\FanoutSearch\QueryLanguage\Standard.cs line 31 was: string json_search_body = Newtonsoft.Json.JsonConvert.SerializeObject(search_body); changed to: string json_search_body; if (search_body is string) { json_search_body = search_body.ToString(); } else { json_search_body = Newtonsoft.Json.JsonConvert.SerializeObject(search_body); }

at the moment all my test pass. but there is one problem, as I think that JSON message that goes to the server is still not good. If I try to use json for start from like this, then I get server response error:

var search = KnowledgeGraph.StartFrom($@"{{""type"":""Person"",""match"":{{""text"":""{word}""}}}}";);

TaviTruman commented 1 year ago

Yeah - I totally don't use Json in any KnowledgeGraph productions:

This works just fine for me:

        var pathsx = KnowledgeGraph
            .StartFrom(123)
            .FollowEdge("isa_agent")
            .FollowEdge("isa_buyer_lead")
            .VisitNode(_ => _.return_if(_.GetField<string>("rdf_subject").Contains("business_process") == _.has("workflow")), select: new List<string> { "open_house","new_client" })
            .VisitNode(queryPart => queryPart.continue_if(queryPart.has_cell_id(0)))
                .ToList();

image

TaviTruman commented 1 year ago

@alenas not all LIKQ language dialects will process json statements. I am just looking at Lambda DSL, and Json DSL processing as there are some test case failing.

TaviTruman commented 1 year ago

@alenas I think I have the fix in place; will need to re-run the existing tests and add a few new tests and the changes to the JSON parser do require some changes to the code.