altium: add unit-tests for ReadProperties() and fix some issues found while doing so

common/plugins/altium/altium_parser.cpp

@@ -27,7 +27,6 @@
 #include <ki_exception.h>
 #include <sstream>
 #include <utf.h>
-#include <wx/translation.h>
 #include <wx/wx.h>
 
 
@@ -78,6 +77,16 @@ ALTIUM_PARSER::ALTIUM_PARSER(
 }
 
 
+ALTIUM_PARSER::ALTIUM_PARSER( std::unique_ptr<char[]>& aContent, size_t aSize )
+{
+    m_subrecord_end = nullptr;
+    m_size = aSize;
+    m_error = false;
+    m_content = std::move( aContent );
+    m_pos = m_content.get();
+}
+
+
 std::map<wxString, wxString> ALTIUM_PARSER::ReadProperties()
 {
     std::map<wxString, wxString> kv;
@@ -89,17 +98,23 @@ std::map<wxString, wxString> ALTIUM_PARSER::ReadProperties()
         return kv;
     }
 
+    if( length == 0 )
+    {
+        return kv;
+    }
+
     // There is one case by kliment where Board6 ends with "|NEARDISTANCE=1000mi".
     // Both the 'l' and the null-byte are missing, which looks like Altium swallowed two bytes.
-    if( m_pos[length - 1] != '\0' )
+    bool hasNullByte = m_pos[length - 1] == '\0';
+    if( !hasNullByte )
     {
         wxLogError( "For Altium import, we assumes a null byte at the end of a list of properties. "
                     "Because this is missing, imported data might be malformed or missing." );
     }
 
-    //we use std::string because std::string can handle NULL-bytes
-    //wxString would end the string at the first NULL-byte
-    std::string str = std::string( m_pos, length - 1 );
+    // we use std::string because std::string can handle NULL-bytes
+    // wxString would end the string at the first NULL-byte
+    std::string str = std::string( m_pos, length - ( hasNullByte ? 1 : 0 ) );
     m_pos += length;
 
     std::size_t token_end = 0;
@@ -107,18 +122,28 @@ std::map<wxString, wxString> ALTIUM_PARSER::ReadProperties()
     {
         std::size_t token_start = str.find( '|', token_end );
         std::size_t token_equal = str.find( '=', token_start );
-        token_end               = str.find( '|', token_equal );
+        token_end = str.find( '|', token_start + 1 );
+
+        if( token_equal >= token_end )
+        {
+            continue; // this looks like an error: skip the entry. Also matches on std::string::npos
+        }
+
+        if( token_end == std::string::npos )
+        {
+            token_end = str.size() + 1; // this is the correct offset
+        }
 
         std::string keyS   = str.substr( token_start + 1, token_equal - token_start - 1 );
         std::string valueS = str.substr( token_equal + 1, token_end - token_equal - 1 );
 
-        //convert the strings to wxStrings, since we use them everywhere
-        //value can have non-ASCII characters, so we convert them from LATIN1/ISO8859-1
+        // convert the strings to wxStrings, since we use them everywhere
+        // value can have non-ASCII characters, so we convert them from LATIN1/ISO8859-1
         wxString key( keyS.c_str(), wxConvISO8859_1 );
         wxString value( valueS.c_str(), wxConvISO8859_1 );
 
         // Altium stores keys either in Upper, or in CamelCase. Lets unify it.
-        kv.insert( { key.Trim().MakeUpper(), value.Trim() } );
+        kv.insert( { key.Trim( false ).Trim( true ).MakeUpper(), value.Trim() } );
     }
 
     return kv;

common/plugins/altium/altium_parser.h

@@ -47,6 +47,7 @@ class ALTIUM_PARSER
 {
 public:
     ALTIUM_PARSER( const CFB::CompoundFileReader& aReader, const CFB::COMPOUND_FILE_ENTRY* aEntry );
+    ALTIUM_PARSER( std::unique_ptr<char[]>& aContent, size_t aSize );
     ~ALTIUM_PARSER() = default;
 
     template <typename Type>

qa/common/plugins/altium/test_altium_parser.cpp

@@ -215,4 +215,89 @@ BOOST_AUTO_TEST_CASE( PropertiesReadKicadUnit )
     }
 }
 
+struct READ_PROPERTIES_CASE
+{
+    std::string                  input;
+    std::map<wxString, wxString> exp_result;
+};
+
+/**
+ * A list of valid test strings and the expected result map
+ */
+static const std::vector<READ_PROPERTIES_CASE> read_properties = {
+    // Empty
+    { "", {} },
+    { "\0", {} },
+    { "|", {} },
+    { "|\0", {} },
+    { "||", {} },
+    { "||\0", {} },
+    // Empty key-value pair
+    { "|=", { { "", "" } } },
+    { "|=\0", { { "", "" } } },
+    { "|  =  ", { { "", "" } } },
+    { "|  =  \0", { { "", "" } } },
+    // Single key-value pair
+    { "|A=\0", { { "A", "" } } },
+    { "|A=B", { { "A", "B" } } },
+    { "|A=B\0", { { "A", "B" } } },
+    { "|A=B|", { { "A", "B" } } },
+    { "|A=B|\0", { { "A", "B" } } },
+    // Multiple key-value pairs
+    { "|A=B|C=D|\0", { { "A", "B" }, { "C", "D" } } },
+    // Same key multiple times
+    { "|A=B|A=C\0", { { "A", "B" } } },
+    { "|A=B|A=C|A=D|A=E|A=F\0", { { "A", "B" } } },
+    // Always upper case key
+    { "|a=b\0", { { "A", "b" } } },
+    { "|abc123=b\0", { { "ABC123", "b" } } },
+    // Trim whitespaces, TODO: correct?
+    { "|A=  B\0", { { "A", "  B" } } },
+    { "|A=B  \0", { { "A", "B" } } },
+    { "|  A=B\0", { { "A", "B" } } },
+    { "|A  =B\0", { { "A", "B" } } },
+    { "|A=\nB\n\0", { { "A", "\nB" } } },
+    // Escaping and other special cases, TODO: extend
+    //{ "|A=||\0", {{"A", "|"}} },
+    { "|A==\0", { { "A", "=" } } },
+    { "|A=a\na\0", { { "A", "a\na" } } },
+    { "|A=a\ta\0", { { "A", "a\ta" } } },
+    // Encoding, TODO: extend
+    // Correct reading errors
+    { "|A|B=C\0", { { "B", "C" } } },
+    { "|A=B|C\0", { { "A", "B" } } },
+};
+
+/**
+ * Test conversation from binary to properties
+ */
+BOOST_AUTO_TEST_CASE( ReadProperties )
+{
+    for( const auto& c : read_properties )
+    {
+        BOOST_TEST_CONTEXT( wxString::Format( wxT( "'%s'" ), c.input ) )
+        {
+            size_t                  size = 4 + c.input.size();
+            std::unique_ptr<char[]> content = std::make_unique<char[]>( size );
+
+            *content.get() = c.input.size();
+            std::memcpy( content.get() + 4, c.input.c_str(), c.input.size() );
+
+            ALTIUM_PARSER parser( content, size );
+
+            std::map<wxString, wxString> result = parser.ReadProperties();
+
+            BOOST_CHECK_EQUAL( parser.HasParsingError(), false );
+            BOOST_CHECK_EQUAL( parser.GetRemainingBytes(), 0 );
+
+            BOOST_CHECK_EQUAL( result.size(), c.exp_result.size() );
+            for( const auto& kv : c.exp_result )
+            {
+                BOOST_CHECK_EQUAL( 1, result.count( kv.first ) );
+                BOOST_CHECK_EQUAL( result.at( kv.first ), kv.second );
+            }
+        }
+    }
+}
+
 BOOST_AUTO_TEST_SUITE_END()