Modified XMLBeautifier to handle split payloads

When receiving network data we were processing it in chunks. Sometimes one XML message got split across multiple chunks. XMLBeautifier was stateless and would create a new parser for every single chunk. This was causing multi-chunk messages to be truncated in the beautified output. The change I made in XMLBeautifier allows it to maintain the state. Test-Information: Tested unger Windows 10 and CentOS. Unit tests pass OK. Change-Id: Idad2a8e0248ed3cbe2b47a12718b909e56ac1279
author: Joanna Hulboj <joanna.hulboj@isode.com> 2017-03-18 18:52:56 (GMT)
committer: Joanna Hulboj <joanna.hulboj@isode.com> 2017-03-28 14:19:18 (GMT)
commit: 0e503ccd97ca5287a14d3147b0deaa99892ccd6f (patch)
tree: 5500f9f6376f8fa6544fc281ed40ea8b18d7129b /Swiften
parent: cc873b3f00db4cd0a778bc2ec04f8748d70a92f9 (diff)
download: swift-0e503ccd97ca5287a14d3147b0deaa99892ccd6f.zip
swift-0e503ccd97ca5287a14d3147b0deaa99892ccd6f.tar.bz2
14 files changed, 172 insertions, 85 deletions
diff --git a/Swiften/Client/ClientXMLTracer.cpp b/Swiften/Client/ClientXMLTracer.cpp
index 4852aa1..e205f41 100644
--- a/Swiften/Client/ClientXMLTracer.cpp
+++ b/Swiften/Client/ClientXMLTracer.cpp
@@ -14,23 +14,19 @@
 
 namespace Swift {
 
-ClientXMLTracer::ClientXMLTracer(CoreClient* client, bool bosh) : bosh(bosh) {
+ClientXMLTracer::ClientXMLTracer(CoreClient* client, bool bosh) : bosh_(bosh) {
 #ifdef SWIFTEN_PLATFORM_WIN32
-    beautifier = new XMLBeautifier(true, false);
+    beautifier_ = std::unique_ptr<XMLBeautifier>(new XMLBeautifier(true, false));
 #else
-    beautifier = new XMLBeautifier(true, true);
+    beautifier_ = std::unique_ptr<XMLBeautifier>(new XMLBeautifier(true, true));
 #endif
-    onDataReadConnection = client->onDataRead.connect(boost::bind(&ClientXMLTracer::printData, this, '<', _1));
-    onDataWrittenConnection = client->onDataWritten.connect(boost::bind(&ClientXMLTracer::printData, this, '>', _1));
-}
-
-ClientXMLTracer::~ClientXMLTracer() {
-    delete beautifier;
+    onDataReadConnection_ = client->onDataRead.connect(boost::bind(&ClientXMLTracer::printData, this, '<', _1));
+    onDataWrittenConnection_ = client->onDataWritten.connect(boost::bind(&ClientXMLTracer::printData, this, '>', _1));
 }
 
 void ClientXMLTracer::printData(char direction, const SafeByteArray& data) {
-    printLine(direction);
-    if (bosh) {
+    if (bosh_) {
+        printLine(direction);
         std::string line = byteArrayToString(ByteArray(data.begin(), data.end()));
 // Disabled because it swallows bits of XML (namespaces, if I recall)
 //        size_t endOfHTTP = line.find("\r\n\r\n");
@@ -42,7 +38,16 @@ void ClientXMLTracer::printData(char direction, const SafeByteArray& data) {
 //        }
     }
     else {
-        std::cerr << beautifier->beautify(byteArrayToString(ByteArray(data.begin(), data.end()))) << std::endl;
+        const auto& str = beautifier_->beautify(byteArrayToString(ByteArray(data.begin(), data.end())));
+
+        if (beautifier_->wasReset()) {
+            printLine(direction);
+        }
+        std::cerr << str;
+        if (beautifier_->getLevel() <= 1) {
+            std::cerr << std::endl;
+        }
+
     }
 }
 
diff --git a/Swiften/Client/ClientXMLTracer.h b/Swiften/Client/ClientXMLTracer.h
index ad28fe2..d3aeb0c 100644
--- a/Swiften/Client/ClientXMLTracer.h
+++ b/Swiften/Client/ClientXMLTracer.h
@@ -6,6 +6,8 @@
 
 #pragma once
 
+#include <memory>
+
 #include <Swiften/Base/API.h>
 #include <Swiften/Base/SafeByteArray.h>
 #include <Swiften/Client/CoreClient.h>
@@ -15,16 +17,15 @@ namespace Swift {
     class SWIFTEN_API ClientXMLTracer {
         public:
             ClientXMLTracer(CoreClient* client, bool bosh = false);
-            ~ClientXMLTracer();
 
         private:
             void printData(char direction, const SafeByteArray& data);
             void printLine(char c);
 
         private:
-            XMLBeautifier *beautifier;
-            bool bosh;
-            boost::signals2::scoped_connection onDataReadConnection;
-            boost::signals2::scoped_connection onDataWrittenConnection;
+            std::unique_ptr<XMLBeautifier> beautifier_;
+            bool bosh_;
+            boost::signals2::scoped_connection onDataReadConnection_;
+            boost::signals2::scoped_connection onDataWrittenConnection_;
     };
 }
diff --git a/Swiften/Client/UnitTest/XMLBeautifierTest.cpp b/Swiften/Client/UnitTest/XMLBeautifierTest.cpp
new file mode 100644
index 0000000..0188634
--- /dev/null
+++ b/Swiften/Client/UnitTest/XMLBeautifierTest.cpp
@@ -0,0 +1,66 @@
+/*
+ * Copyright (c) 2010-2017 Isode Limited.
+ * All rights reserved.
+ * See the COPYING file for more information.
+ */
+
+#include "gtest/gtest.h"
+#include <Swiften/Client/XMLBeautifier.h>
+#include <iostream>
+
+using namespace Swift;
+
+namespace {
+    const static std::string FULL_FORMATTED_OUTPUT("<list>\n <el>aqq</el>\n <el>bzz</el>\n</list>");
+}
+
+TEST(XMLBeautifierTest, testBeautify) {
+    auto beautifier = std::unique_ptr<XMLBeautifier>(new XMLBeautifier(true, false));
+
+    ASSERT_EQ(FULL_FORMATTED_OUTPUT, beautifier->beautify("<list><el>aqq</el><el>bzz</el></list>"));
+    ASSERT_TRUE(beautifier->wasReset());
+    ASSERT_EQ(0, beautifier->getLevel());
+    ASSERT_EQ(FULL_FORMATTED_OUTPUT, beautifier->beautify("<list><el>aqq</el><el>bzz</el></list>"));
+    ASSERT_TRUE(beautifier->wasReset());
+    ASSERT_EQ(0, beautifier->getLevel());
+}
+
+TEST(XMLBeautifierTest, testBeautifyMultipleChunks) {
+    auto beautifier = std::unique_ptr<XMLBeautifier>(new XMLBeautifier(true, false));
+
+    auto result = beautifier->beautify("<list><el>aqq</el>");
+    ASSERT_TRUE(beautifier->wasReset());
+    ASSERT_EQ(1, beautifier->getLevel());
+
+    result += beautifier->beautify("<el>bzz</el></list>");
+    ASSERT_FALSE(beautifier->wasReset());
+    ASSERT_EQ(0, beautifier->getLevel());
+
+    ASSERT_EQ(FULL_FORMATTED_OUTPUT, result);
+}
+
+TEST(XMLBeautifierTest, testBeautifyMultipleChunksMiddleElement) {
+    auto beautifier = std::unique_ptr<XMLBeautifier>(new XMLBeautifier(true, false));
+
+    auto result = beautifier->beautify("<l");
+    ASSERT_TRUE(beautifier->wasReset());
+    ASSERT_EQ(0, beautifier->getLevel());
+
+    result += beautifier->beautify("ist><el>aqq</el><el>bzz</el></list>");
+    ASSERT_FALSE(beautifier->wasReset());
+    ASSERT_EQ(0, beautifier->getLevel());
+
+    ASSERT_EQ(FULL_FORMATTED_OUTPUT, result);
+}
+
+TEST(XMLBeautifierTest, testBeautifyInvalidMultipleChunks) {
+    auto beautifier = std::unique_ptr<XMLBeautifier>(new XMLBeautifier(true, false));
+
+    ASSERT_EQ(std::string("<list>\n <el>aqq"), beautifier->beautify("<list><el>aqq<"));
+    ASSERT_TRUE(beautifier->wasReset());
+    ASSERT_EQ(2, beautifier->getLevel());
+
+    ASSERT_EQ(FULL_FORMATTED_OUTPUT, beautifier->beautify("<list><el>aqq</el><el>bzz</el></list>"));
+    ASSERT_TRUE(beautifier->wasReset());
+    ASSERT_EQ(0, beautifier->getLevel());
+}
diff --git a/Swiften/Client/XMLBeautifier.cpp b/Swiften/Client/XMLBeautifier.cpp
index e2cd58e..97a228f 100644
--- a/Swiften/Client/XMLBeautifier.cpp
+++ b/Swiften/Client/XMLBeautifier.cpp
@@ -14,35 +14,53 @@
 
 #include <sstream>
 #include <stack>
+#include <iostream>
 
 #include <Swiften/Base/Log.h>
 #include <Swiften/Parser/PlatformXMLParserFactory.h>
 
 namespace Swift {
 
-XMLBeautifier::XMLBeautifier(bool indention, bool coloring) : doIndention(indention), doColoring(coloring), intLevel(0), parser(nullptr), lastWasStepDown(false) {
-    factory = new PlatformXMLParserFactory();
+XMLBeautifier::XMLBeautifier(bool indention, bool coloring) : doIndention_(indention), doColoring_(coloring), factory_(new PlatformXMLParserFactory()) {
 }
 
-XMLBeautifier::~XMLBeautifier() {
-    delete factory;
-}
 
 std::string XMLBeautifier::beautify(const std::string &text) {
-    parser = factory->createXMLParser(this);
-    intLevel = 0;
-    buffer.str(std::string());
-    parser->parse(text);
-    delete parser;
-    return buffer.str();
+    wasReset_ = false;
+    if (!parser_) {
+        reset();
+    }
+    buffer_.str(std::string());
+    if (!parser_->parse(text)) {
+        reset();
+        parser_->parse(text);
+    }
+    return buffer_.str();
+}
+
+bool XMLBeautifier::wasReset() const {
+    return wasReset_;
+}
+
+int XMLBeautifier::getLevel() const {
+    return intLevel_;
 }
 
 void XMLBeautifier::indent() {
-    for (int i = 0; i < intLevel; ++i) {
-        buffer << " ";
+    for (int i = 0; i < intLevel_; ++i) {
+        buffer_ << " ";
     }
 }
 
+void XMLBeautifier::reset() {
+    parser_ = factory_->createXMLParser(this);
+    intLevel_ = 0;
+    lastWasStepDown_ = false;
+    std::stack<std::string>().swap(parentNSs_);
+    buffer_.str(std::string());
+    wasReset_ = true;
+}
+
 // all bold but reset
 // static const char colorBlue[] = "\x1b[01;34m";
 static const char colorCyan[] = "\x1b[01;36m";
@@ -86,47 +104,47 @@ std::string XMLBeautifier::styleValue(const std::string& text) const {
 }
 
 void XMLBeautifier::handleStartElement(const std::string& element, const std::string& ns, const AttributeMap& attributes) {
-    if (doIndention) {
-        if (intLevel) buffer << std::endl;
+    if (doIndention_) {
+        if (intLevel_) buffer_ << std::endl;
     }
     indent();
-    buffer << "<" << (doColoring ? styleTag(element) : element);
-    if (!ns.empty() && (!parentNSs.empty() && parentNSs.top() != ns)) {
-        buffer << " ";
-        buffer << (doColoring ? styleAttribute("xmlns") : "xmlns");
-        buffer << "=";
-        buffer << "\"" << (doColoring ? styleNamespace(ns) : ns) << "\"";
+    buffer_ << "<" << (doColoring_ ? styleTag(element) : element);
+    if (!ns.empty() && (!parentNSs_.empty() && parentNSs_.top() != ns)) {
+        buffer_ << " ";
+        buffer_ << (doColoring_ ? styleAttribute("xmlns") : "xmlns");
+        buffer_ << "=";
+        buffer_ << "\"" << (doColoring_ ? styleNamespace(ns) : ns) << "\"";
     }
     if (!attributes.getEntries().empty()) {
         for (const auto& entry : attributes.getEntries()) {
-            buffer << " ";
-            buffer << (doColoring ? styleAttribute(entry.getAttribute().getName()) : entry.getAttribute().getName());
-            buffer << "=";
-            buffer << "\"" << (doColoring ? styleValue(entry.getValue()) : entry.getValue()) << "\"";
+            buffer_ << " ";
+            buffer_ << (doColoring_ ? styleAttribute(entry.getAttribute().getName()) : entry.getAttribute().getName());
+            buffer_ << "=";
+            buffer_ << "\"" << (doColoring_ ? styleValue(entry.getValue()) : entry.getValue()) << "\"";
         }
     }
-    buffer << ">";
-    ++intLevel;
-    lastWasStepDown = false;
-    parentNSs.push(ns);
+    buffer_ << ">";
+    ++intLevel_;
+    lastWasStepDown_ = false;
+    parentNSs_.push(ns);
 }
 
 void XMLBeautifier::handleEndElement(const std::string& element, const std::string& /* ns */) {
-    --intLevel;
-    parentNSs.pop();
-    if (/*hadCDATA.top() ||*/ lastWasStepDown) {
-        if (doIndention) {
-            buffer << std::endl;
+    --intLevel_;
+    parentNSs_.pop();
+    if (/*hadCDATA.top() ||*/ lastWasStepDown_) {
+        if (doIndention_) {
+            buffer_ << std::endl;
         }
         indent();
     }
-    buffer << "</" << (doColoring ? styleTag(element) : element) << ">";
-    lastWasStepDown = true;
+    buffer_ << "</" << (doColoring_ ? styleTag(element) : element) << ">";
+    lastWasStepDown_ = true;
 }
 
 void XMLBeautifier::handleCharacterData(const std::string& data) {
-    buffer << data;
-    lastWasStepDown = false;
+    buffer_ << data;
+    lastWasStepDown_ = false;
 }
 
 }
diff --git a/Swiften/Client/XMLBeautifier.h b/Swiften/Client/XMLBeautifier.h
index 8da935a..7458211 100644
--- a/Swiften/Client/XMLBeautifier.h
+++ b/Swiften/Client/XMLBeautifier.h
@@ -12,6 +12,7 @@
 
 #pragma once
 
+#include <memory>
 #include <sstream>
 #include <stack>
 #include <string>
@@ -27,10 +28,12 @@ namespace Swift {
 
 class SWIFTEN_API XMLBeautifier : public XMLParserClient {
 public:
+
     XMLBeautifier(bool indention, bool coloring);
-    virtual ~XMLBeautifier();
 
     std::string beautify(const std::string&);
+    bool wasReset() const;
+    int getLevel() const;
 
 private:
     void handleStartElement(const std::string& element, const std::string& ns, const AttributeMap& attributes);
@@ -39,6 +42,7 @@ private:
 
 private:
     void indent();
+    void reset();
 
 private:
     std::string styleTag(const std::string& text) const;
@@ -47,16 +51,16 @@ private:
     std::string styleValue(const std::string& text) const;
 
 private:
-    bool doIndention;
-    bool doColoring;
+    const bool doIndention_;
+    const bool doColoring_;
 
-    int intLevel;
-    std::string inputBuffer;
-    std::stringstream buffer;
-    XMLParserFactory* factory;
-    XMLParser* parser;
+    std::unique_ptr<XMLParserFactory> factory_;
+    std::unique_ptr<XMLParser> parser_;
 
-    bool lastWasStepDown;
-    std::stack<std::string> parentNSs;
+    bool wasReset_ = true;
+    int intLevel_ = 0;
+    bool lastWasStepDown_ = false;
+    std::stringstream buffer_;
+    std::stack<std::string> parentNSs_;
 };
 }
diff --git a/Swiften/Parser/BOSHBodyExtractor.cpp b/Swiften/Parser/BOSHBodyExtractor.cpp
index c45d338..803f16a 100644
--- a/Swiften/Parser/BOSHBodyExtractor.cpp
+++ b/Swiften/Parser/BOSHBodyExtractor.cpp
@@ -126,7 +126,7 @@ BOSHBodyExtractor::BOSHBodyExtractor(XMLParserFactory* parserFactory, const Byte
 
     // Parse the body element
     BOSHBodyParserClient parserClient(this);
-    std::shared_ptr<XMLParser> parser(parserFactory->createXMLParser(&parserClient));
+    std::shared_ptr<XMLParser> parser(std::move(parserFactory->createXMLParser(&parserClient)));
     if (!parser->parse(std::string(
             reinterpret_cast<const char*>(vecptr(data)),
             boost::numeric_cast<size_t>(std::distance(data.begin(), i))))) {
diff --git a/Swiften/Parser/PayloadParsers/UnitTest/PayloadsParserTester.h b/Swiften/Parser/PayloadParsers/UnitTest/PayloadsParserTester.h
index 2c1ff8e..dcdbffa 100644
--- a/Swiften/Parser/PayloadParsers/UnitTest/PayloadsParserTester.h
+++ b/Swiften/Parser/PayloadParsers/UnitTest/PayloadsParserTester.h
@@ -22,10 +22,6 @@ namespace Swift {
                 xmlParser = PlatformXMLParserFactory().createXMLParser(this);
             }
 
-            ~PayloadsParserTester() {
-                delete xmlParser;
-            }
-
             bool parse(const std::string& data) {
                 return xmlParser->parse(data);
             }
@@ -60,7 +56,7 @@ namespace Swift {
             }
 
         private:
-            XMLParser* xmlParser;
+            std::unique_ptr<XMLParser> xmlParser;
             FullPayloadParserFactoryCollection factories;
             std::shared_ptr<PayloadParser> payloadParser;
             int level;
diff --git a/Swiften/Parser/PlatformXMLParserFactory.cpp b/Swiften/Parser/PlatformXMLParserFactory.cpp
index 87f70d1..97e1c9e 100644
--- a/Swiften/Parser/PlatformXMLParserFactory.cpp
+++ b/Swiften/Parser/PlatformXMLParserFactory.cpp
@@ -20,11 +20,11 @@ namespace Swift {
 PlatformXMLParserFactory::PlatformXMLParserFactory() {
 }
 
-XMLParser* PlatformXMLParserFactory::createXMLParser(XMLParserClient* client) {
+std::unique_ptr<XMLParser> PlatformXMLParserFactory::createXMLParser(XMLParserClient* client) {
 #ifdef HAVE_LIBXML
-    return new LibXMLParser(client);
+    return std::unique_ptr<XMLParser>(new LibXMLParser(client));
 #else
-    return new ExpatParser(client);
+    return std::unique_ptr<XMLParser>(new ExpatParser(client));
 #endif
 }
 
diff --git a/Swiften/Parser/PlatformXMLParserFactory.h b/Swiften/Parser/PlatformXMLParserFactory.h
index 82b8573..fa3ca19 100644
--- a/Swiften/Parser/PlatformXMLParserFactory.h
+++ b/Swiften/Parser/PlatformXMLParserFactory.h
@@ -14,6 +14,6 @@ namespace Swift {
         public:
             PlatformXMLParserFactory();
 
-            virtual XMLParser* createXMLParser(XMLParserClient*);
+            virtual std::unique_ptr<XMLParser> createXMLParser(XMLParserClient*);
     };
 }
diff --git a/Swiften/Parser/UnitTest/ParserTester.h b/Swiften/Parser/UnitTest/ParserTester.h
index a98eb51..aa01d40 100644
--- a/Swiften/Parser/UnitTest/ParserTester.h
+++ b/Swiften/Parser/UnitTest/ParserTester.h
@@ -16,12 +16,7 @@ namespace Swift {
     template<typename ParserType>
     class ParserTester : public XMLParserClient {
         public:
-            ParserTester(ParserType* parser) : parser_(parser) {
-                xmlParser_ = PlatformXMLParserFactory().createXMLParser(this);
-            }
-
-            ~ParserTester() {
-                delete xmlParser_;
+            ParserTester(ParserType* parser) : xmlParser_(PlatformXMLParserFactory().createXMLParser(this)), parser_(parser) {
             }
 
             bool parse(const std::string& data) {
@@ -41,7 +36,7 @@ namespace Swift {
             }
 
         private:
-            XMLParser* xmlParser_;
+            std::unique_ptr<XMLParser> xmlParser_;
             ParserType* parser_;
     };
 }
diff --git a/Swiften/Parser/XMLParserFactory.h b/Swiften/Parser/XMLParserFactory.h
index 091f45b..595512b 100644
--- a/Swiften/Parser/XMLParserFactory.h
+++ b/Swiften/Parser/XMLParserFactory.h
@@ -6,6 +6,8 @@
 
 #pragma once
 
+#include <memory>
+
 #include <Swiften/Base/API.h>
 
 namespace Swift {
@@ -16,6 +18,6 @@ namespace Swift {
         public:
             virtual ~XMLParserFactory();
 
-            virtual XMLParser* createXMLParser(XMLParserClient*) = 0;
+            virtual std::unique_ptr<XMLParser> createXMLParser(XMLParserClient*) = 0;
     };
 }
diff --git a/Swiften/Parser/XMPPParser.cpp b/Swiften/Parser/XMPPParser.cpp
index 2b45a12..d2a5a98 100644
--- a/Swiften/Parser/XMPPParser.cpp
+++ b/Swiften/Parser/XMPPParser.cpp
@@ -60,7 +60,6 @@ XMPPParser::XMPPParser(
 
 XMPPParser::~XMPPParser() {
     delete currentElementParser_;
-    delete xmlParser_;
 }
 
 bool XMPPParser::parse(const std::string& data) {
diff --git a/Swiften/Parser/XMPPParser.h b/Swiften/Parser/XMPPParser.h
index 09fae38..6595b94 100644
--- a/Swiften/Parser/XMPPParser.h
+++ b/Swiften/Parser/XMPPParser.h
@@ -42,7 +42,7 @@ namespace Swift {
             ElementParser* createElementParser(const std::string& element, const std::string& xmlns);
 
         private:
-            XMLParser* xmlParser_;
+            std::unique_ptr<XMLParser> xmlParser_;
             XMPPParserClient* client_;
             PayloadParserFactoryCollection* payloadParserFactories_;
             enum Level {
diff --git a/Swiften/SConscript b/Swiften/SConscript
index 2182132..c77d80f 100644
--- a/Swiften/SConscript
+++ b/Swiften/SConscript
@@ -382,6 +382,7 @@ if env["SCONS_STAGE"] == "build" :
             File("Client/UnitTest/NickResolverTest.cpp"),
             File("Client/UnitTest/ClientBlockListManagerTest.cpp"),
             File("Client/UnitTest/BlockListImplTest.cpp"),
+            File("Client/UnitTest/XMLBeautifierTest.cpp"),
             File("Compress/UnitTest/ZLibCompressorTest.cpp"),
             File("Compress/UnitTest/ZLibDecompressorTest.cpp"),
             File("Component/UnitTest/ComponentHandshakeGeneratorTest.cpp"),
author	Joanna Hulboj <joanna.hulboj@isode.com>	2017-03-18 18:52:56 (GMT)
committer	Joanna Hulboj <joanna.hulboj@isode.com>	2017-03-28 14:19:18 (GMT)
commit	0e503ccd97ca5287a14d3147b0deaa99892ccd6f (patch)
tree	5500f9f6376f8fa6544fc281ed40ea8b18d7129b /Swiften
parent	cc873b3f00db4cd0a778bc2ec04f8748d70a92f9 (diff)
download	swift-0e503ccd97ca5287a14d3147b0deaa99892ccd6f.zip swift-0e503ccd97ca5287a14d3147b0deaa99892ccd6f.tar.bz2