Adding Unique constraint can cause vapor to crash

[mredig]

I've only noticed this behavior when using a SQLite db. This code seems to work fine on PostgreSQL.

This is my initial migration:

struct UserMigration_v1_0_0: Migration {
	func prepare(on database: Database) -> EventLoopFuture<Void> {
		database.eventLoop.flatten([
			database.schema(UserModel.schema)
				.id()
				.field(UserModel.FieldKeys.email, .string, .required)
				.field(UserModel.FieldKeys.password, .string, .required)
				.unique(on: UserModel.FieldKeys.email)
				.create(),
		])
	}

	func revert(on database: Database) -> EventLoopFuture<Void> {
		database.eventLoop.flatten([
			database.schema(UserModel.schema).delete()
		])
	}
}

(migration 1_1_0 doesn't touch this schema at all)

I then run this migration:

struct UserMigration_v1_2_0: Migration {
	func prepare(on database: Database) -> EventLoopFuture<Void> {
		database.schema(UserModel.schema)
			.field(UserModel.FieldKeys.appleId, .string)
			.unique(on: UserModel.FieldKeys.appleId)
			.update()
	}

	func revert(on database: Database) -> EventLoopFuture<Void> {
		database.schema(UserModel.schema)
			.deleteField(UserModel.FieldKeys.appleId)
			.update()
	}
}

and the app will crash with the following error:

2020-06-14 20:14:10.556995-0500 xctest[44335:23885144] [logging] near ",": syntax error in "ALTER TABLE "user_users" ADD "appleId" TEXT , ADD CONSTRAINT "uq:user_users.appleId" UNIQUE ("appleId")"
Fatal error: Application.shutdown() was not called before Application deinitialized.: file .../vapor/Sources/Vapor/Application.swift, line 167

Now here's where it gets weird (for me, at least). This crashes on both Mac and Linux, but if I make the following change to the followup migration, it doesn't crash on Mac, but still does on Linux. On Mac, I DO get a console warning (which, when examining the DB file, appears to result in the unique constraint simply just ignored in the end).

struct UserMigration_v1_2_0: Migration {
	func prepare(on database: Database) -> EventLoopFuture<Void> {
		database.eventLoop.flatten([
			database.schema(UserModel.schema)
				.field(UserModel.FieldKeys.appleId, .string)
				.update(),
			database.schema(UserModel.schema)
				.unique(on: UserModel.FieldKeys.appleId)
				.update(),
		])
	}

	func revert(on database: Database) -> EventLoopFuture<Void> {
		database.schema(UserModel.schema)
			.deleteField(UserModel.FieldKeys.appleId)
			.update()
	}
}

Here's the warning I get on macOS:

2020-06-14T20:18:02-0500 warning: database-id=sqlite Ignoring schema update. SQLite only supports adding columns to existing tables

I'm effectively asking for the same end result, but breaking it into two steps allows for the macOS build to simply ignore the issue and continue on.

Another thing I simply don't understand... How can this possible cause the app to deinitialize itself? There are other maintained references to the app that would prevent the reference count from reaching 0, so I feel like I'm going crazy!

[mredig]

I should add that, if you remove the .unique constraint on the second migration, there appears to be no issue whatsoever on macOS, but I think Linux still crashes on me.

[tanner0101]

Hi @mredig. Regarding this error:

Fatal error: Application.shutdown() was not called before Application deinitialized

How are you initializing your application? The typical pattern is to do:

let app = Application(...)
defer { app.shutdown() }

This ensures that app will have shutdown called before it deinitializes.

As for the SQLite errors, those are expected. SQLite's ALTER TABLE syntax is really limited: https://www.sqlite.org/lang_altertable.html

You can check for SQLite in your migrations by doing:

if database is SQLiteDatabase {
    // can only add fields in update
}

I've put up a pr (#66) to improve the messaging around this. Thanks!

[mredig]

That is almost the exact code I used to initialize the app. I followed the pattern set forth by Tibor in his Practical Vapor book:

	func createTestApp() throws -> Application {
		let app = Application(.testing)
		try configure(app)
		app.databases.use(.sqlite(.memory), as: .sqlite)
		app.databases.default(to: .sqlite)

		app.fileStorages.use(.local(publicUrl: Environment.appURL.absoluteString,
									publicPath: app.directory.publicDirectory,
									workDirectory: "assets"),
							 as: .local)
		app.fileStorages.default(to: .local)

		try app.autoMigrate().wait()
		return app
	}

...

	func testHomePage() throws {
		let app = try createTestApp()
		defer { app.shutdown() }

		try app.testable(method: .inMemory).test(.GET, "") { res in
			XCTAssertEqual(res.status, .ok)
			let contentType = try XCTUnwrap(res.headers.contentType)
			XCTAssertEqual(contentType, .html)
			XCTAssertTrue(res.body.string.contains("myPage - Home"))
		}
	}

[mredig]

If SQLite can't handle that, what is the suggested method for unit tests?

[tanner0101]

@mredig the defer must go immediately after app is initialized.

// this will not shutdown app if createTestApp throws
let app = try createTestApp()
defer { app.shutdown() }

This code allows for an error to be thrown before the defer block is hit. If createTestApp throws an error (which it will if autoMigrate fails) then shutdown will not be called.

[tanner0101]

If SQLite can't handle that, what is the suggested method for unit tests?

I'd recommend testing your migrations against the same type of database you will deploy to. Vapor's template ships with a docker-compose file for easily spinning this up locally: https://github.com/vapor/template/blob/master/docker-compose.yml#L64

[mredig]

This code allows for an error to be thrown before the defer block is hit. If createTestApp throws an error (which it will if autoMigrate fails) then shutdown will not be called.

That makes a LOT of sense! I'm honestly kind of astounded I didn't realize that myself... lolol