Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Long type value convert bug #1177

Closed
sailingsky opened this issue Sep 17, 2024 · 9 comments
Closed

Long type value convert bug #1177

sailingsky opened this issue Sep 17, 2024 · 9 comments
Labels
question Further information is requested

Comments

@sailingsky
Copy link

sailingsky commented Sep 17, 2024

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):

  • OS: [e.g. Windows 10]
  • CPU architecture: [e.g. x86_64, arm64]
  • sqlite-jdbc version [3.46.1.0]
  • mybatis-spring-boot-starter version[3.0.3]
  • sqlite version[3.45.0]

hope this can help you.

@gotson
Copy link
Collaborator

gotson commented Sep 19, 2024

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

@sailingsky
Copy link
Author

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
Copy link
Collaborator

gotson commented Sep 20, 2024

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
Copy link
Author

so does it have a solution?

@harawata
Copy link

harawata commented Sep 21, 2024

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
Copy link
Collaborator

gotson commented Sep 23, 2024

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
Copy link

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
Copy link
Collaborator

gotson commented Sep 24, 2024

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 gotson added question Further information is requested and removed triage labels Sep 24, 2024
@gotson
Copy link
Collaborator

gotson commented Oct 22, 2024

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

@gotson gotson closed this as completed Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants