Skip to content

Refactor AppDocument to rename name field to documentName. #4564

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Refactor AppDocument to rename name field to documentName. #4564

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9edcc7a
Refactor AppDocument to rename field to .
vikdubey Apr 21, 2025
062d891
Throw unsupported error for name getter and setter.
vikdubey Apr 25, 2025
178e51a
Merge branch 'main' into app-document-refactor
vikdubey Apr 25, 2025
ddd2fa1
Resolve merge conflict.
vikdubey Apr 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app_dart/lib/src/model/firestore/account.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,5 +59,5 @@ final class Account extends AppDocument<Account> {
Account.fromDocument(super.document);

/// Email address of the account.
String get email => p.posix.basename(name!);
String get email => p.posix.basename(documentName!);
}
14 changes: 11 additions & 3 deletions app_dart/lib/src/model/firestore/base.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ final class AppDocumentMetadata<T extends AppDocument<T>> {
abstract class AppDocument<T extends AppDocument<T>> implements g.Document {
AppDocument([g.Document? from])
: _fields = from?.fields ?? {},
name = from?.name,
documentName = from?.name,
createTime = from?.createTime,
updateTime = from?.updateTime;

Expand All @@ -116,7 +116,15 @@ abstract class AppDocument<T extends AppDocument<T>> implements g.Document {
final Map<String, g.Value> _fields;

@override
String? name;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this doesn't really solve the underlying problem, now there is a name and documentName, but you can still use say, task.name and accidentally be referring to the full database/path/to/document instead of task.taskName.

I appreciate the contribution but this isn't enough to warrant merging.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @matanlurey for the prompt review and feedback.

I understand the issue with making both name and documentName accessible. What about we make the name field unaccessible like below,

@OverRide
String? get name {
throw UnsupportedError(
'Accessing name is not allowed. Use documentName instead.');
}

@OverRide
set name(String? value) {
throw UnsupportedError(
'Setting name is not allowed. Use documentName instead.');
}

Please let me know if this would work, I can make code changes accordingly. If not, could you please provide some pointers on potential design. Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately that won't be a safe change to make - <Document>.name is used by the underlying (generated) googeapis.v1.firebase package, so it needs to exist, we just don't want to use it (directly). There are a few things we could do, in order of easiest to most difficult:

  1. Mark String get/set name as @protected. Firebase APIs can still use, we won't (at least not by accident)
  2. Make AppDocument no longer implement/extend Document, and instead using composition

Given the work you already did on this PR, I'd be OK with going with (1) as a stop-gap (it makes the current situation better), and leaving a bug open for (2). What do you think?

String? get name => documentName;

@override
set name(String? value) {
documentName = value;
}

/// The name of the document corresponding to `name` in [g.Document].
String? documentName;

@override
String? updateTime;
Expand All @@ -130,7 +138,7 @@ abstract class AppDocument<T extends AppDocument<T>> implements g.Document {
return {
'fields': fields,
if (createTime != null) 'createTime': createTime!,
if (name != null) 'name': name!,
if (documentName != null) 'documentName': documentName!,
if (updateTime != null) 'updateTime': updateTime!,
};
}
Expand Down
8 changes: 4 additions & 4 deletions app_dart/lib/src/model/firestore/ci_staging.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ final class CiStaging extends AppDocument<CiStaging> {
/// Create [CiStaging] from a Commit Document.
CiStaging.fromDocument(Document other) {
this
..name = other.name
..documentName = other.name
..fields = {...?other.fields}
..createTime = other.createTime
..updateTime = other.updateTime;
Expand All @@ -128,7 +128,7 @@ final class CiStaging extends AppDocument<CiStaging> {
}

// Read it from the document name.
final [owner, repo, _, _] = p.posix.basename(name!).split('_');
final [owner, repo, _, _] = p.posix.basename(documentName!).split('_');
return RepositorySlug(owner, repo);
}

Expand All @@ -140,7 +140,7 @@ final class CiStaging extends AppDocument<CiStaging> {
}

// Read it from the document name.
final [_, _, sha, _] = p.posix.basename(name!).split('_');
final [_, _, sha, _] = p.posix.basename(documentName!).split('_');
return sha;
}

Expand All @@ -152,7 +152,7 @@ final class CiStaging extends AppDocument<CiStaging> {
}

// Read it from the document name.
final [_, _, _, stageName] = p.posix.basename(name!).split('_');
final [_, _, _, stageName] = p.posix.basename(documentName!).split('_');
return CiStage.values.firstWhereOrNull((e) => e.name == stageName);
}

Expand Down
10 changes: 5 additions & 5 deletions app_dart/lib/src/model/firestore/commit.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ final class Commit extends AppDocument<Commit> {
final documentName = p.join(kDatabase, 'documents', collectionId, sha);
try {
final document = await firestore.getDocument(documentName);
return Commit._(document.fields!, name: document.name!);
return Commit._(document.fields!, documentName: document.name!);
} on DetailedApiRequestError catch (e) {
if (e.status == HttpStatus.notFound) {
return null;
Expand All @@ -99,7 +99,7 @@ final class Commit extends AppDocument<Commit> {
fieldMessage: message.toValue(),
fieldRepositoryPath: repositoryPath.toValue(),
fieldSha: sha.toValue(),
}, name: p.posix.join(kDatabase, 'documents', collectionId, sha));
}, documentName: p.posix.join(kDatabase, 'documents', collectionId, sha));
}

/// Creates a Cocoon commit from a Github-authored [commit] on a [branch].
Expand Down Expand Up @@ -133,13 +133,13 @@ final class Commit extends AppDocument<Commit> {
}

factory Commit.fromDocument(Document document) {
return Commit._(document.fields!, name: document.name!);
return Commit._(document.fields!, documentName: document.name!);
}

Commit._(Map<String, Value> fields, {required String name}) {
Commit._(Map<String, Value> fields, {required String documentName}) {
this
..fields = fields
..name = name;
..documentName = documentName;
}

/// The timestamp (in milliseconds since the Epoch) of when the commit
Expand Down
2 changes: 1 addition & 1 deletion app_dart/lib/src/model/firestore/github_build_status.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ final class GithubBuildStatus extends AppDocument<GithubBuildStatus> {
/// Create [GithubBuildStatus] from a GithubBuildStatus Document.
GithubBuildStatus.fromDocument(Document other) {
this
..name = other.name
..documentName = other.name
..fields = {...?other.fields}
..createTime = other.createTime
..updateTime = other.updateTime;
Expand Down
2 changes: 1 addition & 1 deletion app_dart/lib/src/model/firestore/pr_check_runs.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ final class PrCheckRuns extends AppDocument<PrCheckRuns> {
static PrCheckRuns fromDocument(Document prCheckRunsDoc) {
return PrCheckRuns()
..fields = prCheckRunsDoc.fields!
..name = prCheckRunsDoc.name!;
..documentName = prCheckRunsDoc.name!;
}

/// The json string of the pullrequest belonging to this document.
Expand Down
12 changes: 6 additions & 6 deletions app_dart/lib/src/model/firestore/task.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ final class Task extends AppDocument<Task> {
fieldTestFlaky: testFlaky.toValue(),
fieldAttempt: currentAttempt.toValue(),
},
name: p.posix.join(
documentName: p.posix.join(
kDatabase,
'documents',
kTaskCollectionId,
Expand All @@ -190,7 +190,7 @@ final class Task extends AppDocument<Task> {

/// Create [Task] from a task Document.
factory Task.fromDocument(Document document) {
return Task._(document.fields!, name: document.name!);
return Task._(document.fields!, documentName: document.name!);
}

factory Task.fromDatastore(datastore.Task task) {
Expand Down Expand Up @@ -230,10 +230,10 @@ final class Task extends AppDocument<Task> {
);
}

Task._(Map<String, Value> fields, {required String name}) {
Task._(Map<String, Value> fields, {required String documentName}) {
this
..fields = fields
..name = name;
..documentName = documentName;
}

/// Returns a Firestore [Write] that patches the [status] field for [id].
Expand Down Expand Up @@ -342,7 +342,7 @@ final class Task extends AppDocument<Task> {
}

// Read the attempts from the document name.
final documentId = p.basename(name!);
final documentId = p.basename(documentName!);
return TaskId.parse(documentId).currentAttempt;
}

Expand Down Expand Up @@ -410,7 +410,7 @@ final class Task extends AppDocument<Task> {

void resetAsRetry({int? attempt}) {
attempt ??= currentAttempt + 1;
name = p.posix.join(
documentName = p.posix.join(
kDatabase,
'documents',
kTaskCollectionId,
Expand Down
2 changes: 1 addition & 1 deletion app_dart/test/model/firestore/commit_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ void main() {
firestoreService,
sha: storedCommit.sha,
);
expect(resultedCommit.name, storedCommit.name);
expect(resultedCommit.documentName, storedCommit.documentName);
expect(resultedCommit.fields, storedCommit.fields);
});
}
7 changes: 5 additions & 2 deletions app_dart/test/model/firestore/github_build_status_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,12 @@ void main() {

final resultedGithubBuildStatus = await GithubBuildStatus.fromFirestore(
firestoreService: firestoreService,
documentName: githubBuildStatus.name!,
documentName: githubBuildStatus.documentName!,
);
expect(
resultedGithubBuildStatus.documentName,
githubBuildStatus.documentName,
);
expect(resultedGithubBuildStatus.name, githubBuildStatus.name);
expect(resultedGithubBuildStatus.fields, githubBuildStatus.fields);
});
}
7 changes: 5 additions & 2 deletions app_dart/test/model/firestore/github_gold_status_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,12 @@ void main() {

final resultedGithubGoldStatus = await GithubGoldStatus.fromFirestore(
firestoreService: firestoreService,
documentName: githubGoldStatus.name!,
documentName: githubGoldStatus.documentName!,
);
expect(
resultedGithubGoldStatus.documentName,
githubGoldStatus.documentName,
);
expect(resultedGithubGoldStatus.name, githubGoldStatus.name);
expect(resultedGithubGoldStatus.fields, githubGoldStatus.fields);
});
}
6 changes: 3 additions & 3 deletions app_dart/test/model/firestore/task_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ void main() {
final commitSha = task.commitKey!.id!.split('/').last;
final taskDocument = Task.fromDatastore(task);
expect(
taskDocument.name,
taskDocument.documentName,
'$kDatabase/documents/$kTaskCollectionId/${commitSha}_${task.name}_${task.attempts}',
);
expect(taskDocument.createTimestamp, task.createTimestamp);
Expand Down Expand Up @@ -105,7 +105,7 @@ void main() {
firestoreService,
TaskId(commitSha: 'abc123', taskName: 'test', currentAttempt: 1),
);
expect(resultedTask.name, firestoreTask.name);
expect(resultedTask.documentName, firestoreTask.documentName);
expect(resultedTask.fields, firestoreTask.fields);
});
});
Expand All @@ -119,7 +119,7 @@ void main() {
);
task.resetAsRetry();

expect(int.parse(task.name!.split('_').last), 2);
expect(int.parse(task.documentName!.split('_').last), 2);
expect(task.status, Task.statusNew);
expect(task.testFlaky, false);
});
Expand Down