simolus3 / drift

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

Chaining of ColumnBuilder options not working correctly #3271

Closed radaced closed 1 month ago

radaced commented 1 month ago

I have the following table specified:

@DataClassName("ClaimAttachmentEntity")
class ClaimAttachmentEntities extends Table {
  IntColumn get id => integer().autoIncrement()();
  TextColumn get ravenId => text().customConstraint("UNIQUE")();
  TextColumn get claimId => text().customConstraint("REFERENCES claimEntities(ravenId)")()
}

After seeing build warnings that nullability is not specified I saw that there are also ColumnBuilder options like unique() oder references(...).

I changed the table specification to this:

@DataClassName("ClaimAttachmentEntity")
class ClaimAttachmentEntities extends Table {
  IntColumn get id => integer().autoIncrement()();
  TextColumn get ravenId => text().unique()();
  TextColumn get claimId => text().references(ClaimEntities, #ravenId)();
}

The build warnings disappeared and everything got regenerated nicely.

old:

static const VerificationMeta _ravenIdMeta =
    const VerificationMeta('ravenId');
@override
late final GeneratedColumn<String> ravenId = GeneratedColumn<String>(
    'raven_id', aliasedName, false,
    type: DriftSqlType.string,
    requiredDuringInsert: true,
    $customConstraints: 'UNIQUE');
static const VerificationMeta _claimIdMeta =
    const VerificationMeta('claimId');
@override
late final GeneratedColumn<String> claimId = GeneratedColumn<String>(
    'claim_id', aliasedName, false,
    type: DriftSqlType.string,
    requiredDuringInsert: true,
    $customConstraints: 'REFERENCES claimEntities(ravenId)');

new:

static const VerificationMeta _ravenIdMeta =
    const VerificationMeta('ravenId');
@override
late final GeneratedColumn<String> ravenId = GeneratedColumn<String>(
    'raven_id', aliasedName, false,
    type: DriftSqlType.string,
    requiredDuringInsert: true,
    defaultConstraints: GeneratedColumn.constraintIsAlways('UNIQUE'));
static const VerificationMeta _claimIdMeta =
    const VerificationMeta('claimId');
@override
late final GeneratedColumn<String> claimId = GeneratedColumn<String>(
    'claim_id', aliasedName, false,
    type: DriftSqlType.string,
    requiredDuringInsert: true,
    defaultConstraints: GeneratedColumn.constraintIsAlways(
        'REFERENCES claim_entities (raven_id)'));

Apart from these changes I did some actual model changes and generated files to write a migration test. While running the test I got the following error:

claim_attachment_entities:
   columns:
    raven_id:
     Not equal: `NOT NULL UNIQUE` (expected) and `UNIQUE` (actual)
    claim_id:
     Not equal: `NOT NULL REFERENCES claim_entities (raven_id)` (expected) and `REFERENCES claimEntities(ravenId)` (actual)

If I interpret this correctly it seems that the nullability specification for these fields are somehow missing although I didn't change anything about the nullability and by default they should not be nullable. I don't see a possibility to specify explicit nonnullable() or similar (as opposite to the nullable() option).

I assume this is a bug or is this somehow not the intended usage?

I used drift version 2.18.0, but I also couldn't see any fixes regarding to this in newer versions.

Any help or guidance is well appreciated and let me know if you need more infos.

dickermoshe commented 1 month ago

customConstraint resets a columns constraints, so although by default a text() column has a NOT NULL constraint, customConstraint removes it.

So when you remove the customConstraint constraint, it reverts to being NOT NULL.

See https://drift.simonbinder.eu/docs/getting-started/advanced_dart_tables/#custom-constraints

simolus3 commented 1 month ago

After seeing build warnings that nullability is not specified I saw that there are also ColumnBuilder options like unique() oder references(...).

Perhaps we should make the warnings clearer then? This behavior is what the warning was trying to explain. The columns appear to be non-nullable, but they're not. Now that the definition is fixed, the old schema is wrong.

If you didn't actually insert non-nullable values, a simple m.alterTable(TableMigrations(affectedTable)) migration should do the trick.

radaced commented 1 month ago

Thanks for the replies and help.