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

JDBC ResultSet should count columns from 1 and not 0 #32

Closed stefanofornari closed 1 year ago

stefanofornari commented 1 year ago

This PR should fix #31 on tag 4.9.0 I could not apply it on 4.10 because I had issues like java.lang.ClassNotFoundException: com.datastax.oss.driver.api.core.data.CqlVector when running the wrapper. I could not run the test suite because of the dependencies on a docker environment, but the PR is small enough to be included once you see the test suite pass in your environment.

maximevw commented 1 year ago

Good catch @stefanofornari! Indeed the findColumn(String) method is not implemented as described in the JDBC API specifications since we used the 0-based index directly returned by the DataStax driver.

So, your fix is correct and thanks to your finding, I discovered another issue: The methods checkName(String) and checkIndex(int) should be re-written like that:

    private void checkIndex(final int index) throws SQLException {
        if (this.currentRow != null) {
            if (index < 1 || index > this.currentRow.getColumnDefinitions().size()) {
                throw new SQLSyntaxErrorException(String.format(MUST_BE_POSITIVE, index) + StringUtils.SPACE
                    + this.currentRow.getColumnDefinitions().size());
            }
            this.wasNull = this.currentRow.isNull(index - 1);
        } else if (this.driverResultSet != null) {
            if (index < 1 || index > this.driverResultSet.getColumnDefinitions().size()) {
                throw new SQLSyntaxErrorException(String.format(MUST_BE_POSITIVE, index) + StringUtils.SPACE
                    + this.driverResultSet.getColumnDefinitions().size());
            }
        }

    }

    private void checkName(final String name) throws SQLException {
        if (this.currentRow != null) {
            if (!this.currentRow.getColumnDefinitions().contains(name)) {
                throw new SQLSyntaxErrorException(String.format(VALID_LABELS, name));
            }
            this.wasNull = this.currentRow.isNull(name);
        } else if (this.driverResultSet != null) {
            if (!this.driverResultSet.getColumnDefinitions().contains(name)) {
                throw new SQLSyntaxErrorException(String.format(VALID_LABELS, name));
            }
        }
    }

Indeed, if the column is not found, it throws an IllegalArgumentException instead of a SQLSyntaxErrorException because the method isNull() must be called only after checking the index or column name is valid.

Also, your fix and the one described just above should also be applied in the class CassandraMetadataResultSet.

Regarding your ClassNotFoundException problem, it's due to the fact the version 4.10.0 updated the driver core to 4.17.0. Maybe you should rebase your branch to solve this build issue.

stefanofornari commented 1 year ago

I have to say I ran into it just because ORMLite complained about it :) I would actually be great if you could add support for Cassandra to it :D

BTW, when do you think you will be able to release this fix?

maximevw commented 1 year ago

Hello @stefanofornari,

I merged your fix into release/next branch. Then, I will also fix other bugs I found as I detailed in my previous message and take advantage of this opportunity to add some unit tests relative to these changes.

I'll try to release a version 4.10.1 during the next week.

Regarding the support of Cassandra by ORMLite, I think an implementation of BaseDatabaseType should be added here in ormlite-jdbc: https://github.com/j256/ormlite-jdbc/tree/master/src/main/java/com/j256/ormlite/jdbc/db But a deeper analysis and testing is certainly required because some features can't probably be implemented with Cassandra. Did you already create an issue for this in the ORMLite project to know if the author is open to add new contributions? Because I saw the last release was almost 2 years ago. 😟

stefanofornari commented 1 year ago

I'll try to release a version 4.10.1 during the next week.

ok great thanks!

Regarding the support of Cassandra by ORMLite, I think an implementation of BaseDatabaseType should be added here in ormlite-jdbc: https://github.com/j256/ormlite-jdbc/tree/master/src/main/java/com/j256/ormlite/jdbc/db But a deeper analysis and testing is certainly required because some features can't probably be implemented with Cassandra.

Yep; for now I found also missing the use of "allow filtering", which I do not know how to best fit it in ORMLite, but I am sure it can be sorted out.

Did you already create an issue for this in the ORMLite project to know if the author is open to add new contributions? Because I saw the last release was almost 2 years ago. 😟

I have created a discussion topic https://github.com/j256/ormlite-core/discussions/290 , but I haven't got any reaction yet. Later this year I've got an interaction for a fix, so maybe he is not going to be super fast, but we may not be hopeless.

maximevw commented 1 year ago

@stefanofornari FYI, The version 4.10.1 has just been released.

stefanofornari commented 1 year ago

Great thanks! it works like a charm.