Commons DbUtils
  1. Commons DbUtils
  2. DBUTILS-34

BasicRowProcessor loses any information on database field case

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.0
    • Fix Version/s: 1.2
    • Labels:
      None

      Description

      In the BasicRowProcessor class there is a comment about the toMap method:
      " This implementation returns a <code>Map</code> with case insensitive column names as keys. Calls to map.get("COL") and map.get("col") return the same value"

      So the "contract" of this method just says that the "get" method is case insensitive. The current implementation obtains this result by putting all the keys in lowercase. This way we simply "lose" every information about the case of the fields. If you call the "keys" iterator, you don't have anymore the keys you inserted, but the "lowercase" version.

      I find it not completely correct: I would expect the keys to be exactly the ones I set on the database, also if I can get the value in a case insensitive way.
      I still have to find the idea for a pathc, but I'd like to know if you find my reasoning correct.

      I consider the current behaviour a problem, since I had to map the database fields to a bean and the "case" made it impossible, even if I had set my database to return mixed case fieldnames.

      1. BasicRowProcessor.java
        8 kB
        Julien Aymé
      2. BasicRowProcessor.patch
        4 kB
        Julien Aymé

        Activity

        Fabio Insaccanebbia created issue -
        Hide
        Julien Aymé added a comment -

        Hello, I had the same problem as yours, and I fixed it with an implementation of my own of the CaseInsensitiveHashMap:

        Instead of converting the key to lower case before putting it into the map, I chose to keep the key as it was given, and maintain an internal mapping from lower case keys to real keys.

        I join the whole BasicRowProcessor class, and the patch. (Some of the comments can be removed, it's just for the clarity of the code).

        Show
        Julien Aymé added a comment - Hello, I had the same problem as yours, and I fixed it with an implementation of my own of the CaseInsensitiveHashMap: Instead of converting the key to lower case before putting it into the map, I chose to keep the key as it was given, and maintain an internal mapping from lower case keys to real keys. I join the whole BasicRowProcessor class, and the patch. (Some of the comments can be removed, it's just for the clarity of the code).
        Julien Aymé made changes -
        Field Original Value New Value
        Attachment BasicRowProcessor.patch [ 12357116 ]
        Attachment BasicRowProcessor.java [ 12357117 ]
        Hide
        Dan Fabulich added a comment -

        Julien's patch keeps an internal map of given keys to keys in lower case. So you call put("Foo", value), and the lowerCaseMap maps "foo" to "Foo", and then the "real" HashMap stores ("Foo", value).

        I'm concerned about this patch, because every change has to happen in two phases: first you update lowerCaseMap, and THEN you update the real map; I strongly suspect that it's not thread-safe as a result. (I think the two maps can get out of sync, resulting in corruption.)

        More generally, I don't understand the point of this bug. Why do you need to retrieve the keys yourself? Fabio originally wrote "I had to map the database fields to a bean and the 'case' made it impossible" ... but we already have a system for mapping database fields to bean properties. It works fine (right?); it's clearly not "impossible."

        Show
        Dan Fabulich added a comment - Julien's patch keeps an internal map of given keys to keys in lower case. So you call put("Foo", value), and the lowerCaseMap maps "foo" to "Foo", and then the "real" HashMap stores ("Foo", value). I'm concerned about this patch, because every change has to happen in two phases: first you update lowerCaseMap, and THEN you update the real map; I strongly suspect that it's not thread-safe as a result. (I think the two maps can get out of sync, resulting in corruption.) More generally, I don't understand the point of this bug. Why do you need to retrieve the keys yourself? Fabio originally wrote "I had to map the database fields to a bean and the 'case' made it impossible" ... but we already have a system for mapping database fields to bean properties. It works fine (right?); it's clearly not "impossible."
        Hide
        Dan Fabulich added a comment -

        Liam helped me understand the point of this on the commons-dev list. Checked into "bugfixing" branch in revision 743269.

        CaseInsensitiveHashMap ISN'T thread-safe, but then, neither is HashMap! So, uh, never mind.

        Show
        Dan Fabulich added a comment - Liam helped me understand the point of this on the commons-dev list. Checked into "bugfixing" branch in revision 743269. CaseInsensitiveHashMap ISN'T thread-safe, but then, neither is HashMap! So, uh, never mind.
        Hide
        Julien Aymé added a comment -

        Hello, thanks for applying the patch!

        I'd like to add some minor modifications:
        We should consider making the field lowerCaseMap final (I think that this is a Good Practice TM);

        and for the putAll method, I should have used the entrySet instead of the keySet:

        	public void putAll(Map m) {
        	    Iterator iter = m.entrySet().iterator();
        	    while (iter.hasNext()) {
        	    	Map.Entry entry = (Map.Entry) iter.next();
        	        Object key = entry.getKey();
        	        Object value = entry.getValue();
        	        this.put(key, value);
        	    }
        	}
        
        Show
        Julien Aymé added a comment - Hello, thanks for applying the patch! I'd like to add some minor modifications: We should consider making the field lowerCaseMap final (I think that this is a Good Practice TM); and for the putAll method, I should have used the entrySet instead of the keySet: public void putAll(Map m) { Iterator iter = m.entrySet().iterator(); while (iter.hasNext()) { Map.Entry entry = (Map.Entry) iter.next(); Object key = entry.getKey(); Object value = entry.getValue(); this .put(key, value); } }
        Hide
        Dan Fabulich added a comment -

        Done in bugfixing branch revision 743292.

        Show
        Dan Fabulich added a comment - Done in bugfixing branch revision 743292.
        Hide
        Henri Yandell added a comment -

        svn ci -m "Merging in Dab Fabulich's work on https://svn.apache.org/repos/asf/commons/sandbox/dbutils/bugfixing from -r741987:747723. Resolving DBUTILS-34 - DBUTILS-37 - DBUTILS-29 - DBUTILS-14 - DBUTILS-31 - DBUTILS-39 - DBUTILS-41 - DBUTILS-44 - DBUTILS-33 - DBUTILS-42 - DBUTILS-40"

        Sending pom.xml
        Sending src/java/org/apache/commons/dbutils/BasicRowProcessor.java
        Sending src/java/org/apache/commons/dbutils/BeanProcessor.java
        Sending src/java/org/apache/commons/dbutils/QueryRunner.java
        Adding src/java/org/apache/commons/dbutils/handlers/AbstractListHandler.java
        Sending src/java/org/apache/commons/dbutils/handlers/ArrayListHandler.java
        Sending src/java/org/apache/commons/dbutils/handlers/BeanListHandler.java
        Sending src/java/org/apache/commons/dbutils/handlers/ColumnListHandler.java
        Deleting src/java/org/apache/commons/dbutils/handlers/GenericListHandler.java
        Sending src/java/org/apache/commons/dbutils/handlers/MapListHandler.java
        Sending src/test/org/apache/commons/dbutils/BaseTestCase.java
        Adding src/test/org/apache/commons/dbutils/QueryRunnerTest.java
        Transmitting file data .........
        Committed revision 747724.

        Show
        Henri Yandell added a comment - svn ci -m "Merging in Dab Fabulich's work on https://svn.apache.org/repos/asf/commons/sandbox/dbutils/bugfixing from -r741987:747723. Resolving DBUTILS-34 - DBUTILS-37 - DBUTILS-29 - DBUTILS-14 - DBUTILS-31 - DBUTILS-39 - DBUTILS-41 - DBUTILS-44 - DBUTILS-33 - DBUTILS-42 - DBUTILS-40 " Sending pom.xml Sending src/java/org/apache/commons/dbutils/BasicRowProcessor.java Sending src/java/org/apache/commons/dbutils/BeanProcessor.java Sending src/java/org/apache/commons/dbutils/QueryRunner.java Adding src/java/org/apache/commons/dbutils/handlers/AbstractListHandler.java Sending src/java/org/apache/commons/dbutils/handlers/ArrayListHandler.java Sending src/java/org/apache/commons/dbutils/handlers/BeanListHandler.java Sending src/java/org/apache/commons/dbutils/handlers/ColumnListHandler.java Deleting src/java/org/apache/commons/dbutils/handlers/GenericListHandler.java Sending src/java/org/apache/commons/dbutils/handlers/MapListHandler.java Sending src/test/org/apache/commons/dbutils/BaseTestCase.java Adding src/test/org/apache/commons/dbutils/QueryRunnerTest.java Transmitting file data ......... Committed revision 747724.
        Henri Yandell made changes -
        Resolution Fixed [ 1 ]
        Status Open [ 1 ] Closed [ 6 ]
        Fix Version/s 1.2 [ 12312139 ]
        Dan Fabulich made changes -
        Assignee Dan Fabulich [ dfabulich ]

          People

          • Assignee:
            Dan Fabulich
            Reporter:
            Fabio Insaccanebbia
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development