Find a way to reduce amount of boilerplate code in tests

[mfvanek]

See usage of AbstractDbStatement

1. Get rid of constructors/inheritance/boilerplate https://github.com/mfvanek/pg-index-health/blob/ff4de6fe629524527a8f2b90daef1373adb5fa03/pg-index-health/src/test/java/io/github/mfvanek/pg/support/statements/AddCommentOnProceduresStatement.java#L19
2. Separate sql code to make it easier to copy/reuse ???

[mfvanek]

Java 17 and multiline literals?

[BLoHny]

I have question about this, Does this mean using composition instead of inheritance and separating SQL statements into a separate class?

[mfvanek]

Hi @BLoHny

I have question about this, Does this mean using composition instead of inheritance and separating SQL statements into a separate class?

I don't have any ready solution. It's kind of R&D task.

I don't think that we need to get rid of inheritance at all. I would start with constructor in AbstractDbStatement and move field protected final String schemaName; to mutable global variable accessed via getter method or to method argument in DbStatement void execute(@Nonnull Statement statement, @Nonnull String schemaName) throws SQLException;

[mfvanek]

On the next step we can try to get rid of overriding execute method in each descendant. Instead of

    @Override
    public void execute(@Nonnull final Statement statement) throws SQLException {

we can suggest to implement only protected List<String> getSqlToExecute()

As a result current class

public class CreateTableWithUniqueSerialColumn extends AbstractDbStatement {

    public CreateTableWithUniqueSerialColumn(@Nonnull final String schemaName) {
        super(schemaName);
    }

    @Override
    public void execute(@Nonnull final Statement statement) throws SQLException {
        statement.execute(String.format("create table if not exists %1$s.one_more_table(" +
                "id bigserial, " +
                "constraint unique_id unique (id), " +
                "constraint not_reserved_id check (id > 1000), " +
                "constraint less_than_million check (id < 1000000));",
            schemaName));
    }
}

should transforms into something like

public class CreateTableWithUniqueSerialColumn extends AbstractDbStatement {

    @Override
    protected List<String> getSqlToExecute(@Nonnull final String schemaName) {
       return List.of(String.format("create table if not exists %1$s.one_more_table(" +
                "id bigserial, " +
                "constraint unique_id unique (id), " +
                "constraint not_reserved_id check (id > 1000), " +
                "constraint less_than_million check (id < 1000000));",
            schemaName));
    }
}

[mfvanek]

@BLoHny Are you going to work on it?

[mfvanek]

@BLoHny I've also sent an invitation to you to become a collaborator on the project.

[BLoHny]

Depending on the changing return value that execute method in state class,

Shall we proceed to change the existing execute call part like this?

public static void executeOnDatabase(@Nonnull final DataSource dataSource,
                                     @Nonnull final DbStatement callback,
                                     @Nonnull final String schemaName) {
    try (Connection connection = dataSource.getConnection();
         Statement statement = connection.createStatement()) {
        for (String sql : callback.execute(schemaName)) {
            statement.execute(sql);
        }
    } catch (SQLException e) {
        throw new PgSqlException(e);
    }
}

[mfvanek]

Shall we proceed to change the existing execute call part like this?

No, that's not what I meant. First of all, we need to refine the AbstractDbStatementclass - we need to add a new abstract method

    protected abstract List<String> getSqlToExecute(@Nonnull String schemaName);

    @Override
    public void execute(@Nonnull final Statement statement, @Nonnull final String schemaName) throws SQLException {
        for (String sql : getSqlToExecute(schemaName)) {
            statement.execute(sql);
        }
    }

[BLoHny]

Should we proceed like this?

DbStatement

@FunctionalInterface
public interface DbStatement {

    void execute(@Nonnull Statement statement, @Nonnull String schemaName) throws SQLException;
}

AbstractDbStatement

abstract class AbstractDbStatement implements DbStatement {

    protected void throwExceptionIfTableDoesNotExist(
        @Nonnull final Statement statement, 
        @Nonnull final String tableName, 
        @Nonnull final String schemaName
    ) throws SQLException {
        final String checkQuery = String.format("select exists (%n" +
            "   select 1 %n" +
            "   from pg_catalog.pg_class c%n" +
            "   join pg_catalog.pg_namespace n on n.oid = c.relnamespace%n" +
            "   where n.nspname = '%s'%n" +
            "   and c.relname = '%s'%n" +
            "   and c.relkind = 'r'%n" +
            "   );", schemaName, tableName);
        try (ResultSet rs = statement.executeQuery(checkQuery)) {
            if (rs.next()) {
                final boolean schemaExists = rs.getBoolean(1);
                if (schemaExists) {
                    return;
                }
            }
            throw new IllegalStateException(String.format("Table with name '%s' in schema '%s' wasn't created", tableName, schemaName));
        }
    }

    protected abstract List<String> getSqlToExecute(@Nonnull String schemaName);
}

classes that extends AbstractDbStatement

public class CreateTableWithUniqueSerialColumn extends AbstractDbStatement {

    @Override
    protected List<String> getSqlToExecute(@Nonnull String schemaName) {
        return List.of(String.format("create table if not exists %1$s.one_more_table(" +
                "id bigserial, " +
                "constraint unique_id unique (id), " +
                "constraint not_reserved_id check (id > 1000), " +
                "constraint less_than_million check (id < 1000000));",
            schemaName));
    }

    @Override
    public void execute(@Nonnull final Statement statement, @Nonnull final String schemaName) throws SQLException {
        for (String sql : getSqlToExecute(schemaName)) {
            statement.execute(sql);
        }
    }
}

[mfvanek]

 public void execute(@Nonnull final Statement statement, @Nonnull final String schemaName) throws SQLException {

It should be in AbstractDbStatement.

The rest looks OK to me.

P.S. Use pattern Template method for statements like CreateSchemaStatement or CreateTableWithColumnOfBigSerialTypeStatement- just move checks to another postExecute() method

[BLoHny]

Can i try it?

[mfvanek]

Can i try it?

@BLoHny Yes, sure

[BLoHny]

CreateTableWithColumnOfBigSerialTypeStatement - just move checks to another postExecute() method this mean creating a new class or adding a method to AbstractDbStatement?

[mfvanek]

CreateTableWithColumnOfBigSerialTypeStatement - just move checks to another postExecute() method this mean creating a new class or adding a method to AbstractDbStatement?

adding a method to AbstractDbStatement

[BLoHny]

protected void postExecute(@Nonnull final Statement statement, @Nonnull final String schemaName) throws SQLException { } After adding the following method, it was called by inheriting from classes that called Sql directly from the existing statement class.

However, a pmd checkStyle error occurred, so I thought of creating a new abstract class or log with parameter value. Would you have any thoughts?

[mfvanek]

protected void postExecute(@Nonnull final Statement statement, @Nonnull final String schemaName) throws SQLException { } After adding the following method, it was called by inheriting from classes that called Sql directly from the existing statement class.

However, a pmd checkStyle error occurred, so I thought of creating a new abstract class or log with parameter value. Would you have any thoughts?

Just show me the code) Create a branch, commit, push, create a draft PR.