#878: Fixed jvmoption and parameters containing a space (#473)

related to exasol/script-languages-release#878
exasol · Nov 6, 2024 · f2ba8d2 · f2ba8d2
1 parent e42b5a8
commit f2ba8d2
Show file tree

Hide file tree

Showing 12 changed files with 206 additions and 21 deletions.
diff --git a/exaudfclient/base/javacontainer/script_options/BUILD b/exaudfclient/base/javacontainer/script_options/BUILD
@@ -9,7 +9,8 @@ cc_library(
             ":extractor_impl.cc", ":converter_legacy.cc", ":converter_legacy.h",
             ":converter_v2.cc", ":converter_v2.h",
             ":keywords.h", ":keywords.cc", ":checksum.h", ":checksum.cc", ":parser_ctpg.cc",
-            ":parser_ctpg_script_importer.cc", ":parser_ctpg_script_importer.h", ":string_ops.h", ":string_ops.cc"],
+            ":parser_ctpg_script_importer.cc", ":parser_ctpg_script_importer.h",
+            ":parser_ctpg_jvm_options_parser.cc", ":parser_ctpg_jvm_options_parser.h", ":string_ops.h", ":string_ops.cc"],
     deps = ["@ssl//:ssl", "//base/script_options_parser/legacy:script_option_lines_parser_legacy",
             "//base/script_options_parser/ctpg:script_option_lines_parser_ctpg", "//base/utils:utils",
             "//base/exaudflib:header", "//base/exaudflib:exaudflib-deps", "//base/swig_factory:swig_factory_if",

diff --git a/exaudfclient/base/javacontainer/script_options/converter.cc b/exaudfclient/base/javacontainer/script_options/converter.cc
@@ -18,21 +18,6 @@ void Converter::convertScriptClassName(const std::string & value) {
     m_jvmOptions.push_back("-Dexasol.scriptclass=" + value);
 }
 
-void Converter::convertJvmOption(const std::string & value) {
-    for (size_t start = 0, delim = 0; ; start = delim + 1) {
-        start = value.find_first_not_of(m_whitespace, start);
-        if (start == std::string::npos)
-            break;
-        delim = value.find_first_of(m_whitespace, start);
-        if (delim != std::string::npos) {
-            m_jvmOptions.push_back(value.substr(start, delim - start));
-        }
-        else {
-            m_jvmOptions.push_back(value.substr(start));
-            break;
-        }
-    }
-}
 
 
 } //namespace JavaScriptOptions

diff --git a/exaudfclient/base/javacontainer/script_options/converter.h b/exaudfclient/base/javacontainer/script_options/converter.h
@@ -23,7 +23,7 @@ class Converter {
 
         void convertScriptClassName(const std::string & value);
 
-        void convertJvmOption(const std::string & value);
+        virtual void convertJvmOption(const std::string & value) = 0;
 
         std::vector<std::string>&& moveJvmOptions() {
             return std::move(m_jvmOptions);
@@ -33,7 +33,7 @@ class Converter {
 
         virtual void iterateJarPaths(tJarIteratorCallback callback) const = 0;
 
-    private:
+    protected:
 
         std::vector<std::string> m_jvmOptions;
 

diff --git a/exaudfclient/base/javacontainer/script_options/converter_legacy.cc b/exaudfclient/base/javacontainer/script_options/converter_legacy.cc
@@ -21,6 +21,16 @@ void ConverterLegacy::convertExternalJar(const std::string& value) {
     }
 }
 
+void ConverterLegacy::convertJvmOption(const std::string& value) {
+    std::string::size_type start = 0, delim = 0;
+    while ((start = value.find_first_not_of(m_whitespace, delim)) != std::string::npos) {
+        delim = value.find_first_of(m_whitespace, start);
+        const std::string::size_type len = (std::string::npos == delim) ?  std::string::npos : delim - start;
+        m_jvmOptions.push_back(value.substr(start, len));
+    }
+}
+
+
 void ConverterLegacy::iterateJarPaths(Converter::tJarIteratorCallback callback) const {
     std::for_each(m_jarPaths.begin(), m_jarPaths.end(), callback);
 }

diff --git a/exaudfclient/base/javacontainer/script_options/converter_legacy.h b/exaudfclient/base/javacontainer/script_options/converter_legacy.h
@@ -24,7 +24,9 @@ class ConverterLegacy : public Converter {
     public:
         ConverterLegacy();
 
-        void convertExternalJar(const std::string & value);
+        void convertExternalJar(const std::string & value) override;
+
+        void convertJvmOption(const std::string & value)  override;
 
         void iterateJarPaths(tJarIteratorCallback callback) const override;
 

diff --git a/exaudfclient/base/javacontainer/script_options/converter_v2.cc b/exaudfclient/base/javacontainer/script_options/converter_v2.cc
@@ -1,5 +1,6 @@
 #include "base/javacontainer/script_options/converter_v2.h"
 #include "base/javacontainer/script_options/string_ops.h"
+#include "base/javacontainer/script_options/parser_ctpg_jvm_options_parser.h"
 #include <iostream>
 #include <sstream>
 #include <algorithm>
@@ -21,6 +22,11 @@ void ConverterV2::convertExternalJar(const std::string & value) {
     }
 }
 
+
+void ConverterV2::convertJvmOption(const std::string & value) {
+    JvmOptionsCTPG::parseJvmOptions(value, m_jvmOptions);
+}
+
 void ConverterV2::iterateJarPaths(Converter::tJarIteratorCallback callback) const {
     std::for_each(m_jarPaths.begin(), m_jarPaths.end(), callback);
 }

diff --git a/exaudfclient/base/javacontainer/script_options/converter_v2.h b/exaudfclient/base/javacontainer/script_options/converter_v2.h
@@ -23,7 +23,9 @@ class ConverterV2 : public Converter {
     public:
         ConverterV2();
 
-        void convertExternalJar(const std::string & value);
+        void convertExternalJar(const std::string & value) override;
+
+        void convertJvmOption(const std::string & value)  override;
 
         void iterateJarPaths(tJarIteratorCallback callback) const override;
 

diff --git a/exaudfclient/base/javacontainer/script_options/parser_ctpg_jvm_options_parser.cc b/exaudfclient/base/javacontainer/script_options/parser_ctpg_jvm_options_parser.cc
@@ -0,0 +1,98 @@
+#include "base/javacontainer/script_options/parser_ctpg_jvm_options_parser.h"
+#include "base/script_options_parser/ctpg/ctpg.hpp"
+#include <sstream>
+
+using namespace exaudf_ctpg;
+using namespace exaudf_ctpg::ftors;
+
+namespace SWIGVMContainers {
+
+namespace JavaScriptOptions {
+
+namespace JvmOptionsCTPG {
+
+
+const auto to_options(std::string&& e)
+{
+    tJvmOptions options{e};
+    return options;
+}
+
+auto&& add_option(std::string&& e, tJvmOptions&& ob)
+{
+    ob.push_back(std::move(e));
+    return std::move(ob);
+}
+
+auto convert_escape_sequence(std::string_view escape_seq)
+{
+    if (escape_seq == R"_(\ )_") {
+        return std::string_view(" ");
+    } else if (escape_seq == R"_(\t)_") {
+        return std::string_view("\t");
+    } else if (escape_seq == R"_(\f)_") {
+        return std::string_view("\f");
+    } else if (escape_seq == R"_(\v)_") {
+        return std::string_view("\v");
+    } else if (escape_seq == R"_(\\)_") {
+        return std::string_view("\\");
+    } else {
+        throw std::invalid_argument(std::string("Internal parser error: Unexpected escape sequence " + std::string(escape_seq)));
+    }
+}
+
+
+constexpr char not_separator_pattern[] = R"_([^ \x09\x0c\x0b])_";
+constexpr char whitespaces_pattern[] = R"_([ \x09\x0c\x0b]+)_";
+constexpr char escape_pattern[] = R"_(\\\\|\\t|\\ |\\f|\\v)_";
+
+constexpr regex_term<not_separator_pattern> not_separator("not_separator");
+constexpr regex_term<whitespaces_pattern> whitespaces("whitespace");
+constexpr regex_term<escape_pattern> escape_seq("escape_seq");
+
+constexpr nterm<tJvmOptions> text("text");
+constexpr nterm<std::string> option_element("option_element");
+
+
+constexpr parser jvm_option_parser(
+    text,
+    terms(escape_seq, not_separator, whitespaces),
+    nterms(text, option_element),
+    rules(
+        text()
+            >= [] () {return tJvmOptions();},
+        text(option_element)
+            >= [](auto&& e) { return to_options(std::move(e)); },
+        text(text, whitespaces)
+            >= [](auto&& ob, skip) { return std::move(ob); },
+        text(text, whitespaces, option_element)
+            >= [](auto&& ob, skip, auto&& e) { return add_option(std::move(e), std::move(ob)); },
+        option_element(not_separator)
+            >= [](auto ns) { return std::string(ns); },
+        option_element(option_element, not_separator)
+            >= [](auto &&ob, auto c) { return std::move(ob.append(c)); },
+        option_element(option_element, escape_seq)
+            >= [](auto &&ob, auto es) { return std::move(ob.append(convert_escape_sequence(es))); }
+    )
+);
+
+
+void parseJvmOptions(const std::string & jvmOptions, tJvmOptions& result) {
+    std::stringstream error_buffer;
+    auto && res = jvm_option_parser.parse(
+        parse_options{}.set_skip_whitespace(false),
+        buffers::string_buffer(jvmOptions.c_str()),
+        error_buffer);
+    if (res.has_value()) {
+        result.insert(result.end(), res.value().begin(), res.value().end());
+    }
+    else {
+        throw std::invalid_argument(error_buffer.str());
+    }
+}
+
+} //namespace JvmOptionsCTPG
+
+} //namespace JavaScriptOptions
+
+} //namespace SWIGVMContainers
diff --git a/exaudfclient/base/javacontainer/script_options/parser_ctpg_jvm_options_parser.h b/exaudfclient/base/javacontainer/script_options/parser_ctpg_jvm_options_parser.h
@@ -0,0 +1,26 @@
+#ifndef SCRIPTOPTIONLINEPARSERCTPGJVMOPTIONSPARSER_H
+#define SCRIPTOPTIONLINEPARSERCTPGJVMOPTIONSPARSER_H 1
+
+#include <vector>
+#include <string>
+
+
+namespace SWIGVMContainers {
+
+namespace JavaScriptOptions {
+
+namespace JvmOptionsCTPG {
+
+typedef std::vector<std::string> tJvmOptions;
+
+void parseJvmOptions(const std::string & jvmOptions, tJvmOptions& result);
+
+
+
+} //namespace CTPG
+
+} //namespace JavaScriptOptions
+
+} //namespace SWIGVMContainers
+
+#endif //SCRIPTOPTIONLINEPARSERCTPGSCRIPTIMPORTER_H
diff --git a/exaudfclient/base/javacontainer/script_options/test/converter_test.cc b/exaudfclient/base/javacontainer/script_options/test/converter_test.cc
@@ -60,3 +60,33 @@ INSTANTIATE_TEST_SUITE_P(
     ConverterV2JarTest,
     ::testing::ValuesIn(jar_strings_v2)
 );
+
+class ConverterV2JvmOptionsTest : public ::testing::TestWithParam<std::pair<std::string, std::vector<std::string>>> {};
+
+TEST_P(ConverterV2JvmOptionsTest, jvm_option) {
+    const std::pair<std::string, std::vector<std::string>> option_value = GetParam();
+    const std::string jvm_option_value = option_value.first;
+
+    ConverterV2 converter;
+    converter.convertJvmOption(option_value.first);
+    std::vector<std::string> result = std::move(converter.moveJvmOptions());
+    ASSERT_EQ(result, option_value.second);
+}
+
+const std::vector<std::pair<std::string, std::vector<std::string>>> jvm_options_strings_v2 =
+        {
+            std::make_pair("optionA=abc optionB=def", std::vector<std::string>({"optionA=abc", "optionB=def"})),
+            std::make_pair("optionA=abc\\ def optionB=ghi", std::vector<std::string>({"optionA=abc def", "optionB=ghi"})),
+            std::make_pair("optionA=abc\\tdef optionB=ghi", std::vector<std::string>({"optionA=abc\tdef", "optionB=ghi"})),
+            std::make_pair("   optionA=abc\\tdef optionB=ghi", std::vector<std::string>({"optionA=abc\tdef", "optionB=ghi"})),
+            std::make_pair("   optionA=abc\\tdef\\\\\t\t optionB=ghi", std::vector<std::string>({"optionA=abc\tdef\\", "optionB=ghi"})),
+            std::make_pair("   optionA=abc\\tdef\\\\\\t\\t optionB=ghi", std::vector<std::string>({"optionA=abc\tdef\\\t\t", "optionB=ghi"})),
+            std::make_pair("   optionA=abc\\tdef\\\\\\t\\t optionB=ghi   ", std::vector<std::string>({"optionA=abc\tdef\\\t\t", "optionB=ghi"})),
+            std::make_pair("   optionA=abc\\tdef\\\\\\t\\t optionB=ghi\\ \\t   ", std::vector<std::string>({"optionA=abc\tdef\\\t\t", "optionB=ghi \t"}))
+        };
+
+INSTANTIATE_TEST_SUITE_P(
+    Converter,
+    ConverterV2JvmOptionsTest,
+    ::testing::ValuesIn(jvm_options_strings_v2)
+);
diff --git a/exaudfclient/base/javacontainer/test/cpp/javacontainer_extractor_v2_test.cc b/exaudfclient/base/javacontainer/test/cpp/javacontainer_extractor_v2_test.cc
@@ -56,7 +56,12 @@ const std::vector<std::pair<std::string, std::string>> escape_sequences =
             std::make_pair("\\ -Dhttp.agent=ABCDEF", "-Dhttp.agent=ABCDEF"),
             std::make_pair("\\t-Dhttp.agent=ABCDEF", "-Dhttp.agent=ABCDEF"),
             std::make_pair("\\f-Dhttp.agent=ABCDEF", "-Dhttp.agent=ABCDEF"),
-            std::make_pair("\\v-Dhttp.agent=ABCDEF", "-Dhttp.agent=ABCDEF")
+            std::make_pair("\\v-Dhttp.agent=ABCDEF", "-Dhttp.agent=ABCDEF"),
+            std::make_pair("\\v-Dhttp.agent=ABC\\tDEF", "-Dhttp.agent=ABC\tDEF"),
+            std::make_pair("\\v-Dhttp.agent=ABC\\ DEF", "-Dhttp.agent=ABC DEF"),
+            std::make_pair("\\v-Dhttp.agent=ABC\\fDEF", "-Dhttp.agent=ABC\fDEF"),
+            std::make_pair("\\v-Dhttp.agent=ABC\\vDEF", "-Dhttp.agent=ABC\vDEF"),
+            std::make_pair("\\v-Dhttp.agent=\"ABC\\tDEF\"", "-Dhttp.agent=\"ABC\tDEF\"")
         };
 
 INSTANTIATE_TEST_SUITE_P(

diff --git a/test_container/tests/test/java/general.py b/test_container/tests/test/java/general.py
@@ -686,6 +686,26 @@ class TEST_JVM_OPT_INVALID_STACK_SIZE {
         with self.assertRaisesRegex(Exception, 'unknown error'):
             rows = self.query('SELECT test_jvm_opt_invalid_stack_size() FROM dual')
 
+    @useData([("-Dmyoption=Hello\ World", "Hello World"), ("-Dmyoption=\"Hello\ World\"", "\"Hello World\""),
+              ("-Dmyoption=Hello\\tWorld", "Hello\tWorld"), ("-Dmyoption=Hello\\vWorld", "Hello\vWorld"),
+              ("-Dmyoption=Hello\\\\World", "Hello\\World"), ("-Dmyoption=Hello\\fWorld", "Hello\fWorld"),
+              ("-Dmyoption=Hello\ World\\t\\t   ", "Hello World\t\t")])
+    def test_jvm_opt_escape_sequence(self, jvm_option_value, expected_return_value):
+        self.query(udf.fixindent('''
+                CREATE OR REPLACE java SCALAR SCRIPT
+                test_jvm_opt_with_escape()
+                RETURNS VARCHAR(10000) AS
+                %env SCRIPT_OPTIONS_PARSER_VERSION=2;
+                %jvmoption ''' + jvm_option_value + ''';
+                class TEST_JVM_OPT_WITH_ESCAPE {
+                    static String run(ExaMetadata exa, ExaIterator ctx) throws Exception {
+                        return System.getProperty("myoption");
+                    }
+                }
+                '''))
+        self.assertRowsEqual([(expected_return_value,)],
+                             self.query('''SELECT test_jvm_opt_with_escape()'''))
+
 
 class JavaScriptClass(udf.TestCase):