diff --git a/.github/workflows/check_bazel_tests.yml b/.github/workflows/check_bazel_tests.yml index 9d0e28dc2..4f3e183ce 100644 --- 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 index 1235d297a..9405ddb7d 100644 --- a/exaudfclient/base/javacontainer/javacontainer_impl.cc +++ b/exaudfclient/base/javacontainer/javacontainer_impl.cc @@ -56,9 +56,7 @@ void JavaVMImpl::parseScriptOptions(std::unique_ptrmoveJvmOptions()); - 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 index f2e5de3e4..1d0e39ded 100644 --- a/exaudfclient/base/javacontainer/script_options/converter.h +++ b/exaudfclient/base/javacontainer/script_options/converter.h @@ -3,15 +3,22 @@ #include #include -#include +#include 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 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 & 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 index 0dfbf9f78..23e84f5c5 100644 --- 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 #include +#include 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 index bddd9151d..90aa2a0d4 100644 --- a/exaudfclient/base/javacontainer/script_options/converter_legacy.h +++ b/exaudfclient/base/javacontainer/script_options/converter_legacy.h @@ -14,6 +14,11 @@ 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: @@ -21,9 +26,7 @@ class ConverterLegacy : public Converter { void convertExternalJar(const std::string & value); - const std::set & 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 index 4c78e7dcf..e41e07a32 100644 --- 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 #include +#include 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 index a5f135de9..c829c7330 100644 --- a/exaudfclient/base/javacontainer/script_options/converter_v2.h +++ b/exaudfclient/base/javacontainer/script_options/converter_v2.h @@ -3,7 +3,6 @@ #include #include -#include #include #include "base/javacontainer/script_options/converter.h" @@ -14,6 +13,11 @@ 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: @@ -21,13 +25,11 @@ class ConverterV2 : public Converter { void convertExternalJar(const std::string & value); - const std::set & getJarPaths() const { - return m_jarPaths; - } + void iterateJarPaths(tJarIteratorCallback callback) const override; private: - std::set m_jarPaths; + std::vector m_jarPaths; }; diff --git a/exaudfclient/base/javacontainer/script_options/extractor.h b/exaudfclient/base/javacontainer/script_options/extractor.h index 2db401b98..8d1f66902 100644 --- a/exaudfclient/base/javacontainer/script_options/extractor.h +++ b/exaudfclient/base/javacontainer/script_options/extractor.h @@ -3,20 +3,46 @@ #include #include -#include +#include 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 tJarIteratorCallback; + virtual ~Extractor() {} - virtual const std::set & 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&& 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 index 842911e9b..d5273aa69 100644 --- a/exaudfclient/base/javacontainer/script_options/extractor_impl.cc +++ b/exaudfclient/base/javacontainer/script_options/extractor_impl.cc @@ -16,12 +16,12 @@ ExtractorImpl::ExtractorImpl(std::unique_ptr s , m_converter() {} template -inline const std::set & ExtractorImpl::getJarPaths() const { - return m_converter.getJarPaths(); +void ExtractorImpl::iterateJarPaths(Extractor::tJarIteratorCallback callback) const { + return m_converter.iterateJarPaths(callback); } template -inline std::vector&& ExtractorImpl::moveJvmOptions() { +std::vector&& ExtractorImpl::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 index 849faa311..ef7520c3e 100644 --- a/exaudfclient/base/javacontainer/script_options/extractor_impl.h +++ b/exaudfclient/base/javacontainer/script_options/extractor_impl.h @@ -17,6 +17,10 @@ namespace SWIGVMContainers { namespace JavaScriptOptions { +/** + * Concrete implementation for the Extractor class. + * Given template parameter TParser and TConverter define concrete behavior. + */ template class ExtractorImpl : public Extractor { @@ -24,7 +28,7 @@ class ExtractorImpl : public Extractor { ExtractorImpl(std::unique_ptr swigFactory); - const std::set & getJarPaths() const override; + virtual void iterateJarPaths(tJarIteratorCallback callback) const override; std::vector&& 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 index 04b8b00e9..dc2725bc8 100644 --- 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>> {}; +class LegacyConverterJarTest : public ::testing::TestWithParam>> {}; TEST_P(LegacyConverterJarTest, jar) { - const std::pair> option_value = GetParam(); + const std::pair> 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 result; + converter.iterateJarPaths([&](auto jar) {result.push_back(jar);}); + ASSERT_EQ(result, option_value.second); } -const std::vector>> jar_strings = +const std::vector>> jar_strings = { - std::make_pair("test.jar:test2.jar", std::set({"test.jar", "test2.jar"})), - std::make_pair("\"test.jar:test2.jar\"", std::set({"\"test.jar", "test2.jar\""})), - std::make_pair("t\\:est.jar:test2.jar", std::set({"t\\", "est.jar", "test2.jar"})), + std::make_pair("test.jar:test2.jar", std::vector({"test.jar", "test2.jar"})), //basic splitting + std::make_pair("test.jar:test.jar", std::vector({"test.jar"})), //filter duplicates + std::make_pair("testDEF.jar:testABC.jar", std::vector({"testABC.jar", "testDEF.jar"})), //alphabetical order }; INSTANTIATE_TEST_SUITE_P( @@ -33,27 +35,29 @@ INSTANTIATE_TEST_SUITE_P( -class ConverterV2JarTest : public ::testing::TestWithParam>> {}; +class ConverterV2JarTest : public ::testing::TestWithParam>> {}; TEST_P(ConverterV2JarTest, jar) { - const std::pair> option_value = GetParam(); + const std::pair> 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 result; + converter.iterateJarPaths([&](auto jar) {result.push_back(jar);}); + ASSERT_EQ(result, option_value.second); } -const std::vector>> jar_escape_sequences = +const std::vector>> jar_strings_v2 = { - std::make_pair("test.jar:test2.jar", std::set({"test.jar", "test2.jar"})), - std::make_pair("\"test.jar:test2.jar\"", std::set({"\"test.jar", "test2.jar\""})), - std::make_pair("t\\:est.jar:test2.jar", std::set({"t\\", "est.jar", "test2.jar"})), + std::make_pair("test.jar:test2.jar", std::vector({"test.jar", "test2.jar"})), //basic splitting + std::make_pair("test.jar:test.jar", std::vector({"test.jar", "test.jar"})), //keep duplicates + std::make_pair("testDEF.jar:testABC.jar", std::vector({"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 index 36fb36203..a3ff25cc7 100644 --- 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/exaudfclient/base/javacontainer/test/cpp/javacontainer_ctpg_test.cc b/exaudfclient/base/javacontainer/test/cpp/javacontainer_extractor_v2_test.cc similarity index 98% rename from exaudfclient/base/javacontainer/test/cpp/javacontainer_ctpg_test.cc rename to exaudfclient/base/javacontainer/test/cpp/javacontainer_extractor_v2_test.cc index e66168e50..ec615b637 100644 --- a/exaudfclient/base/javacontainer/test/cpp/javacontainer_ctpg_test.cc +++ b/exaudfclient/base/javacontainer/test/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 #include +using ::testing::MatchesRegex; + class JavaContainerEscapeSequenceTest : public ::testing::TestWithParam> {}; TEST_P(JavaContainerEscapeSequenceTest, quoted_jvm_option) { diff --git a/exaudfclient/base/javacontainer/test/cpp/javacontainer_test.cc b/exaudfclient/base/javacontainer/test/cpp/javacontainer_test.cc index 3ea3a28ce..88de7e0d4 100644 --- a/exaudfclient/base/javacontainer/test/cpp/javacontainer_test.cc +++ b/exaudfclient/base/javacontainer/test/cpp/javacontainer_test.cc @@ -49,7 +49,7 @@ TEST(JavaContainer, basic_jar_script_class_with_white_spaces) { TEST(JavaContainer, basic_jar_with_white_spaces) { const std::string script_code = "%jar base/javacontainer/test/test.jar \t ;"; -#ifndef USE_CTPG_PARSER //The parsers behave differently: The legacy parser removes trailing white spaces. +#ifndef USE_EXTRACTOR_V2 //The parsers behave differently: The legacy parser removes trailing white spaces. JavaVMTest vm(script_code); EXPECT_EQ(vm.getJavaVMInternalStatus().m_classpath, "/exaudf/base/javacontainer/exaudf_deploy.jar:base/javacontainer/test/test.jar"); #else @@ -67,6 +67,35 @@ TEST(JavaContainer, basic_jar_with_white_spaces) { #endif } +TEST(JavaContainer, basic_jars_ordering) { + /* + * This test validates correct behavior of collecting the %jar options in Extractor V1/V2. + * Both jar files referenced in `script_code` do not exist. + * The JavaVM will throw an exception with the first JAR file it gets from Extractor. + * For extractor V1: Our assumption is that the exception will be for `base/javacontainer/test/abc.jar` + * as this is alphabetically the first JAR file. (re-ordering) + * For extractor V2: Our assumption is that the exception will be for `base/javacontainer/test/test1.jar` + * as this is the first JAR file. (no re-ordering) + */ + const std::string script_code = "%jar base/javacontainer/test/test1.jar:base/javacontainer/test/abc.jar;"; + +#ifndef USE_EXTRACTOR_V2 + const char* regexExpectedException = "^.*Java VM cannot find 'base/javacontainer/test/abc\\.jar': No such file or directory$"; +#else + const char* regexExpectedException = "^.*Java VM cannot find 'base/javacontainer/test/test1\\.jar': No such file or directory$"; +#endif + EXPECT_THROW({ + try + { + JavaVMTest vm(script_code); + } + catch( const SWIGVMContainers::JavaVMach::exception& e ) + { + EXPECT_THAT( e.what(), MatchesRegex(regexExpectedException)); + throw; + } + }, SWIGVMContainers::JavaVMach::exception ); +} TEST(JavaContainer, basic_inline) { const std::string script_code = "import java.time.LocalDateTime;" @@ -495,7 +524,7 @@ TEST(JavaContainer, import_script_script_class_option_ignored) { EXPECT_EQ(vm.getJavaVMInternalStatus().m_localClasspath, "/tmp"); const std::string expected_script_code = "package com.exasol;\r\n" -#ifndef USE_CTPG_PARSER //The parsers behave differently: The legacy parser incorrectly keeps imported scriptclass options +#ifndef USE_EXTRACTOR_V2 //The parsers behave differently: The legacy parser incorrectly keeps imported scriptclass options "%scriptclass com.exasol.udf_profiling.UdfProfiler;" #endif "\n" diff --git a/exaudfclient/base/javacontainer/test/cpp/javavm_test.cc b/exaudfclient/base/javacontainer/test/cpp/javavm_test.cc index fd2b89b1a..b1f17f785 100644 --- a/exaudfclient/base/javacontainer/test/cpp/javavm_test.cc +++ b/exaudfclient/base/javacontainer/test/cpp/javavm_test.cc @@ -18,7 +18,7 @@ JavaVMTest::JavaVMTest(std::string scriptCode, std::unique_ptr swigFactory) { SWIGVMContainers::SWIGVM_params->script_code = scriptCode.data(); -#ifndef USE_CTPG_PARSER +#ifndef USE_EXTRACTOR_V2 std::unique_ptr extractor = std::make_unique(std::move(swigFactory)); #else diff --git a/exaudfclient/docs/java_script_options_parser.drawio.png b/exaudfclient/docs/java_script_options_parser.drawio.png new file mode 100644 index 000000000..c51fac44e Binary files /dev/null and b/exaudfclient/docs/java_script_options_parser.drawio.png differ