From 73f832643207c8d0eeee02a578ad8297fc4a8e91 Mon Sep 17 00:00:00 2001 From: Thomas Ubensee <34603111+tomuben@users.noreply.github.com> Date: Wed, 23 Oct 2024 14:21:52 -0300 Subject: [PATCH] Implemented Quoting and Correct Order for JARs 1. Use std::vector for collecting JARs in converter v2 2. Implemented StringOps::removeQuotesSafely() + tests 3. Use StringOps::removeQuotesSafely() for JARs --- .github/workflows/check_bazel_tests.yml | 2 - .../base/javacontainer/javacontainer_impl.cc | 6 +-- .../javacontainer/script_options/converter.cc | 4 +- .../javacontainer/script_options/converter.h | 10 ++-- .../script_options/converter_legacy.h | 6 ++- .../script_options/converter_v2.cc | 22 ++++----- .../script_options/converter_v2.h | 8 ++-- .../javacontainer/script_options/extractor.h | 4 +- .../script_options/extractor_impl.cc | 6 +-- .../script_options/extractor_impl.h | 2 +- .../script_options/string_ops.cc | 41 +++++++++++++++++ .../javacontainer/script_options/string_ops.h | 3 ++ .../javacontainer/script_options/test/BUILD | 12 +---- .../script_options/test/converter_test.cc | 46 +++++++++++-------- .../script_options/test/string_ops_tests.cc | 27 +++++++++++ .../test/cpp/javacontainer_ctpg_test.cc | 19 ++++++++ .../test/cpp/javacontainer_test.cc | 14 ++++++ 17 files changed, 166 insertions(+), 66 deletions(-) diff --git a/.github/workflows/check_bazel_tests.yml b/.github/workflows/check_bazel_tests.yml index 4d0b1555..9d0e28dc 100644 --- a/.github/workflows/check_bazel_tests.yml +++ b/.github/workflows/check_bazel_tests.yml @@ -62,8 +62,6 @@ jobs: name: "script_options_parser_ctpg_with_asan" - test: "--config=asan //base/script_options_parser/legacy/..." name: "script_options_parser_legacy_with_asan" - - test: "//base/javacontainer/test:javacontainer-test-converter" - name: "javacontainer-test-converter" steps: - uses: actions/checkout@v4 - name: Install JDK and ZMQ diff --git a/exaudfclient/base/javacontainer/javacontainer_impl.cc b/exaudfclient/base/javacontainer/javacontainer_impl.cc index 0f514a4c..cf36a3a7 100644 --- a/exaudfclient/base/javacontainer/javacontainer_impl.cc +++ b/exaudfclient/base/javacontainer/javacontainer_impl.cc @@ -55,11 +55,7 @@ void JavaVMImpl::parseScriptOptions(std::unique_ptrmoveJvmOptions()); - - for (set::iterator it = extractor->getJarPaths().begin(); it != extractor->getJarPaths().end(); - ++it) { - addJarToClasspath(*it); - } + extractor->iterateJarPaths([&](const std::string& s) { addJarToClasspath(s);}); } void JavaVMImpl::shutdown() { diff --git a/exaudfclient/base/javacontainer/script_options/converter.cc b/exaudfclient/base/javacontainer/script_options/converter.cc index 7139f2de..1f641866 100644 --- a/exaudfclient/base/javacontainer/script_options/converter.cc +++ b/exaudfclient/base/javacontainer/script_options/converter.cc @@ -7,8 +7,8 @@ namespace SWIGVMContainers { namespace JavaScriptOptions { Converter::Converter() -: m_jvmOptions() -, m_whitespace(" \t\f\v") {} +: m_whitespace(" \t\f\v") +, m_jvmOptions() {} void Converter::convertScriptClassName(const std::string & value) { std::string trimmedValue(value); diff --git a/exaudfclient/base/javacontainer/script_options/converter.h b/exaudfclient/base/javacontainer/script_options/converter.h index c640a5f2..07b53563 100644 --- a/exaudfclient/base/javacontainer/script_options/converter.h +++ b/exaudfclient/base/javacontainer/script_options/converter.h @@ -3,7 +3,8 @@ #include #include -#include +#include +#include namespace SWIGVMContainers { @@ -20,17 +21,18 @@ class Converter { void convertJvmOption(const std::string & value); - virtual const std::set & getJarPaths() const = 0; + virtual void iterateJarPaths(std::function callback) const = 0; std::vector&& moveJvmOptions() { return std::move(m_jvmOptions); } - private: + protected: + const std::string m_whitespace; + private: std::vector m_jvmOptions; - const std::string m_whitespace; }; diff --git a/exaudfclient/base/javacontainer/script_options/converter_legacy.h b/exaudfclient/base/javacontainer/script_options/converter_legacy.h index bddd9151..a3401c36 100644 --- a/exaudfclient/base/javacontainer/script_options/converter_legacy.h +++ b/exaudfclient/base/javacontainer/script_options/converter_legacy.h @@ -21,8 +21,10 @@ class ConverterLegacy : public Converter { void convertExternalJar(const std::string & value); - const std::set & getJarPaths() const { - return m_jarPaths; + void iterateJarPaths(std::function callback) const override { + for (const auto & jar: m_jarPaths) { + callback(jar); + } } private: diff --git a/exaudfclient/base/javacontainer/script_options/converter_v2.cc b/exaudfclient/base/javacontainer/script_options/converter_v2.cc index 6df9b962..4a94da62 100644 --- a/exaudfclient/base/javacontainer/script_options/converter_v2.cc +++ b/exaudfclient/base/javacontainer/script_options/converter_v2.cc @@ -11,23 +11,19 @@ ConverterV2::ConverterV2() , m_jarPaths() {} void ConverterV2::convertExternalJar(const std::string & value) { - std::string formattedValue(value); - StringOps::trim(formattedValue); - if (formattedValue.size() > 1 && formattedValue.front() == '\"' && formattedValue.back() == '\"') { - formattedValue = formattedValue.substr(1, formattedValue.size()-2); - } - + std::string unquotedValue(value); + StringOps::removeQuotesSafely(unquotedValue, m_whitespace); for (size_t start = 0, delim = 0; ; start = delim + 1) { - delim = formattedValue.find(":", start); + delim = unquotedValue.find(":", start); if (delim != std::string::npos) { - std::string jar = formattedValue.substr(start, delim - start); - if (m_jarPaths.find(jar) == m_jarPaths.end()) - m_jarPaths.insert(jar); + std::string jar = unquotedValue.substr(start, delim - start); + StringOps::removeQuotesSafely(jar, m_whitespace); + m_jarPaths.push_back(jar); } else { - std::string jar = formattedValue.substr(start); - if (m_jarPaths.find(jar) == m_jarPaths.end()) - m_jarPaths.insert(jar); + std::string jar = unquotedValue.substr(start); + StringOps::removeQuotesSafely(jar, m_whitespace); + m_jarPaths.push_back(jar); break; } } diff --git a/exaudfclient/base/javacontainer/script_options/converter_v2.h b/exaudfclient/base/javacontainer/script_options/converter_v2.h index a5f135de..1244a59e 100644 --- a/exaudfclient/base/javacontainer/script_options/converter_v2.h +++ b/exaudfclient/base/javacontainer/script_options/converter_v2.h @@ -21,13 +21,15 @@ class ConverterV2 : public Converter { void convertExternalJar(const std::string & value); - const std::set & getJarPaths() const { - return m_jarPaths; + void iterateJarPaths(std::function callback) const override { + for (const auto & jar : m_jarPaths) { + callback(jar); + } } 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 2db401b9..45546ad0 100644 --- a/exaudfclient/base/javacontainer/script_options/extractor.h +++ b/exaudfclient/base/javacontainer/script_options/extractor.h @@ -3,7 +3,7 @@ #include #include -#include +#include namespace SWIGVMContainers { @@ -13,7 +13,7 @@ namespace JavaScriptOptions { struct Extractor { virtual ~Extractor() {} - virtual const std::set & getJarPaths() const = 0; + virtual void iterateJarPaths(std::function callback) const = 0; virtual std::vector&& moveJvmOptions() = 0; diff --git a/exaudfclient/base/javacontainer/script_options/extractor_impl.cc b/exaudfclient/base/javacontainer/script_options/extractor_impl.cc index 842911e9..9c6496a2 100644 --- a/exaudfclient/base/javacontainer/script_options/extractor_impl.cc +++ b/exaudfclient/base/javacontainer/script_options/extractor_impl.cc @@ -1,6 +1,4 @@ #include "base/javacontainer/script_options/extractor_impl.h" -#include "base/javacontainer/script_options/parser.h" - #include "base/utils/debug_message.h" #include @@ -16,8 +14,8 @@ ExtractorImpl::ExtractorImpl(std::unique_ptr s , m_converter() {} template -inline const std::set & ExtractorImpl::getJarPaths() const { - return m_converter.getJarPaths(); +inline void ExtractorImpl::iterateJarPaths(std::function callback) const { + m_converter.iterateJarPaths(callback); } template diff --git a/exaudfclient/base/javacontainer/script_options/extractor_impl.h b/exaudfclient/base/javacontainer/script_options/extractor_impl.h index 849faa31..e071958c 100644 --- a/exaudfclient/base/javacontainer/script_options/extractor_impl.h +++ b/exaudfclient/base/javacontainer/script_options/extractor_impl.h @@ -24,7 +24,7 @@ class ExtractorImpl : public Extractor { ExtractorImpl(std::unique_ptr swigFactory); - const std::set & getJarPaths() const override; + void iterateJarPaths(std::function callback) const override; std::vector&& moveJvmOptions() override; void extract(std::string & scriptCode); diff --git a/exaudfclient/base/javacontainer/script_options/string_ops.cc b/exaudfclient/base/javacontainer/script_options/string_ops.cc index df1b6081..5a59743c 100644 --- a/exaudfclient/base/javacontainer/script_options/string_ops.cc +++ b/exaudfclient/base/javacontainer/script_options/string_ops.cc @@ -1 +1,42 @@ #include "base/javacontainer/script_options/string_ops.h" +#include + +namespace SWIGVMContainers { + +namespace JavaScriptOptions { + +namespace StringOps { + +void removeQuotesSafely(std::string & s, const std::string & whitespaces) { + const size_t startQuoteIdx = s.find_first_of("\"'"); + const size_t endQuoteIdx = s.find_last_of("\"'"); + + if (startQuoteIdx != std::string::npos && endQuoteIdx != std::string::npos && startQuoteIdx < endQuoteIdx && + s[startQuoteIdx] == s[endQuoteIdx]) { + //Search backwards if there any none whitespace characters in front of quote. If yes, we ignore the quote. + if (startQuoteIdx > 0) { + const size_t startingNotWhitespace = s.find_last_not_of(whitespaces, startQuoteIdx-1); + if (startingNotWhitespace != std::string::npos) { + return; + } + } + + //Search forward if there any none whitespace characters after ending quote. If yes, we ignore the quote. + if (endQuoteIdx < s.size() -1 ) { + const size_t trailingNotWhitespace = s.find_first_not_of(whitespaces, endQuoteIdx+1); + if (trailingNotWhitespace != std::string::npos) { + return; + } + } + s = s.substr(startQuoteIdx+1, endQuoteIdx-startQuoteIdx-1); + std::cerr << "DEBUG0 :" << startQuoteIdx << "-" << endQuoteIdx << std::endl; + std::cerr << "DEBUG1 :" << s << std::endl; + } +} + +} //namespace StringOps + + +} //namespace JavaScriptOptions + +} //namespace SWIGVMContainers diff --git a/exaudfclient/base/javacontainer/script_options/string_ops.h b/exaudfclient/base/javacontainer/script_options/string_ops.h index 8e0ff72c..c382b545 100644 --- a/exaudfclient/base/javacontainer/script_options/string_ops.h +++ b/exaudfclient/base/javacontainer/script_options/string_ops.h @@ -31,6 +31,9 @@ inline void trim(std::string &s) { rtrim(s); } +void removeQuotesSafely(std::string & s, const std::string & whitespaces); + + } //namespace StringOps diff --git a/exaudfclient/base/javacontainer/script_options/test/BUILD b/exaudfclient/base/javacontainer/script_options/test/BUILD index 1cfb1e31..c898c29c 100644 --- a/exaudfclient/base/javacontainer/script_options/test/BUILD +++ b/exaudfclient/base/javacontainer/script_options/test/BUILD @@ -1,16 +1,8 @@ cc_test( name = "java-script-options-tests", - srcs = ["ctpg_script_importer_test.cc", "swig_factory_test.cc", "swig_factory_test.h", "string_ops_tests.cc"], - deps = [ - "//base/javacontainer/script_options:java_script_option_lines", - "@googletest//:gtest_main", - ], -) - -cc_test( - name = "javacontainer-test-converter", - srcs = ["converter_test.cc"], + srcs = ["ctpg_script_importer_test.cc", "swig_factory_test.cc", "swig_factory_test.h", + "string_ops_tests.cc", "converter_test.cc"], deps = [ "//base/javacontainer/script_options:java_script_option_lines", "@googletest//:gtest_main", diff --git a/exaudfclient/base/javacontainer/script_options/test/converter_test.cc b/exaudfclient/base/javacontainer/script_options/test/converter_test.cc index 2f9bec0d..64b1c1df 100644 --- a/exaudfclient/base/javacontainer/script_options/test/converter_test.cc +++ b/exaudfclient/base/javacontainer/script_options/test/converter_test.cc @@ -7,22 +7,26 @@ using namespace SWIGVMContainers::JavaScriptOptions; -class ConverterJarTest : public ::testing::TestWithParam>> {}; +class ConverterJarTest : public ::testing::TestWithParam>> {}; TEST_P(ConverterJarTest, 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"})), + std::make_pair("\"test.jar:test2.jar\"", std::vector({"\"test.jar", "test2.jar\""})), + std::make_pair("t\\:est.jar:test2.jar", std::vector({"est.jar", "t\\", "test2.jar"})), }; INSTANTIATE_TEST_SUITE_P( @@ -33,27 +37,33 @@ INSTANTIATE_TEST_SUITE_P( -class ConverterJarEscapeSequenceTest : public ::testing::TestWithParam>> {}; +class ConverterV2JarTest : public ::testing::TestWithParam>> {}; -TEST_P(ConverterJarEscapeSequenceTest, jar) { - const std::pair> option_value = GetParam(); +TEST_P(ConverterV2JarTest, jar) { + 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("\"test .jar:test2.jar\"", std::set({"test .jar", "test2.jar"})), + std::make_pair("test.jar:test2.jar", std::vector({"test.jar", "test2.jar"})), + std::make_pair("\"test.jar:test2.jar\"", std::vector({"test.jar", "test2.jar"})), + std::make_pair("\"test .jar:test2.jar\"", std::vector({"test .jar", "test2.jar"})), + std::make_pair("\"test .jar\":test2.jar", std::vector({"test .jar", "test2.jar"})), + std::make_pair("'test .jar':test2.jar", std::vector({"test .jar", "test2.jar"})), + std::make_pair("\"test .jar':test2.jar", std::vector({"\"test .jar'", "test2.jar"})), + std::make_pair(" \"test .jar ' : ' test2.jar ' \t", std::vector({" \"test .jar ' ", " test2.jar "})), + std::make_pair(" abc \"test .jar ' : ' test2.jar ' \t", std::vector({" abc \"test .jar ' ", " test2.jar "})), }; INSTANTIATE_TEST_SUITE_P( Converter, - ConverterJarEscapeSequenceTest, - ::testing::ValuesIn(jar_escape_sequences) + ConverterV2JarTest, + ::testing::ValuesIn(jar_strings_v2) ); diff --git a/exaudfclient/base/javacontainer/script_options/test/string_ops_tests.cc b/exaudfclient/base/javacontainer/script_options/test/string_ops_tests.cc index 8eaf722b..ec8a8e1f 100644 --- a/exaudfclient/base/javacontainer/script_options/test/string_ops_tests.cc +++ b/exaudfclient/base/javacontainer/script_options/test/string_ops_tests.cc @@ -25,3 +25,30 @@ TEST(StringOpsTest, trimWithNoneASCII) { EXPECT_EQ(sample, "\xa0Hello World\xa0"); } +class QuotedTest : public ::testing::TestWithParam> {}; + +TEST_P(QuotedTest, test) { + const std::pair quoted_strings = GetParam(); + std::string quoted_string = quoted_strings.first; + StringOps::removeQuotesSafely(quoted_string, " \t"); + EXPECT_EQ(quoted_string, quoted_strings.second); +} + +const std::vector> quoted_strings = + { + std::make_pair("string", "string"), + std::make_pair("\"string", "\"string"), + std::make_pair("\"string\"", "string"), + std::make_pair("\"string'", "\"string'"), + std::make_pair("'string'", "string"), + std::make_pair("' string '", " string "), + std::make_pair("' ' string '", " ' string "), + std::make_pair(" ' ' string '", " ' string "), + std::make_pair(" aaa ' ' string '", " aaa ' ' string '"), + }; + +INSTANTIATE_TEST_SUITE_P( + QuotedTest, + QuotedTest, + ::testing::ValuesIn(quoted_strings) +); diff --git a/exaudfclient/base/javacontainer/test/cpp/javacontainer_ctpg_test.cc b/exaudfclient/base/javacontainer/test/cpp/javacontainer_ctpg_test.cc index b27a1dc1..350e835e 100644 --- a/exaudfclient/base/javacontainer/test/cpp/javacontainer_ctpg_test.cc +++ b/exaudfclient/base/javacontainer/test/cpp/javacontainer_ctpg_test.cc @@ -128,3 +128,22 @@ TEST(JavaContainer, basic_jar_with_quoted_white_spaces) { } }, SWIGVMContainers::JavaVMach::exception ); } + + +TEST(JavaContainer, basic_jars_order_remains) { + const std::string script_code = "%jar \"base/javacontainer/test/test1.jar\":base/javacontainer/test/abc.jar;"; + + EXPECT_THROW({ + try + { + JavaVMTest vm(script_code); + } + catch( const SWIGVMContainers::JavaVMach::exception& e ) + { + EXPECT_THAT( e.what(), MatchesRegex("^.*Java VM cannot find 'base/javacontainer/test/test1\\.jar': No such file or directory$")); + throw; + } + }, SWIGVMContainers::JavaVMach::exception ); +} + + diff --git a/exaudfclient/base/javacontainer/test/cpp/javacontainer_test.cc b/exaudfclient/base/javacontainer/test/cpp/javacontainer_test.cc index 44942c4a..3ea3a28c 100644 --- a/exaudfclient/base/javacontainer/test/cpp/javacontainer_test.cc +++ b/exaudfclient/base/javacontainer/test/cpp/javacontainer_test.cc @@ -49,8 +49,22 @@ 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. JavaVMTest vm(script_code); EXPECT_EQ(vm.getJavaVMInternalStatus().m_classpath, "/exaudf/base/javacontainer/exaudf_deploy.jar:base/javacontainer/test/test.jar"); +#else + EXPECT_THROW({ + try + { + JavaVMTest vm(script_code); + } + catch( const SWIGVMContainers::JavaVMach::exception& e ) + { + EXPECT_THAT( e.what(), MatchesRegex("^.*Java VM cannot find 'base/javacontainer/test/test\\.jar \t ': No such file or directory$")); + throw; + } + }, SWIGVMContainers::JavaVMach::exception ); +#endif }