Generate copyWithCompanion on data classes #3022
Conversation
The companion class makes an extremely useful way to bundle up a set
of changes to make to a record. This copyWithCompanion method makes
it possible to apply those changes directly to a record as represented
in Dart, as an instance of the data class.
For example, it can be used by a wrapper that keeps a write-through
cache in Dart for a given database table. Here's slightly simplified
from a real example we have in Zulip:
/// Update an account in the store, returning the new version.
///
/// The account with the given account ID will be updated.
/// It must already exist in the store.
Future<Account> updateAccount(int accountId, AccountsCompanion data) async {
assert(!data.id.present);
await (_db.update(_db.accounts)
..where((a) => a.id.equals(accountId))
).write(data);
final result = _accounts.update(accountId,
(value) => value.copyWithCompanion(data));
notifyListeners();
return result;
}
It's possible to write such a method by hand, of course (in an
extension), or to call copyWith directly. But it needs a line for
each column of the table, which makes either of those error-prone:
in particular, because copyWith naturally has its parameters all
optional, it would be very easy for someone adding a new column to
overlook the need to add a line to this method, and then updates to
the new column would just silently get dropped. So this is a case
where there's a large benefit to generating the code.
|
CI is failing, with a number of failures that I don't reproduce locally and appear unrelated to the PR. For example, it has: But that test file passes for me: It seems like there's a mismatch somewhere over a parameter named |
|
Something funny is going on with the entire package. It seems some dependencies were changes on upstream packages and things are kinda broken. I'm hoping Simone can figure it out. I've been pulling my hair out over this. |
|
Thank you for the contribution Could you please clarify what you're adding here, it looks like a wayo combine Companion objects, right? It would be helpful if you could put forth a proposal with an "Issue" and a "Solution". Would make this review much easier. (It doesn't seem |
Did you see any particular failures about this? We are caching dependencies across CI because that speeds things up, but we're re-running My best guess is that this might be because of the |
There was a problem hiding this comment.
I agree that re-running the build at least in the drift/ package would be helpful :) Then we can also have a small unit test calling this method on a generated data class/companion part as part of drift/test/database/data_class_test.dart.
This is definitely useful though and I agree with the changed, I just wonder if copyFromCompanion could be a clearer name.
Interesting, that makes sense! Thanks for the fix. I'll start by just rebasing (or merging? from the Git history, it looks like your preference is to merge, so I'll do that), to rerun the CI with just that change, in order to try to confirm that that was the issue while I work on those other changes. |
Yeah, that would be a very reasonable name — definitely "copy from companion" reads better as a standalone phrase than "copy with companion" does. I went for this name with the thought that it's very much like the existing (and conventional across Dart libraries generally) Hmm, thinking more, I guess there's one way in which So possibly |
Well, it looks like the CI result after that change was the same. So that change makes sense, but doesn't seem to have fixed that problem on its own. Anyway, clearly the generated files should get updated in any case 🙂, so I'll see about doing that and we'll find out if that was indeed the trigger for these other failures. |
Ran the command:
dart run build_runner build --delete-conflicting-outputs
|
Hmm, interesting! All those failures were in fact pointing to a bug in my changes — involving the interaction with a feature I wasn't aware of. When the error says: that offending TextColumn get descriptionInUpperCase =>
text().generatedAs(description.upper())();and that causes it to not be represented in companion classes. After reading a bit more code, it looks like there isn't a way to compute what the value of that field should be, short of making a SQL query. (Fundamentally, doing so would require attempting to reimplement SQL expressions in Dart, which would be audacious; and I don't see anything that attempts that.) So perhaps the best thing here is that I guess possibly the ideal answer is that |
|
OK, all tests are now passing! Thanks for your patience. I skipped emitting this method when there are generated columns; updated the generated files in drift/; and also added one of these:
|
Sure. The example call site in the PR description is actually the "After" picture as well as part of the "Before": /// Update an account in the store, returning the new version.
///
/// The account with the given account ID will be updated.
/// It must already exist in the store.
Future<Account> updateAccount(int accountId, AccountsCompanion data) async {
assert(!data.id.present);
await (_db.update(_db.accounts)
..where((a) => a.id.equals(accountId))
).write(data);
final result = _accounts.update(accountId,
(value) => value.copyWithCompanion(data));
notifyListeners();
return result;
}In the live code where I'm using a version of that example already, it's made possible by having written the method by hand in an extension, as I mentioned. Here's what that looks like: extension AccountExtension on Account {
Account copyWithCompanion(AccountsCompanion data) { // TODO(drift): generate this
return Account(
id: data.id.present ? data.id.value : id,
realmUrl: data.realmUrl.present ? data.realmUrl.value : realmUrl,
userId: data.userId.present ? data.userId.value : userId,
email: data.email.present ? data.email.value : email,
apiKey: data.apiKey.present ? data.apiKey.value : apiKey,
zulipVersion: data.zulipVersion.present ? data.zulipVersion.value : zulipVersion,
zulipMergeBase: data.zulipMergeBase.present ? data.zulipMergeBase.value : zulipMergeBase,
zulipFeatureLevel: data.zulipFeatureLevel.present ? data.zulipFeatureLevel.value : zulipFeatureLevel,
ackedPushToken: data.ackedPushToken.present ? data.ackedPushToken.value : ackedPushToken,
);
}
}So the "Before" consists of that hand-written There's an alternative "Before" option, which is to call /// Update an account in the store, returning the new version.
///
/// The account with the given account ID will be updated.
/// It must already exist in the store.
///
/// Fields that are given will be updated,
/// and fields not given will be left unmodified.
///
/// The `realmUrl` and `userId` fields should never change on an account,
/// and therefore are not accepted as parameters.
Future<Account> updateAccount(int accountId, {
String? email,
String? apiKey,
String? zulipVersion,
Value<String?> zulipMergeBase = const Value.absent(),
int? zulipFeatureLevel,
Value<String?> ackedPushToken = const Value.absent(),
}) async {
assert(_accounts.containsKey(accountId));
await doUpdateAccount(accountId, AccountsCompanion(
email: Value.absentIfNull(email),
apiKey: Value.absentIfNull(apiKey),
zulipVersion: Value.absentIfNull(zulipVersion),
zulipMergeBase: zulipMergeBase,
zulipFeatureLevel: Value.absentIfNull(zulipFeatureLevel),
ackedPushToken: ackedPushToken,
));
final result = _accounts.update(accountId, (value) => value.copyWith(
email: email,
apiKey: apiKey,
zulipVersion: zulipVersion,
zulipMergeBase: zulipMergeBase,
zulipFeatureLevel: zulipFeatureLevel,
ackedPushToken: ackedPushToken,
));
notifyListeners();
return result;
}As you can see, that's pretty repetitive. And more importantly, it's error-prone — because both the |
|
Thanks for the contribution! |
This has been addressed upstream in simolus3/drift#3022 Signed-off-by: Zixuan James Li <[email protected]>
This has been addressed in Greg's upstream PR: simolus3/drift#3022 Signed-off-by: Zixuan James Li <[email protected]>
This has been addressed in Greg's upstream PR: simolus3/drift#3022 Signed-off-by: Zixuan James Li <[email protected]>
This has been addressed in Greg's upstream PR: https://git hub.com/simolus3/drift/pull/3022 Signed-off-by: Zixuan James Li <[email protected]>
This has been addressed in Greg's upstream PR: simolus3/drift#3022 which was pulled in by the Drift upgrade in 8b564e4 (zulip#1117). Signed-off-by: Zixuan James Li <[email protected]>
The companion class makes an extremely useful way to bundle up a set of changes to make to a record. This copyWithCompanion method makes it possible to apply those changes directly to a record as represented in Dart, as an instance of the data class.
For example, it can be used by a wrapper that keeps a write-through cache in Dart for a given database table. Here's slightly simplified from a real example we have in Zulip:
It's possible to write such a method by hand, of course (in an extension), or to call copyWith directly. But it needs a line for each column of the table, which makes either of those error-prone: in particular, because copyWith naturally has its parameters all optional, it would be very easy for someone adding a new column to overlook the need to add a line to this method, and then updates to the new column would just silently get dropped. So this is a case where there's a large benefit to generating the code.