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

Numeric wrapper classes coverted based on its values #782

Closed
vepo opened this issue Nov 17, 2023 · 2 comments
Closed

Numeric wrapper classes coverted based on its values #782

vepo opened this issue Nov 17, 2023 · 2 comments
Assignees

Comments

@vepo
Copy link

vepo commented Nov 17, 2023

Using latest GraalVM.js Engine, we are facing some issues on methods that accepts Object as parameters and validates the received class. Our biggest problem involves Kafka Connect Struct which on the method put validates the number type its been used as parameter.

The main issue with GraalVM.js is that even using numeric wrapper class, all numbers are converted based on its value. This mean that if I use Long.valueOf("5"), this will be converted in a integer value. If I use Long.valueOf("2147483648") it will be converted in a long value.

Is there a way to keep the numeric wrapper class?

This is the code I used for testing.

package dev.vepo.graalvmjs.numbers;

import java.security.InvalidParameterException;

import javax.script.ScriptEngine;
import javax.script.ScriptException;

import com.oracle.truffle.js.scriptengine.GraalJSScriptEngine;
import org.graalvm.polyglot.Context;
import org.graalvm.polyglot.HostAccess;

public class NumberConversionError {

  public static void expectLong(Object shouldBeLong) {
    System.out.println(shouldBeLong.getClass());
    if (!(shouldBeLong instanceof Long)) {
      throw new InvalidParameterException("This is not a long value!");
    }
  }

  public static void main(String[] args) throws ScriptException {
    ScriptEngine engine = GraalJSScriptEngine.create(null, Context.newBuilder("js")
                                                                  .allowHostAccess(HostAccess.ALL)
                                                                  .allowHostClassLookup(s -> true)
                                                                  .allowExperimentalOptions(true)
                                                                  .out(System.out)
                                                                  .err(System.err)
                                                                  .option("js.nashorn-compat", "true"));
    engine.eval("""
                    var NumberConversionError = Java.type("dev.vepo.graalvmjs.numbers.NumberConversionError");
                    var Long = Java.type("java.lang.Long");
                    print(NumberConversionError);
                    NumberConversionError.expectLong(Long.valueOf("2147483648"));
                    NumberConversionError.expectLong(Long.valueOf("5"));
                """);
  }
}

The sample code can be found here: https://github.com/vepo/graalvm-js-number-conversion

@oubidar-Abderrahim oubidar-Abderrahim self-assigned this Dec 6, 2023
@oubidar-Abderrahim
Copy link
Member

Thank you for reporting this, we'll take a look into it shortly.
Tracked internally on GR 50804

@iamstolis
Copy link
Member

Is there a way to keep the numeric wrapper class?

Not when you are invoking a Java method that takes Object. When you need a particular primitive type, I suggest you to invoke a (helper) Java method that takes a parameter of the desired type. I understand that this can be clumsy but it is the safe approach.

Some background: JavaScript has just one number type (ignoring bigint). This type corresponds to Java double. That's why graal-js works with the numbers as if they were doubles. It is using various types (like int) internally but that's an implementation detail. The good news is that the internals of graal-js support long in a limited way. So, it was not that difficult to ensure that long is not converted to a different numeric type eagerly (as it was done in your test-case). In other words, your long-based test-case should work with the mentioned change.

I understand that it would be nice to be able to pass other numeric types (byte, short, float) in the same way. Unfortunately, that would require larger and more risky changes within graal-js. So, we are not pursuing this direction (for now). We may revisit this decision if we see some other compelling use-cases.

@iamstolis iamstolis self-assigned this Dec 7, 2023
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

3 participants