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

Assertion error checking #39

Closed
sualeh opened this issue Nov 12, 2023 · 9 comments
Closed

Assertion error checking #39

sualeh opened this issue Nov 12, 2023 · 9 comments
Labels
bug Something isn't working

Comments

@sualeh
Copy link
Contributor

sualeh commented Nov 12, 2023

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

maximevw commented Nov 12, 2023

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

@maximevw maximevw added the bug Something isn't working label Nov 12, 2023
@sualeh
Copy link
Contributor Author

sualeh commented Nov 12, 2023

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

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

sualeh commented Nov 12, 2023

Awesome! Thanks for the quick work, @maximevw!

@sualeh
Copy link
Contributor Author

sualeh commented Nov 13, 2023

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

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

sualeh commented Nov 13, 2023

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

@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 added a commit that referenced this issue Nov 18, 2023
Return false when the method isSearchable(int) is called on the metadata
of a result set without table or schema name.
Also, fix issue preventing to retrieve the metadata of an empty
CassandraMetadataResultSet, and add null safety on some methods of
CassandraResultSet and CassandraMetadataResultSet.
@maximevw
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants