simolus3 / drift

Drift is an easy to use, reactive, typesafe persistence library for Dart & Flutter.
https://drift.simonbinder.eu/
MIT License
2.44k stars 352 forks source link

Code generation creates database table with wrong type #3034

Open 91jaeminjo opened 3 weeks ago

91jaeminjo commented 3 weeks ago

Describe the bug I have a class, let's say x that I use in dart. I also have a table in drift that's also named x. in the database.dart file, I use local.x and just x to refer to the database table.

It used to be fine until last December or so, but when I regenerated code this week, the database table code is generated with local.x.

Thank you in advance for your help!

dickermoshe commented 3 weeks ago

I've had this issue with other code generation tools too. Could you post the relevant portions of the generated code that is incorrect to help debug this?

91jaeminjo commented 3 weeks ago

The db table name is endpoints. The generated code creates the below class Endpoints which should extend Table with Table with TableInfo<Endpoints, Endpoint> but as shown below (as well as all places where Endpoint is referenced), the Endpoint is replaced by location.Endpoint. The location.Endpoint is imported and used in my database dart file.

class Endpoints extends Table with TableInfo<Endpoints, location.Endpoint> {

Please let me know if you need more information.

dickermoshe commented 3 weeks ago

@simolus3 It seems that this is not related to the manager API. I'm surprised this worked in the past.

Could you post the table spec, something reproducible will make it much easier to fix this.

91jaeminjo commented 3 weeks ago

Yea I tried disabling generate_manager: false and that didn't fix it.

CREATE TABLE endpoints (
    id INT NOT NULL PRIMARY KEY AUTOINCREMENT,
    name TEXT NOT NULL,
    external_id TEXT,
    uri TEXT NOT NULL,
    description TEXT NOT NULL,
    writable BOOLEAN NOT NULL DEFAULT FALSE,

    UNIQUE (name)
);

This is an example of the db table in the drift file, and I forgot to mention that location.Endpoint is a built value class which also has generated code.

abstract class Endpoint implements Built<Endpoint, EndpointBuilder> {
  int get id;
  Uri get uri;

this class has some checks in the constructor, has a serializer static Serializer<Endpoint> get serializer => _$endpointSerializer;

not sure if these would be relevant..

dickermoshe commented 3 weeks ago

Tell me if this is what is happening

endpoints.drift

CREATE TABLE endpoints (
    id INT NOT NULL PRIMARY KEY AUTOINCREMENT,
    name TEXT NOT NULL,
    external_id TEXT,
    uri TEXT NOT NULL,
    description TEXT NOT NULL,
    writable BOOLEAN NOT NULL DEFAULT FALSE,

    UNIQUE (name)
);

endpoints.dart

class Endpoint{
   // ...
}

db.dart

import 'package:drift/drift.dart';
import 'package:app/endpoints.dart' as location;
part 'db.g.dart';

@DriftDatabase(
  include: {'endpoints.drift'},
)
class AppDb extends _$AppDb {
  AppDb() : super(_openConnection());

  @override
  int get schemaVersion => 1;
}

db.g.dart

//... Generated Code 
class Endpoints extends Table with TableInfo<Endpoints, location.Endpoint> {
  // ...
}
//... More Generated Code 

The bug is that we should be using Endpoint that drift created, not location.Endpoint which is your own class.

Is the above correct?

91jaeminjo commented 3 weeks ago

yes. Sorry I could have been more clear. Thanks for your help.

One difference from my actual code is that local.Endpoint is a built value, but not sure if that matters. I am trying to reproduce this problem in a mini side project right now

91jaeminjo commented 3 weeks ago

Ok I get the same problem when I made location.Endpoint just a class not built with built value, and the db.dart doesn't even use the location.Endpoint.

As long as that import '/endpoint.dart' as location is present in the db.dart file, I see this issue.

dickermoshe commented 3 weeks ago

@simolus3 I'm not going to have time to fix this for the next 2 weeks (I've got some personal things going on) I've triaged this bug for now. I just want to make sure that you weren't under the assumption that I've got this covered. I do not.

simolus3 commented 3 weeks ago

No worries! This is actually a known problem and a side-effect from trying to be more reliable about finding import aliases. It fixed some cases and made some others worse :(

The problem is that we want drift-generated code to automatically be able to use import aliases (since it inherits those as it is a part file). But for legacy reasons we don't always have the exact definition source file available, only the name (Location in this case). And then we have to guess.

One way to fix this is to use modular code generation. It requires a few changes to your code, but reliably fixes this problem as drift can then generate its own files with imports and we avoid having to guess prefixes. You can also work around the issue by avoiding the duplicate name (e.g. by defining a typedef EndpointLocatation = Location in another file and then only importing the alias instead of anything directly named Location). But that sucks obviously, and drift should be better here. I'll take a look if we can consider this case when finding prefixes.