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

Use Classgraph to Import Unknown Packages Not Yet Loaded #3395

Merged
merged 4 commits into from
Feb 3, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions engine/table/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ dependencies {
// TODO(deephaven-core#3204): t-digest 3.3 appears to have higher errors than 3.2
implementation 'com.tdunning:t-digest:3.2'
implementation 'com.squareup:javapoet:1.13.0'
implementation 'io.github.classgraph:classgraph:4.8.154'

implementation project(':plugin')
implementation depCommonsLang3
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@
import io.deephaven.io.logger.Logger;
import io.deephaven.plugin.type.ObjectTypeLookup;
import io.deephaven.util.annotations.VisibleForTesting;
import io.github.classgraph.ClassGraph;
import io.github.classgraph.ClassInfo;
import io.github.classgraph.ScanResult;
import org.codehaus.groovy.control.CompilationUnit;
import org.codehaus.groovy.control.Phases;
import org.codehaus.groovy.tools.GroovyClass;
Expand Down Expand Up @@ -270,10 +273,9 @@ private Exception maybeRewriteStackTrace(String scriptName, String currentScript
return e;
}

private static String classForNameString(String className) throws ClassNotFoundException {
private static Class<?> loadClass(String className) throws ClassNotFoundException {
try {
Class.forName(className);
return className;
return Class.forName(className, false, GroovyDeephavenSession.class.getClassLoader());
} catch (ClassNotFoundException e) {
if (className.contains(".")) {
// handle inner class cases
Expand All @@ -282,7 +284,7 @@ private static String classForNameString(String className) throws ClassNotFoundE
String tail = className.substring(index + 1);
String newClassName = head + "$" + tail;

return classForNameString(newClassName);
return loadClass(newClassName);
} else {
throw e;
}
Expand All @@ -291,7 +293,7 @@ private static String classForNameString(String className) throws ClassNotFoundE

private static boolean classExists(String className) {
try {
classForNameString(className);
loadClass(className);
return true;
} catch (ClassNotFoundException e) {
return false;
Expand All @@ -300,7 +302,7 @@ private static boolean classExists(String className) {

private static boolean functionExists(String className, String functionName) {
try {
Method[] ms = Class.forName(classForNameString(className)).getMethods();
Method[] ms = loadClass(className).getMethods();

for (Method m : ms) {
if (m.getName().equals(functionName)) {
Expand All @@ -316,7 +318,7 @@ private static boolean functionExists(String className, String functionName) {

private static boolean fieldExists(String className, String fieldName) {
try {
Field[] fs = Class.forName(classForNameString(className)).getFields();
Field[] fs = loadClass(className).getFields();

for (Field f : fs) {
if (f.getName().equals(fieldName)) {
Expand Down Expand Up @@ -393,9 +395,9 @@ public static String isValidImportString(Logger log, String importString) {
}
} else {
if (isWildcard) {
okToImport = classExists(body) || (Package.getPackage(body) != null); // Note: this might not find a
// valid package that has never
// been loaded
okToImport = classExists(body) || (Package.getPackage(body) != null)
|| packageIsVisibleToClassGraph(body);

if (!okToImport) {
if (ALLOW_UNKNOWN_GROOVY_PACKAGE_IMPORTS) {
// Check for proper form of a package. Pass a package star import that is plausible. Groovy is
Expand All @@ -404,7 +406,7 @@ public static String isValidImportString(Logger log, String importString) {
"(\\p{javaJavaIdentifierStart}\\p{javaJavaIdentifierPart}*\\.)+\\p{javaJavaIdentifierStart}\\p{javaJavaIdentifierPart}*";
if (body.matches(javaIdentifierPattern)) {
log.info().append("Package or class \"").append(body)
.append("\" could not be verified. If this is a package, it could mean that no class from that package has been seen by the classloader.")
.append("\" could not be verified.")
.endl();
okToImport = true;
} else {
Expand All @@ -414,7 +416,7 @@ public static String isValidImportString(Logger log, String importString) {
}
} else {
log.warn().append("Package or class \"").append(body)
.append("\" could not be verified. If this is a package, it could mean that no class from that package has been seen by the classloader.")
.append("\" could not be verified.")
.endl();
}
}
Expand All @@ -435,6 +437,15 @@ public static String isValidImportString(Logger log, String importString) {
}
}

private static boolean packageIsVisibleToClassGraph(String packageImport) {
try (ScanResult scanResult = new ClassGraph().enableClassInfo().acceptPackages(packageImport).scan()) {
final Optional<ClassInfo> firstClassFound = scanResult.getAllClasses().stream().findFirst();
// force load the class so that the jvm is aware of the package
firstClassFound.ifPresent(ClassInfo::loadClass);
nbauernfeind marked this conversation as resolved.
Show resolved Hide resolved
return firstClassFound.isPresent();
}
}

private void updateScriptImports(String importString) {
String fixedImportString = isValidImportString(log, importString);
if (fixedImportString != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
*/
package io.deephaven.engine.util.scripts;

import io.deephaven.base.verify.Assert;
import io.deephaven.engine.table.Table;
import io.deephaven.engine.table.TableDefinition;
import io.deephaven.engine.util.GroovyDeephavenSession;
Expand All @@ -13,6 +12,7 @@
import io.deephaven.plugin.type.ObjectTypeLookup.NoOp;
import org.apache.commons.lang3.mutable.MutableInt;
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;

Expand Down Expand Up @@ -58,7 +58,7 @@ public void testNullCast() {
final Table y = fetchTable("y");
final TableDefinition definition = y.getDefinition();
final Class<?> colClass = definition.getColumn("X").getDataType();
Assert.equals(colClass, "colClass", java.util.List.class);
Assert.assertEquals(colClass, java.util.List.class);
}

@Test
Expand All @@ -71,9 +71,9 @@ public void testScriptDefinedClass() {
"}\n" +
"obj = new MyObj(1)\n" +
"result = emptyTable(1).select(\"A = obj.a\")");
Assert.neqNull(fetch("obj", Object.class), "fetchObject");
Assert.assertNotNull(fetch("obj", Object.class));
final Table result = fetchTable("result");
Assert.eqFalse(result.isFailed(), "result.isFailed()");
Assert.assertFalse(result.isFailed());
}

@Test
Expand All @@ -85,8 +85,19 @@ public void testScriptResultOrder() {
final String[] names = new String[] {"x", "z", "y", "u"};
final MutableInt offset = new MutableInt();
changes.created.forEach((name, type) -> {
Assert.eq(name, "name", names[offset.getAndIncrement()], "names[offset.getAndIncrement()]");
Assert.assertEquals(name, names[offset.getAndIncrement()]);
});
}

@Test
public void testUnloadedWildcardPackageImport() {
// it's unlikely we have loaded anything from this groovy package
final String packageString = "groovy.time";

if (this.getClass().getClassLoader().getDefinedPackage(packageString) != null) {
Assert.fail("Package '" + packageString + "' is already loaded, test with a more obscure package.");
}
session.evaluateScript("import " + packageString + ".*");
}
}

2 changes: 1 addition & 1 deletion extensions/classgraph/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ dependencies {

compileOnly 'javax.inject:javax.inject:1'

api 'io.github.classgraph:classgraph:4.8.149'
api 'io.github.classgraph:classgraph:4.8.154'

Classpaths.inheritAutoService(project)
}