HIVE-28984: Replace nashorn-core with graalvm which is compatible wit…

[maheshrajus]

…h ASF license

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

[ayushtkn]

I think no need to ignore the test takes care of ignoring if nashorn isn't in classpath

[maheshrajus]

OK, fixed

[maheshrajus]

This test case using "jdk.nashorn.internal.ir.debug.ObjectSizeCalculator" for calculating the object size.
For correcting this test case we have jira https://issues.apache.org/jira/browse/HIVE-28997 to address it. thx !

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[okumin]

Both libraries are licensed as UPL. It's fine as it's classified as Category A.

• https://mvnrepository.com/artifact/org.graalvm.js/js-scriptengine/23.0.8
• https://mvnrepository.com/artifact/org.graalvm.js/js/23.0.8

[okumin]

I'm checking this document.
https://www.graalvm.org/latest/reference-manual/js/NashornMigrationGuide/

I guess allowAllAccess(true) is redundant: https://github.com/oracle/graaljs/blob/release/graal-vm/23.0/graal-js/src/com.oracle.truffle.js.scriptengine/src/com/oracle/truffle/js/scriptengine/GraalJSScriptEngine.java#L346

[maheshrajus]

@okumin thanks for the review.
i checked and debug the above code. so here we are setting builder.allowAllAccess(true) in the method updateForNashornCompatibilityMode(). But this method call when contextConfig is null. But in our current case we are setting flags related to nashorn compatibility("js.nashorn-compat") in Context config and calling method GraalJSScriptEngine.create(). So we will not hit the null case(see code below) and the method(updateForNashornCompatibilityMode) will not call. So here setting "allowAllAccess(true)" in Context builder is ok in my opinion. Please let me know your opinion on same. thank you !

        if (contextConfig == null) {      --> in our current case this will not be NULL.
            contextConfigToUse = Context.newBuilder(new String[]{"js"}).allowExperimentalOptions(true);
            contextConfigToUse.option("js.syntax-extensions", "true");
            contextConfigToUse.option("js.load", "true");
            contextConfigToUse.option("js.print", "true");
            contextConfigToUse.option("js.global-arguments", "true");
            contextConfigToUse.option("js.charset", "UTF-8");
            if (NASHORN_COMPATIBILITY_MODE) {
                updateForNashornCompatibilityMode(contextConfigToUse);
            } else if (Boolean.getBoolean("graaljs.insecure-scriptengine-access")) {
                updateForScriptEngineAccessibility(contextConfigToUse);
            }
        }

    private static void updateForNashornCompatibilityMode(Context.Builder builder) {
        builder.allowAllAccess(true);
        builder.allowHostAccess(NASHORN_HOST_ACCESS);
        builder.useSystemExit(true);
    }

[okumin]

You'll be right. What if .allowAllAccess(true) is removed? The document doesn't mention the option.

try (Context context = Context.newBuilder().allowExperimentalOptions(true).option("js.nashorn-compat", "true").build()) {
      context.eval("js", "print(__LINE__)");
  }

If GraalJS works with Hive without the option, it's better from the point of view of security.
https://www.graalvm.org/sdk/javadoc/org/graalvm/polyglot/Context.Builder.html#allowAllAccess(boolean)

[maheshrajus]

yeah, i removed property .allowAllAccess(true) and checked it previously, we are getting error "failed due to: Unknown identifier: get". So in our case we need to pass this field. thanks !

[maheshrajus]

@okumin @ayushtkn Can you please review and approve the changes? thanks !

[ayushtkn]

LGTM