#990: Forward jar options without changing order (#470)

related to exasol/script-languages-release#990 --------- Co-authored-by: Torsten Kilias <[email protected]>
exasol · Oct 25, 2024 · 1897b7a · 1897b7a
1 parent 23d5d1a
commit 1897b7a
Show file tree

Hide file tree

Showing 16 changed files with 143 additions and 56 deletions.
diff --git a/.github/workflows/check_bazel_tests.yml b/.github/workflows/check_bazel_tests.yml
@@ -34,10 +34,10 @@ jobs:
         include:
           - test: "//base/javacontainer/test:ExaStackTraceCleanerTest"
             name: "ExaStackTraceCleanerTest"
-          - test: "//base/javacontainer/test:javacontainer-test-legacy-parser"
-            name: "javacontainer-test-legacy-parser"
-          - test: "//base/javacontainer/test:javacontainer-test-ctpg-parser"
-            name: "javacontainer-test-ctpg-parser"
+          - test: "//base/javacontainer/test:javacontainer-test-extractor-legacy"
+            name: "javacontainer-test-extractor-legacy"
+          - test: "//base/javacontainer/test:javacontainer-test-extractor-v2"
+            name: "javacontainer-test-extractor-v2"
           - test: "//base/javacontainer/script_options/..."
             name: "javacontainer-script_options"
           - test: "//base/exaudflib/test/..."
@@ -46,18 +46,18 @@ jobs:
             name: "script_options_parser_ctpg"
           - test: "//base/script_options_parser/legacy/..."
             name: "script_options_parser_legacy"
-          - test: "--run_under='valgrind --leak-check=yes' --config=valgrind //base/javacontainer/test:javacontainer-test-legacy-parser"
-            name: "javacontainer-test-legacy-parser-with-valgrind"
-          - test: "--run_under='valgrind --leak-check=yes' --config=valgrind //base/javacontainer/test:javacontainer-test-ctpg-parser"
-            name: "javacontainer-test-ctpg-parser-with-valgrind"
+          - test: "--run_under='valgrind --leak-check=yes' --config=valgrind //base/javacontainer/test:javacontainer-test-extractor-legacy"
+            name: "javacontainer-test-extractor-legacy-with-valgrind"
+          - test: "--run_under='valgrind --leak-check=yes' --config=valgrind //base/javacontainer/test:javacontainer-test-extractor-v2"
+            name: "javacontainer-test-extractor-v2-with-valgrind"
           - test: "--run_under='valgrind --leak-check=yes' --config=valgrind //base/script_options_parser/ctpg/..."
             name: "script_options_parser_ctpg_with_valgrind"
           - test: "--run_under='valgrind --leak-check=yes' --config=valgrind //base/script_options_parser/legacy/..."
             name: "script_options_parser_legacy_with_valgrind"
-          - test: "--config=asan //base/javacontainer/test:javacontainer-test-legacy-parser"
-            name: "javacontainer-test-legacy-parser-with-asan"
-          - test: "--config=asan //base/javacontainer/test:javacontainer-test-ctpg-parser"
-            name: "javacontainer-test-ctpg-parser-with-asan"
+          - test: "--config=asan //base/javacontainer/test:javacontainer-test-extractor-legacy"
+            name: "javacontainer-test-extractor-legacy-with-asan"
+          - test: "--config=asan //base/javacontainer/test:javacontainer-test-extractor-v2"
+            name: "javacontainer-test-extractor-v2-with-asan"
           - test: "--config=asan //base/script_options_parser/ctpg/..."
             name: "script_options_parser_ctpg_with_asan"
           - test: "--config=asan //base/script_options_parser/legacy/..."

diff --git a/exaudfclient/base/javacontainer/javacontainer_impl.cc b/exaudfclient/base/javacontainer/javacontainer_impl.cc
@@ -56,9 +56,7 @@ void JavaVMImpl::parseScriptOptions(std::unique_ptr<JavaScriptOptions::Extractor
 
     m_jvmOptions = std::move(extractor->moveJvmOptions());
 
-    for (const auto & jar :  extractor->getJarPaths()) {
-        addJarToClasspath(jar);
-    }
+    extractor->iterateJarPaths([&](const std::string& s) { addJarToClasspath(s);});
 }
 
 void JavaVMImpl::shutdown() {

diff --git a/exaudfclient/base/javacontainer/script_options/converter.h b/exaudfclient/base/javacontainer/script_options/converter.h
@@ -3,15 +3,22 @@
 
 #include <string>
 #include <vector>
-#include <set>
+#include <functional>
 
 namespace SWIGVMContainers {
 
 namespace JavaScriptOptions {
 
+/**
+ * This class retrieves the raw Java script option values (scriptclass, jvmoption, jar)
+ * and converts them to the proper format expected by the JvmContainerImpl class.
+ * Besides the converter functions it provides methods to access the converted values.
+ */
 class Converter {
 
     public:
+        typedef std::function<void(const std::string &option)> tJarIteratorCallback;
+
         Converter();
 
         void convertScriptClassName(const std::string & value);
@@ -24,8 +31,7 @@ class Converter {
 
         virtual void convertExternalJar(const std::string & value) = 0;
 
-        virtual const std::set<std::string> & getJarPaths() const = 0;
-
+        virtual void iterateJarPaths(tJarIteratorCallback callback) const = 0;
 
     private:
 

diff --git a/exaudfclient/base/javacontainer/script_options/converter_legacy.cc b/exaudfclient/base/javacontainer/script_options/converter_legacy.cc
@@ -2,6 +2,7 @@
 #include "base/javacontainer/script_options/string_ops.h"
 #include <iostream>
 #include <sstream>
+#include <algorithm>
 
 namespace SWIGVMContainers {
 
@@ -20,6 +21,11 @@ void ConverterLegacy::convertExternalJar(const std::string& value) {
     }
 }
 
+void ConverterLegacy::iterateJarPaths(Converter::tJarIteratorCallback callback) const {
+    std::for_each(m_jarPaths.begin(), m_jarPaths.end(), callback);
+}
+
+
 
 } //namespace JavaScriptOptions
 

diff --git a/exaudfclient/base/javacontainer/script_options/converter_legacy.h b/exaudfclient/base/javacontainer/script_options/converter_legacy.h
@@ -14,16 +14,19 @@ namespace SWIGVMContainers {
 
 namespace JavaScriptOptions {
 
+/**
+ * This class is a specialization for the generic converter class.
+ * It implements conversion of the jar option according to the requirements in the old
+ * parser implementation.
+ */
 class ConverterLegacy : public Converter {
 
     public:
         ConverterLegacy();
 
         void convertExternalJar(const std::string & value);
 
-        const std::set<std::string> & getJarPaths() const {
-            return m_jarPaths;
-        }
+        void iterateJarPaths(tJarIteratorCallback callback) const override;
 
     private:
 

diff --git a/exaudfclient/base/javacontainer/script_options/converter_v2.cc b/exaudfclient/base/javacontainer/script_options/converter_v2.cc
@@ -2,6 +2,7 @@
 #include "base/javacontainer/script_options/string_ops.h"
 #include <iostream>
 #include <sstream>
+#include <algorithm>
 
 namespace SWIGVMContainers {
 
@@ -16,10 +17,15 @@ void ConverterV2::convertExternalJar(const std::string & value) {
     std::string jar;
 
     while (std::getline(stream, jar, ':')) {
-        m_jarPaths.insert(jar);
+        m_jarPaths.push_back(jar);
     }
 }
 
+void ConverterV2::iterateJarPaths(Converter::tJarIteratorCallback callback) const {
+    std::for_each(m_jarPaths.begin(), m_jarPaths.end(), callback);
+}
+
+
 } //namespace JavaScriptOptions
 
 } //namespace SWIGVMContainers
diff --git a/exaudfclient/base/javacontainer/script_options/converter_v2.h b/exaudfclient/base/javacontainer/script_options/converter_v2.h
@@ -3,7 +3,6 @@
 
 #include <string>
 #include <vector>
-#include <set>
 #include <memory>
 
 #include "base/javacontainer/script_options/converter.h"
@@ -14,20 +13,23 @@ namespace SWIGVMContainers {
 
 namespace JavaScriptOptions {
 
+/**
+ * This class is a specialization for the generic converter class.
+ * It implements conversion of the jar option according to the requirements in the new
+ * parser implementation.
+ */
 class ConverterV2 : public Converter {
 
     public:
         ConverterV2();
 
         void convertExternalJar(const std::string & value);
 
-        const std::set<std::string> & getJarPaths() const {
-            return m_jarPaths;
-        }
+        void iterateJarPaths(tJarIteratorCallback callback) const override;
 
     private:
 
-        std::set<std::string> m_jarPaths;
+        std::vector<std::string> m_jarPaths;
 
 };
 

diff --git a/exaudfclient/base/javacontainer/script_options/extractor.h b/exaudfclient/base/javacontainer/script_options/extractor.h
@@ -3,20 +3,46 @@
 
 #include <string>
 #include <vector>
-#include <set>
+#include <functional>
 
 
 namespace SWIGVMContainers {
 
 namespace JavaScriptOptions {
 
+/**
+ * Abstract interface for the Extractor class.
+ * Defines methods to extract the Java options from the script code.
+ * The extraction process searches for the known Java Options and handles them appropriatly.
+ * The script code is then modified, where the found options are removed.
+ * The interface defines methods to access the found Jar- and JvmOption options.
+ * The scriptclass and import options are processed internally by the respective extractor implementation.
+ */
 struct Extractor {
+
+    typedef std::function<void(const std::string &option)> tJarIteratorCallback;
+
     virtual ~Extractor() {}
 
-    virtual const std::set<std::string> & getJarPaths() const  = 0;
+    /**
+     * Access the found Jar paths. For each found jar path the given callback function is called with the jar option as argument.
+     */
+    virtual void iterateJarPaths(tJarIteratorCallback callback) const = 0;
 
+    /**
+     * Access the Jvm option options. The extractor implementations must store the found Jvm Options in a std::vector.
+     * The vector is returned as rvalue.
+     */
     virtual std::vector<std::string>&& moveJvmOptions() = 0;
 
+    /**
+     * Run the extraction. This will:
+     * 1. Add the first `scriptclass` option to the list of Jvm options.
+     * 2. Replace and (nested) reference of an `import` script
+     * 3. Find and store all Jar options
+     * 4. Find and store all `jvmoption` options
+     * 5. Remove `scriptclass`, `jar`, `import` and `jvmoption` from the script code. The behavior is implementation specific.
+     */
     virtual void extract(std::string & scriptCode) = 0;
 };
 

diff --git a/exaudfclient/base/javacontainer/script_options/extractor_impl.cc b/exaudfclient/base/javacontainer/script_options/extractor_impl.cc
@@ -16,12 +16,12 @@ ExtractorImpl<TParser, TConverter>::ExtractorImpl(std::unique_ptr<SwigFactory> s
 , m_converter() {}
 
 template<typename TParser, typename TConverter>
-inline const std::set<std::string> & ExtractorImpl<TParser, TConverter>::getJarPaths() const {
-    return m_converter.getJarPaths();
+void ExtractorImpl<TParser, TConverter>::iterateJarPaths(Extractor::tJarIteratorCallback callback) const {
+    return m_converter.iterateJarPaths(callback);
 }
 
 template<typename TParser, typename TConverter>
-inline std::vector<std::string>&& ExtractorImpl<TParser, TConverter>::moveJvmOptions() {
+std::vector<std::string>&& ExtractorImpl<TParser, TConverter>::moveJvmOptions() {
     return std::move(m_converter.moveJvmOptions());
 }
 

diff --git a/exaudfclient/base/javacontainer/script_options/extractor_impl.h b/exaudfclient/base/javacontainer/script_options/extractor_impl.h
@@ -17,14 +17,18 @@ namespace SWIGVMContainers {
 
 namespace JavaScriptOptions {
 
+/**
+ * Concrete implementation for the Extractor class.
+ * Given template parameter TParser and TConverter define concrete behavior.
+ */
 template<typename TParser, typename TConverter>
 class ExtractorImpl : public Extractor {
 
     public:
 
         ExtractorImpl(std::unique_ptr<SwigFactory> swigFactory);
 
-        const std::set<std::string> & getJarPaths() const override;
+        virtual void iterateJarPaths(tJarIteratorCallback callback) const override;
         std::vector<std::string>&& moveJvmOptions() override;
 
         void extract(std::string & scriptCode);

diff --git a/exaudfclient/base/javacontainer/script_options/test/converter_test.cc b/exaudfclient/base/javacontainer/script_options/test/converter_test.cc
@@ -7,22 +7,24 @@
 using namespace SWIGVMContainers::JavaScriptOptions;
 
 
-class LegacyConverterJarTest : public ::testing::TestWithParam<std::pair<std::string, std::set<std::string>>> {};
+class LegacyConverterJarTest : public ::testing::TestWithParam<std::pair<std::string, std::vector<std::string>>> {};
 
 TEST_P(LegacyConverterJarTest, jar) {
-    const std::pair<std::string, std::set<std::string>> option_value = GetParam();
+    const std::pair<std::string, std::vector<std::string>> option_value = GetParam();
     const std::string jar_option_value = option_value.first;
 
     ConverterLegacy converter;
     converter.convertExternalJar(option_value.first);
-    ASSERT_EQ(converter.getJarPaths(), option_value.second);
+    std::vector<std::string> result;
+    converter.iterateJarPaths([&](auto jar) {result.push_back(jar);});
+    ASSERT_EQ(result, option_value.second);
 }
 
-const std::vector<std::pair<std::string, std::set<std::string>>> jar_strings =
+const std::vector<std::pair<std::string, std::vector<std::string>>> jar_strings =
         {
-            std::make_pair("test.jar:test2.jar", std::set<std::string>({"test.jar", "test2.jar"})),
-            std::make_pair("\"test.jar:test2.jar\"", std::set<std::string>({"\"test.jar", "test2.jar\""})),
-            std::make_pair("t\\:est.jar:test2.jar", std::set<std::string>({"t\\", "est.jar", "test2.jar"})),
+            std::make_pair("test.jar:test2.jar", std::vector<std::string>({"test.jar", "test2.jar"})), //basic splitting
+            std::make_pair("test.jar:test.jar", std::vector<std::string>({"test.jar"})), //filter duplicates
+            std::make_pair("testDEF.jar:testABC.jar", std::vector<std::string>({"testABC.jar", "testDEF.jar"})), //alphabetical order
         };
 
 INSTANTIATE_TEST_SUITE_P(
@@ -33,27 +35,29 @@ INSTANTIATE_TEST_SUITE_P(
 
 
 
-class ConverterV2JarTest : public ::testing::TestWithParam<std::pair<std::string, std::set<std::string>>> {};
+class ConverterV2JarTest : public ::testing::TestWithParam<std::pair<std::string, std::vector<std::string>>> {};
 
 TEST_P(ConverterV2JarTest, jar) {
-    const std::pair<std::string, std::set<std::string>> option_value = GetParam();
+    const std::pair<std::string, std::vector<std::string>> option_value = GetParam();
     const std::string jar_option_value = option_value.first;
     std::cerr << "DEBUG: " << jar_option_value << std::endl;
 
     ConverterV2 converter;
     converter.convertExternalJar(option_value.first);
-    ASSERT_EQ(converter.getJarPaths(), option_value.second);
+    std::vector<std::string> result;
+    converter.iterateJarPaths([&](auto jar) {result.push_back(jar);});
+    ASSERT_EQ(result, option_value.second);
 }
 
-const std::vector<std::pair<std::string, std::set<std::string>>> jar_escape_sequences =
+const std::vector<std::pair<std::string, std::vector<std::string>>> jar_strings_v2 =
         {
-            std::make_pair("test.jar:test2.jar", std::set<std::string>({"test.jar", "test2.jar"})),
-            std::make_pair("\"test.jar:test2.jar\"", std::set<std::string>({"\"test.jar", "test2.jar\""})),
-            std::make_pair("t\\:est.jar:test2.jar", std::set<std::string>({"t\\", "est.jar", "test2.jar"})),
+            std::make_pair("test.jar:test2.jar", std::vector<std::string>({"test.jar", "test2.jar"})), //basic splitting
+            std::make_pair("test.jar:test.jar", std::vector<std::string>({"test.jar", "test.jar"})), //keep duplicates
+            std::make_pair("testDEF.jar:testABC.jar", std::vector<std::string>({"testDEF.jar", "testABC.jar"})), //maintain order
         };
 
 INSTANTIATE_TEST_SUITE_P(
     Converter,
     ConverterV2JarTest,
-    ::testing::ValuesIn(jar_escape_sequences)
+    ::testing::ValuesIn(jar_strings_v2)
 );
diff --git a/exaudfclient/base/javacontainer/test/BUILD b/exaudfclient/base/javacontainer/test/BUILD
@@ -17,7 +17,7 @@ JAVACONTAINER_PERF_TEST_SRCS = ["cpp/javacontainer_perf_test.cc", "cpp/exaudf_wr
             "cpp/swig_factory_test.h", "cpp/swig_factory_test.cc"]
 
 cc_test(
-    name = "javacontainer-test-legacy-parser",
+    name = "javacontainer-test-extractor-legacy",
     srcs = JAVACONTAINER_TEST_SRCS,
     deps = [
         "//base/javacontainer:javacontainer",
@@ -27,13 +27,13 @@ cc_test(
 )
 
 cc_test(
-    name = "javacontainer-test-ctpg-parser",
-    srcs = JAVACONTAINER_TEST_SRCS + ["cpp/javacontainer_ctpg_test.cc"],
+    name = "javacontainer-test-extractor-v2",
+    srcs = JAVACONTAINER_TEST_SRCS + ["cpp/javacontainer_extractor_v2_test.cc"],
     deps = [
         "//base/javacontainer:javacontainer",
         "@googletest//:gtest_main",
     ],
-    defines = ["USE_CTPG_PARSER"],
+    defines = ["USE_EXTRACTOR_V2"],
     data = ["test.jar", "other_test.jar"]
 )
 
@@ -55,6 +55,6 @@ cc_test(
         "//base/javacontainer:javacontainer",
         "@googletest//:gtest_main",
     ],
-    defines = ["USE_CTPG_PARSER"],
+    defines = ["USE_EXTRACTOR_V2"],
     data = ["test.jar", "other_test.jar"]
 )
diff --git a/...ainer/test/cpp/javacontainer_ctpg_test.cc → ...st/cpp/javacontainer_extractor_v2_test.cc b/...ainer/test/cpp/javacontainer_ctpg_test.cc → ...st/cpp/javacontainer_extractor_v2_test.cc
@@ -3,9 +3,12 @@
 #include "gmock/gmock.h"
 #include "base/javacontainer/test/cpp/javavm_test.h"
 #include "base/javacontainer/test/cpp/swig_factory_test.h"
+#include "base/javacontainer/javacontainer.h"
 #include <string.h>
 #include <memory>
 
+using ::testing::MatchesRegex;
+
 class JavaContainerEscapeSequenceTest : public ::testing::TestWithParam<std::pair<std::string, std::string>> {};
 
 TEST_P(JavaContainerEscapeSequenceTest, quoted_jvm_option) {