97397c4 · reshape · git.iko.soy

git.iko.soy/reshape

on main help sign in

Handle NOT NULL for column removal with complex down

97397c4fabianlindfors3 years ago

src/migrations/remove_column.rs+93−1

compare with main

unifiedsplit

@@ -3,7 +3,7 @@ use crate::{

3 db::{Conn, Transaction},

4 schema::Schema,

5};

6−use anyhow::{bail, Context};

6+use anyhow::{anyhow, bail, Context};

7use serde::{Deserialize, Serialize};

9#[derive(Serialize, Deserialize, Debug)]

@@ -33,6 +33,15 @@ impl RemoveColumn {

33 self.column

34 )

35 }

36+

37+ fn null_constraint_trigger_name(&self, ctx: &MigrationContext) -> String {

38+ format!(

39+ "{}_remove_column_{}_{}_nn",

40+ ctx.prefix(),

41+ self.table,

42+ self.column

43+ )

44+ }

36}

45}

38#[typetag::serde(name = "remove_column")]

47#[typetag::serde(name = "remove_column")]

@@ -51,6 +60,9 @@ impl Action for RemoveColumn {

51 schema: &Schema,

60 schema: &Schema,

52 ) -> anyhow::Result<()> {

61 ) -> anyhow::Result<()> {

53 let table = schema.get_table(db, &self.table)?;

62 let table = schema.get_table(db, &self.table)?;

63+ let column = table

64+ .get_column(&self.column)

65+ .ok_or_else(|| anyhow!("no such column {} exists", self.column))?;

55 // Add down trigger

67 // Add down trigger

56 if let Some(down) = &self.down {

68 if let Some(down) = &self.down {

@@ -122,6 +134,62 @@ impl Action for RemoveColumn {

122 })

134 })

123 .collect();

135 .collect();

124

136

137+ let maybe_null_check = if !column.nullable {

138+ // Replace NOT NULL constraint with a constraint trigger that only triggers on the old schema.

139+ // As we are using a complex down function, we must remove the NOT NULL check for the new schema.

140+ // NOT NULL is not checked at the end of a transaction, but immediately upon update.

141+ let query = format!(

142+ r#"

143+ CREATE OR REPLACE FUNCTION {trigger_name}()

144+ RETURNS TRIGGER AS $$

145+ BEGIN

146+ IF NOT reshape.is_new_schema() THEN

147+ IF NEW.{column} IS NULL THEN

148+ RAISE EXCEPTION '{column} can not be null';

149+ END IF;

150+ END IF;

151+ RETURN NEW;

152+ END

153+ $$ language 'plpgsql';

154+

155+ DROP TRIGGER IF EXISTS "{trigger_name}" ON "{table}";

156+

157+ CREATE CONSTRAINT TRIGGER "{trigger_name}"

158+ AFTER INSERT OR UPDATE

159+ ON "{table}"

160+ FOR EACH ROW

161+ EXECUTE PROCEDURE {trigger_name}();

162+ "#,

163+ table = self.table,

164+ trigger_name = self.null_constraint_trigger_name(ctx),

165+ column = self.column,

166+ );

167+ db.run(&query)

168+ .context("failed to create null constraint trigger")?;

169+

170+ db.run(&format!(

171+ r#"

172+ ALTER TABLE {table}

173+ ALTER COLUMN {column}

174+ DROP NOT NULL

175+ "#,

176+ table = self.table,

177+ column = self.column

178+ ))

179+ .context("failed to remove column not null constraint")?;

180+

181+ format!(

182+ r#"

183+ IF {value} IS NULL THEN

184+ RAISE EXCEPTION '{column_name} can not be null';

185+ END IF;

186+ "#,

187+ column_name = self.column,

188+ )

189+ } else {

190+ "".to_string()

191+ };

192+

125 // TODO: If column is NOT NULL, remove the constraint and perform a NULL check in some other way.

193 // TODO: If column is NOT NULL, remove the constraint and perform a NULL check in some other way.

126 // Otherwise, it's not possible to update the NOT NULL column from another table even in the same transaction.

194 // Otherwise, it's not possible to update the NOT NULL column from another table even in the same transaction.

127 // Either make the NULL check in here or maybe use a constraint trigger: https://www.postgresql.org/docs/9.0/sql-createconstraint.html.

195 // Either make the NULL check in here or maybe use a constraint trigger: https://www.postgresql.org/docs/9.0/sql-createconstraint.html.

@@ -135,6 +203,7 @@ impl Action for RemoveColumn {

135 DECLARE

203 DECLARE

136 {declarations}

204 {declarations}

137 BEGIN

205 BEGIN

206+ {maybe_null_check}

138 UPDATE "migration_{existing_schema_name}"."{changed_table}" "{changed_table}"

207 UPDATE "migration_{existing_schema_name}"."{changed_table}" "{changed_table}"

139 SET "{column_name}" = {value}

208 SET "{column_name}" = {value}

140 WHERE {where};

209 WHERE {where};

@@ -157,6 +226,8 @@ impl Action for RemoveColumn {

157 }

226 }

158 }

227 }

159

228

229+ if !column.nullable {}

230+

160 Ok(())

231 Ok(())

161 }

232 }

162

233

@@ -194,10 +265,14 @@ impl Action for RemoveColumn {

194

265

195 DROP TRIGGER IF EXISTS "{trigger_name}" ON "{trigger_table}";

266 DROP TRIGGER IF EXISTS "{trigger_name}" ON "{trigger_table}";

196 DROP FUNCTION IF EXISTS "{trigger_name}";

267 DROP FUNCTION IF EXISTS "{trigger_name}";

268+

269+ DROP TRIGGER IF EXISTS "{null_trigger_name}" ON "{table}";

270+ DROP FUNCTION IF EXISTS "{null_trigger_name}";

197 "#,

271 "#,

198 table = self.table,

272 table = self.table,

199 column = self.column,

273 column = self.column,

200 trigger_name = self.trigger_name(ctx),

274 trigger_name = self.trigger_name(ctx),

275+ null_trigger_name = self.null_constraint_trigger_name(ctx),

201 );

276 );

202 db.run(&query)

277 db.run(&query)

203 .context("failed to drop column and down trigger")?;

278 .context("failed to drop column and down trigger")?;

@@ -214,6 +289,18 @@ impl Action for RemoveColumn {

214 }

289 }

215

290

216 fn abort(&self, ctx: &MigrationContext, db: &mut dyn Conn) -> anyhow::Result<()> {

291 fn abort(&self, ctx: &MigrationContext, db: &mut dyn Conn) -> anyhow::Result<()> {

292+ // We might have removed the NOT NULL check so we reinstate it

293+ db.run(&format!(

294+ r#"

295+ ALTER TABLE {table}

296+ ALTER COLUMN {column}

297+ SET NOT NULL

298+ "#,

299+ table = self.table,

300+ column = self.column

301+ ))

302+ .context("failed to reinstate column not null constraint")?;

303+

217 // Remove function and trigger

304 // Remove function and trigger

218 let trigger_table = match &self.down {

305 let trigger_table = match &self.down {

219 Some(Transformation::Update {

306 Some(Transformation::Update {

@@ -227,8 +314,13 @@ impl Action for RemoveColumn {

227 r#"

314 r#"

228 DROP TRIGGER IF EXISTS "{trigger_name}" ON "{trigger_table}";

315 DROP TRIGGER IF EXISTS "{trigger_name}" ON "{trigger_table}";

229 DROP FUNCTION IF EXISTS "{trigger_name}";

316 DROP FUNCTION IF EXISTS "{trigger_name}";

317+

318+ DROP TRIGGER IF EXISTS "{null_trigger_name}" ON "{table}";

319+ DROP FUNCTION IF EXISTS "{null_trigger_name}";

230 "#,

320 "#,

321+ table = self.table,

231 trigger_name = self.trigger_name(ctx),

322 trigger_name = self.trigger_name(ctx),

323+ null_trigger_name = self.null_constraint_trigger_name(ctx),

232 ))

324 ))

233 .context("failed to drop down trigger")?;

325 .context("failed to drop down trigger")?;

234

326

tests/complex.rs+10

@@ -30,10 +30,12 @@ fn extract_relation_into_new_table() {

30 [[actions.columns]]

31 name = "account_id"

32 type = "INTEGER"

33+ nullable = false

34 [[actions.columns]]

35 [[actions.columns]]

35 name = "account_role"

36 name = "account_role"

36 type = "TEXT"

37 type = "TEXT"

38+ nullable = false

37 "#,

39 "#,

38 );

40 );

@@ -57,6 +59,7 @@ fn extract_relation_into_new_table() {

57 [[actions.columns]]

59 [[actions.columns]]

58 name = "role"

60 name = "role"

59 type = "TEXT"

61 type = "TEXT"

62+ nullable = false

61 [actions.up]

64 [actions.up]

62 table = "users"

65 table = "users"

@@ -134,6 +137,13 @@ fn extract_relation_into_new_table() {

134 == 1

137 == 1

135 );

138 );

136

139

140+ // Ensure NOT NULL constraint still applies to old schema

141+ let result = old_db

142+ .simple_query(

143+ "INSERT INTO users (id, account_id, account_role) VALUES (2, NULL, 'developer')",

144+ );

145+ assert!(result.is_err());

146+

137 // Ensure updated user role in old schema updates connection in new schema

147 // Ensure updated user role in old schema updates connection in new schema

138 old_db

148 old_db

139 .simple_query("UPDATE users SET account_role = 'admin' WHERE id = 2")

149 .simple_query("UPDATE users SET account_role = 'admin' WHERE id = 2")