Prepare schema migration/testing code for upcoming database changes #1248

PIG208 · 2025-01-03T03:56:12Z

This prepares for #97 (PR #1167).

chrisbobbe

Thanks! Small comments below, and I'll defer to Greg for a more detailed review of the substantive changes, as I'm not yet up-to-speed on this system. 🙂

chrisbobbe · 2025-01-30T23:24:11Z

lib/model/store.dart

+    //   on iOS, -> "Library/Application Support" via https://developer.apple.com/documentation/foundation/nssearchpathdirectory/nsapplicationsupportdirectory
+    //   on Linux, -> "${XDG_DATA_HOME:-~/.local/share}/com.zulip.flutter/"
+    //
+    // This is reasonable for iOS per Apple's recommendation:


How about "This is the best fit for iOS per Apple's recommendation", if that's true? That would feel more satisfying than just saying it's a reasonable option, because then you wonder if there are other reasonable options and what those look like.

Same for Android and Linux below, and I don't yet follow your reasoning about Android. (Linux is clear from the removed line "That Linux answer is definitely not a fit.")

Will put the relevant pieces back here. I dug into the path for Android more, and found that there is getDatabasePath, which is not provided by path_provider. It would be nice to have support for that, though.

Opened #1321 and refer to the issue instead. We should be good with using getApplicationSupportDirectory for this PR.

Will be skipping this commit after a CZO discussion.

chrisbobbe · 2025-01-30T23:34:58Z

lib/model/database.dart

  //    $ tools/check --fix drift
+  //    and generate database code with build_runner.


Is there a specific command we can put here, or perhaps refer the reader to some documentation, even if it's just dart run build_runner --help? (The dart run seems helpful to a contributor arriving here for the first time.)

I didn't include the command for this as there are different ways to generate them, and some are more efficient the the others. We should point to the relevant piece of documentation ("Generated files" in README.md), or move this section to the README.

chrisbobbe · 2025-01-30T23:50:00Z

pubspec.yaml

-  drift: ^2.5.0
+  drift: ^2.23.0


Need help checking if podfiles need an update; deps: Upgrade drift to 2.23.0

Confirmed by running tools/upgrade pod at this commit, on a Mac, that no changes to ios/Podfile.lock or macos/Podfile.lock are needed.

Thanks for confirming!

PIG208 · 2025-02-04T01:25:29Z

Thanks for the review! This has been updated.

PIG208 · 2025-02-05T20:55:34Z

Rebased on top of #1324 for CI, and adjust the commit message so that this is not really about upgrading drift, but rather utilizing new drift features.

chrisbobbe · 2025-02-12T23:48:20Z

Thanks for the revision! Marking for Greg's review:

I'll defer to Greg for a more detailed review of the substantive changes, as I'm not yet up-to-speed on this system. 🙂

gnprice

Thanks @PIG208 for working on this, and @chrisbobbe for the previous review!

Comments below.

I think the PR title also rather undersells what this is about. "Using new features" isn't particularly appealing in itself — lots of our dependencies are adding new features all the time, and most of them aren't anything we want.

The real point, I think, is that this refactors our handling of schema migrations, to better set us up for doing more migrations in the future. And then in order to do that, it relies on some features that Drift didn't yet have in 2023 when this migrations code was first written.

gnprice · 2025-02-19T04:50:57Z

test/model/database_test.dart

-      ));
-      final accountV1 = await before.select(before.accounts).watchSingle().first;
-      await before.close();
+    }, skip: true); // TODO(#1172): unskip this


I think we don't need a skipped test for this in the tree; #1172 suffices for tracking the fact we'd like to have this work, and the test case is preserved nicely there.

gnprice · 2025-02-19T04:52:36Z

test/model/database_test.dart

@@ -137,6 +137,7 @@ void main() {
        ...accountV1.toJson(),
        'ackedPushToken': null,
      });
+      await after.close();


Can you explain a bit more why this change is needed?

db test: Add missing `after.close` call Signed-off-by: Zixuan James Li <[email protected]>

What goes wrong if this line is left out?

It addresses a latent warning that happens when we have multiple tests that use the same AppDatabase class:

WARNING (drift): It looks like you've created the database class DatabaseAtV2 multiple times. When these two databases use the same QueryExecutor, race conditions will occur and might corrupt the database. Try to follow the advice at https://drift.simonbinder.eu/faq/#using-the-database or, if you know what you're doing, set driftRuntimeOptions.dontWarnAboutMultipleDatabases = true Here is the stacktrace from when the database was opened a second time: #0 GeneratedDatabase._handleInstantiated (package:drift/src/runtime/api/db_base.dart:96:30) #1 GeneratedDatabase._whenConstructed (package:drift/src/runtime/api/db_base.dart:73:12)

which comes from this method:

bool _handleInstantiated() { if (!_openedDbCount.containsKey(runtimeType) || driftRuntimeOptions.dontWarnAboutMultipleDatabases) { _openedDbCount[runtimeType] = 1; return true; } final count = _openedDbCount[runtimeType] = _openedDbCount[runtimeType]! + 1; if (count > 1) { driftRuntimeOptions.debugPrint( 'WARNING (drift): It looks like you\'ve created the database class ' '$runtimeType multiple times. When these two databases use the same ' 'QueryExecutor, race conditions will occur and might corrupt the ' 'database. \n' 'Try to follow the advice at https://drift.simonbinder.eu/faq/#using-the-database ' 'or, if you know what you\'re doing, set ' 'driftRuntimeOptions.dontWarnAboutMultipleDatabases = true\n' 'Here is the stacktrace from when the database was opened a second ' 'time:\n${StackTrace.current}\n' 'This warning will only appear on debug builds.', ); } return true; }

_openedDbCount is a global variable.

It is not an issue at this commit because 1 out of the 2 tests calls AppDatabase.close. While tests don't run concurrently, it is still better to avoid leaking states like this.

With awaitFakeAsync and testWidgets we have some checks/assertions to detect this as soon as the test completes, but I'm not sure if the need to check for this sort of thing justifies having a dedicated test helepr for migrations, unless there are other functionalities to be offered.

Great, thanks. Yeah, just mention in the commit message that this otherwise leaks state, and would produce a warning if we had another test after this that also created a database instance.

gnprice · 2025-02-19T04:57:54Z

test/model/database_test.dart

+        group('from $fromVersion', () {
+          for (final toVersion in versions.skip(i + 1)) {
+            test('to $toVersion', () async {
+              final schema = await verifier.schemaAt(fromVersion);


What's the difference between the schemaAt here and the startAt in the existing test?

From a quick look at the implementation, it seems like we end up doing the same other step that startAt would do. So we can simplify a bit by going back to using that.

gnprice · 2025-02-19T05:04:15Z

test/model/database_test.dart

+      for (final (i, fromVersion) in versions.indexed) {
+        group('from $fromVersion', () {
+          for (final toVersion in versions.skip(i + 1)) {
+            test('to $toVersion', () async {


This will end up making quadratically many tests as we accumulate migrations. Doesn't matter when there's only a handful, but it'll start get annoying when we have a few dozen migrations — here's a quick demo:

+ for (final _ in List.generate(1000, (_) {})) for (final (i, fromVersion) in versions.indexed) {

At that point this test file takes several extra seconds to run on my machine. Those 1000 iterations correspond to about 45 migrations.

Let's keep this linear, until we have concrete reasons to do more. I think the following should be pretty effective:

Test each single-step migration.

Test the migration from each past schema up to the latest schema.

gnprice · 2025-02-19T05:10:57Z

test/model/database_test.dart

+      test('upgrade to v2', () async {
+        late final v1.AccountsData oldAccountData;
+        await verifier.testWithDataIntegrity(


Is the change from the old "upgrade to v2, with data" test to this test an NFC change? Reading the implementation of testWithDataIntegrity, I believe it is, but I might be missing something.

Let's make that explicit in the commit message — either that it is NFC, or describing whatever subtle differences from that might exist.

Ended up dropping the change after #1248 (comment).

I too agree that the helper is verbose and this part is mostly NFC.

gnprice · 2025-02-19T05:50:08Z

lib/model/database.dart

        if (from > to) {
          // TODO(log): log schema downgrade as an error
-          // This should only ever happen in dev.  As a dev convenience,
-          // drop everything from the database and start over.


This comment feels like it's more to the point at this location, just before calling _resetDatabase — it's explaining why we would want to do such a thing as resetting the database.

gnprice · 2025-02-19T05:55:35Z

lib/model/database.dart

-          await m.createAll();
+          await _resetDatabase(m);
          return;


I read "reset database" as sounding like it's going to reset the database, so to a clean empty state — I wouldn't expect it to then create the current schema's tables.

One good name would be _dropAndCreateAll.

gnprice · 2025-02-19T05:59:09Z

lib/model/database.dart

+        // SQL injection could be a concern, but the database would've been
+        // compromised if the table name is corrupted.
+        await m.database.customStatement('DROP TABLE $tableName');


"could be a concern" sounds like it's saying this is something we think we should worry about here, which isn't the idea 🙂

Suggested change

// SQL injection could be a concern, but the database would've been

// compromised if the table name is corrupted.

await m.database.customStatement('DROP TABLE $tableName');

// No need to worry about SQL injection; this table name

// was already a table name in the database, not something

// that should be affected by user data.

await m.database.customStatement('DROP TABLE $tableName');

gnprice · 2025-02-19T06:02:45Z

lib/model/database.dart

+        // Skip sqlite-internal tables, https://www.sqlite.org/fileformat2.html#intschema
+        // https://github.com/simolus3/drift/blob/0901c984a987e54ba0b67e227adbfdcf3c08eeb4/drift_dev/lib/src/services/schema/verifier_common.dart#L9-L22


nit: I'm not sure how to interpret a comma here. How about:

Suggested change

// Skip sqlite-internal tables, https://www.sqlite.org/fileformat2.html#intschema

// https://github.com/simolus3/drift/blob/0901c984a987e54ba0b67e227adbfdcf3c08eeb4/drift_dev/lib/src/services/schema/verifier_common.dart#L9-L22

// Skip sqlite-internal tables. See for comparison:

// https://www.sqlite.org/fileformat2.html#intschema

// https://github.com/simolus3/drift/blob/0901c984a/drift_dev/lib/src/services/schema/verifier_common.dart#L9-L22

(That bit of Drift code is actually a bit puzzling:

/// Attempts to recognize whether [name] is likely the name of an internal /// sqlite3 table (like `sqlite3_sequence`) that we should not consider when /// comparing schemas. bool isInternalElement(String name, List<String> virtualTables) { // Skip sqlite-internal tables, https://www.sqlite.org/fileformat2.html#intschema if (name.startsWith('sqlite_')) return true;

The example in the doc would not get recognized by the actual implementation. Unless it happens to be in the virtualTables, but I don't think that's what that argument is for.)

Hmm, good catch. I believe that is a typo. The sqlite.org link does mention a table called sqlite_sequence, and nothing relevant comes up in my Google search for "sqlite3_sequence".

gnprice · 2025-02-19T06:15:22Z

lib/model/database.dart

  //  * Write a migration in `onUpgrade` below.
  //  * Write tests.
  @override
  int get schemaVersion => 2; // See note.

+  Future<void> _resetDatabase(Migrator m) async {


Testing for this is blocked until #1172.

Why does something block testing this?

It seems like we should be able to test it like so:

Make a database that has the current schema, plus some arbitrary extra column on accounts and some arbitrary extra table. And has a version greater than the current version.

Attempt a migration.

Check that the schema now is the correct one.

I guess this function doesn't exactly meet the spec of onUpgrade, because it assumes to is the current latest schema version. That's what #1172 is about.

But that doesn't seem like it's an obstacle for testing that it correctly does the thing it's intended to do, which is to correctly migrate given that assumption.

The test ended up being a bit more complicated, but it turned out quite doable. We can fix #1172 at the same time.

PIG208 · 2025-02-20T04:05:34Z

Thanks for the review! This is ready again now.

This was slightly misleading as the implementation suggests that 'sqlite_' is the prefix for these tables. This was brought up here: zulip/zulip-flutter#1248 (comment) Signed-off-by: Zixuan James Li <[email protected]>

gnprice

Thanks for the revision! Generally these changes look good. Comments below.

gnprice · 2025-02-20T06:06:07Z

lib/model/database.dart

-    final file = File(path.join(dbFolder.path, 'db.sqlite'));
-    return NativeDatabase.createInBackground(file);
-  });
+// TODO(upstream): generate this


Suggested change

// TODO(upstream): generate this

// TODO(drift): generate this

"TODO(upstream)" means Flutter 🙂 (or potentially Dart)

Cf. 49053b3.

gnprice · 2025-02-20T06:12:45Z

test/model/database_test.dart

+      for (final fromVersion in versions) {
+        if (fromVersion == versions.last) break;
+
+        final nextVersion = fromVersion + 1;


The versions list seems designed to avoid an assumption that the version numbers are consecutive ints. By iterating through it, we're close to avoiding that assumption here. Might as well be consistent and avoid it all the way — so no arithmetic like "+ 1" on the version numbers.

Instead, the loop can just remember the previous value it saw.

(If it becomes cleanest to make this be two separate loops for the two kinds of tests, that's fine — that might be cleanest anyway.)

gnprice · 2025-02-20T06:18:48Z

test/model/database_test.dart

+      await after.close();
+    });
+
+    group('migrate without data', () {


nit in commit message:

Modify `build.yaml` by specifying the location of the database. This step is needed because `make-migrations` does not accept this through command line arguments. ``` targets: $default: builders: // ... drift_dev: options: databases: default: lib/model/database.dart ```

should use # for comment, not // — the latter isn't YAML comment syntax

Separately in same commit message:

This saves us from writing the simple migration tests between schemas without data in the future. For the test that upgrade the schema with data, with the helper, while it ended up having more lines than the original, the test becomes more structured this way.

The second sentence is explaining why you did do something that this version doesn't do, right? (after #1248 (comment) )

gnprice · 2025-02-20T06:26:08Z

lib/model/schema_versions.g.dart

+import 'package:drift/drift.dart'; // ignore_for_file: type=lint,unused_import
+
+// GENERATED BY drift_dev, DO NOT MODIFY.
+final class Schema2 extends i0.VersionedSchema {


Generating schema_versions.g.dart is crucial because we otherwise only have access to the latest schema when running migrations. Migrations that require dropping columns will be problematic because the latest schema will then have no information about the dropped column.

s/will be/would be/ — otherwise it reads like a warning about something that's still the case after this commit

Or clarifying another aspect too:

That would mean that if we later drop or alter a table or column mentioned in an existing migration, the meaning of the existing migration would change. The affected existing migration would then migrate to an unintended state that doesn't match what later migrations expect, and it or a later migration might fail.

Separately, let's include a link to this comment:
#1248 (comment)

because there's additional discussion there.

gnprice · 2025-02-20T06:30:47Z

tools/check


-    check_no_changes "schema updates" "${schema_dir}"
+    check_no_changes "schema or migration helper updates" "${outputs[@]}"


nit:

Suggested change

check_no_changes "schema or migration helper updates" "${outputs[@]}"

check_no_changes "schema or migration-helper updates" "${outputs[@]}"

Otherwise it's hard to parse — looks most of all like updates to helpers for either "schema" or "migration" ("((schema or migration) helper) updates").

gnprice · 2025-02-20T06:41:37Z

test/model/database_test.dart

+      final schema = await verifier.schemaAt(2);
+
+      // Prepare the database by creating extra tables and columns unknown to
+      // us.  This represent the scenario during developement when checking out


nit:

Suggested change

// us. This represent the scenario during developement when checking out

// us. This represent the scenario during development when checking out

gnprice · 2025-02-20T06:42:53Z

test/model/database_test.dart

+      await check(verifier.migrateAndValidate(
+        before, 2, validateDropped: true)).throws<SchemaMismatch>();
+      // Override the schema version.
+      // TODO(upstream): Expose a better interface for testing this.


gnprice · 2025-02-20T06:45:07Z

test/model/database_test.dart

+      // We need a new connection because migrations are only run when the
+      // database is first opened (see [DelegatedDatabase.ensureOpen]), and
+      // running custom statements opens it.
+      final after = AppDatabase(schema.newConnection());


I think there's an even better (and simpler) reason to do this: this is the part that simulates starting up the app on the older version, the one that doesn't have the new table and column.

gnprice · 2025-02-20T06:46:40Z

test/model/database_test.dart

+      // Prepare the database by creating extra tables and columns unknown to
+      // us.  This represent the scenario during developement when checking out
+      // a branch that does not contain some changes to the schema on another
+      // branch.
+      final before = AppDatabase(schema.newConnection());
+      await before.customStatement('CREATE TABLE test_extra (num int)');
+      await before.customStatement('ALTER TABLE accounts ADD extra_column int');


Relatedly, this stanza is simulating just the first step in that scenario: the step where the app is run at that future version that has additional tables and columns, and therefore leaves them behind in the database.

gnprice · 2025-02-20T06:53:04Z

test/model/database_test.dart

+      // Override the schema version.
+      // TODO(upstream): Expose a better interface for testing this.
+      await before.customStatement('PRAGMA user_version = 999;');


Yeah, I like this a lot better than the other version 🙂 (from before
91fec5b squash? Use a better alternative to accessing the delegates
). So let's do that squash.

In addition to being simpler code, this version is accurately simulating what the database would look like after running a future version of the app with added migrations. In some sense it's arguably Drift internals… but it's something that Drift has written to the database, and therefore that Drift is unavoidably committed to keeping stable; it'd take significant migration work for Drift to change it. (It's very similar to a server API that way, as in https://zulip.readthedocs.io/en/latest/testing/philosophy.html#integration-testing-or-unit-testing .)

By contrast the .delegate.versionDelegate as DynamicVersionDelegate).setSchemaVersion stuff is some API that I guess might be documented Drift API, but even if so, it isn't anything we're otherwise depending on being stable — it might change in some future Drift refactor and we definitely wouldn't need to care, unless we'd written it here into our tests.

Generating schema_versions.g.dart is crucial because we otherwise only have access to the latest schema when running migrations. That would mean that if we later drop or alter a table or column mentioned in an existing migration, the meaning of the existing migration would change. The affected existing migration would then migrate to an unintended state that doesn't match what later migrations expect, and it or a later migration might fail. See also discussion: zulip#1248 (comment) --- An alternative to using all these commands for generating files is `dart run drift_dev make-migrations`, which is essentially a wrapper for the `schema {dump,generate,steps}` subcommands. `make-migrations` let us manage multiple database schemas by configuring them with `build.yaml`, and it dictates the which subdirectories the generated files will be created at. Because `make-migrations` does not offer the same level of customizations to designate exactly where the output files will be, opting out from it for now. We can revisit this if it starts to offer features that are not available with the subcommands, or that we find the need for managing multiple databases. See also: https://drift.simonbinder.eu/migrations/step_by_step/#manual-generation Signed-off-by: Zixuan James Li <[email protected]>

PIG208 · 2025-02-20T07:44:37Z

Thanks for the review! Updated the PR.

Signed-off-by: Zixuan James Li <[email protected]>

As we add more tests for future migrations, Drift will start complaining about opened databases that are not closed. Always remember doing this will ensure that we don't leak states to other database tests. Signed-off-by: Zixuan James Li <[email protected]>

We have already upgraded drift; this reflects that we rely on newer drift versions, but does not actually require changes to the lock files. Signed-off-by: Zixuan James Li <[email protected]>

This saves us from writing more simple migration tests between schema versions without data in the future. --- The tests are inspired by the test template generated from `dart run drift_dev make-migrations`. To reproduce the outputs, go through the following steps: Modify `build.yaml` by specifying the location of the database. This step is needed because `make-migrations` does not accept this through command line arguments. ``` targets: $default: builders: # ... drift_dev: options: databases: default: lib/model/database.dart ``` Then, run the following commands: ``` dart run drift_dev make-migrations cp test/model/schemas/*.json drift_schemas/default/ dart run drift_dev make-migrations ``` The first `make-migrations` run generates the initial schema and test files without looking at the versions we have in test/model/schemas. Copying the schema files and running `make-migrations` will end up creating `test/drift/default/migration_test.dart`, along with other generated files. See also: https://drift.simonbinder.eu/Migrations/#usage Signed-off-by: Zixuan James Li <[email protected]>

Generating schema_versions.g.dart is crucial because we otherwise only have access to the latest schema when running migrations. That would mean that if we later drop or alter a table or column mentioned in an existing migration, the meaning of the existing migration would change. The affected existing migration would then migrate to an unintended state that doesn't match what later migrations expect, and it or a later migration might fail. See also discussion: zulip#1248 (comment) --- An alternative to using all these commands for generating files is `dart run drift_dev make-migrations`, which is essentially a wrapper for the `schema {dump,generate,steps}` subcommands. `make-migrations` let us manage multiple database schemas by configuring them with `build.yaml`, and it dictates the which subdirectories the generated files will be created at. Because `make-migrations` does not offer the same level of customizations to designate exactly where the output files will be, opting out from it for now. We can revisit this if it starts to offer features that are not available with the subcommands, or that we find the need for managing multiple databases. See also: https://drift.simonbinder.eu/migrations/step_by_step/#manual-generation Signed-off-by: Zixuan James Li <[email protected]>

It is a thin wrapper that does what we are already doing in our migration code. While not required, an advantage of this is that it causes compilation errors for missing migration steps.

We previously missed tables that are not known to the schema. This becomes an issue if a new table is added at a newer schema level. When we go back to an earlier schema, that new table remains in the database, so subsequent attempts to upgrade to the later schema level that adds the table will fail, because it already exists. The test for this relies on some undocumented drift internals to modify the schema version it sees when running the migration. References: https://github.com/simolus3/drift/blob/18cede15/drift/lib/src/runtime/executor/helpers/engines.dart#L459-L495 https://github.com/simolus3/drift/blob/18cede15/drift/lib/src/sqlite3/database.dart#L198-L211 https://github.com/simolus3/sqlite3.dart/blob/4de46afd/sqlite3/lib/src/implementation/database.dart#L69-L85 Fixes: zulip#1172 Signed-off-by: Zixuan James Li <[email protected]>

gnprice · 2025-02-20T07:59:10Z

Thanks! Looks good; merging.

This was slightly misleading as the implementation suggests that 'sqlite_' is the prefix for these tables. This was brought up here: zulip/zulip-flutter#1248 (comment) Signed-off-by: Zixuan James Li <[email protected]>

PIG208 added the maintainer review PR ready for review by Zulip maintainers label Jan 3, 2025

PIG208 force-pushed the pr-prep-drift branch from ebac46b to a40f476 Compare January 3, 2025 04:05

PIG208 requested a review from chrisbobbe January 3, 2025 04:05

PIG208 assigned chrisbobbe Jan 3, 2025

PIG208 force-pushed the pr-prep-drift branch from a40f476 to df7e41c Compare January 7, 2025 02:28

PIG208 force-pushed the pr-prep-drift branch from df7e41c to 7f44a2f Compare January 22, 2025 18:29

chrisbobbe reviewed Jan 30, 2025

View reviewed changes

PIG208 force-pushed the pr-prep-drift branch from 7f44a2f to 125afea Compare February 4, 2025 01:25

PIG208 requested a review from chrisbobbe February 4, 2025 01:25

PIG208 added this to the M5-a: Server 10 milestone Feb 5, 2025

PIG208 force-pushed the pr-prep-drift branch from 125afea to fc3fb31 Compare February 5, 2025 20:54

PIG208 changed the title ~~Upgrade Drift to 2.23.0; start using new features~~ Start using new drift features since 2.5.0 Feb 5, 2025

PIG208 force-pushed the pr-prep-drift branch 2 times, most recently from d5810b9 to bddcede Compare February 8, 2025 04:48

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Feb 12, 2025

chrisbobbe assigned gnprice and unassigned chrisbobbe Feb 12, 2025

chrisbobbe requested review from gnprice and removed request for chrisbobbe February 12, 2025 23:48

gnprice mentioned this pull request Feb 19, 2025

downgraded schema does not always comply with the actual schema version #1172

Closed

gnprice reviewed Feb 19, 2025

View reviewed changes

PIG208 changed the title ~~Start using new drift features since 2.5.0~~ Prepare schema migration/testing code for upcoming database changes Feb 19, 2025

PIG208 force-pushed the pr-prep-drift branch 2 times, most recently from 07a922a to 7a00e3c Compare February 20, 2025 01:31

PIG208 mentioned this pull request Feb 20, 2025

Fix typo that refers to an non-existent internal table simolus3/drift#3479

Merged

PIG208 force-pushed the pr-prep-drift branch from 96ab32a to 91fec5b Compare February 20, 2025 04:38

gnprice reviewed Feb 20, 2025

View reviewed changes

PIG208 force-pushed the pr-prep-drift branch from 91fec5b to 591f4c1 Compare February 20, 2025 07:43

PIG208 added 8 commits February 19, 2025 23:58

db [nfc]: Remove dead code

d7fbff2

Signed-off-by: Zixuan James Li <[email protected]>

db [nfc]: Mention build_runner for schema changes

5ad630e

Signed-off-by: Zixuan James Li <[email protected]>

deps [nfc]: Set minimum version of drift to 2.23.0

d302584

We have already upgraded drift; this reflects that we rely on newer drift versions, but does not actually require changes to the lock files. Signed-off-by: Zixuan James Li <[email protected]>

db [nfc]: Use step-by-step migration helper

5fb8158

It is a thin wrapper that does what we are already doing in our migration code. While not required, an advantage of this is that it causes compilation errors for missing migration steps.

gnprice force-pushed the pr-prep-drift branch from 591f4c1 to 601936d Compare February 20, 2025 07:59

gnprice merged commit 601936d into zulip:main Feb 20, 2025

		// $ tools/check --fix drift
		// and generate database code with build_runner.

		// Skip sqlite-internal tables, https://www.sqlite.org/fileformat2.html#intschema
		// https://github.com/simolus3/drift/blob/0901c984a987e54ba0b67e227adbfdcf3c08eeb4/drift_dev/lib/src/services/schema/verifier_common.dart#L9-L22

-        // Skip sqlite-internal tables, https://www.sqlite.org/fileformat2.html#intschema
-        // https://github.com/simolus3/drift/blob/0901c984a987e54ba0b67e227adbfdcf3c08eeb4/drift_dev/lib/src/services/schema/verifier_common.dart#L9-L22
+        // Skip sqlite-internal tables.  See for comparison:
+        //   https://www.sqlite.org/fileformat2.html#intschema
+        //   https://github.com/simolus3/drift/blob/0901c984a/drift_dev/lib/src/services/schema/verifier_common.dart#L9-L22

	// TODO(upstream): generate this
	// TODO(drift): generate this


		check_no_changes "schema updates" "${schema_dir}"
		check_no_changes "schema or migration helper updates" "${outputs[@]}"

	check_no_changes "schema or migration helper updates" "${outputs[@]}"
	check_no_changes "schema or migration-helper updates" "${outputs[@]}"

	// us. This represent the scenario during developement when checking out
	// us. This represent the scenario during development when checking out

Prepare schema migration/testing code for upcoming database changes #1248

Prepare schema migration/testing code for upcoming database changes #1248

Conversation

PIG208 commented Jan 3, 2025 • edited Loading

chrisbobbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

PIG208 commented Feb 4, 2025

PIG208 commented Feb 5, 2025

chrisbobbe commented Feb 12, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 Feb 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Feb 20, 2025

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PIG208 commented Feb 20, 2025

gnprice commented Feb 20, 2025

PIG208 commented Jan 3, 2025 •

edited

Loading

PIG208 Jan 31, 2025 •

edited

Loading

PIG208 Feb 19, 2025 •

edited

Loading