simolus3 / drift

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

Bug :( generateInsertable throws error when using it with freezed class #1587

Closed topperspal closed 2 years ago

topperspal commented 2 years ago

Constructor parameter id has no matching field. When "generateInsertable" enabled, all constructor parameter must have a matching field. Alternatively, you can declare a getter field.

micimize commented 2 years ago

With custom row classes, you have to make sure all fields in your data class constructor have a corresponding column in the table and vice-versa. If you can't find the discrepancy, paste actual data class and table snippets in code blocks

topperspal commented 2 years ago

With custom row classes, you have to make sure all fields in your data class constructor have a corresponding column in the table and vice-versa. If you can't find the discrepancy, paste actual data class and table snippets in code blocks

@freezed
class User with _$User {
  const factory User({required int id}) = _User;
}
@UseRowClass(User, generateInsertable: true)
class Users extends Table {
  IntColumn get id => integer()();
}

This code produces the following error -

Constructor parameter id has no matching field. When "generateInsertable" enabled, all constructor parameter must have a matching field. Alternatively, you can declare a getter field.
   ╷
19 │   const factory User({required int id}) = _User;
simolus3 commented 2 years ago

Thanks for the report! So I think there are two things going wrong here:

targets:
  freezed_only:
    auto_apply_builders: false
    builders:
      freezed:
        enabled: true
  $default:
    dependencies: [":freezed_only"]
    builders:
      freezed:
        enabled: false
clragon commented 2 years ago

I am somehow experiencing this again.

my build.yaml is looking like this:

targets:
  freezed_only:
    auto_apply_builders: false
    builders:
      freezed:
        enabled: true
  $default:
    dependencies: [ ":freezed_only" ]
    builders:
      freezed:
        enabled: false
      json_serializable:
        options:
          field_rename: snake
      drift_dev:
        options:
          generate_connect_constructor: true
          use_data_class_name_for_companions: true

but during build I still get:

There were some errors while running moor_generator on package:e1547/history/data/database.dart:
[WARNING] drift_dev:drift_dev on lib/history/data/database.dart:
line 7, column 7 of package:e1547/history/data/entry.dart: This class used as a custom row class for which an insertable is generated. This means that it must define getters for all columns, but some are missing: host
  ╷
7 │ class History with _$History {
  │       ^^^^^^^
  ╵
simolus3 commented 2 years ago

Thanks for the report @clragon. Do you have a way to push a full example that I could take a look at (a tiny project with that build config, a freezed class and simple drift database should be enough)? Having something to run would help a lot to fix this quickly :)

clragon commented 2 years ago

alright I finally looked into this again and it seems that I am just silly.

My json type is different from my sql type, though I have done this intentionally. One of the members isnt represented in the custom row type. I cant remember if there was a way of expressing intentionally omitting a field like this.

But when I originally read the docs, this was mentioned as a possibility:

A table can have additional columns not reflected in a custom data class. Drift will simply not load those columns when mapping a row.

So I kinda assumed it wouldnt complain.

clragon commented 2 years ago

though, @simolus3 since the docs mention this as a possibility and there are legitimate reasons to exclude certain fields from custom rows, did I just miss a way of supressing this warning, or is there none yet? should I perhaps open a new issue instead?

simolus3 commented 2 years ago

A table can have additional columns not reflected in a custom data class.

This sentence applies to the mapping from the database to your custom class.

When mapping from Dart back to the database, I think it's better to require all columns. IMO, it would be a confusing default behavior to not complain about this, since some columns will never be written in that case (so they should probably be removed from the database as well?). I didn't try this, but you could probably add stub getters like Null get host => null to suppress this warning. It doesn't look great, but at least you're then stating explicitly that the column will never be written.

Can you describe your use case for this? I kind of assume that this is broken code since you have a column you effectively never use. Am I overlooking something?

clragon commented 2 years ago

The code is working fine actually. I have an example in my repo here.

The usecase is just that I dont want this property present on my data objects to prevent accessing it. So, objects are pulled out of the database without this property. I could have made a model that isnt the custom row, but then I have to have a mapper that simply removes this property. Its basically a shortcut.

The column is used during queries, since the table has it, just not when exporting the object.

This actually causes an interesting problem that prevents me from using update.replace, so I use this instead:

  Future<void> replace(Follow item) =>
      ((update(followsTable))..where((tbl) => tbl.id.equals(item.id)))
          .write(item.toInsertable());

Since I cannot use replace on the update, as that would insert an object that doesnt have this property set.