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

add code for check_not_valid_constraints.sql #374

Closed
wants to merge 0 commits into from

Conversation

BLoHny
Copy link
Collaborator

@BLoHny BLoHny commented Apr 27, 2024

Relates to #362

@BLoHny
Copy link
Collaborator Author

BLoHny commented Apr 27, 2024

The current test code doesn't seem to be good code.
Could you help me?

Copy link
Owner

@mfvanek mfvanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are checkstyle issues (see tests results).
Please ensure that project builds locally: ./gradlew build
See https://github.com/mfvanek/pg-index-health/blob/master/CONTRIBUTING.md

@mfvanek
Copy link
Owner

mfvanek commented Apr 27, 2024

@BLoHny
Copy link
Collaborator Author

BLoHny commented Apr 29, 2024

The current test code doesn't seem to be good code. Could you help me?

  1. You can copy and modify existing class https://github.com/mfvanek/pg-index-health/blob/master/pg-index-health/src/test/java/io/github/mfvanek/pg/support/statements/AddLinksBetweenAccountsAndClientsStatement.java in order to bring not valid FK.
  2. You can add one more column to https://github.com/mfvanek/pg-index-health/blob/master/pg-index-health/src/test/java/io/github/mfvanek/pg/support/statements/CreateTableWithCheckConstraintOnSerialPrimaryKey.java and not valid check constraint on it
// AddInvalidForeignKeyStatement.java
    @Override
    public void execute(@Nonnull final Statement statement) throws SQLException {
        statement.execute(String.format("alter table if exists %1$s.accounts " +
                        "add constraint c_accounts_fk_invalid foreign key (invalid_client_id) references %1$s.invalid_clients (invalid_id);",
                schemaName));
    }
// CreateTableWithCheckConstraintOnSerialPrimaryKey.java
    @Override
    public void execute(@Nonnull final Statement statement) throws SQLException {
        statement.execute(String.format("create table if not exists %1$s.another_table(" +
                        "id bigserial primary key, " +
                        "invalid_column integer, " +
                        "constraint not_reserved_id check (id > 1000), " +
                        "constraint less_than_million check (id < 1000000), " +
                        "constraint always_false check (invalid_column > 100 AND invalid_column < 0));",
                schemaName));
    }

Is this modified code suitable?

@mfvanek
Copy link
Owner

mfvanek commented Apr 29, 2024

Is this modified code suitable?

Unfortunately, no, it isn't.

When I say "not valid constraint" it's not about true or false constraint is, it's about whether database uses this constraint or not. Not valid constraint is like a draft. Please take a look at the original article https://habr.com/ru/articles/800121/

It should be like this

alter table if exists %1$s.accounts add constraint c_accounts_fk_client_id_not_validated_yet foreign key (client_id) references %1$s.clients (id) not valid;

In tests after you find all not valid constraints you can can call validate constraint and recheck the database: all issues should go away

@BLoHny
Copy link
Collaborator Author

BLoHny commented Apr 30, 2024

    @ParameterizedTest
    @ValueSource(strings = {PgContext.DEFAULT_SCHEMA_NAME, "custom"})
    void onDatabaseWithNotValidatedForeignKey(final String schemaName) {
        executeTestOnDatabase(schemaName, DatabasePopulator::withInvalidForeignKey, ctx ->
                assertThat(check)
                        .executing(ctx)
                        .hasSize(1)
                        .containsExactly(
                                Constraint.of(ctx.enrichWithSchema("accounts"), "c_accounts_fk_client_id_not_validated_yet",
                                        ConstraintType.FOREIGN_KEY)
                        )
        );
    }

Is it okay to proceed with the test code like this?

@mfvanek
Copy link
Owner

mfvanek commented Apr 30, 2024

Hi @BLoHny

Is it okay to proceed with the test code like this?

I'd suggest a few enhancements:

  1. Rename withInvalidForeignKey to withNotValidConstraints
  2. Add not valid FK and not valid check constraint
  3. Add dbp -> dbp.withNotValidConstraints().withUniqueConstraintOnSerialColumn() to ensure that sql query ignores unique constraints and PK's

@BLoHny
Copy link
Collaborator Author

BLoHny commented Apr 30, 2024

        statement.execute(String.format("alter table if exists %1$s.accounts " +
                        "add constraint c_accounts_chk_client_id_not_validated_yet check (client_id > 0) not valid;",
                schemaName));

After add more sql query..

                        .containsExactly(
                                Constraint.of(ctx.enrichWithSchema("accounts"), "c_accounts_chk_client_id_not_validated_yet",
                                        ConstraintType.CHECK),
                                Constraint.of(ctx.enrichWithSchema("accounts"), "c_accounts_fk_client_id_not_validated_yet",
                                        ConstraintType.FOREIGN_KEY)
                        )

add test code

proceed like this?

@mfvanek
Copy link
Owner

mfvanek commented Apr 30, 2024

proceed like this?

Yes, looks good

@BLoHny
Copy link
Collaborator Author

BLoHny commented Apr 30, 2024

open new pull request #382 by conflict

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants