xerial / sqlite-jdbc

SQLite JDBC Driver
Apache License 2.0
2.85k stars 619 forks source link

Long type value convert bug #1177

Closed sailingsky closed 2 weeks ago

sailingsky commented 1 month ago

Describe the bug Long type value convert bug

To Reproduce use sqlite create a table(eg:test),it has two columns(id INTEGER, name TEXT), insert two records, like

INSERT INTO "main"."test" ("id", "name") VALUES (221, 'te2');
INSERT INTO "main"."test" ("id", "name") VALUES (850361281191579648, 'na');

then, use mybaits write a query interface and a mapper xml config:

public interface TestMapper {

    List<Map<String,Object>> queryMaps();
}
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE mapper PUBLIC "-//mybatis.org//DTD Mapper 3.0//EN" "http://mybatis.org/dtd/mybatis-3-mapper.dtd">
<mapper namespace="com.example.sqlitetest.mapper.TestMapper">

    <select id="queryMaps" resultType="java.util.Map">
        select * from test
    </select>
</mapper>

when you run this query ,it will return wrong id value. the second row's id value converted to Integer type be overflowed.

[{"name":"te2","id":221},{"name":"na","id":1881903104}]

I tracked mybatis source code and the sqlite-jdbc source code, found root cause at JDBC3ResultSet.java#getColumnClassName(int col).

            case SQLITE_INTEGER:
                long val = getLong(col);
                if (val > Integer.MAX_VALUE || val < Integer.MIN_VALUE) {
                    return "java.lang.Long";
                } else {
                    return "java.lang.Integer";
                }

Expected behavior expect return correct Long value.

Environment (please complete the following information):

hope this can help you.

gotson commented 1 month ago

Can you explain why this is a bug here and not in what MyBatis does ? I am not sure to understand.

sailingsky commented 1 month ago

alright, you can see mybatis code : DefaultResultSetHandler.java#createAutomaticMappings --->ResultSetWrapper.java#getTypeHandler , the key code is:

        final Class<?> javaType = resolveClass(classNames.get(index)); 

classNames will be init when ResultSetWrapper be constructed

  public ResultSetWrapper(ResultSet rs, Configuration configuration) throws SQLException {
    this.typeHandlerRegistry = configuration.getTypeHandlerRegistry();
    this.resultSet = rs;
    final ResultSetMetaData metaData = rs.getMetaData();
    final int columnCount = metaData.getColumnCount();
    for (int i = 1; i <= columnCount; i++) {
      columnNames.add(configuration.isUseColumnLabel() ? metaData.getColumnLabel(i) : metaData.getColumnName(i));
      jdbcTypes.add(JdbcType.forCode(metaData.getColumnType(i)));
      classNames.add(metaData.getColumnClassName(i));
    }
  }

then it will call sqlite-jdbc's JDBC3ResultSet.java#getColumnClassName method, it return 'java.lang.Integer'.

code1

autoMappingsCache save the link between column and typehandler(id -> java.lang.Integer), all records typehandler mapping will based on autoMappingsCache, so the bug produced.

I don't know if this is mybatis problem or sqlite-jdbc problem, I will try to raise an issue for mybatis.

gotson commented 1 month ago

autoMappingsCache save the link between column and typehandler(id -> java.lang.Integer)

that's most likely the problem. This would be fine with any other SQL database, but SQLite can store anything in a column. One row could be text, the other int, and another one blob, all in the same column.

sailingsky commented 1 month ago

so does it have a solution?

harawata commented 1 month ago

Hello,

I understand that there are some cases that are difficult to determine the right class name for a column, but there seems to be a room for improvement when the column is defined as BIGINT at least.

Can we, for example, look up the column type name first? (I ran mvn test and this change did not break any existing test)

diff --git a/src/main/java/org/sqlite/jdbc3/JDBC3ResultSet.java b/src/main/java/org/sqlite/jdbc3/JDBC3ResultSet.java
index 7647112..c51b3c8 100644
--- a/src/main/java/org/sqlite/jdbc3/JDBC3ResultSet.java
+++ b/src/main/java/org/sqlite/jdbc3/JDBC3ResultSet.java
@@ -580,8 +580,11 @@ public abstract class JDBC3ResultSet extends CoreResultSet {
     public String getColumnClassName(int col) throws SQLException {
         switch (safeGetColumnType(markCol(col))) {
             case SQLITE_INTEGER:
-                long val = getLong(col);
-                if (val > Integer.MAX_VALUE || val < Integer.MIN_VALUE) {
+                String typeName = getColumnTypeName(col);
+                if ("BIGINT".equals(typeName)
+                        || "INT8".equals(typeName)
+                        || "UNSIGNED BIG INT".equals(typeName)
+                        || isLong(getLong(col))) {
                     return "java.lang.Long";
                 } else {
                     return "java.lang.Integer";
@@ -985,4 +988,8 @@ public abstract class JDBC3ResultSet extends CoreResultSet {
     private String safeGetColumnName(int col) throws SQLException {
         return stmt.pointer.safeRun((db, ptr) -> db.column_name(ptr, checkCol(col)));
     }
+
+    private boolean isLong(long val) {
+        return val > Integer.MAX_VALUE || val < Integer.MIN_VALUE;
+    }
 }

And here is a new test I added to ResultSetTest.

@Test
void gh1177_getColumnClassName_BIGINT() throws SQLException {
  stat.executeUpdate("create table gh1177 (c1 BIGINT)");
  stat.executeUpdate("insert into gh1177 values (850361281191579648)");
  {
    ResultSet rs = stat.executeQuery("select c1 from gh1177 order by c1");
    ResultSetMetaData meta = rs.getMetaData();
    assertThat(meta.getColumnClassName(1)).isEqualTo(Long.class.getName());
  }
  stat.executeUpdate("insert into gh1177 values (22)");
  {
    ResultSet rs = stat.executeQuery("select c1 from gh1177 order by c1");
    ResultSetMetaData meta = rs.getMetaData();
    assertThat(meta.getColumnClassName(1)).isEqualTo(Long.class.getName());
  }
}

It still returns "java.lang.Integer" in some situations, but using CAST() can be a workaround, I think.

If it's worth a review, I would be happy to submit a PR.

p.s. I'm pretty sure you all are aware, but ResultSetMetadata is a metadata for the ResultSet and the result of getColumnClassName() does not change for each row, so there is nothing wrong with caching the result.

gotson commented 1 month ago

Can we, for example, look up the column type name first?

there's 2 different methods from what i have seen in JDBC, one on the table level, which looks at the optional type hints, and return a SQL type, and the one you are referring to which looks at the exact result set ROW metadata.

For instance in your case, if you were to move to the next row of the result set, it would return Long for the column.

To me this is an issue with the dynamic nature of SQLite and the fact that MyBatis caches the type of the first row and reuses that on all subsequent rows.

and the result of getColumnClassName() does not change for each row

it actually does

harawata commented 1 month ago

Hello @gotson ,

Thank you for the comment! I didn't know about SQLite's "dynamic nature". JDBC API clearly does not take it into account either.

@sailingsky , In general, retrieving metadata is a costly operation and it is unlikely that MyBatis abandons the metadata caching. As I explained, you may have to use POJO instead of java.util.Map if you use SQLite.

gotson commented 1 month ago

I didn't know about SQLite's "dynamic nature".

a good read about it is https://sqlite.org/datatype3.html

JDBC API clearly does not take it into account either.

no, SQLite is probably the only DB that has this

gotson commented 2 weeks ago

Closing this, since it is not a bug but expected behaviour.