drift icon indicating copy to clipboard operation
drift copied to clipboard

Circular error when deserializing drift modules

Open andresthinkme opened this issue 1 year ago • 21 comments

After upgrading from Drift 2.19.1 to 2.20.2, I get an error message when trying to build.

The error message is "Could not deserialize DriftElementId(package:axibis_base/database/sales/sales_invoices_dao.dart, sales_invoices) Internal error while deserializing DriftElementId(package:axibis_base/database/sales/sales_invoices_dao.dart, sales_invoices): Bad state: Circular error when deserializing drift modules. This is a bug in drift_dev!"

When I changeg back to version 2.19.1 this was solved. Nothing changed in my code after the update.

andresthinkme avatar Sep 19 '24 07:09 andresthinkme

Hi, the error is not critical (and also likely happened in the previous version, we just didn't log it). Thanks for the report though, this is still something we should fix. Is it possible for you share the definition of SaleInvoices?

simolus3 avatar Sep 19 '24 11:09 simolus3

Hi @simolus3, we get the exact same warning in our code for a bunch of different things. Let me give you an example so you have something to go off of:

[WARNING] drift_dev on lib/src/operator_db/operator_db_base.dart:
Could not deserialize DriftElementId(package:database/src/operator_db/tables/execution_header_additional_info_table.dart, execution_header_additional_infos)
Internal error while deserializing DriftElementId(package:database/src/operator_db/tables/execution_header_action_additional_info_table.dart, execution_header_action_additional_infos): Bad state: Circular error when deserializing drift modules. This is a bug in drift_dev! at 
#0      ElementDeserializer._readElementReference (package:drift_dev/src/analysis/serializer.dart:435:7)
#1      ElementDeserializer._readDriftElement (package:drift_dev/src/analysis/serializer.dart:494:15)
#2      ElementDeserializer.readDriftElement (package:drift_dev/src/analysis/serializer.dart:466:28)
<asynchronous suspension>
#3      ElementDeserializer._readElementReference (package:drift_dev/src/analysis/serializer.dart:443:14)
<asynchronous suspension>
#4      ElementDeserializer._readDriftElement (package:drift_dev/src/analysis/serializer.dart:494:9)
<asynchronous suspension>
#5      ElementDeserializer.readDriftElement (package:drift_dev/src/analysis/serializer.dart:466:22)
<asynchronous suspension>
#6      DriftResolver._restoreOrResolve (package:drift_dev/src/analysis/resolver/resolver.dart:48:16)
<asynchronous suspension>
#7      DriftResolver.resolveReferencedElement (package:drift_dev/src/analysis/resolver/resolver.dart:153:26)
<asynchronous suspension>
#8      LocalElementResolver.resolveDartReferenceOrReportError (package:drift_dev/src/analysis/resolver/resolver.dart:256:9)
<asynchronous suspension>
#9      DartAccessorResolver.resolve (package:drift_dev/src/analysis/resolver/dart/accessor.dart:61:21)
<asynchronous suspension>
#10     DriftResolver._resolveDiscovered (package:drift_dev/src/analysis/resolver/resolver.dart:103:22)
<asynchronous suspension>
#11     DriftResolver._restoreOrResolve (package:drift_dev/src/analysis/resolver/resolver.dart:60:12)
<asynchronous suspension>
#12     DriftResolver.resolveEntrypoint (package:drift_dev/src/analysis/resolver/resolver.dart:42:12)
<asynchronous suspension>
#13     DriftAnalysisDriver.resolveElement (package:drift_dev/src/analysis/driver/driver.dart:269:16)
<asynchronous suspension>
#14     DriftAnalysisDriver._analyzeLocalElements (package:drift_dev/src/analysis/driver/driver.dart:254:7)
<asynchronous suspension>
#15     DriftAnalysisDriver.resolveElements (package:drift_dev/src/analysis/driver/driver.dart:321:5)
<asynchronous suspension>
#16     DriftAnalysisDriver.fullyAnalyze (package:drift_dev/src/analysis/driver/driver.dart:329:19)
<asynchronous suspension>
#17     _DriftBuildRun._analyze (package:drift_dev/src/backends/build/drift_builder.dart:193:20)
<asynchronous suspension>
#18     _DriftBuildRun.run (package:drift_dev/src/backends/build/drift_builder.dart:171:9)
<asynchronous suspension>
#19     DriftBuilder.build (package:drift_dev/src/backends/build/drift_builder.dart:108:5)
<asynchronous suspension>
#20     runBuilder.buildForInput (package:build/src/generate/run_builder.dart:83:7)
<asynchronous suspension>
#21     Future.wait.<anonymous closure> (dart:async/future.dart:534:21)
<asynchronous suspension>
#22     scopeLogAsync.<anonymous closure> (package:build/src/builder/logging.dart:32:40)
<asynchronous suspension>

I've added the table definition below, as a txt file:

execution_header_action_additional_info_table.txt

Dumoulin-Lander avatar Sep 26 '24 18:09 Dumoulin-Lander

Thanks for the details! I suspect this might be related to the ExecutionHeaderAdditionalInfos reference, could you share the definition of that table as well?

simolus3 avatar Sep 26 '24 19:09 simolus3

Hi @simolus3,

Sorry for the late reply. Here is the definition for the table and the dataclass used for it.

@UseRowClass(SalesInvoice) class SalesInvoices extends Table { TextColumn get id => text().clientDefault(() => locator<Uuid>().v4())(); TextColumn get number => text().unique()(); TextColumn get ticketId => text().nullable().references(Tickets, #id)(); TextColumn get paymentMethodId => text().nullable().references(PaymentMethods, #id)(); TextColumn get paymentStatus => textEnum<PaymentStatus>()(); TextColumn get structuredMessage => text().withLength(min: 12, max: 12).nullable()(); TextColumn get customerId => text().nullable().references(Customers, #id)(); TextColumn get customerNumber => text().nullable()(); TextColumn get customerName => text().nullable()(); TextColumn get customerVatNumber => text().nullable()(); TextColumn get customerEmail => text().nullable()(); TextColumn get customerPhone => text().nullable()(); TextColumn get customerAddress => text().nullable()(); TextColumn get customerAddress2 => text().nullable()(); TextColumn get customerPostCode => text().nullable()(); TextColumn get customerCity => text().nullable()(); TextColumn get customerCountry => text().nullable()(); TextColumn get comment => text().nullable()(); DateTimeColumn get dueDate => dateTime().withDefault(currentDateAndTime)(); TextColumn get ourReference => text().nullable()(); TextColumn get yourReference => text().nullable()(); DateTimeColumn get createdAt => dateTime().withDefault(currentDateAndTime)(); TextColumn get syncStatus => textEnum<SyncStatus>()();

@override Set<Column> get primaryKey => {id}; }

class SalesInvoice implements Insertable<SalesInvoice> { final String id; final String number; final String? ticketId; final String? paymentMethodId; final PaymentStatus paymentStatus; final String? structuredMessage; final String? customerId; final String? customerNumber; final String? customerName; final String? customerVatNumber; final String? customerEmail; final String? customerPhone; final String? customerAddress; final String? customerAddress2; final String? customerPostCode; final String? customerCity; final String? customerCountry; final String? comment; final DateTime dueDate; final String? ourReference; final String? yourReference; final DateTime createdAt; // enum final SyncStatus syncStatus; }

andresthinkme avatar Sep 27 '24 09:09 andresthinkme

We're also seeing this issue. It's a warning, but it's causing our CI builds to fail.

Fraa-124 avatar Sep 29 '24 09:09 Fraa-124

@Dumoulin-Lander You have two tables referencing each other (ExecutionHeaderAdditionalInfos.actionId references ExecutionHeaderActionAdditionalInfos, which references the first table through executionHeaderAdditionalInfoId). Drift's error message here should be much better, but this is a problem in your schema and it can cause issues in the database when foreign keys are enabled (because we can't create either table with a correct foreign key constraint before creating the other one).

@andresthinkme and @Fraa-124, could you also check whether your table might have circular references to other tables linking back to the original table? That would explain the error. @Fraa-124 I'm surprised that this would fail the build, are you running it in a special mode to fail on warnings?

simolus3 avatar Sep 29 '24 12:09 simolus3

@simolus3 Why can't we create both tables and then add the foreign key columns afterward. This is what Django does.

dickermoshe avatar Sep 29 '24 17:09 dickermoshe

e.g.

from django.db import models

class Student(models.Model):
    name = models.CharField(max_length=100)
    group = models.ForeignKey("StudentGroup", on_delete=models.CASCADE)

class StudentGroup(models.Model):
    name = models.CharField(max_length=100)
    top_student = models.ForeignKey(
        Student, on_delete=models.CASCADE, related_name="top_student"
    )

There is a circular reference here. But here is how django can creates it

BEGIN;
--
-- Create model Student
--
CREATE TABLE "hello_student" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "name" varchar(100) NOT NULL);
--
-- Create model StudentGroup
--
CREATE TABLE "hello_studentgroup" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "name" varchar(100) NOT NULL, "top_student_id" bigint NOT NULL REFERENCES "hello_student" ("id") DEFERRABLE INITIALLY DEFERRED);
--
-- Add field group to student
--
CREATE TABLE "new__hello_student" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "name" varchar(100) NOT NULL, "group_id" bigint NOT NULL REFERENCES "hello_studentgroup" ("id") DEFERRABLE INITIALLY DEFERRED);
INSERT INTO "new__hello_student" ("id", "name", "group_id") SELECT "id", "name", NULL FROM "hello_student";
DROP TABLE "hello_student";
ALTER TABLE "new__hello_student" RENAME TO "hello_student";
CREATE INDEX "hello_studentgroup_top_student_id_dfeeb764" ON "hello_studentgroup" ("top_student_id");
CREATE INDEX "hello_student_group_id_3dd4e5e4" ON "hello_student" ("group_id");
COMMIT;

This looks really complex. I don't know how feasible it would be to bake this into drift, but it's technically possible. even in sqlite

dickermoshe avatar Sep 29 '24 21:09 dickermoshe

With the alterTable machinery we already have in place, it shouldn't be too hard to pull off (even though we'll likely create SQL that is even more verbose, haha).

That's something for the future though, the broken state is that we don't currently support circular references (and there are several places in drift_dev that are supposed to detect them and loudly complain, none of them apparently working correctly). That's the part I need to take a look at first - for instance, the default migration logic creating all tables relies on them being topologically sorted at build time which wouldn't work if we silently drop circular references.

simolus3 avatar Sep 30 '24 20:09 simolus3

Alright, I've found the root cause but it's not going to be easy to resolve (or at least I don't have an obvious solution). We're running modular analysis to improve build performance, and the tables are defined in different files.

So let's say that we had two files, a.dart and b.dart with tables referencing each other. When drift_dev:analyzer runs on a.dart, it finds the local table, starts resolving it and discovers the reference to the table in b.dart. While attempting to resolve that table, it detects the circular error and logs the error in the definition of table b (but not in table a, because cycles are broken where they are first detected). To avoid logging errors multiple times however, drift_dev:analyzer only logs errors in a.dart (and thinks they aren't any). When running on b.dart, this game repeats (it thinks there's an error in a.dart due to the circular reference but doesn't report this).

We should probably keep track of transitive attempted dependencies somehow to file the error on both tables, which would fix this issue (or at least give it a better error message).

simolus3 avatar Sep 30 '24 21:09 simolus3

I am getting the same error any quick solutions.

[WARNING] drift_dev on lib/services/database/pharmacy/drug_formulas.dart:
Could not deserialize DriftElementId(package:system_server/services/database/pharmacy/drug_formulas.dart, drug_formulas)
Internal error while deserializing DriftElementId(package:system_server/services/database/files/server_files.dart, server_files): Bad state: Circular error when deserializing drift modules. This is a bug in drift_dev! at 
#0      ElementDeserializer._readElementReference (package:drift_dev/src/analysis/serializer.dart:435:7)
#1      ElementDeserializer._readDriftElement (package:drift_dev/src/analysis/serializer.dart:494:15)
#2      ElementDeserializer.readDriftElement (package:drift_dev/src/analysis/serializer.dart:466:28)
<asynchronous suspension>
#3      ElementDeserializer._readElementReference (package:drift_dev/src/analysis/serializer.dart:443:14)
<asynchronous suspension>

iampopal avatar Oct 06 '24 11:10 iampopal

Note that this error is mostly harmless and doesn't change the generated code. Could you also check whether the table in drug_formulas.dart references a table that itself then references the drug_formulas.dart table again?

simolus3 avatar Oct 06 '24 11:10 simolus3

Thank you so much for quickly replaying.

I actually had error of manager and reference need to be added

Then updated drift to latest version and it start showing: Circular error when deserializing

After seeing this I downgraded back and start seeing @Reference required for manager again.

Then I read the manager documentation and use cases so I actually do not use it and not need it. So I disabled it from config yml file of drift.

Now my issue is solved.

Can you please tell me about manager is it required to use or does it helps.

On Sun, 6 Oct 2024 at 4:12 PM Simon Binder @.***> wrote:

Note that this error is mostly harmless and doesn't change the generated code. Could you also check whether the table in drug_formulas.dart references a table that itself then references the drug_formulas.dart table again?

— Reply to this email directly, view it on GitHub https://github.com/simolus3/drift/issues/3227#issuecomment-2395406637, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIUFLXGNT5SHKT4EQKPDQADZ2EO2XAVCNFSM6AAAAABOPJ7WNOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJVGQYDMNRTG4 . You are receiving this because you commented.Message ID: @.***>

iampopal avatar Oct 06 '24 15:10 iampopal

You don't need to use it but it's just an easier way to write queries. What was the manager bug? If there is a bug I want to fix it.

dickermoshe avatar Oct 06 '24 16:10 dickermoshe

@simolus3 This error message should probably be clarified.

dickermoshe avatar Oct 06 '24 16:10 dickermoshe

The manager bug was that it required to add @Reference at every relationship I had in latest version.

Going back to older versions fixes that. On Sun, 6 Oct 2024 at 8:40 PM Moshe Dicker @.***> wrote:

@simolus3 https://github.com/simolus3 This error message should probably be clarified.

— Reply to this email directly, view it on GitHub https://github.com/simolus3/drift/issues/3227#issuecomment-2395492720, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIUFLXDGPPPAQCUCEFRBL2TZ2FOGJAVCNFSM6AAAAABOPJ7WNOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJVGQ4TENZSGA . You are receiving this because you commented.Message ID: @.***>

iampopal avatar Oct 06 '24 16:10 iampopal

Oh, but that would just print a warning, those can be ignored

dickermoshe avatar Oct 06 '24 17:10 dickermoshe

But after those warnings the build was not working. New files ware not generating. Doing above steps help solve the problem.

Thank you for all you guys being very active and responding.

On Sun, 6 Oct 2024 at 10:04 PM Moshe Dicker @.***> wrote:

Oh, but that would just print a warning, those can be ignored

— Reply to this email directly, view it on GitHub https://github.com/simolus3/drift/issues/3227#issuecomment-2395519189, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIUFLXDHI6WZWYWCQSHMW7DZ2FYCFAVCNFSM6AAAAABOPJ7WNOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJVGUYTSMJYHE . You are receiving this because you commented.Message ID: @.***>

iampopal avatar Oct 06 '24 17:10 iampopal

Would you mind sharing it? Or at least just the table definitions which were causing this issue?

dickermoshe avatar Oct 06 '24 17:10 dickermoshe

I was also getting these warnings due to having my tables declared in multiple files. For now, I've fixed this by using dart's part directive. While I would still prefer to use dart's recommended approach of using single packages and imports for most cases, the amount of warnings I was getting started to clutter my console so much that I was missing build error messages.

File structure:

/
| /database.dart
| /tables/
| | /tables.dart
| | /table_a.dart
| | /table_b.dart
| | /table_c.dart
| | /[other tables]
| | /table_z.dart

tables.dart


import 'package:something.dart'; // Any necessary imports for tables a-z go here

part 'table_a.dart';
part 'table_b.dart';
part 'table_c.dart';
// ... other tables
part 'table_z.dart';

table_a.dart, table_b.dart, ..., table_z.dart


part of 'tables.dart';

class TableA extends Table {
// Table declaration
}

database.dart


import 'package:your_package/[database_path]/tables/tables.dart';

// Database declaration

Andre-lbc avatar Jan 03 '25 16:01 Andre-lbc