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

Built-in JavetProxyConverter: Better error reporting #434

Open
newk5 opened this issue Dec 29, 2024 · 3 comments
Open

Built-in JavetProxyConverter: Better error reporting #434

newk5 opened this issue Dec 29, 2024 · 3 comments

Comments

@newk5
Copy link

newk5 commented Dec 29, 2024

Hey so when I first started using the very first versions of Javet back in version 0.x.x I needed to proxy a java API so it could be called from javascript. Back then, Javet did not have this feature, so I created my own way to do it, which was very time consuming and error prone. Nowadays I was reading the Javet documentation I noticed that Javet now supports this with JavetProxyConverter which is great! I noticed it even supports function overloading even though javascript doesn't, by making all functions in javascript take varArgs and then resolving them at runtime to the appropriate java method.

However it doesn't report any errors or warnings and doesn't even throw any exceptions when calling a method with missmatched parameter count or wrong parameter types. I understand this can't be detected on javascript side because all proxied methods take varArgs, but it can be detected on the java side. And in my opinion it should, otherwise this hides many errors.

An example:

public class TestObj {
    
    
    public void test(){
        System.out.println("Called test");
    }
    public void test(int v){
        System.out.println("Called test2 ");
    }
}

try (NodeRuntime v8Runtime = V8Host.getNodeInstance().createV8Runtime()) {
       File f = new File("main.js");
       JavetProxyConverter c = new JavetProxyConverter();
       v8Runtime.setConverter(c);
       TestObj t = new TestObj();
       v8Runtime.getGlobalObject().set("testObj", t);
       v8Runtime.getExecutor(f).execute();
}

main.js:

testObj.test(1,2,3); //silently fails with no error or warning and continues execution
testObj.test();
testObj.test(2);
console.log("After test");

This prints:
"Called test"
"Called test2"
"After test"

In my opinion it should stop execution on the first call to testObj.test(1,2,3) because no such method exists and it should report an error message about wrong parameter count and the same should be done if the parameter types are the wrong type. Like I said I understand this is not possible to do on Javascript script side, because all proxied functions are varArgs but on the Java side in ScoredExecutable.java, it seems like it's already doing all those checks, but when the totalScore is 0 it doesn't report why its 0. I think all cases where totalScore = 0, it should report the corresponding error and stop execution. It would be very helpful to have the line number aswell where that error happened too.

I understand we can already create our own JavetProxyConverter but my suggestion is about improving the current built-in proxy converter. What do you think?

@caoccao
Copy link
Owner

caoccao commented Dec 29, 2024

Thank you for reporting this and sharing your thoughts. That makes sense.

Calling a JS function with arguments mismatch is valid with the rules as follows.

  1. If input argument size > function argument size, truncate the input argument size to the function argument size.
  2. If input argument size < function argument size, append the input arguments with undefined till its size matches the function argument size.
  3. Do not report error at all.

Javet follows these rules a few years ago. I guess what you discovered is more like a bug. It's supposed to call the most appropriate Java function with the rules above. Somehow, it doesn't. I think it's a bug and will fix it.

Please let me know if that works for you, or if you have any questions.

@newk5
Copy link
Author

newk5 commented Dec 29, 2024

Hmm I see. For me error reporting is the priority, I'd like to avoid implicit behaviour if possible. The reason is, usually when I want to proxy java objects, this means I'm trying to expose some API's only available in Java, to the node.js world, so node.js users can consume my Java APIs. When a user is consuming a new API that they are not familiar with, my priority is for them to learn it, learn the function names, how many params each takes, argument types etc... I want for them to know as soon as possible when they're doing something wrong, I dont want to hide it. I think most people when using a java proxy converter, probably have the same goals. I would prefer if the built-in proxy converter worked this way, but if there's demand for implicit behaviour like the rules you mentioned, maybe a sort of "ProxyMode" feature can be introduced, like: "ProxyMode.LAX" and "ProxyMode.STRICT", where "LAX" would work with the rules like you described and "STRICT" would correctly report miss-matching parameter count or parameter types. What do you think?

@caoccao
Copy link
Owner

caoccao commented Dec 30, 2024

I think the current behavior is to return undefined, and yes, it can throw an error instead.

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

No branches or pull requests

2 participants