ing-bank / cassandra-jdbc-wrapper

A JDBC wrapper of Java Driver for Apache Cassandra®, which offers a simple JDBC compliant API to work with CQL3.
Apache License 2.0
74 stars 25 forks source link

Assertion error checking #39

Closed sualeh closed 12 months ago

sualeh commented 1 year ago

CResultSetMetaData makes an assumption that all table names are CQL identifiers. However, for some metadata resultset, table names may be empty strings. This can cause an assertion error in CResultSetMetaData::isSearchable. Perhaps the flatmap can check for empty table names, and do something different to prevent the assertion error? The error comes from KeyspaceMetadata::getTableCqlIdentifier::fromCqlStrings::needsDoubleQuotes.

maximevw commented 1 year ago

Thank you @sualeh for this finding.

Could you confirm this issue occurs when working on the metadata of a CassandraMetadataResultSet, typically obtained through a method of the CassandraDatabaseMetaData class? This might help to see how to handle such cases properly: I think the problem happens when there is no row in a metadata result set, typically because such result sets are built programmatically (without table name, explaining the issue) and this may lead to a lot of different issues/inconsistencies in result sets metadata (in the absence of rows in the result set). Fixing that will require a little refactoring (but it is definitely necessary) and I need to take the time to implement and test it.

In all the cases, I suggest the method isSearchable() returns false if the table name is empty/null because in this case, we cannot determine if the column is really searchable (and typically metadata result sets aren't "queryable" since they are not related to real tables in Cassandra).

sualeh commented 1 year ago

@maximevw Thanks for jumping on these issues quickly. Here is a project that illustrates the AssertError: https://github.com/sualeh/cassandra-issue39

You will need to have Docker installed, since it uses Test Containers.

maximevw commented 1 year ago

@sualeh, thanks for providing the example.

After modifying the implementation of isSearchable() as described in my previous message and refactoring the method SchemaMetadataResultSetBuilder.buildSchemas(String), I was able to run your test successfully:

Column: catalog='Test Cluster', schema='books', table='', column='TABLE_SCHEM'
Column information: searchable='false'
Column: catalog='Test Cluster', schema='books', table='', column='TABLE_CATALOG'
Column information: searchable='false'

Now, it also works when I replace the line 51 in your example by:

final ResultSet resultSet = databaseMetaData.getSchemas(null, "invalid_ks"); 
// returns empty ResultSet because the keyspace invalid_ks doesn't exist

So, this fixes the second issue preventing to get metadata from an empty CassandraMetadataResultSet. I have to generalize this to all the supported metadata result sets. I'll keep you informed here.

sualeh commented 1 year ago

Awesome! Thanks for the quick work, @maximevw!

sualeh commented 1 year ago

I would love to have a release of https://github.com/ing-bank/cassandra-jdbc-wrapper with these fixes, so I can remove the additional catches I put into https://github.com/schemacrawler/SchemaCrawler. How soon do you plan to release a new version? (No pressure.)

maximevw commented 1 year ago

Hi @sualeh, I would be delighted to help you with a new release soon. I plan to release the next version (4.11.0) including these fixes by the end of the month or beginning of December. But, I have to finalize their development first. 😉 Also, I would like to include some modifications on the database metadata to reflect the latest changes introduced by Cassandra 5 and test the integration with this latest version a little bit deeper. Stay tuned!

sualeh commented 1 year ago

@maximevw SchemaCrawler does a good job going deep into the JDBC metadata calls. I found the issues I reported due to SchemaCrawler test failres.

Would you like to include a SchemaCrawler unit test? I can help you with that. Basically, porting CassandraTest over to your project.

maximevw commented 1 year ago

@sualeh I think it's always a good idea to enhance unit testing. So, feel free to submit a new pull request helping to improve the code coverage of this driver.

maximevw commented 12 months ago

The fix has been integrated to the branch release/next. It will available in the next release (4.11.0).