Skip to content

Commit

Permalink
Implemented Quoting and Correct Order for JARs
Browse files Browse the repository at this point in the history
1. Use std::vector for collecting JARs in converter v2
2. Implemented StringOps::removeQuotesSafely() + tests
3. Use StringOps::removeQuotesSafely() for JARs
  • Loading branch information
tomuben committed Oct 23, 2024
1 parent a546d01 commit 73f8326
Show file tree
Hide file tree
Showing 17 changed files with 166 additions and 66 deletions.
2 changes: 0 additions & 2 deletions .github/workflows/check_bazel_tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 1 addition & 5 deletions exaudfclient/base/javacontainer/javacontainer_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,7 @@ void JavaVMImpl::parseScriptOptions(std::unique_ptr<JavaScriptOptions::Extractor
DBG_FUNC_CALL(cerr,setClasspath());

m_jvmOptions = std::move(extractor->moveJvmOptions());

for (set<string>::iterator it = extractor->getJarPaths().begin(); it != extractor->getJarPaths().end();
++it) {
addJarToClasspath(*it);
}
extractor->iterateJarPaths([&](const std::string& s) { addJarToClasspath(s);});
}

void JavaVMImpl::shutdown() {
Expand Down
4 changes: 2 additions & 2 deletions exaudfclient/base/javacontainer/script_options/converter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
10 changes: 6 additions & 4 deletions exaudfclient/base/javacontainer/script_options/converter.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@

#include <string>
#include <vector>
#include <set>
#include <functional>
#include <iterator>

namespace SWIGVMContainers {

Expand All @@ -20,17 +21,18 @@ class Converter {

void convertJvmOption(const std::string & value);

virtual const std::set<std::string> & getJarPaths() const = 0;
virtual void iterateJarPaths(std::function<void(const std::string &option)> callback) const = 0;

std::vector<std::string>&& moveJvmOptions() {
return std::move(m_jvmOptions);
}

private:
protected:
const std::string m_whitespace;

private:
std::vector<std::string> m_jvmOptions;

const std::string m_whitespace;
};


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ class ConverterLegacy : public Converter {

void convertExternalJar(const std::string & value);

const std::set<std::string> & getJarPaths() const {
return m_jarPaths;
void iterateJarPaths(std::function<void(const std::string &option)> callback) const override {
for (const auto & jar: m_jarPaths) {
callback(jar);
}
}

private:
Expand Down
22 changes: 9 additions & 13 deletions exaudfclient/base/javacontainer/script_options/converter_v2.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down
8 changes: 5 additions & 3 deletions exaudfclient/base/javacontainer/script_options/converter_v2.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,15 @@ class ConverterV2 : public Converter {

void convertExternalJar(const std::string & value);

const std::set<std::string> & getJarPaths() const {
return m_jarPaths;
void iterateJarPaths(std::function<void(const std::string &option)> callback) const override {
for (const auto & jar : m_jarPaths) {
callback(jar);
}
}

private:

std::set<std::string> m_jarPaths;
std::vector<std::string> m_jarPaths;

};

Expand Down
4 changes: 2 additions & 2 deletions exaudfclient/base/javacontainer/script_options/extractor.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

#include <string>
#include <vector>
#include <set>
#include <functional>


namespace SWIGVMContainers {
Expand All @@ -13,7 +13,7 @@ namespace JavaScriptOptions {
struct Extractor {
virtual ~Extractor() {}

virtual const std::set<std::string> & getJarPaths() const = 0;
virtual void iterateJarPaths(std::function<void(const std::string &option)> callback) const = 0;

virtual std::vector<std::string>&& moveJvmOptions() = 0;

Expand Down
Original file line number Diff line number Diff line change
@@ -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 <iostream>

Expand All @@ -16,8 +14,8 @@ 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();
inline void ExtractorImpl<TParser, TConverter>::iterateJarPaths(std::function<void(const std::string &option)> callback) const {
m_converter.iterateJarPaths(callback);
}

template<typename TParser, typename TConverter>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class ExtractorImpl : public Extractor {

ExtractorImpl(std::unique_ptr<SwigFactory> swigFactory);

const std::set<std::string> & getJarPaths() const override;
void iterateJarPaths(std::function<void(const std::string &option)> callback) const override;
std::vector<std::string>&& moveJvmOptions() override;

void extract(std::string & scriptCode);
Expand Down
41 changes: 41 additions & 0 deletions exaudfclient/base/javacontainer/script_options/string_ops.cc
Original file line number Diff line number Diff line change
@@ -1 +1,42 @@
#include "base/javacontainer/script_options/string_ops.h"
#include <iostream>

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
3 changes: 3 additions & 0 deletions exaudfclient/base/javacontainer/script_options/string_ops.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ inline void trim(std::string &s) {
rtrim(s);
}

void removeQuotesSafely(std::string & s, const std::string & whitespaces);


} //namespace StringOps


Expand Down
12 changes: 2 additions & 10 deletions exaudfclient/base/javacontainer/script_options/test/BUILD
Original file line number Diff line number Diff line change
@@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,26 @@
using namespace SWIGVMContainers::JavaScriptOptions;


class ConverterJarTest : public ::testing::TestWithParam<std::pair<std::string, std::set<std::string>>> {};
class ConverterJarTest : public ::testing::TestWithParam<std::pair<std::string, std::vector<std::string>>> {};

TEST_P(ConverterJarTest, 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"})),
std::make_pair("\"test.jar:test2.jar\"", std::vector<std::string>({"\"test.jar", "test2.jar\""})),
std::make_pair("t\\:est.jar:test2.jar", std::vector<std::string>({"est.jar", "t\\", "test2.jar"})),
};

INSTANTIATE_TEST_SUITE_P(
Expand All @@ -33,27 +37,33 @@ INSTANTIATE_TEST_SUITE_P(



class ConverterJarEscapeSequenceTest : 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(ConverterJarEscapeSequenceTest, jar) {
const std::pair<std::string, std::set<std::string>> option_value = GetParam();
TEST_P(ConverterV2JarTest, jar) {
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("\"test .jar:test2.jar\"", std::set<std::string>({"test .jar", "test2.jar"})),
std::make_pair("test.jar:test2.jar", std::vector<std::string>({"test.jar", "test2.jar"})),
std::make_pair("\"test.jar:test2.jar\"", std::vector<std::string>({"test.jar", "test2.jar"})),
std::make_pair("\"test .jar:test2.jar\"", std::vector<std::string>({"test .jar", "test2.jar"})),
std::make_pair("\"test .jar\":test2.jar", std::vector<std::string>({"test .jar", "test2.jar"})),
std::make_pair("'test .jar':test2.jar", std::vector<std::string>({"test .jar", "test2.jar"})),
std::make_pair("\"test .jar':test2.jar", std::vector<std::string>({"\"test .jar'", "test2.jar"})),
std::make_pair(" \"test .jar ' : ' test2.jar ' \t", std::vector<std::string>({" \"test .jar ' ", " test2.jar "})),
std::make_pair(" abc \"test .jar ' : ' test2.jar ' \t", std::vector<std::string>({" abc \"test .jar ' ", " test2.jar "})),
};

INSTANTIATE_TEST_SUITE_P(
Converter,
ConverterJarEscapeSequenceTest,
::testing::ValuesIn(jar_escape_sequences)
ConverterV2JarTest,
::testing::ValuesIn(jar_strings_v2)
);
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,30 @@ TEST(StringOpsTest, trimWithNoneASCII) {
EXPECT_EQ(sample, "\xa0Hello World\xa0");
}

class QuotedTest : public ::testing::TestWithParam<std::pair<std::string, std::string>> {};

TEST_P(QuotedTest, test) {
const std::pair<std::string, std::string> 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<std::pair<std::string, std::string>> 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)
);
Original file line number Diff line number Diff line change
Expand Up @@ -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 );
}


Loading

0 comments on commit 73f8326

Please sign in to comment.