Jooq: empty MockResult throws DataAccessException

[codeturner]

Documentation for MockResult states that you can specify null for that data element to indicate that no rows will be returned. This worked great up until v3.10, where returning a MockResult object with a null data element will cause a DataAccessException: Error while reading field ... at JDBC index: 2

To reproduce in v.3.10.2 (assuming that a table called DEVICE exists):

DSL.using(new MockConnection(Mock.of(0)))
    .selectFrom(Tables.DEVICE)
    .fetch();

[lukaseder]

The change in 3.10 is due to #5818. See also #6774.

Documentation for MockResult states that you can specify null for that data element to indicate that no rows will be returned.

Would you mind pointing to the exact wording? Perhaps the documentation is inconsistent, which should be fixed in another issue.

I'm closing this as a duplicate of #5818 / #6774

[codeturner]

I'm not sure how this bug is a duplicate of those other two issues because I am neither returning a null or an empty MockResult[]. In my case, I am simply setting MockResult.data to null.

The javadoc for MockResult.data states that "if the given query execution did not provide any results, this may be null".

Also, the code for Mock.of(int rows) literally returns a MockResult object with data set to null:

public final class Mock {
    public static final MockDataProvider of(int rows) {
        return of(new MockResult(rows, null));
    }
    ...
}

Besides these examples, using new MockResult(0, null) is IMO the most logical way to express a result that returns no results in response to the executed query. I've written hundreds of unit tests using this method and it works great in testing the 'no records found' path.

[lukaseder]

The idea is that if the result parameter is null, then the MockResult models an update count, as when calling an INSERT statement. That's what the Javadoc intends to say (will look into ways to improve that doc).

The change in #5818 resulted in a fetch operation on statements that do not return results to return a result with a single row/column containing that update count. #5818 wasn't implemented with mocking in mind but it does influence mocking nevertheless, because you're fetching from a connection / statement that doesn't return any result. The alternative would have been throwing an exception.

Besides these examples, using new MockResult(0, null) is IMO the most logical way to express a result that returns no results in response to the executed query. I've written hundreds of unit tests using this method and it works great in testing the 'no records found' path.

What columns would you expect from a null result? Would you expect a table dum / dee style result? That's what happened before #5818, but it wasn't documented and broke quite a bit of other API.

In order to get an empty result, you have to ... provide an empty result (empty = no rows. columns are still mandatory).

[codeturner]

I feel that the MockResult should specify the actual records to be returned. In the case where a select statement should return no records, it makes sense to specify no records to the MockResult. When I query a database that doesn't find any matches, it doesn't return an empty row. It returns nothing.

MariaDB [(none)]> select * from mysql.user where user = "";
Empty set (0.00 sec)

Similarly, mocking should behave the same way:

assertEquals(new ArrayList<>(), DSL.using(new MockConnection(Mock.of(new MockResult(0, null))))
                .selectFrom(PERSON)
                .fetch());

Thus, I wouldn't expect any columns from a null result. I would expect an empty collection in this case, or null in the case of a fetchOne.

I understand that this mocking business isn't your primary focus, but the ability to write unit tests to verify SQL statements is what makes jOOQ so amazing. It's one of the reasons I refuse to use JPA for our complex data structures. But this change has broken hundreds of our unit tests, keeping me stuck to v3.9. Having to go back and specify empty records for the no result case is unfortunate.

[lukaseder]

When I query a database that doesn't find any matches, it doesn't return an empty row. It returns nothing.

That's just what is being displayed to you by your client. The actual result really contains all the columns. Here's what that query yields in MySQL Workbench:

With the command line tool, you could use the --quick option to force the display of the column names for empty results:
https://forums.mysql.com/read.php?10,225850

That's a MySQL command line flag, but I imagine it also works in MariaDB

Thus, I wouldn't expect any columns from a null result. I would expect an empty collection in this case, or null in the case of a fetchOne.

Those are your expectations, but they don't match my experience with 22 RDBMS. PostgreSQL is the only database I've seen so far that can (buggily) manage zero-column tables and/or results. The SQL standard does not allow for tables (or SELECT statements) to have zero columns.

The wording in the Javadoc is: "If the given query execution did not provide any results, this may be null". What's meant by "provide any results" is the fact that the JDBC driver should return false on Statement.execute(), i.e. there is an update count, and no call to Statement.getMoreResults() should be made to obtain any ResultSet.

How else would you encode this, currently?

I understand that this mocking business isn't your primary focus, but the ability to write unit tests to verify SQL statements is what makes jOOQ so amazing. It's one of the reasons I refuse to use JPA for our complex data structures. But this change has broken hundreds of our unit tests, keeping me stuck to v3.9. Having to go back and specify empty records for the no result case is unfortunate.

I think you're going down the wrong road trying to convince me that a change of behaviour (which wasn't clearly specified before) is wrong, because it broke your assumptions. I'm very happy to discuss your actual use-case and see how we can work together to help you succeed with that.

For instance, the quickest fix might be to always return the same Result instance containing, say, 100 columns and no rows. But still, I wouldn't consider that clean, because in SQL, all results have columns even if there are no rows. Suddenly not returning any columns on a SELECT a, b, c FROM x query would be surprising, and in fact, your tests should fail in that case.

But this change has broken hundreds of our unit tests, keeping me stuck to v3.9

I'm sorry to hear that, but again, why did you assume a result can have no columns? Just because it happened to be convenient to write this way? It really doesn't make any sense from a SQL perspective.

Having to go back and specify empty records for the no result case is unfortunate.

See it from the bright side. Your tests were based on wrong assumptions and thus maybe, some bugs slipped through that are now being detected...

[lukaseder]

The wording in the Javadoc is: "If the given query execution did not provide any results, this may be null". What's meant by "provide any results" is the fact that the JDBC driver should return false on Statement.execute(), i.e. there is an update count, and no call to Statement.getMoreResults() should be made to obtain any ResultSet.

This is what definitely needs more explicit clarification. I've created and fixed #6923

[codeturner]

I'm sorry to hear that, but again, why did you assume a result can have no columns? Just because it happened to be convenient to write this way? It really doesn't make any sense from a SQL perspective.

I see your point. In my defense, jOOQ is a Java library that makes SQL an abstraction. My assumptions are rooted in how a Java API should work, not SQL. So, when I see a Java method return null or an empty collection, it makes sense to me that my mocking would not have to specify a return value.

But I see where you are coming from, so I'll just have to fix those tests. Thanks for your feedback.

[lukaseder]

I see your point. In my defense, jOOQ is a Java library that makes SQL an abstraction. My assumptions are rooted in how a Java API should work, not SQL. So, when I see a Java method return null or an empty collection, it makes sense to me that my mocking would not have to specify a return value.

But it wouldn't even make sense in Java (in this particular case) :-) The way the MockResult works is, it mimicks the JDBC API.

Either you have no result (null, just as in Statement.executeQuery() == null), and then you really don't even have a result (how would you even encode that, otherwise?). In that case, the row count applies and mimicks Statement.executeUpdate() == rowCount.

So yes. I come from SQL and JDBC, and null meaning no result, not empty result, is the only reasonable interpretation here.

[codeturner]

The way the MockResult works is, it mimicks the JDBC API.

Yeah, it makes sense from that perspective.

As I understand it, columns must always be returned, and without an empty record, there is no way to tell the library what those columns are. Thus, MockResult.rows works as a trigger, of sorts:

• If rows > 0, then the data element contains rows to return, and columns are determined from the first of these results
• If rows == 0, then the data element should be a single row which is only used to determine the columns

When writing tests, it's a pain to have to create the empty row result. In my dream world, it'd be cool if the MockExecuteContext could contain a list of the expected columns. That way, the MockDataProvider could generate the empty row dynamically.

[lukaseder]

As I understand it, columns must always be returned, and without an empty record, there is no way to tell the library what those columns are.

Yes. Except (just to be sure), there's a need for an empty result, not an empty record (which is a result of one row).

Thus, MockResult.rows works as a trigger, of sorts:

• If rows > 0, then the data element contains rows to return, and columns are determined from the first of these results
• If rows == 0, then the data element should be a single row which is only used to determine the columns

No, the trigger is MockResult.data. If it is non-null, then a result can be expected from Statement.executeQuery(), or in other words, Statement.execute() will return true.

If it is null, then Statement.execute() will return false and an update count (the MockResult.rows) can be fetched.

When writing tests, it's a pain to have to create the empty row result. In my dream world, it'd be cool if the MockExecuteContext could contain a list of the expected columns. That way, the MockDataProvider could generate the empty row dynamically.

How could it know the type of the empty row? It has absolutely no context. You could even use MockDataProvider in a classic JDBC-based application or with Hibernate etc, without using jOOQ to run your queries...

Why is it a pain in the first place? I mean, you certainly test the cases as well where you actually do return a result. So the row types for those cases must match.

[codeturner]

Thanks for your help. In the end, I solved this by following your suggestion of returning a Result instance containing a bunch of dummy fields. Actually, I just pre-allocate an array of org.mockito.Mockito mock Field objects and call DSL.using(SQLDialect.DEFAULT).newResult(mockFields). This works great, even though it is obviously not clean.

Just so you can understand why this was a pain for me, imagine trying to test code that builds a complex model using data from several tables using a handful of separate queries. Or, imagine testing code that attempts to persist that complex model back into several tables. Both of these examples will require the construction of a MockDataProvider that will have to return a correct MockResult for each of these SQL executions.

My solution to testing code like this was to create a testing framework that simply requires the developer writing the tests to specify a pattern that matches the SQL statement and a list of Record objects to return. If they specified no Record objects to return, it would create an empty result, which is where it was using the now invalid new MockResult(0, null). If the framework doesn't find a matching SQL statement, it simply fails. This test framework also allows developers to specify a list of string arrays of raw data that can be coerced into a Result using DSLContext.fetchFromStringData. This is helpful to return the primary key of an update/insert statement. The framework also allows a developer to verify that certain SQL patterns where reached a certain number of times, similar to the junit verify method.

This test framework has been a game changer for us since we can now validate that the produced SQL matches what we expect, which has been extremely useful for complicated queries when it is easy to form statements incorrectly using the fluent interface. Also, we can upgrade versions of Jooq without fear since we can validate for ourselves that any changes to the actual SQL statements will still meet our expectations.

And thus, our interaction with MockResult has been warped by our use of this framework. Modifying this framework to now require the developer to include the returned columns would add a bunch of overhead to the hundreds of test cases that expect no rows returned.

[lukaseder]

This works great, even though it is obviously not clean.

I'm glad to hear it works now! Just to be sure we're on the same level of understanding here. It was not clean before! You exploited a flaw, and now you're exploiting another flaw, which is not guaranteed to work.

For instance, if your client code relies on dynamically discovering column names (e.g. through JDBC's ResultSetMetaData), then both the previous and the current solution would be wrong.

The only correct solution is to always provide the correct number of columns. Another idea: Maybe, you could use the jOOQ parser to parse the select statements and extract the expected column names from it, in the absence of actual columns?

List<Field<?>> list = ((Select) DSLContext.parser().parse("SELECT a, b, c")).getSelect();

Just so you can understand why this was a pain for me, imagine trying to test code that builds a complex model using data from several tables using a handful of separate queries. Or, imagine testing code that attempts to persist that complex model back into several tables

Well, our position on mocking databases is documented on the MockDataProvider (updated with #5452). It is really hard per se to mock databases, especially when state transfers and transactions are involved. The object of argument in this issue is really not the main issue of what makes mocking a tough problem to solve.

In my opinion, actual integration tests are a much better approach, but let's have this discussion at some other time ;-)

If they specified no Record objects to return, it would create an empty result

Don't get me wrong. I understand the convenience in your approach and in your expectations, but it's a matter of philosophy. jOOQ (and SQL) are type safe languages. A hypothetical Record0 type is simply not the same thing as a Record3<Integer, String, Date> type. If MockResult wouldn't lose type safety by being transmitted through the JDBC API, you should have even gotten a compilation error by not specifying the correct result record type, both previously (null = Record0) and now with the workaround (dummy record = fixed RecordN<[T1, T2, ..., TN]>)

which is where it was using the now invalid new MockResult(0, null)

It was invalid before, but happened to work in some cases, including yours.

This test framework has been a game changer for us since we can now validate that the produced SQL matches what we expect [...] Also, we can upgrade versions of Jooq without fear since we can validate for ourselves that any changes to the actual SQL statements will still meet our expectations

Would be cool to read about that in a blog post!

[lukaseder]

I have thought about this again. What would be really helpful in your case would be a way to access the original jOOQ Query reference via the MockContext, if such a reference is available (i.e. if the JDBC statement is executed by jOOQ, not by some other client).

I've created a new feature request for this: #6999