This is an automated email from the ASF dual-hosted git repository. jhyde pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/calcite.git
commit 644a3f0727038e955791ad068c01d6dffc27a34d Author: Julian Hyde <[email protected]> AuthorDate: Wed Jun 7 11:00:15 2023 -0700 [CALCITE-5762] Create class TestUnsafe, that contains unsafe methods used by tests --- build.gradle.kts | 3 +- .../concurrent/ConcurrentTestCommandScript.java | 85 ++-------------- .../java/org/apache/calcite/util/TestUnsafe.java | 113 +++++++++++++++++++++ 3 files changed, 120 insertions(+), 81 deletions(-) diff --git a/build.gradle.kts b/build.gradle.kts index dd45718913..87d7b9399c 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -761,8 +761,7 @@ allprojects { "**/org/apache/calcite/adapter/os/Processes${'$'}ProcessFactory.class", "**/org/apache/calcite/adapter/os/OsAdapterTest.class", "**/org/apache/calcite/runtime/Resources${'$'}Inst.class", - "**/org/apache/calcite/test/concurrent/ConcurrentTestCommandScript.class", - "**/org/apache/calcite/test/concurrent/ConcurrentTestCommandScript${'$'}ShellCommand.class", + "**/org/apache/calcite/util/TestUnsafe.class", "**/org/apache/calcite/util/Unsafe.class", "**/org/apache/calcite/test/Unsafe.class" ) diff --git a/core/src/test/java/org/apache/calcite/test/concurrent/ConcurrentTestCommandScript.java b/core/src/test/java/org/apache/calcite/test/concurrent/ConcurrentTestCommandScript.java index e0e96aa6dd..b9e6136037 100644 --- a/core/src/test/java/org/apache/calcite/test/concurrent/ConcurrentTestCommandScript.java +++ b/core/src/test/java/org/apache/calcite/test/concurrent/ConcurrentTestCommandScript.java @@ -17,24 +17,17 @@ package org.apache.calcite.test.concurrent; import org.apache.calcite.jdbc.SqlTimeoutException; +import org.apache.calcite.util.TestUnsafe; import org.apache.calcite.util.Unsafe; import org.apache.calcite.util.Util; -import org.slf4j.Logger; - -import java.io.BufferedInputStream; -import java.io.BufferedOutputStream; import java.io.BufferedReader; import java.io.BufferedWriter; import java.io.File; import java.io.IOException; -import java.io.InputStream; -import java.io.OutputStream; import java.io.PrintWriter; -import java.io.Reader; import java.io.StringReader; import java.io.StringWriter; -import java.io.Writer; import java.lang.reflect.Constructor; import java.sql.Connection; import java.sql.DriverManager; @@ -279,68 +272,6 @@ public class ConcurrentTestCommandScript return chars; } - /** - * Runs an external application process. - * - * @param pb ProcessBuilder for the application - * @param logger if not null, command and exit status will be logged here - * @param appInput if not null, data will be copied to application's stdin - * @param appOutput if not null, data will be captured from application's - * stdout and stderr - * @return application process exit value - */ - static int runAppProcess( - ProcessBuilder pb, - Logger logger, - Reader appInput, - Writer appOutput) throws IOException, InterruptedException { - pb.redirectErrorStream(true); - if (logger != null) { - logger.info("start process: " + pb.command()); - } - Process p = pb.start(); - - // Setup the input/output streams to the subprocess. - // The buffering here is arbitrary. Javadocs strongly encourage - // buffering, but the size needed is very dependent on the - // specific application being run, the size of the input - // provided by the caller, and the amount of output expected. - // Since this method is currently used only by unit tests, - // large-ish fixed buffer sizes have been chosen. If this - // method becomes used for something in production, it might - // be better to have the caller provide them as arguments. - if (appInput != null) { - OutputStream out = - new BufferedOutputStream( - p.getOutputStream(), - 100 * 1024); - int c; - while ((c = appInput.read()) != -1) { - out.write(c); - } - out.flush(); - } - if (appOutput != null) { - InputStream in = - new BufferedInputStream( - p.getInputStream(), - 100 * 1024); - int c; - while ((c = in.read()) != -1) { - appOutput.write(c); - } - appOutput.flush(); - in.close(); - } - p.waitFor(); - - int status = p.exitValue(); - if (logger != null) { - logger.info("exit status=" + status + " from " + pb.command()); - } - return status; - } - /** * Gets ready to execute: loads script FILENAME applying external variable * BINDINGS. @@ -1561,17 +1492,13 @@ public class ConcurrentTestCommandScript Integer threadId = executor.getThreadId(); storeMessage(threadId, command); - // argv[0] is found on $PATH. Working directory is the script's home - // directory. - // - // WARNING: ProcessBuilder is security-sensitive. Its use is currently - // safe because this code is under "core/test". Developers must not move - // this code into "core/main". - ProcessBuilder pb = new ProcessBuilder(argv); - pb.directory(scriptDirectory); try { + // argv[0] is found on $PATH. + // Working directory is the script's home directory. // direct stdout & stderr to the threadWriter - int status = runAppProcess(pb, null, null, getThreadWriter(threadId)); + int status = + TestUnsafe.runAppProcess(argv, scriptDirectory, null, null, + getThreadWriter(threadId)); if (status != 0) { storeMessage(threadId, "command " + command + ": exited with status " + status); diff --git a/core/src/test/java/org/apache/calcite/util/TestUnsafe.java b/core/src/test/java/org/apache/calcite/util/TestUnsafe.java new file mode 100644 index 0000000000..e0d2f8f7ce --- /dev/null +++ b/core/src/test/java/org/apache/calcite/util/TestUnsafe.java @@ -0,0 +1,113 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.calcite.util; + +import org.checkerframework.checker.nullness.qual.Nullable; +import org.slf4j.Logger; + +import java.io.BufferedInputStream; +import java.io.BufferedOutputStream; +import java.io.File; +import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.io.Reader; +import java.io.Writer; +import java.util.List; + +/** + * Unsafe methods to be used by tests. + * + * <p>Contains methods that call JDK methods that the + * <a href="https://github.com/policeman-tools/forbidden-apis";>forbidden + * APIs checker</a> does not approve of. + * + * <p>This class is excluded from the check, so methods called via this class + * will not fail the build. + * + * <p>Why is this in {@code core/src/test} and not in {@code testkit/src/main}? + * Because some of the methods (e.g. {@link #runAppProcess}) are so unsafe that + * they must not be on the class-path of production code. + */ +public abstract class TestUnsafe { + /** + * Runs an external application process. + * + * @param argumentList command name and its arguments + * @param directory working directory + * @param logger if not null, command and exit status will be logged here + * @param appInput if not null, data will be copied to application's stdin + * @param appOutput if not null, data will be captured from application's + * stdout and stderr + * @return application process exit value + */ + public static int runAppProcess(List<String> argumentList, File directory, + @Nullable Logger logger, @Nullable Reader appInput, + @Nullable Writer appOutput) throws IOException, InterruptedException { + + // WARNING: ProcessBuilder is security-sensitive. Its use is currently + // safe because this code is under "core/test". Developers must not move + // this code into "core/main". + final ProcessBuilder pb = new ProcessBuilder(argumentList); + pb.directory(directory); + pb.redirectErrorStream(true); + if (logger != null) { + logger.info("start process: " + pb.command()); + } + Process p = pb.start(); + + // Setup the input/output streams to the subprocess. + // The buffering here is arbitrary. Javadocs strongly encourage + // buffering, but the size needed is very dependent on the + // specific application being run, the size of the input + // provided by the caller, and the amount of output expected. + // Since this method is currently used only by unit tests, + // large-ish fixed buffer sizes have been chosen. If this + // method becomes used for something in production, it might + // be better to have the caller provide them as arguments. + if (appInput != null) { + OutputStream out = + new BufferedOutputStream( + p.getOutputStream(), + 100 * 1024); + int c; + while ((c = appInput.read()) != -1) { + out.write(c); + } + out.flush(); + } + if (appOutput != null) { + InputStream in = + new BufferedInputStream( + p.getInputStream(), + 100 * 1024); + int c; + while ((c = in.read()) != -1) { + appOutput.write(c); + } + appOutput.flush(); + in.close(); + } + p.waitFor(); + + int status = p.exitValue(); + if (logger != null) { + logger.info("exit status=" + status + " from " + pb.command()); + } + return status; + } +}
