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

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

Open
PIG208 opened this issue Dec 18, 2024 · 2 comments
Open
Labels
a-model Implementing our data model (PerAccountStore, etc.)
Milestone

Comments

@PIG208
Copy link
Member

PIG208 commented Dec 18, 2024

We try to handle the case when a schema downgrade happens:

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.
for (final entity in allSchemaEntities) {
// This will miss any entire tables (or indexes, etc.) that
// don't exist at this version. For a dev-only feature, that's OK.
await m.drop(entity);
}
await m.createAll();
return;
}

This will work in dev because if you checkout and build code with an older schema version, the schema shipped with the build matches the version we are migrating to.

But we can't easily test this:

    test('downgrading', () async {
      final connection = await verifier.startAt(2);
      final db = AppDatabase(connection);
      await verifier.migrateAndValidate(db, 1);
      await db.close();
    });
result
package:drift_dev/src/services/schema/verifier_common.dart 25:5  verify
package:drift_dev/src/services/schema/verifier_impl.dart 44:5    VerifierImplementation.migrateAndValidate

Schema does not match
accounts:
 columns:
  additional:
   Contains the following unexpected entries: acked_push_token

This is because m.createAll() only has access to the latest schema, so while the version is supposedly v1, additional calls to drop the extra columns are needed.

A good step forward would be moving to step-by-step migrations. That won't fix this issue, but it offers a helpful way to access older schemas at specific versions. As this is only a dev-only feature, fixing this will be a low priority.

This feature was added in https://github.com/simolus3/drift/releases/tag/drift-2.10.0, long after we initially added drift as a dependency. It is among the many helpful things added since 2.5.2 (as of writing, we are at 2.19.1+1, and the latest is 2.22.0).

@PIG208 PIG208 added the a-model Implementing our data model (PerAccountStore, etc.) label Dec 18, 2024
@PIG208 PIG208 added this to the M6: Post-launch milestone Dec 18, 2024
@PIG208 PIG208 changed the title Runtime error when schema downgrading downgraded schema does not comply to the actual schema version Dec 18, 2024
@PIG208 PIG208 changed the title downgraded schema does not comply to the actual schema version downgraded schema does not comply with the actual schema version Dec 18, 2024
@PIG208 PIG208 changed the title downgraded schema does not comply with the actual schema version downgraded schema does not always comply with the actual schema version Dec 18, 2024
@PIG208 PIG208 modified the milestones: M6: Post-launch, M7: Future Dec 18, 2024
@PIG208
Copy link
Member Author

PIG208 commented Dec 30, 2024

An issue closely related to this, is that switching from a branch that knows about tables to one that doesn't, will cause a schema downgrade to happen without removing those tables.

If you switch back to the branch with the new tables, attempts to upgrade will always fail, because the tables already exist. To make the app usable again, the database file has to be deleted. Perhaps we should do that by default when downgrading.

@gnprice
Copy link
Member

gnprice commented Dec 30, 2024

To make the app usable again, the database file has to be deleted. Perhaps we should do that by default when downgrading.

Yeah, that'd more fully accomplish what that code is aimed at doing.

Given the context of this code — we've already opened the database when we learn what the schema version is — it might be annoying to arrange that smoothly, though. It might be easier to instead ask the database what all the tables are, and delete those.

PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Jan 3, 2025
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.

Testing for this is blocked until zulip#1172.

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Jan 3, 2025
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.

Testing for this is blocked until zulip#1172.

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Jan 3, 2025
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.

Testing for this is blocked until zulip#1172.

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Jan 7, 2025
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.

Testing for this is blocked until zulip#1172.

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Jan 22, 2025
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.

Testing for this is blocked until zulip#1172.

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Jan 31, 2025
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.

Testing for this is blocked until zulip#1172.

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Jan 31, 2025
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.

Testing for this is blocked until zulip#1172.

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Feb 3, 2025
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.

Testing for this is blocked until zulip#1172.

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Feb 4, 2025
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.

Testing for this is blocked until zulip#1172.

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Feb 4, 2025
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.

Testing for this is blocked until zulip#1172.

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Feb 5, 2025
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.

Testing for this is blocked until zulip#1172.

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Feb 6, 2025
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.

Testing for this is blocked until zulip#1172.

Signed-off-by: Zixuan James Li <[email protected]>
PIG208 added a commit to PIG208/zulip-flutter that referenced this issue Feb 8, 2025
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.

Testing for this is blocked until zulip#1172.

Signed-off-by: Zixuan James Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-model Implementing our data model (PerAccountStore, etc.)
Projects
Status: No status
Development

No branches or pull requests

2 participants