Bug 33054

Summary: sql:query loses query column order
Product: Taglibs Reporter: Joachim Martin <joachimm>
Component: Standard TaglibAssignee: Tomcat Developers Mailing List <dev>
Severity: enhancement    
Priority: P5    
Version: 1.0.6   
Target Milestone: ---   
Hardware: All   
OS: All   
Attachments: Unit test for this issue

Description Joachim Martin 2005-01-11 23:33:53 UTC
sql:query appears to be using a TreeMap to hold row data.  This means
that it cannot be used to execute and display dynamic (user supplied)
SQL, because the column order is incorrect.

Please consider using SequencedHashMap from Jakarta commons-collections,
which will preserve entry order in the map.  I realize this will introduce
some backward compatibility issues for some people, but they probably
should be using the Map interface anyway.

FYI, http://displaytag.sourceforge.net/ does a great job displaying
the output of sql:query, but since the columns are not in the right
order, it's not very helpful.

This is in:
Comment 1 Henri Yandell 2006-12-27 17:07:28 UTC
Attempting to recreate (TreeMap definitely in use). The example to output the a
sql statement seems to work fine:

  <%-- Get the column names for the header of the table --%>
  <c:forEach var="columnName" items="${deejays.columnNames}">
    <th><c:out value="${columnName}"/></th>

  <%-- Get the value of each column while iterating over rows --%>
  <c:forEach var="row" items="${db.rowsByIndex}">
      <c:forEach var="column" items="${row}">
        <td><c:out value="${column}"/></td>

I suspect because the columnNames variable is the correct order. So that's a
definite workaround for this issue.
Comment 2 Henri Yandell 2006-12-27 17:10:57 UTC
Scratch that last comment. It's not the columnNames.

So despite the TreeMap; I can't reproduce this easily. I have a pretty small
table (3 rows) and my select statement returns 28 columns without any obvious
change in the order. So needs more digging to understand how this might come about.
Comment 3 Henri Yandell 2006-12-27 17:35:37 UTC
So here's a better test:

  <c:forEach var="row" items="${db.rows}">
      <c:forEach var="column" items="${row}">
        <td><c:out value="${column}"/></td>

You have to use rows and not rowIndex. The former uses the TreeMap and the
latter doesn't.

Definitely proves the bug. Things are ordered case insensitively by column name
(as the code is pretty honest about). From the javadoc for getRows:

     * Returns an array of SortedMap objects. The SortedMap
     * object key is the ColumnName and the value is the ColumnValue.
     * SortedMap was created using the CASE_INSENSITIVE_ORDER
     * Comparator so the key is the case insensitive representation
     * of the ColumnName.

There's nothing in the spec, so I think the next step here is to develop a
patch, and then to consider whether a test can easily be created.
Comment 4 Henri Yandell 2007-01-03 13:14:46 UTC
Pulling in
seems quite possible. Its deprecated because a version that is more embedded in
Collections now exists (ListOrderedMap), but pulling that version in involves a
lot more in the way of classes.

It's not thread-safe, but neither is TreeMap so that shouldn't be a problem. The
only backwards compatibility issue I can see is that TreeMap implements
SortedMap which adds some methods which SequencedHashMap won't be offering. It
seems unlikely that many people are using those methods.

So next up - a test case.
Comment 5 Henri Yandell 2007-01-03 14:43:56 UTC
Created attachment 19347 [details]
Unit test for this issue

Adds Derby as a dependency for the cactus tests
Comment 6 Henri Yandell 2007-01-03 15:41:42 UTC
So while I can happily test for this, and we can easily put a SequencedHashMap
in, the spec API for javax.servlet.jsp.jstl.sql.Result defines the method
getRows() as returning a SortedMap and not a Map.

SequencedHashMap is not a SortedMap - the insertion-ordering that this issue
calls for doesn't match the SortedMap API which indicates ordering should be
managed via  comparator.

It would be possible to hack the fork of SequencedHashMap around a bit to make
it a SortedMap implementation (given that it won't be public); but I think
that's abusing the spec far too much.


Looking at the sample page, I believe the following is a useable workaround for
this issue:

  <%-- Get the column names for the header of the table --%>
  <c:forEach var="columnName" items="${db.columnNames}">
    <th><c:out value="${columnName}"/></th>

  <%-- Get the value of each column while iterating over rows --%>
  <c:forEach var="row" items="${db.rowsByIndex}">
      <c:forEach var="column" items="${row}">
        <td><c:out value="${column}"/></td>

We should put that in a FAQ.
Comment 7 Henri Yandell 2007-01-29 16:25:41 UTC
svn ci -m "Adding unit test for Bug 33054 - however the assertion is turned off
because this is not something we can fix. It's being added as an example of how
to do a sql tag test so it doesn't get lost. " 

Sending        build-tests.xml
Sending        build_sample_standard.properties
Adding         test/org/apache/taglibs/standard/tag/el/sql
Adding         test/org/apache/taglibs/standard/tag/el/sql/Test33054.java
Adding         test/web/org/apache/taglibs/standard/tag/el/sql
Adding         test/web/org/apache/taglibs/standard/tag/el/sql/Test33054.jsp
Transmitting file data ....
Committed revision 501245.
Comment 8 Henri Yandell 2007-01-29 17:03:29 UTC
Added to the FAQ:


Otherwise, I can't see us fixing this so I'm resoling it WONTFIX.