Skip to content

Commit ee45870

Browse files
authored
Add and use a TimeRange class to represent before/after timestamp. (#4719)
I started to work on flutter/flutter#169234 and realized the current way timestamp filtering is done with Firebase is inflexible (and personally, confusing). This adds a small-ish `TimeRange` class in `cocoon_commons` and uses it instead of `int? timestamp` to determine which commits to find. (Even if we want a different approach than flutter/flutter#169234, this seems worth it?)
1 parent 2f46024 commit ee45870

8 files changed

Lines changed: 361 additions & 28 deletions

File tree

app_dart/lib/src/request_handlers/get_status.dart

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
import 'dart:async';
66

7+
import 'package:cocoon_common/core_extensions.dart';
78
import 'package:cocoon_common/rpc_model.dart' as rpc_model;
89
import 'package:github/github.dart';
910
import 'package:meta/meta.dart';
@@ -40,17 +41,23 @@ final class GetStatus extends RequestHandler {
4041
final branch =
4142
request.uri.queryParameters[kBranchParam] ?? Config.defaultBranch(slug);
4243
final commitNumber = config.commitNumber;
43-
final lastCommitTimestamp =
44-
lastCommitSha != null
45-
? (await Commit.fromFirestoreBySha(
46-
_firestore,
47-
sha: lastCommitSha,
48-
)).createTimestamp
49-
: _now().millisecondsSinceEpoch;
44+
45+
final DateTime lastCommitCreated;
46+
if (lastCommitSha != null) {
47+
final commit = await Commit.fromFirestoreBySha(
48+
_firestore,
49+
sha: lastCommitSha,
50+
);
51+
lastCommitCreated = DateTime.fromMillisecondsSinceEpoch(
52+
commit.createTimestamp,
53+
);
54+
} else {
55+
lastCommitCreated = _now();
56+
}
5057

5158
final commits = await _buildStatusService.retrieveCommitStatusFirestore(
5259
limit: commitNumber,
53-
timestamp: lastCommitTimestamp,
60+
created: TimeRange.before(lastCommitCreated, exclusive: true),
5461
branch: branch,
5562
slug: slug,
5663
);

app_dart/lib/src/service/build_status_service.dart

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
import 'dart:async';
66

7+
import 'package:cocoon_common/core_extensions.dart';
78
import 'package:cocoon_common/task_status.dart';
89
import 'package:cocoon_server/logging.dart';
910
import 'package:github/github.dart';
@@ -88,14 +89,14 @@ interface class BuildStatusService {
8889
/// The returned stream will be ordered by most recent commit first, then
8990
/// the next newest, and so on.
9091
Future<List<CommitTasksStatus>> retrieveCommitStatusFirestore({
92+
required RepositorySlug slug,
9193
required int limit,
92-
int? timestamp,
94+
TimeRange? created,
9395
String? branch,
94-
required RepositorySlug slug,
9596
}) async {
9697
final commits = await _firestore.queryRecentCommits(
9798
limit: limit,
98-
timestamp: timestamp,
99+
created: created,
99100
branch: branch,
100101
slug: slug,
101102
);

app_dart/lib/src/service/firestore.dart

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
import 'dart:async';
66

7+
import 'package:cocoon_common/core_extensions.dart';
78
import 'package:cocoon_common/task_status.dart';
89
import 'package:cocoon_server/access_client_provider.dart';
910
import 'package:cocoon_server/google_auth_provider.dart';
@@ -67,23 +68,40 @@ mixin FirestoreQueries {
6768
Transaction? transaction,
6869
});
6970

71+
static Map<String, String> _filterByTimeRAnge(
72+
String fieldName,
73+
TimeRange range,
74+
) {
75+
return switch (range) {
76+
IndefiniteTimeRange() => const {},
77+
SpecificTimeRange(:final start, :final end, :final exclusive) => {
78+
if (start != null)
79+
'$fieldName ${exclusive ? '>' : '>='}':
80+
'${start.millisecondsSinceEpoch}',
81+
if (end != null)
82+
'$fieldName ${exclusive ? '<' : '<='}':
83+
'${end.millisecondsSinceEpoch}',
84+
},
85+
};
86+
}
87+
7088
/// Queries for recent commits.
7189
///
7290
/// The [limit] argument specifies the maximum number of commits to retrieve.
7391
///
7492
/// The returned commits will be ordered by most recent [Commit.timestamp].
7593
Future<List<Commit>> queryRecentCommits({
7694
int limit = 100,
77-
int? timestamp,
95+
TimeRange? created,
7896
String? branch,
7997
required RepositorySlug slug,
8098
}) async {
81-
timestamp ??= DateTime.now().millisecondsSinceEpoch;
8299
branch ??= Config.defaultBranch(slug);
100+
created ??= TimeRange.indefinite;
83101
final filterMap = <String, Object>{
84102
'${Commit.fieldBranch} =': branch,
85103
'${Commit.fieldRepositoryPath} =': slug.fullName,
86-
'${Commit.fieldCreateTimestamp} <': timestamp,
104+
..._filterByTimeRAnge(Commit.fieldCreateTimestamp, created),
87105
};
88106
final orderMap = <String, String>{
89107
Commit.fieldCreateTimestamp: kQueryOrderDescending,

app_dart/test/request_handlers/get_status_test.dart

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ void main() {
6161
expect(result, containsPair('Commits', isEmpty));
6262
});
6363

64-
test('reports statuses without input commit key', () async {
64+
test('reports statuses without input commit SHA', () async {
6565
buildStatusService = FakeBuildStatusService(
6666
commitTasksStatuses: [
6767
CommitTasksStatus(generateFirestoreCommit(1, sha: commit1.sha), []),
@@ -79,11 +79,20 @@ void main() {
7979
expect(result, containsPair('Commits', hasLength(2)));
8080
});
8181

82-
test('reports statuses with input commit key', () async {
82+
test('reports statuses with input commit SHA', () async {
8383
buildStatusService = FakeBuildStatusService(
8484
commitTasksStatuses: [
85-
CommitTasksStatus(generateFirestoreCommit(1, sha: commit1.sha), []),
86-
CommitTasksStatus(generateFirestoreCommit(2, sha: commit2.sha), []),
85+
// Newer
86+
CommitTasksStatus(
87+
generateFirestoreCommit(2, sha: commit2.sha, createTimestamp: 2),
88+
[],
89+
),
90+
91+
// Older
92+
CommitTasksStatus(
93+
generateFirestoreCommit(1, sha: commit1.sha, createTimestamp: 1),
94+
[],
95+
),
8796
],
8897
);
8998
handler = GetStatus(

app_dart/test/src/service/fake_build_status_service.dart

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,13 @@
22
// Use of this source code is governed by a BSD-style license that can be
33
// found in the LICENSE file.
44

5+
import 'package:cocoon_common/core_extensions.dart';
56
import 'package:cocoon_service/src/service/build_status_provider/commit_tasks_status.dart';
67
import 'package:cocoon_service/src/service/build_status_service.dart';
78
import 'package:collection/collection.dart';
89
import 'package:github/github.dart';
910

11+
// TODO(matanlurey): Remove this shim over Firestore and use FakeFirestore instead.
1012
class FakeBuildStatusService implements BuildStatusService {
1113
FakeBuildStatusService({this.cumulativeStatus, this.commitTasksStatuses});
1214

@@ -27,19 +29,21 @@ class FakeBuildStatusService implements BuildStatusService {
2729
@override
2830
Future<List<CommitTasksStatus>> retrieveCommitStatusFirestore({
2931
int limit = 100,
30-
int? timestamp,
32+
TimeRange? created,
3133
String? branch,
3234
required RepositorySlug slug,
3335
}) async {
3436
commitTasksStatuses!.sortBy((c) => c.commit.createTimestamp);
35-
return commitTasksStatuses!
36-
.where(
37-
(status) =>
38-
((timestamp == null)
39-
? true
40-
: status.commit.createTimestamp < timestamp) &&
41-
status.commit.branch == branch,
42-
)
43-
.toList();
37+
return commitTasksStatuses!.where((status) {
38+
if (status.commit.branch != branch) {
39+
return false;
40+
}
41+
if (created != null) {
42+
return created.contains(
43+
DateTime.fromMillisecondsSinceEpoch(status.commit.createTimestamp),
44+
);
45+
}
46+
return true;
47+
}).toList();
4448
}
4549
}

packages/cocoon_common/lib/core_extensions.dart

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
import 'dart:convert';
66
import 'dart:typed_data';
77

8+
export 'src/time_range.dart';
9+
810
extension BytesStreamExtension on Stream<List<int>> {
911
/// Collects and returns all of the integer lists as list of bytes.
1012
Future<Uint8List> collectBytes({bool copy = true}) async {
Lines changed: 161 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,161 @@
1+
// Copyright 2019 The Flutter Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style license that can be
3+
// found in the LICENSE file.
4+
5+
import 'package:meta/meta.dart';
6+
7+
/// A definition of a time range, which can be specific or indefinite.
8+
@immutable
9+
sealed class TimeRange {
10+
const TimeRange();
11+
12+
/// Matches any time.
13+
static const TimeRange indefinite = IndefiniteTimeRange();
14+
15+
/// Creates a range matching time between [start] and [end].
16+
///
17+
/// If [exclusive] is set, the specific moments described in [start] and [end]
18+
/// are excluded.
19+
///
20+
/// [end] must be after [start].
21+
factory TimeRange.between(
22+
DateTime start, //
23+
DateTime end, {
24+
bool exclusive,
25+
}) = SpecificTimeRange.between;
26+
27+
/// Creates a range matching time before [end].
28+
///
29+
/// If [exclusive] is set, the specific moment described in [end] is excluded.
30+
factory TimeRange.before(
31+
DateTime end, { //
32+
bool exclusive,
33+
}) = SpecificTimeRange.before;
34+
35+
/// Creates a range matching time after [start].
36+
///
37+
/// If [exclusive] is set, the specific moment described in [start] is
38+
/// excluded.
39+
factory TimeRange.after(
40+
DateTime start, { //
41+
bool exclusive,
42+
}) = SpecificTimeRange.after;
43+
44+
@override
45+
@nonVirtual
46+
bool operator ==(Object other) {
47+
return other is TimeRange && start == other.start && end == other.end;
48+
}
49+
50+
@override
51+
@nonVirtual
52+
int get hashCode => Object.hash(start, end);
53+
54+
/// A starting point in time.
55+
///
56+
/// If `null`, the time range is infinite in the past.
57+
DateTime? get start;
58+
59+
/// An ending point in time.
60+
///
61+
/// If `null`, the time range is infinite in the future.
62+
DateTime? get end;
63+
64+
/// Returns whether [time] is within `this` range.
65+
bool contains(DateTime time);
66+
67+
@override
68+
String toString() {
69+
if (start == null && end == null) {
70+
return 'TimeRange.indefinite';
71+
}
72+
if (start == null) {
73+
return 'TimeRange.before($end)';
74+
}
75+
if (end == null) {
76+
return 'TimeRange.after($start)';
77+
}
78+
return 'TimeRange.between($start, $end)';
79+
}
80+
}
81+
82+
/// No specific start or end time.
83+
final class IndefiniteTimeRange extends TimeRange {
84+
@literal
85+
const IndefiniteTimeRange();
86+
87+
@override
88+
Null get start => null;
89+
90+
@override
91+
Null get end => null;
92+
93+
@override
94+
bool contains(DateTime time) => true;
95+
}
96+
97+
/// A specific time range, which includes at _least_ [start] or [end], or both.
98+
final class SpecificTimeRange extends TimeRange {
99+
/// Creates a time range with a specific [end] time.
100+
///
101+
/// If [exclusive] is set, the specific moment described in [end] is excluded.
102+
SpecificTimeRange.before(
103+
DateTime this.end, { //
104+
this.exclusive = false,
105+
}) : start = null;
106+
107+
/// Creates a time range with a specific [start] time.
108+
///
109+
/// If [exclusive] is set, the specific moment described in [start] is
110+
/// excluded.
111+
SpecificTimeRange.after(
112+
DateTime this.start, { //
113+
this.exclusive = false,
114+
}) : end = null;
115+
116+
/// Creates a time range with a specific [start] and [end] time.
117+
///
118+
/// If [exclusive] is set, the specific moment described in [start] and [end]
119+
/// are excluded.
120+
///
121+
/// [start] must be before [end].
122+
SpecificTimeRange.between(
123+
DateTime this.start, //
124+
DateTime this.end, {
125+
this.exclusive = false,
126+
}) {
127+
if (start!.isAfter(end!)) {
128+
throw ArgumentError.value(
129+
start,
130+
'start',
131+
'Start time must be before end time.',
132+
);
133+
}
134+
}
135+
136+
/// Whether [start], or [end], if specified, should exclude that instant.
137+
final bool exclusive;
138+
139+
/// Whether [start], or [end], if specified, should include that instant.
140+
bool get inclusive => !exclusive;
141+
142+
@override
143+
final DateTime? start;
144+
145+
@override
146+
final DateTime? end;
147+
148+
@override
149+
bool contains(DateTime time) {
150+
if (start case final start? when start.isAfter(time)) {
151+
return false;
152+
}
153+
if (end case final end? when end.isBefore(time)) {
154+
return false;
155+
}
156+
if (exclusive) {
157+
return time != start && time != end;
158+
}
159+
return true;
160+
}
161+
}

0 commit comments

Comments
 (0)