How can I make my XML parser more pythonic?

Question

I have a function that takes an XML document and parses out specific XML elements and appends those to a list for use by an Oracle executemany call. The placement of the elements matters as the executemany string uses positional binds, though I could easily use a dictionary to use named binds instead. I feel that while this works, it is pretty slow since it has to reparse the data 5x per document instead of pulling the whole thing into an array once. I know xml.etree is probably what I want here instead of minidom, but I'm not sure how to implement it.

The data I'm working with is structured like this:

<troubleshooter>
    <vendorsAndVersions>
        <versionInfo>
            <type>TAX</type>
            <vendor>My vendor</vendor>
            <version>My version 23121</version>
        </versionInfo>
        <versionInfo>
            <type>OS</type>
            <vendor>Windows Server 2008 R2 amd64</vendor>
            <version>6.1</version>
        </versionInfo>
        <versionInfo>
            <type>APPSERVER</type>
            <vendor>JBoss Web</vendor>
            <version>3.0.0-CR1</version>
        </versionInfo>
        <versionInfo>
            <type>DATABASE</type>
            <vendor>Microsoft SQL Server</vendor>
            <version>10.50.1600</version>
        </versionInfo>
        <versionInfo>
            <type>JAVA</type>
            <vendor>Sun Microsystems Inc.</vendor>
            <version>1.6.0_26</version>
       </versionInfo>
       <versionInfo>
           <type>JDBC</type>
           <vendor>Microsoft SQL Server JDBC Driver 3.0</vendor>
           <version>3.0.1301.101</version>
       </versionInfo>
   </vendorsAndVersions>
   ...
</troubleshooter>

And the function looks like this:

def parseDocumentData(document, cusNumber):
    "Take document from calling function, validate that the structure is \
    correct, then process the XML elements and return the data as a list \
    of lists."
    # Check filename for customer number if cusNumber isn't passed.
    if cusNumber == None:
        cusNumber = os.path.splitext(os.path.basename(document))[0]
    cusNumber = str(cusNumber).encode('ascii')
    # Verify that the destination customer record exists. Source was verified by input function.
    _dbCur.execute('select count(*) from pm_customer where customer_id=' + cusNumber)
    cusVerify = _dbCur.fetchone()[0]
    if cusVerify == 0:
        _dbCur.execute('insert into pm_customer (customer_id) values (' + cusNumber + ')')
        print 'Customer number,', cusNumber, 'was created. Moving on.'
    else:
        print 'Customer number,', cusNumber, 'exists. Moving on.'
    parseDoc = minidom.parse(document)
    dataSet = parseDoc.getElementsByTagName("versionInfo")
    isValid = None
    global dbWriteList
    prepList = []

    # This for loop parses the document and matches XML objects using the typePattern list.
    # As each object is found, it is appended to the prepList. The prepList is then appended
    # to the dbWriteList list after the for loop has completed.
    for data in dataSet:
        typePattern = re.compile('(TAX|OS|APPSERVER|DATABASE|JAVA)')
        # Define XML elements to extract.
        try:
            vendorObj = data.getElementsByTagName("vendor")[0].childNodes[0].data
        except:
            vendorObj = 'Vendor Unavailable'
        try:
            versionObj = data.getElementsByTagName("version")[0].childNodes[0].data
        except:
            versionObj = 'Version Unavailable'
        # Convert XML elements to ascii strings. This makes it easier to write the objects to the database.
        vendorObj = str((vendorObj[:30] + '...')).encode('ascii') if len(vendorObj) > 30 else str(vendorObj).encode('ascii')
        versionObj = str((versionObj[:65] + '...')).encode('ascii') if len(versionObj) > 65 else str(versionObj).encode('ascii')

        typeObj = data.getElementsByTagName("type")[0].childNodes[0].data

        if typePattern.match(typeObj):
            isValid = 1
            # TAX
            if typeObj == xmlTypeTuple[0]:
                prepList.append(versionObj)
            # OS
            if typeObj == xmlTypeTuple[1]:
                prepList.append(vendorObj)
                prepList.append(versionObj)
            # APPSERVER
            if typeObj == xmlTypeTuple[2]:
                prepList.append(vendorObj)
                prepList.append(versionObj)
            # DATABASE
            if typeObj == xmlTypeTuple[3]:
                prepList.append(vendorObj)
                prepList.append(versionObj)
            # JAVA
            if typeObj == xmlTypeTuple[4]:
                prepList.append(vendorObj)
                prepList.append(versionObj)
                prepList.append(cusNumber)

    if isValid == 1:
        print 'File processed successfully. Moving on.'
        dbWriteList.append(prepList)
        completedFiles.append(document)
    else:
        print 'File did not contain valid XML elements:', document
        failedFiles.append(document)

    return

This gives me a string that looks like:

['my version 23121', 'Windows Server 2008 R2 amd64', '6.1', 'JBoss Web', '3.0.0-CR1', 'Microsoft SQL Server', '10.50.1600', 'Sun Microsystems Inc.', '1.6.0_26', 'customer number']

which gets appended to the list that is passed to the executemany command in the next function. Using executemany has cut a lot of processing time off, but look at this code and see myself repeating myself and it makes me think that this is not the best way to approach it.

Answer 1

1. Comments on your code

1. Your database queries use string concatenation and so are subject to SQL injection attacks. Consider this pair of lines:

   cusNumber = os.path.splitext(os.path.basename(document))[0]
   # ...
   _dbCur.execute('select count(*) from pm_customer where customer_id=' + cusNumber)
   

   Imagine that an attacker were able to trick you into a processing a customer file with the name 0;drop database.xml. This would cause you to execute the SQL command

   select count(*) from pm_customer where customer_id=0;drop database
   

   which I am sure wasn't what you intended to happen. You need to use SQL query parameters to pass untrusted data safely to SQL queries:

   _dbCur.execute('select count(*) from pm_customer where customer_id=?', cusNumber)
   

2. If you just want to know if a customer number is present in the database, use an EXISTS query to make it clear what you mean and to give the database engine a chance to make a better execution plan:

   select exists(select * from pm_customer where customer_id=?)
   

   Use a COUNT query only when you actually want to know the number of matching records.

   But there's no point in checking to see whether the customer exists in the database if you are just going to add them if they are not present. You should use a MERGE statement, like this:

   MERGE INTO pm_customer ON (customer_id = ?)
   WHEN NOT MATCHED THEN INSERT (customer_id) VALUES (?)
   

   But even better than that would be to save up all the customers that you plan to add to the database, and then add in one big MERGE statement which you call via executemany (preferably the same statement in which you add the customer attributes).

3. You get the contents of the <vendor> and <version> elements by executing getElementsByTagName queries. It would be more efficient to process the elements in the order they appear. (See section 2 below for an illustration of how to do this.)

4. Your code is delicate: it will break if the order of data within the XML file changes. This is because it assembles prepList in the order in which the <versionInfo> elements appear in the XML. But the values in prepList will eventually be fed to a prepared database query which expects them to appear in the right order (first the TAX data, then the OS data, etc.). It would be better to collect the data in whatever order it appears, and then put it into the right order at the end. That way your code will not break if the <versionInfo> elements appear in a different order.

5. You don't give the definition of xmlTypeTuple but it must be something like this:

   xmlTypeTuple = 'TAX OS APPSERVER DATABASE JAVA'.split()
   

   You then repeat this sequence of type names in the line

   typePattern = re.compile('(TAX|OS|APPSERVER|DATABASE|JAVA)')
   

   This violates the DRY (Don't Repeat Yourself) principle: if you had to add a new type it would be easy to make a mistake and add it to one of these and not the other. So you should write something like this:

   typePattern = re.compile('({})'.format('|'.join(map(re.escape, xmlTypeTuple))))
   

   (But see the next comment.)

6. You match the content of the <type> element against typePattern. But then you immediately check the content to see if it's equal to one of the members of the xmlTypeTuple list. It's not necessary to carry out both of these tests. If the latter passes, the former must too. So I would suggest dropping the regular expression match altogether and just seeing if the content is a member of xmlTypeTuple.

7. It's not clear what the point of isValid is. You set it to true as soon as the content of any single <type> element matches against typePattern. But you supply default values for all vendors and versions, so why is this necessary? Why is an empty customer record considered invalid?

   (Edited to add: I see that in the revised code in your answer you changed the behaviour so that isValid is set to true if there is an element with type TAX. This is still not clear to me. What's special about TAX?)

8. There's no need for your empty return statement: when Python reaches the end of a function body, it returns automatically.

9. Your design involves global state variables dbWriteList, completedFiles, and failedFiles. It is usually best practice to encapsulate persistent state into objects belonging to classes, and to turn the functions operating on that state into methods on the objects. See section 2 for some example code showing how you might do this.

10. Your success message

     print 'File processed successfully. Moving on.'
    

    would be more useful if it contained the name of the document that was processed successfully.

11. Failure messages like

    print 'File did not contain valid XML elements:', document
    

    ought to be printed to standard error, not standard output.

12. The Python style guide (PEP8) says that names of variables should be in lower case with underscores. So cusNumber should be something like customer_id.

13. You don't check for XML parsing errors. minidom.parse can raise various exceptions, for example:

    >>> minidom.parse('nonexistent.xml')
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "minidom.py", line 1920, in parse
        return expatbuilder.parse(file)
      File "expatbuilder.py", line 922, in parse
        fp = open(file, 'rb')
    IOError: [Errno 2] No such file or directory: 'nonexistent.xml'
    
    >>> minidom.parse('malformed.xml')
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "minidom.py", line 1920, in parse
        return expatbuilder.parse(file)
      File "expatbuilder.py", line 924, in parse
        result = builder.parseFile(fp)
      File "expatbuilder.py", line 207, in parseFile
        parser.Parse(buffer, 0)
    xml.parsers.expat.ExpatError: not well-formed (invalid token): line 1, column 1
    

    If you want errors like this to just result in the file being added to failedFiles, then you should check for them.

14. Code like

    typeObj = data.getElementsByTagName("type")[0].childNodes[0].data
    

    will raise an exception if there are no elements with the tag name type, or if the first such elements has no child nodes, or if the first child node is not a text node. If you want errors like this to just result in the file being added to failedFiles, then you should check for them.

15. It looks odd to me that you don't record the <vendor> for the TAX type, but do record it for all other types. Is that right? It might be worth a comment explaining why.

16. It's a bad idea to have constants like 30 and 65 in the text. You can fetch these from the database by executing a query like

    SELECT LENGTH(vendor), LENGTH(version) FROM pm_customer
    

    Also I find it suspicious that you test the length against 30 and if it's longer, you truncate it to 30 character and then append .... Wouldn't this make it 33 characters long, which would be too long? Shouldn't you truncate it to 27 characters if you want the string with ... to fit into a 30-character field?

17. I don't like the way you do string encoding. Normally one would expect the Python database interface to perform any necessary string encoding for you. This would certainly be the case in the database systems I am familiar with (MySQL, PostgreSQL, and SQLite). However, you're using Oracle, and for all I know things are different there. But you might think about this issue and read some documentation.

18. Your variable names are generally rather vague. Consider dbWriteList. Yes, it's a list of things that are going to be written to a database. But we don't really care that it's a list (that's just a minor detail of its implementation), and what you plan to do to it is much less important than what it is. So I would use a name like customer_records.

    Most of your names are vague in this way. I suggest having a good think about the meaning of each of them and then renaming them all. Like this:

    • parseDocumentData → load_customer_file
    • _dbCur → db_cursor
    • cusNumber → customer_id
    • prepList → customer
    • vendorObj → vendor
    • versionObj → version
    • completedFiles → valid_customer_files
    • failedFiles → invalid_customer_files

2. Improved code

This is how I would start refactoring the code. Not having an Oracle database to hand, and not knowing your database schema, I haven't been able to test the MERGE statement. But hopefully you will get the idea.

from xml.dom import minidom
import os
import sys

class CustomerLoader(object):
    """
    Process customer XML files and add them to the database.
    Call the load_customer(filename) method for each customer file.
    Then call the commit() method to commit them all to the database.
    """

    data_tags = 'type vendor version'.split()
    types = 'TAX OS APPSERVER DATABASE JAVA'.split()
    merge_customer = '''
        MERGE INTO pm_customers
        ON (customer_id = ?)
        WHEN MATCHED THEN UPDATE SET
             tax_version = ?,
             os_vendor = ?,
             os_version = ?,
             appserver_vendor = ?,
             appserver_version = ?,
             database_vendor = ?,
             database_version = ?,
             java_vendor = ?,
             java_version = ?
        WHEN NOT MATCHED THEN INSERT
             (customer_id, tax_version, os_vendor, os_version,
              appserver_vendor, appserver_version, database_vendor,
              database_version, java_vendor, java_version)
        VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)
        '''

    def __init__(self, cursor):
        self.cursor = cursor
        self.reset()
        cursor.execute('SELECT LENGTH(os_vendor), LENGTH(os_version) FROM pm_customer')
        self.vendor_length, self.version_length = cursor.fetchone()

    def reset(self):
        self.customers = []
        self.valid_customer_files = []
        self.invalid_customer_files = []

    def encode(self, s, length):
        """
        Return `s`, truncated to `length` characters and encoded as ASCII.
        """
        s = s.encode('ascii')
        if len(s) <= length:
            return s
        else:
            return s[:length - 3] + '...'.encode('ascii')

    def load_customer(self, filename, customer_id = None):
        """
        Load customer data for `customer_id` from `filename` but do
        not commit it to the database yet. If `customer_id` is not
        supplied, use the basename of `filename`.
        """
        if customer_id is None:
            customer_id = os.path.splitext(os.path.basename(filename))[0]
        try:
            dom = minidom.parse(filename)
            customer_data = {t: dict(vendor = 'Vendor unavailable',
                                     version = 'Version unavailable')
                             for t in self.types}
            for version_info in dom.getElementsByTagName('versionInfo'):
                info = dict()
                for element in version_info.childNodes:
                    if (element.nodeType == element.ELEMENT_NODE
                        and element.tagName in self.data_tags
                        and len(element.childNodes) == 1
                        and element.childNodes[0].nodeType == element.TEXT_NODE):
                        info[element.tagName] = element.childNodes[0].data
                if 'type' not in info:
                    raise Exception("versionInfo is missing <type> element")
                customer_data[info['type']] = info
            customer = [customer_id]
            for t in self.types:
                if t != 'TAX':
                    # Omit the TAX vendor. Why?
                    customer.append(self.encode(customer_data[t]['vendor'],
                                    self.vendor_length))
                customer.append(self.encode(customer_data[t]['version'],
                                self.version_length))
            self.customers.append(customer)
            self.valid_customer_files.append(filename)
            sys.stdout.write("Loaded customer {}\n".format(filename))
        except Exception as e:
            self.invalid_customer_files.append(filename)
            sys.stderr.write("Failed to load customer {}: {}\n".format(filename, e))

    def commit(self):
        """
        Commit the loaded customers to the database and reset the loader 
        object.
        """
        self.cursor.executemany(self.merge_customer,
                                (c + c for c in self.customers))
        self.reset()

Answer 2

I was able to simplify this by using BeautifulSoup4 to replace the block of code that searches for typePattern if: statements with one that matches against a simplified list and appends in one go. This still has to iterate through the section of code, but it is very fast. The slowest part is loading a large XML (3-5MB) file into memory before it gets parsed. I came to this solution before the answer above was posted. Here is the replacement code with the revised parser:

def parseDocumentData(document, cusNumber):
    "Take document from calling function, validate that the structure is \
    correct, then process the XML elements and return the data as a list."

    isValid = None
    global dbWriteList
    prepList = []

    # Check filename for customer number if cusNumber isn't passed.
    if cusNumber == None:
        cusNumber = os.path.splitext(os.path.basename(document))[0]
    cusNumber = str(cusNumber).encode('ascii')
    # Verify that the destination customer record exists. Source was verified by input function.
    _dbCur.execute('select count(*) from pm_customer where customer_id=' + cusNumber)
    cusVerify = _dbCur.fetchone()[0]
    if cusVerify == 0:
        _dbCur.execute('insert into pm_customer (customer_id) values (' + cusNumber + ')')
        print 'Customer number,', cusNumber, 'was created. Moving on.'
    else:
        print 'Customer number,', cusNumber, 'exists. Moving on.'

    xmlItems = BeautifulSoup(open(document))

    # This for loop parses the document and matches XML objects using the typePattern list.
    # As each object is found, it is appended to the prepList. The prepList is then appended
    # to the dbWriteList list after the for loop has completed.
    for data in xmlItems.find_all('versioninfo'):
        typePattern = ('OS', 'APPSERVER', 'DATABASE', 'JAVA')
        if data.type.text == 'TAX':
            prepList.append(data.version.text.encode('ascii'))
            isValid = 1
        elif data.type.text in typePattern:
            prepList.append(data.vendor.text[:30].encode('ascii'))
            prepList.append(data.version.text[:65].encode('ascii'))
    prepList.append(cusNumber)

    if isValid == 1:
        print 'File processed successfully. Moving on.'
        dbWriteList.append(prepList)
        completedFiles.append(document)
    else:
        print 'File did not contain valid XML elements:', document
        failedFiles.append(document)

    return