FirebirdSQL / jaybird

JDBC driver for Firebird
https://www.firebirdsql.org/en/jdbc-driver/
GNU Lesser General Public License v2.1
90 stars 23 forks source link

Jaybird 5 trims returned data #729

Closed EPluribusUnum closed 1 year ago

EPluribusUnum commented 1 year ago

Hi Mark!

org.firebirdsql.jdbc.FBResultSet has hard coded setTrimTrailing(true) call. Prior Jaybird versions returned the string without trim. This is a breaking change, Jaybird should/must return original data returned by the Firebird engine, we need the whitespace. Please add an option to turn off this behaviour.

Thank you!

mrotteveel commented 1 year ago

Jaybird doesn't hardcode the call trimTrailing(true), it depends on the parameter trimStrings of the prepareVars method. Jaybird 5 should trim under the same conditions as Jaybird 4.

I tried this with the following test, and the value is not trimmed as expected:

    @Test
    void testNoTrimByDefault() throws Exception {
        try (Connection connection = getConnectionViaDriverManager();
             Statement stmt = connection.createStatement();
             ResultSet rs = stmt.executeQuery("select 'abc   ' from rdb$database")) {
            assertTrue(rs.next(), "expected a row");
            assertEquals("abc   ", rs.getString(1), "value was unexpectedly trimmed");
        }
    }

Please provide a reproduction program.

mrotteveel commented 1 year ago

Is this by any chance with CallableStatement#getString?

EPluribusUnum commented 1 year ago

Yes, I tried with a procedure call

mrotteveel commented 1 year ago

It looks like in FBCallableStatement it has a code path that does trim strings if you access through getString of the callable statement instead of a result set. I'll need to check the history to see why.

mrotteveel commented 1 year ago

OK, I can reproduce it with:

   @Test
    void callableStatementShouldNotTrim() throws Exception{
        executeDDL(con, """
                create procedure char_return returns (val char(5)) as
                begin
                  val = 'A';
                end""");

        try (CallableStatement cstmt = con.prepareCall("execute procedure char_return")) {
            cstmt.execute();
            assertEquals("A    ", cstmt.getString(1));
        }
    }
mrotteveel commented 1 year ago

This trim behaviour was also present in Jaybird 4 (and in Jaybird 3). If you experience different behaviour compared to Jaybird 4, I would really like to see a reproducible test case.

I guess you might be using getObject(..) instead of getString(..), as previous versions only applied the trim behaviour in getString(..), but I would like to see that confirmed.

mrotteveel commented 1 year ago

Behaviour was introduced by accident in #167 (but only in 3.0, not in 2.2.6)

EPluribusUnum commented 1 year ago

4.0.7.java8 (no trimming)

DefaultDatatypeCoder.decodeString(byte[]) line: 235 
FBWorkaroundStringField(FBStringField).getString() line: 163    
FBWorkaroundStringField.getString() line: 111   
FBWorkaroundStringField(FBField).getObject() line: 346  
FBResultSet(AbstractResultSet).getObject(int) line: 509 
FBCallableStatement(AbstractCallableStatement).getObject(int) line: 462 
DelegatingCallableStatement.getObject(int) line: 463    
DelegatingCallableStatement.getObject(int) line: 463    
ConfigurableConnection$ConfigurableCallableStatement(AbstractCallableStatementWrapper).getObject(int) line: 1029    
SQLCallableStatement(AbstractCallableStatementWrapper).getObject(int) line: 1029    
ProcedureCallableStatement(AbstractCallableStatementWrapper).getObject(int) line: 1029  
DBUtils.callableStatementToMap(CallableStatement, String) line: 1110    
DBUtils.executeProcIntoMap(Connection, String, Map<String,Object>) line: 3080   
TestManager.runSQL(SQL, List<Param>, Connection, boolean) line: 1066    
TestManager.access$2(TestManager, SQL, List, Connection, boolean) line: 982 
TestManager$TestRunner.check() line: 283    
TestManager$TestRunner(AbstractTestManager$AbstractTestRunner).run() line: 886  
AbstractTestManager$TestExecuter.call() line: 1050  
AbstractTestManager$TestExecuter.call() line: 1013  
FutureTask<V>.run() line: 266   
ThreadPoolExecutor.runWorker(ThreadPoolExecutor$Worker) line: 1149  
ThreadPoolExecutor$Worker.run() line: 624   
Thread.run() line: 750  

FBWorkaroundStringField.getString() line: 111; Here trimString = false

5.0.0.java8-beta-1 (trimming)

FBStringField.applyTrimTrailing(String) line: 214   
FBStringField.getString() line: 187 
FBStringField.getObject() line: 79  
FBResultSet.getObject(int) line: 518    
FBCallableStatement.getObject(int) line: 511    
DelegatingCallableStatement.getObject(int) line: 463    
DelegatingCallableStatement.getObject(int) line: 463    
ConfigurableConnection$ConfigurableCallableStatement(AbstractCallableStatementWrapper).getObject(int) line: 1029    
SQLCallableStatement(AbstractCallableStatementWrapper).getObject(int) line: 1029    
ProcedureCallableStatement(AbstractCallableStatementWrapper).getObject(int) line: 1029  
DBUtils.callableStatementToMap(CallableStatement, String) line: 1110    
DBUtils.executeProcIntoMap(Connection, String, Map<String,Object>) line: 3080   
TestManager.runSQL(SQL, List<Param>, Connection, boolean) line: 1066    
TestManager.access$2(TestManager, SQL, List, Connection, boolean) line: 982 
TestManager$TestRunner.check() line: 283    
TestManager$TestRunner(AbstractTestManager$AbstractTestRunner).run() line: 886  
AbstractTestManager$TestExecuter.call() line: 1050  
AbstractTestManager$TestExecuter.call() line: 1 
FutureTask<V>.run() line: 266   
ThreadPoolExecutor.runWorker(ThreadPoolExecutor$Worker) line: 1149  
ThreadPoolExecutor$Worker.run() line: 624   
Thread.run() line: 750  

FBStringField.applyTrimTrailing(String) line: 214; Here trimTrailing = true

mrotteveel commented 1 year ago

Looking at your that stacktrace, your code is using getObject(..), which in Jaybird 5 does the exact same thing as getString(..), while in previous versions only FBResultSet.getString(..) would trim in this case, but FBResultSet.getObject(..) would not. If you run your code against Jaybird 4.0.7.java8 using getString instead of getObject, you will find that it did trim in that version as well.

To be clear, I acknowledge this is a bug (introduced in Jaybird 3, but now more visible because it is now also exposed through getObject and not only getString), but a reproducible test case to confirm I'm actually fixing the bug you're experiencing, and not a similar adjacent bug would be very helpful.

mrotteveel commented 1 year ago

In Jaybird 4 the trim happens in AbstractResultSet.getString(int) based on the value of trimStrings of the result set, the trimString in FBWorkaroundStringField is a red herring, because it was never used.

mrotteveel commented 1 year ago
    @Test
    public void callableStatementShouldNotTrim_729() throws Exception {
        executeDDL(con,
                "create procedure char_return returns (val char(5)) as\n" +
                "begin\n" +
                "  val = 'A';\n" +
                "end");

        try (CallableStatement cstmt = con.prepareCall("execute procedure char_return")) {
            cstmt.execute();
            ResultSet rs = cstmt.getResultSet();
            assertTrue("Expected a row", rs.next());
            assertEquals("A    ", rs.getString(1));
            assertEquals("A    ", cstmt.getObject(1));
            assertEquals("A    ", cstmt.getString(1));
        }
    }

In Jaybird 4, this test fails on cstmt.getString, in Jaybird 5 it already fails on cstmt.getObject.

Is this comparable to your usage?

EPluribusUnum commented 1 year ago
        conn.createStatement().execute(
            "create or alter procedure char_return returns (val varchar(5)) as\n" + //varchar return
                "begin\n" +
                "  val = 'A ';\n" + //note the trailing space
                "end");
        conn.commit();
        CallableStatement cs = conn.prepareCall("execute procedure char_return(?)");
        cs.registerOutParameter(1, Types.VARCHAR);
        cs.execute();
        assertEquals("A ", cs.getObject(1).toString());

This passes on 4.0.7 and fails on 5.0.0

mrotteveel commented 1 year ago

Yes, but if you use cs.getString(1) it will also fail on Jaybird 4.0.7, right?

EPluribusUnum commented 1 year ago

Yes, with getString(1) it fails on 4.0.7 too.

mrotteveel commented 1 year ago

Fix can be tested with jaybird-5.0.1.java11-SNAPSHOT (and jaybird-5.0.1.java8-SNAPSHOT) from https://oss.sonatype.org/content/repositories/snapshots