Newtonsoft.Json interop lacks array prototype/functions

[BhaaLseN]

So...I thought I was done with my upgrades after #1766 but then I came across a runtime error in one of the tools I got floating around. For lack of time, that one is still on .NET 6 (other components in the project that need to be updated first) and relies on Newtonsoft.Json rather than System.Text.Json.

The runtime error was this one:

System.ArgumentException: Accessed JArray values with invalid key value: "map". Int32 array index expected.
 at Newtonsoft.Json.Linq.JArray.get_Item(Object key)
--- End of stack trace from previous location ---
 at Jint.Runtime.ExceptionHelper.ThrowMeaningfulException(Engine engine, TargetInvocationException exception)
 at Jint.Runtime.Interop.Reflection.ReflectionAccessor.GetValue(Engine engine, Object target)
 at Jint.Runtime.Descriptors.Specialized.ReflectionDescriptor.get_CustomValue()
 at Jint.Native.Object.ObjectInstance.UnwrapJsValue(PropertyDescriptor desc, JsValue thisObject)
 at Jint.Engine.GetValue(Reference reference, Boolean returnReferenceToPool)
 at Jint.Runtime.Interpreter.Expressions.JintCallExpression.EvaluateInternal(EvaluationContext context)
 at Jint.Runtime.Interpreter.Expressions.JintMemberExpression.EvaluateInternal(EvaluationContext context)
 at Jint.Runtime.Interpreter.Expressions.JintCallExpression.EvaluateInternal(EvaluationContext context)
 at Jint.Runtime.Interpreter.Expressions.JintExpression.GetValue(EvaluationContext context)
 at Jint.Runtime.Interpreter.Expressions.JintAssignmentExpression.SimpleAssignmentExpression.AssignToIdentifier(EvaluationContext context, JintIdentifierExpression left, JintExpression right, Boolean hasEvalOrArguments)
 at Jint.Runtime.Interpreter.Expressions.JintAssignmentExpression.SimpleAssignmentExpression.EvaluateInternal(EvaluationContext context)
 at Jint.Runtime.Interpreter.Expressions.JintExpression.GetValue(EvaluationContext context)
 at Jint.Runtime.Interpreter.Statements.JintExpressionStatement.ExecuteInternal(EvaluationContext context)
 at Jint.Runtime.Interpreter.Statements.JintBlockStatement.ExecuteSingle(EvaluationContext context)
 at Jint.Runtime.Interpreter.Statements.JintBlockStatement.ExecuteBlock(EvaluationContext context)
 at Jint.Runtime.Interpreter.Statements.JintIfStatement.ExecuteInternal(EvaluationContext context)
 at Jint.Runtime.Interpreter.Statements.JintBlockStatement.ExecuteBlock(EvaluationContext context)
 at Jint.Runtime.Interpreter.Statements.JintIfStatement.ExecuteInternal(EvaluationContext context)
 at Jint.Runtime.Interpreter.Statements.JintBlockStatement.ExecuteBlock(EvaluationContext context)
 at Jint.Runtime.Interpreter.Statements.JintIfStatement.ExecuteInternal(EvaluationContext context)
 at Jint.Native.Function.ScriptFunction.Call(JsValue thisObject, JsValue[] arguments)
 at Jint.Runtime.Interpreter.Expressions.JintCallExpression.EvaluateInternal(EvaluationContext context)
 at Jint.Runtime.Interpreter.Expressions.JintExpression.GetValue(EvaluationContext context)
 at Jint.Runtime.Interpreter.Statements.JintExpressionStatement.ExecuteInternal(EvaluationContext context)
 at Jint.Engine.ScriptEvaluation(ScriptRecord scriptRecord)
 at Jint.Engine.ExecuteWithConstraints[T](Boolean strict, Func`1 callback)
 at Jint.Engine.Execute(Script script)
 at Jint.Engine.Evaluate(String code)

And the code is pretty innocent, easy to turn into a test method that could go into Jint.Tests.PublicInterface\InteropTests.NewtonsoftJson.cs:

[Fact]
public void ArraysShouldPassThroughCorrectly()
{
    var engine = new Engine();

    const string json = @"
    {
        'entries': [
            { 'id': 1, 'name': 'One' },
            { 'id': 2, 'name': 'Two' },
            { 'id': 3, 'name': 'Three' }
        ]
    }";

    var obj = JObject.Parse(json);
    engine.SetValue("o", obj);
    var names = engine.Evaluate("o.entries.map(function(e) { return e.name; })").AsArray();
    Assert.Equal((uint) 3, names.Length);
    Assert.Equal("One", names[0]);
    Assert.Equal("Two", names[1]);
    Assert.Equal("Three", names[2]);
}

The Evaluate call already fails, and I tried debugging this; but I didn't really find a suitable place to slot in. I couldn't find any code dedicated to making Newtonsoft.Json work, so it probably just happens to work for the most things (with the reflection magic in ObjectWrapper.)

Rather than taking the .map as prototype member function, it attempts to look up an array/object index with the same name (and fails.)

If anyone has a good location to add this to (and/or an idea on how to do it in a sustainable way,) let me know, and I can try taking a look on the weekend.

[BhaaLseN]

I still couldn't find a decent location to slot this in, but I guess I can share my current workaround.

Since it isn't large JSON payloads nor performance critical, I just roundtrip through a string:

if (data is JObject jObject)
    data = new JsonParser(engine).Parse(jObject.ToString());

Might not be useful to everyone, but it could be a viable workaround for some.

[lahma]

Maybe you can find some interop ideas from STJ converter test cases that handle indexing and attach array prototype to objects when needed.

[BhaaLseN]

Actually, I missed this call to engine._referenceResolver.TryGetCallable in JintCallExpression.EvaluateInternal. Maybe that can help.

[lahma]

I'm starting to get flashbacks, Newtonsoft is quire horrible as it throws exceptions from indexers that seem valid to be used, Jint gets confused by them and workarounds are are hard source seems valid (it's not missing property) but calling it will throw ("map").

[BhaaLseN]

Somethings weird. Using a custom IReferenceResolver and checking what the passed callee is (thats a Reference with an ObjectWrapper of the JArray, including the Prototype set to engine.Intrinsics.Array.PrototypeObject) I noticed that the prototype is missing all the array properties that are initialized by ArrayPrototype. ArrayPrototype._initialized is false.

At the same time, there is an undefined member property called "map" which isn't PropertyDescriptor.Undefined.
ObjectWrapper.GetOwnProperty tries to find an appropriate accessor, and because it isn't PropertyDescriptor.Undefined it also caches the result using ObjectWrapper.SetProperty.

descriptor = Value: {undefined}, Flags: Enumerable | CustomJsValue | NonData
PropertyDescriptor.Undefined = Value: null, Flags: CustomJsValue

That descriptor is returned regardless, so no matter if the caching happens or not, it is trying to do something with that undefined accessor.

Mostly documenting this for my future self, so I can remember later when I get back to this.

[lahma]

#1828 includes a fix for this too

[BhaaLseN]

Cool! I'll take a look later and see if I can just push my tests as-is to get some more coverage in (unless that PR already includes similar ones.)