Skip to content

Commit 01fceff

Browse files
johnsimonsjohnsimons
committed
Better naming of methods
The two by-id lookups differed only in that one reads unresolved groups and the other archived ones, which neither the names nor the signatures showed. The four status-scoped members are now named after what they return: GetUnresolvedGroupsByClassifier, GetArchivedGroupsByClassifier, GetUnresolvedGroup and GetArchivedGroup. GetUnresolvedGroup returns a single view like its archived twin, so neither controller picks the first result off a list any more. GetCurrentForwardingBatch moves to IRetryDocumentDataStore, where the NowForwarding pointer it reads already lives. QueryFailureGroupViewOnGroupId is deleted because it ran the same query as GetUnresolvedGroup, so RetryAllInGroupHandler calls that instead. Both group-by-id endpoints now return 404 for an unknown group instead of an empty 204, matching GetErrorByIdController. ServicePulse reads these only to label a group, and its client already fails on the empty 204, so this gives it a status it can actually handle.
1 parent f89cee4 commit 01fceff

11 files changed

Lines changed: 59 additions & 81 deletions

File tree

‎src/ServiceControl.Persistence.EFCore/Implementation/GroupsDataStore.cs‎

Lines changed: 14 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ namespace ServiceControl.Persistence.EFCore.Implementation;
1212

1313
public class GroupsDataStore(IServiceScopeFactory scopeFactory) : DataStoreBase(scopeFactory), IGroupsDataStore
1414
{
15-
public Task<IList<FailureGroupView>> GetFailureGroupsByClassifier(string classifier, string classifierFilter) =>
15+
public Task<IList<FailureGroupView>> GetUnresolvedGroupsByClassifier(string classifier, string classifierFilter) =>
1616
ExecuteWithDbContext(dbContext =>
1717
{
1818
var groups = ByClassifier(dbContext, classifier);
@@ -25,30 +25,15 @@ public Task<IList<FailureGroupView>> GetFailureGroupsByClassifier(string classif
2525
return MostRecent(groups.AggregateGroups(WithStatus(dbContext, FailedMessageStatus.Unresolved)));
2626
});
2727

28-
public Task<IList<FailureGroupView>> GetArchivedFailureGroupsByClassifier(string classifier) =>
28+
public Task<IList<FailureGroupView>> GetArchivedGroupsByClassifier(string classifier) =>
2929
ExecuteWithDbContext(dbContext => MostRecent(
3030
ByClassifier(dbContext, classifier).AggregateGroups(WithStatus(dbContext, FailedMessageStatus.Archived))));
3131

32-
// Implemented once retry batches are persisted, together with IRetryDocumentDataStore.
33-
public Task<RetryBatch> GetCurrentForwardingBatch() =>
34-
throw new NotImplementedException();
35-
36-
public Task<QueryResult<IList<FailureGroupView>>> GetGroup(string groupId, string status, string modified) =>
37-
ExecuteWithDbContext(async dbContext =>
38-
{
39-
var groups = await ById(dbContext, groupId, FailedMessageStatus.Unresolved, status, modified).ToListAsync();
32+
public Task<QueryResult<FailureGroupView>> GetUnresolvedGroup(string groupId, string status, string modified) =>
33+
ExecuteWithDbContext(dbContext => SingleGroup(dbContext, groupId, FailedMessageStatus.Unresolved, status, modified));
4034

41-
return new QueryResult<IList<FailureGroupView>>(groups, groups.ToQueryStatsInfo());
42-
});
43-
44-
public Task<QueryResult<FailureGroupView>> GetFailureGroupView(string groupId, string status, string modified) =>
45-
ExecuteWithDbContext(async dbContext =>
46-
{
47-
var groups = await ById(dbContext, groupId, FailedMessageStatus.Archived, status, modified).ToListAsync();
48-
49-
// A missing group is reported as a null result, the same as the RavenDB persister does.
50-
return new QueryResult<FailureGroupView>(groups.FirstOrDefault()!, groups.ToQueryStatsInfo());
51-
});
35+
public Task<QueryResult<FailureGroupView>> GetArchivedGroup(string groupId, string status, string modified) =>
36+
ExecuteWithDbContext(dbContext => SingleGroup(dbContext, groupId, FailedMessageStatus.Archived, status, modified));
5237

5338
public Task<QueryResult<IList<FailedMessageView>>> GetGroupErrors(string groupId, string status, string modified, SortInfo sortInfo, PagingInfo pagingInfo) =>
5439
ExecuteWithDbContext(dbContext => InGroup(dbContext, groupId, status, modified).ToPagedResult(pagingInfo, sortInfo));
@@ -67,18 +52,18 @@ static IQueryable<FailedMessageGroupEntity> ByClassifier(ServiceControlDbContext
6752
.AsNoTracking()
6853
.Where(group => group.Type == classifier);
6954

70-
/// <summary>
71-
/// The status a group is read at, before the caller's own status and modified filters narrow it
72-
/// further. RavenDB reads open groups out of an unresolved-only index and archived groups out of
73-
/// an archived-only one, which is what <paramref name="baseline" /> stands in for here.
74-
/// </summary>
75-
static IQueryable<FailureGroupView> ById(ServiceControlDbContext dbContext, string groupId, FailedMessageStatus baseline, string status, string modified) =>
76-
dbContext.FailedMessageGroups
55+
static async Task<QueryResult<FailureGroupView>> SingleGroup(ServiceControlDbContext dbContext, string groupId, FailedMessageStatus baseline, string status, string modified)
56+
{
57+
var groups = await dbContext.FailedMessageGroups
7758
.AsNoTracking()
7859
.Where(group => group.GroupId == groupId)
7960
.AggregateGroups(WithStatus(dbContext, baseline)
8061
.FilterByStatus(status)
81-
.FilterByLastModifiedRange(modified));
62+
.FilterByLastModifiedRange(modified))
63+
.ToListAsync();
64+
65+
return new QueryResult<FailureGroupView>(groups.FirstOrDefault()!, groups.ToQueryStatsInfo());
66+
}
8267

8368
static IQueryable<FailedMessageEntity> WithStatus(ServiceControlDbContext dbContext, FailedMessageStatus status) =>
8469
dbContext.FailedMessages

‎src/ServiceControl.Persistence.EFCore/Implementation/RetryDocumentDataStore.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,6 @@ public Task GetBatchesForFailedQueueAddress(DateTime cutoff, string failedQueueA
3636
public Task GetBatchesForFailureGroup(string groupId, string groupTitle, string groupType, DateTime cutoff, Func<string, DateTime, Task> callback) =>
3737
throw new NotImplementedException();
3838

39-
public Task<FailureGroupView> QueryFailureGroupViewOnGroupId(string groupId) =>
39+
public Task<RetryBatch> GetCurrentForwardingBatch() =>
4040
throw new NotImplementedException();
4141
}

‎src/ServiceControl.Persistence.RavenDB/Recoverability/GroupsDataStore.cs‎

Lines changed: 7 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ namespace ServiceControl.Persistence.RavenDB.Recoverability
1414

1515
class GroupsDataStore(IRavenSessionProvider sessionProvider) : IGroupsDataStore
1616
{
17-
public async Task<IList<FailureGroupView>> GetFailureGroupsByClassifier(string classifier, string classifierFilter)
17+
public async Task<IList<FailureGroupView>> GetUnresolvedGroupsByClassifier(string classifier, string classifierFilter)
1818
{
1919
using var session = await sessionProvider.OpenSession();
2020
var query = Queryable.Where(session.Query<FailureGroupView, FailureGroupsViewIndex>(), v => v.Type == classifier);
@@ -40,7 +40,7 @@ public async Task<IList<FailureGroupView>> GetFailureGroupsByClassifier(string c
4040
return groups;
4141
}
4242

43-
public async Task<IList<FailureGroupView>> GetArchivedFailureGroupsByClassifier(string classifier)
43+
public async Task<IList<FailureGroupView>> GetArchivedGroupsByClassifier(string classifier)
4444
{
4545
using var session = await sessionProvider.OpenSession();
4646
var groups = session
@@ -55,30 +55,21 @@ public async Task<IList<FailureGroupView>> GetArchivedFailureGroupsByClassifier(
5555
return results;
5656
}
5757

58-
public async Task<RetryBatch> GetCurrentForwardingBatch()
58+
public async Task<QueryResult<FailureGroupView>> GetUnresolvedGroup(string groupId, string status, string modified)
5959
{
6060
using var session = await sessionProvider.OpenSession();
61-
var nowForwarding = await session.Include<RetryBatchNowForwarding, RetryBatch>(r => r.RetryBatchId)
62-
.LoadAsync<RetryBatchNowForwarding>(RetryDocumentDataStore.NowForwardingDocumentId);
63-
64-
return nowForwarding == null ? null : await session.LoadAsync<RetryBatch>(nowForwarding.RetryBatchId);
65-
}
66-
67-
public async Task<QueryResult<IList<FailureGroupView>>> GetGroup(string groupId, string status, string modified)
68-
{
69-
using var session = await sessionProvider.OpenSession();
70-
var queryResult = await session.Advanced
61+
var document = await session.Advanced
7162
.AsyncDocumentQuery<FailureGroupView, FailureGroupsViewIndex>()
7263
.Statistics(out var stats)
7364
.WhereEquals(group => group.Id, groupId)
7465
.FilterByStatusWhere(status)
7566
.FilterByLastModifiedRange(modified)
76-
.ToListAsync();
67+
.FirstOrDefaultAsync();
7768

78-
return queryResult.ToQueryResult(stats);
69+
return new QueryResult<FailureGroupView>(document, stats.ToQueryStatsInfo());
7970
}
8071

81-
public async Task<QueryResult<FailureGroupView>> GetFailureGroupView(string groupId, string status, string modified)
72+
public async Task<QueryResult<FailureGroupView>> GetArchivedGroup(string groupId, string status, string modified)
8273
{
8374
using var session = await sessionProvider.OpenSession();
8475
var document = await session.Advanced

‎src/ServiceControl.Persistence.RavenDB/RetryDocumentDataStore.cs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -200,12 +200,13 @@ public async Task GetBatchesForFailureGroup(string groupId, string groupTitle, s
200200
}
201201
}
202202

203-
public async Task<FailureGroupView> QueryFailureGroupViewOnGroupId(string groupId)
203+
public async Task<RetryBatch> GetCurrentForwardingBatch()
204204
{
205205
using var session = await sessionProvider.OpenSession();
206-
var group = await session.Query<FailureGroupView, FailureGroupsViewIndex>()
207-
.FirstOrDefaultAsync(x => x.Id == groupId);
208-
return group;
206+
var nowForwarding = await session.Include<RetryBatchNowForwarding, RetryBatch>(r => r.RetryBatchId)
207+
.LoadAsync<RetryBatchNowForwarding>(NowForwardingDocumentId);
208+
209+
return nowForwarding == null ? null : await session.LoadAsync<RetryBatch>(nowForwarding.RetryBatchId);
209210
}
210211

211212
public static string MakeDocumentId(string messageUniqueId) => "RetryBatches/" + messageUniqueId;

‎src/ServiceControl.Persistence.Tests/Recoverability/GroupsDataStoreTests.cs‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ await Insert(
2525
InGroup(group, failedAt: Noon),
2626
InGroup(group, failedAt: Noon.AddHours(2)));
2727

28-
var view = (await GroupsStore.GetFailureGroupsByClassifier(Classifier, null)).Single();
28+
var view = (await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, null)).Single();
2929

3030
using (Assert.EnterMultipleScope())
3131
{
@@ -46,7 +46,7 @@ public async Task Returns_only_the_groups_of_the_requested_classifier()
4646

4747
await Insert(InGroup(requested), InGroup(other));
4848

49-
var groups = await GroupsStore.GetFailureGroupsByClassifier(Classifier, null);
49+
var groups = await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, null);
5050

5151
Assert.That(groups.Select(group => group.Id), Is.EqualTo(new[] { requested.Id }));
5252
}
@@ -59,7 +59,7 @@ public async Task Narrows_the_groups_to_the_classifier_filter()
5959

6060
await Insert(InGroup(matching), InGroup(other));
6161

62-
var groups = await GroupsStore.GetFailureGroupsByClassifier(Classifier, "OrderPlaced");
62+
var groups = await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, "OrderPlaced");
6363

6464
Assert.That(groups.Select(group => group.Id), Is.EqualTo(new[] { matching.Id }));
6565
}
@@ -74,7 +74,7 @@ await Insert(
7474
InGroup(group).ToFailedMessage(FailedMessageStatus.Archived),
7575
InGroup(group).ToFailedMessage(FailedMessageStatus.Resolved));
7676

77-
var view = (await GroupsStore.GetFailureGroupsByClassifier(Classifier, null)).Single();
77+
var view = (await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, null)).Single();
7878

7979
Assert.That(view.Count, Is.EqualTo(1));
8080
}
@@ -88,7 +88,7 @@ await Insert(
8888
InGroup(group).ToFailedMessage(),
8989
InGroup(group).ToFailedMessage(FailedMessageStatus.Archived));
9090

91-
var view = (await GroupsStore.GetArchivedFailureGroupsByClassifier(Classifier)).Single();
91+
var view = (await GroupsStore.GetArchivedGroupsByClassifier(Classifier)).Single();
9292

9393
using (Assert.EnterMultipleScope())
9494
{
@@ -109,7 +109,7 @@ await Insert(
109109
InGroup(newest, failedAt: Noon.AddHours(4)),
110110
InGroup(middle, failedAt: Noon.AddHours(2)));
111111

112-
var groups = await GroupsStore.GetFailureGroupsByClassifier(Classifier, null);
112+
var groups = await GroupsStore.GetUnresolvedGroupsByClassifier(Classifier, null);
113113

114114
Assert.That(groups.Select(group => group.Title), Is.EqualTo(new[] { "Newest", "Middle", "Oldest" }));
115115
}
@@ -121,9 +121,9 @@ public async Task Returns_a_single_group_by_id()
121121

122122
await Insert(InGroup(requested), InGroup(NewGroup("OrderCancelled")));
123123

124-
var result = await GroupsStore.GetGroup(requested.Id, null, null);
124+
var result = await GroupsStore.GetUnresolvedGroup(requested.Id, null, null);
125125

126-
var view = result.Results.Single();
126+
var view = result.Results;
127127

128128
using (Assert.EnterMultipleScope())
129129
{
@@ -137,9 +137,9 @@ public async Task Returns_no_group_for_an_unknown_id()
137137
{
138138
await Insert(InGroup(NewGroup("OrderPlaced")));
139139

140-
var result = await GroupsStore.GetGroup(Guid.NewGuid().ToString(), null, null);
140+
var result = await GroupsStore.GetUnresolvedGroup(Guid.NewGuid().ToString(), null, null);
141141

142-
Assert.That(result.Results, Is.Empty);
142+
Assert.That(result.Results, Is.Null);
143143
}
144144

145145
[Test]
@@ -149,7 +149,7 @@ public async Task Returns_an_archived_group_view_by_id()
149149

150150
await Insert(InGroup(group).ToFailedMessage(FailedMessageStatus.Archived));
151151

152-
var result = await GroupsStore.GetFailureGroupView(group.Id, null, null);
152+
var result = await GroupsStore.GetArchivedGroup(group.Id, null, null);
153153

154154
using (Assert.EnterMultipleScope())
155155
{
@@ -163,7 +163,7 @@ public async Task Returns_no_archived_group_view_for_an_unknown_id()
163163
{
164164
await Insert(InGroup(NewGroup("OrderPlaced")).ToFailedMessage(FailedMessageStatus.Archived));
165165

166-
var result = await GroupsStore.GetFailureGroupView(Guid.NewGuid().ToString(), null, null);
166+
var result = await GroupsStore.GetArchivedGroup(Guid.NewGuid().ToString(), null, null);
167167

168168
Assert.That(result.Results, Is.Null);
169169
}

‎src/ServiceControl.Persistence/IGroupsDataStore.cs‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,11 @@ namespace ServiceControl.Persistence
88

99
public interface IGroupsDataStore
1010
{
11-
Task<IList<FailureGroupView>> GetFailureGroupsByClassifier(string classifier, string classifierFilter);
12-
Task<IList<FailureGroupView>> GetArchivedFailureGroupsByClassifier(string classifier);
13-
Task<RetryBatch> GetCurrentForwardingBatch();
11+
Task<IList<FailureGroupView>> GetUnresolvedGroupsByClassifier(string classifier, string classifierFilter);
12+
Task<IList<FailureGroupView>> GetArchivedGroupsByClassifier(string classifier);
1413

15-
Task<QueryResult<IList<FailureGroupView>>> GetGroup(string groupId, string status, string modified);
16-
Task<QueryResult<FailureGroupView>> GetFailureGroupView(string groupId, string status, string modified);
14+
Task<QueryResult<FailureGroupView>> GetUnresolvedGroup(string groupId, string status, string modified);
15+
Task<QueryResult<FailureGroupView>> GetArchivedGroup(string groupId, string status, string modified);
1716
Task<QueryResult<IList<FailedMessageView>>> GetGroupErrors(string groupId, string status, string modified, SortInfo sortInfo, PagingInfo pagingInfo);
1817
Task<QueryStatsInfo> GetGroupErrorsCount(string groupId, string status, string modified);
1918

‎src/ServiceControl.Persistence/IRetryDocumentDataStore.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,13 @@ Task<string> CreateBatchDocument(string retrySessionId, string requestId, RetryT
2121
Task<QueryResult<IList<RetryBatch>>> QueryOrphanedBatches(string retrySessionId);
2222
Task<IList<RetryBatchGroup>> QueryAvailableBatches();
2323

24+
// GroupFetcher
25+
Task<RetryBatch> GetCurrentForwardingBatch();
26+
2427
// RetriesGateway
2528
Task GetBatchesForAll(DateTime cutoff, Func<string, DateTime, Task> callback);
2629
Task GetBatchesForEndpoint(DateTime cutoff, string endpoint, Func<string, DateTime, Task> callback);
2730
Task GetBatchesForFailedQueueAddress(DateTime cutoff, string failedQueueAddresspoint, FailedMessageStatus status, Func<string, DateTime, Task> callback);
2831
Task GetBatchesForFailureGroup(string groupId, string groupTitle, string groupType, DateTime cutoff, Func<string, DateTime, Task> callback);
29-
30-
// RetryAllInGroupHandler
31-
Task<FailureGroupView> QueryFailureGroupViewOnGroupId(string groupId);
3232
}
3333
}

‎src/ServiceControl/MessageFailures/Api/ArchiveMessagesController.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ await auditLog.AuditedOperation(user, MessageActionKind.Archive, Permissions.Err
4747
[HttpGet]
4848
public async Task<IActionResult> GetArchiveMessageGroups(string classifier = "Exception Type and Stack Trace")
4949
{
50-
var results = await dataStore.GetArchivedFailureGroupsByClassifier(classifier);
50+
var results = await dataStore.GetArchivedGroupsByClassifier(classifier);
5151

5252
Response.WithDeterministicEtag(EtagHelper.CalculateEtag(results));
5353

@@ -74,11 +74,11 @@ await auditLog.AuditedOperation(user, MessageActionKind.Archive, Permissions.Err
7474
[HttpGet]
7575
public async Task<ActionResult<FailureGroupView>> GetGroup(string groupId, string status = default, string modified = default)
7676
{
77-
var result = await dataStore.GetFailureGroupView(groupId, status, modified);
77+
var result = await dataStore.GetArchivedGroup(groupId, status, modified);
7878

7979
Response.WithEtag(result.QueryStats.ETag);
8080

81-
return result.Results;
81+
return result.Results == null ? NotFound() : result.Results;
8282
}
8383
}
8484
}

‎src/ServiceControl/Recoverability/API/FailureGroupsController.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -107,13 +107,13 @@ public async Task<RetryHistory> GetRetryHistory()
107107
[Authorize(Policy = Permissions.ErrorRecoverabilityGroupsView)]
108108
[Route("recoverability/groups/id/{groupId:required:minlength(1)}")]
109109
[HttpGet]
110-
public async Task<FailureGroupView> GetGroup(string groupId, string status = default, string modified = default)
110+
public async Task<ActionResult<FailureGroupView>> GetGroup(string groupId, string status = default, string modified = default)
111111
{
112-
var result = await store.GetGroup(groupId, status, modified);
112+
var result = await store.GetUnresolvedGroup(groupId, status, modified);
113113

114114
Response.WithEtag(result.QueryStats.ETag);
115115

116-
return result.Results.FirstOrDefault();
116+
return result.Results == null ? NotFound() : result.Results;
117117
}
118118
}
119119
}

‎src/ServiceControl/Recoverability/API/GroupFetcher.cs‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,18 @@
88

99
public class GroupFetcher
1010
{
11-
public GroupFetcher(IGroupsDataStore store, IRetryHistoryDataStore retryStore, RetryingManager retryingManager, IArchiveMessages archiver)
11+
public GroupFetcher(IGroupsDataStore store, IRetryHistoryDataStore retryStore, IRetryDocumentDataStore retryDocumentStore, RetryingManager retryingManager, IArchiveMessages archiver)
1212
{
1313
this.store = store;
1414
this.retryStore = retryStore;
15+
this.retryDocumentStore = retryDocumentStore;
1516
this.retryingManager = retryingManager;
1617
this.archiver = archiver;
1718
}
1819

1920
public async Task<GroupOperation[]> GetGroups(string classifier, string classifierFilter)
2021
{
21-
var dbGroups = await store.GetFailureGroupsByClassifier(classifier, classifierFilter);
22+
var dbGroups = await store.GetUnresolvedGroupsByClassifier(classifier, classifierFilter);
2223
var retryHistory = await retryStore.GetRetryHistory();
2324
var unacknowledgedRetries = retryHistory.GetUnacknowledgedByClassifier(classifier);
2425

@@ -32,7 +33,7 @@ public async Task<GroupOperation[]> GetGroups(string classifier, string classifi
3233
openGroups = MapOpenGroups(openGroups, archiver.GetArchivalOperations()).ToList();
3334
openGroups = openGroups.Where(group => !closedGroups.Any(closedGroup => closedGroup.Id == group.Id)).ToList();
3435

35-
var currentForwardingBatch = await store.GetCurrentForwardingBatch();
36+
var currentForwardingBatch = await retryDocumentStore.GetCurrentForwardingBatch();
3637
MakeSureForwardingBatchIsIncludedAsOpen(classifier, currentForwardingBatch, openGroups);
3738

3839
var groups = openGroups.Union(closedGroups);
@@ -190,6 +191,7 @@ static HistoricRetryOperation GetLatestHistoricOperation(RetryHistory history, s
190191

191192
readonly IGroupsDataStore store;
192193
readonly IRetryHistoryDataStore retryStore;
194+
readonly IRetryDocumentDataStore retryDocumentStore;
193195
readonly RetryingManager retryingManager;
194196
readonly IArchiveMessages archiver;
195197
}

0 commit comments

Comments
 (0)