search

vlingo / xoom-symbio-jdbc

The VLINGO XOOM SYMBIO implementation for JDBC for Reactive storage using Event-Sourcing, Key-Value, and Object storage.
https://vlingo.io
Mozilla Public License 2.0
4 stars 12 forks source link

JDBCStorageDelegate.tableExists() returns different result depending on the database. #60

Closed chytonpide closed 2 years ago

chytonpide commented 2 years ago

Description

JDBCStorageDelegate.tableExists() uses DatabaseMetaData.getTables() its inside. However, JDBC DatabaseMetaData has a different scope depending on the database. So, JDBCStorageDelegate.tableExists() returns different result depending on the database.

For example, In Mysql, when you create tableA and on databaseA and then create tableA (the same name) on databaseB, DatabaseMetaData.getTables(null, null, "tableA", null) returns two results. Regardless of the current connection, DatabaseMetaData is about the scope of the entire database.

On the other hand, In Postgresql, DatabaseMetaData is about the scope of the currently connected database. If you have the connection to databaseA, DatabaseMetaData.getTables(null, null, "tableA", null) returns only one result.

This is the test for DatabaseMetaData. It fails with the Mysql driver but success with the Postgresql driver.

import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;;
import org.junit.jupiter.api.Test;

import java.sql.*;
import java.util.Properties;

import static org.junit.jupiter.api.Assertions.assertEquals;

public class DatabaseMetaDataTest {

    private final static String USERNAME = "root";
    private final static String PASSWORD = "root";
    private final static String URL = "jdbc:mysql://localhost:3306/";

    //private final static String USERNAME = "postgres";
    //private final static String PASSWORD = "a123456";
    //private final static String URL = "jdbc:postgresql://localhost:5432/";

    private final static String DB_NAME_1 = "test_db_1";
    private final static String DB_NAME_2 = "test_db_2";
    private final static String TABLE_NAME = "tbl_1";
    private final static Properties USER_PROPS = userProps();

    @BeforeAll
    public static void setup() throws SQLException {
        Connection conn = DriverManager.getConnection(URL, USER_PROPS);
        conn.createStatement().execute("DROP DATABASE IF EXISTS " + DB_NAME_1);
        conn.createStatement().execute("DROP DATABASE IF EXISTS " + DB_NAME_2);

        conn.createStatement().execute("CREATE DATABASE " + DB_NAME_1);
        conn.createStatement().execute("CREATE DATABASE " + DB_NAME_2);
        conn.close();
    }

    @Test
    public void testThatGetTables() throws SQLException {
        //given
        Connection connToDb1 = DriverManager.getConnection(URL + DB_NAME_1, USER_PROPS);
        connToDb1.createStatement().execute("CREATE TABLE " + TABLE_NAME + "(id INTEGER PRIMARY KEY)");
        connToDb1.close();

        //A table has the same name is created on DB2 also.
        Connection connToDb2 = DriverManager.getConnection(URL + DB_NAME_2, USER_PROPS);
        connToDb2.createStatement().execute("CREATE TABLE " + TABLE_NAME + "(id INTEGER PRIMARY KEY)");
        connToDb2.close();

        //when
        connToDb1 = DriverManager.getConnection(URL + DB_NAME_1, USER_PROPS);
        DatabaseMetaData db1MetaData = connToDb1.getMetaData();
        ResultSet rs = connToDb1.getMetaData().getTables(null, null, TABLE_NAME, null);

        int tableCount = 0;
        while(rs.next()) {
            tableCount ++;
        }
        connToDb1.close();

        //then
        //This Assertion failed because of that the rowCount is 2. but, It success with Postgresql.
        assertEquals(1, tableCount );
    }

    @AfterAll
    public static void tearDown() throws SQLException {
        Connection conn = DriverManager.getConnection(URL, USER_PROPS);
        conn.createStatement().execute("DROP DATABASE " + DB_NAME_1);
        conn.createStatement().execute("DROP DATABASE " + DB_NAME_2);
        conn.close();
    }

    private static Properties userProps() {
        Properties props = new Properties();
        props.setProperty("user",USERNAME);
        props.setProperty("password",PASSWORD);
        return props;
    }
}

Influence

It causes a problem when query and command databases are placed in the same Mysql instance. tableExists() returns true even if tables are created in only one of the two databases in Mysql. It causes to skip creating tables for Dispatcher that has the same name on each database.

I have encountered this problem with this xoom-turbo.properties when I created a project using xoom-designer.

...
database=MYSQL
database.name=cqrsdemo_command
database.driver=com.mysql.cj.jdbc.Driver
database.url=jdbc:mysql://localhost:3306/
database.username=root
database.password=root
database.originator=main

query.database=MYSQL
query.database.name=cqrsdemo_query
query.database.driver=com.mysql.cj.jdbc.Driver
query.database.url=jdbc:mysql://localhost:3306/
query.database.username=root
query.database.password=root
query.database.originator=main
....

Suggestion

It may be a solution to include the database name in the tables name for the Dispatcher or Adding the constraint that the query database and command database should be placed in the different database instances.

VaughnVernon commented 2 years ago

@chytonpide Thanks for the very complete issue report. Can you clarify with a code snippet what you mean by this?

include the database name in the tables name for the Dispatcher

I think this is the more correct way. I am concerned about breaking changes, however.

chytonpide commented 2 years ago

@VaughnVernon I mean that adding database name on dispatchableTableName() to every concrete class of JDBCStorageDelegate like this.

@Override
protected String dispatchableTableName() {
  return MessageFormat.format(TBL_VLINGO_SYMBIO_DISPATCHABLES, configuration.databaseName);
}
String TBL_VLINGO_SYMBIO_DISPATCHABLES = "{0}_tbl_xoom_symbio_dispatchables";

An alternative

For Mysql and Postgres it would be also working that passing the database name as catalog parameter to DatabaseMetaData.getTables() on JDBCStorageDelegate like this.

private boolean tableExists(final String tableName) throws Exception {
  final DatabaseMetaData metadata = connection.getMetaData();
  try (final ResultSet resultSet = metadata.getTables(connectionProvider.databaseName, null, tableName, null)) {
    return resultSet.next();
  }
}

But It is not working on HSQLDB. On HSQLDB catalog means not the database or namespace. DatabaseMetaData seems to have a slightly different meaning and scope for each database. So What about changing the tableExists method into a protected abstract method on JDBCStorageDelegate? 

protected abstract boolean tableExists(final String tableName) throws Exception;
VaughnVernon commented 2 years ago

@chytonpide Wouldn't the database name be required for all tables used by XOOM, or only for Dispatchables because it is the only table with a common name across databases?

chytonpide commented 2 years ago

@VaughnVernon You are right. I'm sorry. Yes, the database name is required for all tables used by XOOM with the storage type as the state store.

chytonpide commented 2 years ago

I would like to talk in more detail about the alternative that moves the private method tableExists on JDBCStorageDelegate to its child classes.

JDBCStorageDelegate

The responsibility to implement tableExists is moved to its child classes.

public abstract class JDBCStorageDelegate<T> implements StorageDelegate,
    DispatcherControl.DispatcherControlDelegate<Entry<?>, State<?>> {
...
protected abstract boolean tableExists(final String tableName) throws Exception
...

}

PostgresStorageDelegate

The implementation of tableExists is exactly the same as the current private method tableExists on JDBCStorageDelegate. I think there are no any other side effects because only the location of the implementation is changed.

public class PostgresStorageDelegate extends JDBCStorageDelegate<Object> implements StorageDelegate, PostgresQueries {

  ...
  @Override
  protected boolean tableExists(final String tableName) throws Exception {
  final DatabaseMetaData metadata = connection.getMetaData();
  try (final ResultSet resultSet = metadata.getTables(null, null, tableName, null)) {
    return resultSet.next();
  }
  ...
}

YugaByteStorageDelegate

Same as PostgresStorageDelegate case.

public class YugaByteStorageDelegate extends PostgresStorageDelegate {
  ...
  ...
}

HSQLDBStorageDelegate

Same as PostgresStorageDelegate case.

public class HSQLDBStorageDelegate extends JDBCStorageDelegate<Blob> implements StorageDelegate, HSQLDBQueries {
  ...
  @Override
  protected boolean tableExists(final String tableName) throws Exception {
  final DatabaseMetaData metadata = connection.getMetaData();
  try (final ResultSet resultSet = metadata.getTables(null, null, tableName, null)) {
    return resultSet.next();
  }
  ...
}

Mysql

The implementation of tableExists is changed. connectionProvider.databasename is passed as argument to the DatabaseMetaData.getTables. DatabaseMetaData.getTables's scope is limited to the currently connected database.

public class MySQLStorageDelegate extends JDBCStorageDelegate<Object> implements StateStore.StorageDelegate, MySQLQueries{
  ...
  @Override
  protected boolean tableExists(final String tableName) throws Exception {
  final DatabaseMetaData metadata = connection.getMetaData();
  try (final ResultSet resultSet = metadata.getTables(connectionProvider.databaseName, null, tableName, null)) {
    return resultSet.next();
  }
  ...
}
VaughnVernon commented 2 years ago

@chytonpide Good idea, thanks.

Would using the database (catalog) scope work for all database types?

metadata.getTables(connectionProvider.databaseName, null, tableName, null)

If so, that is the logical change for all database types.

We could still make tableExists() a protected method in case a specific database type implementation must override it.

chytonpide commented 2 years ago

@VaughnVernon No it didn't work on Hsqldb. But, others work well with the catalog parameter. This is the test result and the test code.

We could still make tableExists() a protected method in case a specific database type implementation must override it.

I agree with this. It seems more common for the catalog to mean DB. I will test the other relational DB as well.

chytonpide commented 2 years ago

@VaughnVernon I have tested MariaDB, MsSqlServer, and OracleDB additionally. These JDBC driver's getTable work with the catalog parameter. (passing database name)

This JDBC dirver's getTable didn't work with the catalog parameter. (passing database name)

VaughnVernon commented 2 years ago

@chytonpide I have increased tableExists() visibility to protected and also added protected String catalogName() to JDBCStorageDelegate. Now tableExists() calls catalogName() so that in most cases only catalogName() would have to be overridden. Still, either or both methods can be overridden.

Also the HSQLDBStorageDelegate now overrides only catalogName() to return null rather than databaseName.

Note: MariaDB is supported by using MySQL. The two database types are 100% compatible.

Do you have XOOM Symbio JDBC implementations for MS SQL Server and Oracle?

chytonpide commented 2 years ago

Thanks for your help!

Do you have XOOM Symbio JDBC implementations for MS SQL Server and Oracle?

No, I don't have XOOM Symbio JDBC implementations for MS SQL Server and Oracle. I just tested DatabaseMetaData of JDBC drivers. This is my test code for Oracle and MS SQL Server.
I am afraid I can't contribute to them as this is my first time using Oracle and MS SQL Server. (Actually, even the installation of Oracle was quite difficult for me.)

VaughnVernon commented 2 years ago

Thanks for confirming, @chytonpide.

I understand now how you testes. I appreciate the trouble you went through to test SQL Server and Oracle.

BTW, in case I was not clear enough about MariaDB, you would use use xoom-symbio-jdbc for MySQL to support MariaDB. They are the same from a XOOM perspective.