Filter table not applied on diesel migration --diff-schema

[weiznich]

Discussed in #3712

^{Originally posted by forest1102 July 25, 2023}
I am trying out a new option diff-schema.
However, when I use the filter field in diesel.toml just omits tables used in other crates. diesel migration generate edit_column_name --diff-schema results in writing code to delete tables not listed in the filter fields. I expect migration do nothing about unlisted tables.

Inputs

initial database info

CREATE TABLE "table_a"(
	"id" INT4 NOT NULL PRIMARY KEY,
	"code" VARCHAR NOT NULL
);
CREATE TABLE "table_b"(
	"id" INT4 NOT NULL PRIMARY KEY,
);
CREATE TABLE "table_c"(
	"id" INT4 NOT NULL PRIMARY KEY,
);

diesel.toml

[print_schema]



file = "schema_a/src/schema.rs"
filter = { only_tables = [
    "^table_a$",
] }

schema_a/src/schema.rs

diesel::table! {
    table_a (id) {
        id -> Int8,
-       code -> Varchar,
+      script -> Varchar,
    }
}

command

diesel migration generate edit_column_name --diff-schema

Actual Result

up.sql

DROP TABLE IF EXISTS "table_b";
ALTER TABLE "table_a" DROP COLUMN "code";
ALTER TABLE "table_a" ADD COLUMN "script" VARCHAR NOT NULL;
DROP TABLE IF EXISTS "table_c";

Expected

up.sql

ALTER TABLE "table_a" DROP COLUMN "code";
ALTER TABLE "table_a" ADD COLUMN "script" VARCHAR NOT NULL;

Version

diesel 
 Version: 2.1.0
 Supported Backends: postgres
```</div>

[weiznich]

The issue here is that the diff-schema code currently just ignores these filters. That means we need to add support for these filters at the relevant location here:

diesel/diesel_cli/src/migrations/diff_schema.rs

Line 46 in f4d5f93

let tables_from_database = crate::infer_schema_internals::load_table_names(&mut conn, None)?;

See the print-schema code for how to apply the filtering:

diesel/diesel_cli/src/print_schema.rs

Lines 180 to 182 in f4d5f93

	.into_iter()
	.filter(|t| !config.filter.should_ignore_table(t))
	.collect::<Vec<_>>();

A fix of that issue should likely move the definition of the Filtering type to a share location like config.rs. That this code:

diesel/diesel_cli/src/print_schema.rs

Lines 15 to 40 in f4d5f93

	type Regex = RegexWrapper<::regex::Regex>;
	pub enum Filtering {
	OnlyTables(Vec<Regex>),
	ExceptTables(Vec<Regex>),
	None,
	}
	#[allow(clippy::derivable_impls)] // that's not supported on rust 1.65
	impl Default for Filtering {
	fn default() -> Self {
	Filtering::None
	}
	}
	impl Filtering {
	pub fn should_ignore_table(&self, name: &TableName) -> bool {
	use self::Filtering::*;
	match *self {
	OnlyTables(ref regexes) => !regexes.iter().any(|regex| regex.is_match(&name.sql_name)),
	ExceptTables(ref regexes) => regexes.iter().any(|regex| regex.is_match(&name.sql_name)),
	None => false,
	}
	}
	}

In addition to the actual fix we want to add at least a test case (for example the provided one) like that one here:

diesel/diesel_cli/tests/migration_generate.rs

Lines 258 to 261 in f4d5f93

	#[test]
	fn migration_generate_from_diff_drop_table_composite_key() {
	test_generate_migration("diff_drop_table_composite_key", Vec::new());
	}

(Also checkout the corresponding folder in tests/generate_migrations for the full setup)

[gmanninglive]

hey @weiznich I've started working on this, it's going pretty well thanks your guide above!

I have a couple of questions though:

1. Should the cli command migration be updated to accept the filters as in print-schema command? If yes then, I may have some more questions 😄
2. Would you like a draft PR up to track progress or would you rather I wait till I believe it's ready for review?