Skip to content

Commit 280d2b0

Browse files
ramonsmitsramonsmits
authored
Merge pull request #5477 from Particular/fix/failed-messages-sort-by-time-of-failure
Fix failed-message list sort by "Time of failure"
2 parents 4a0a50f + c433ffa commit 280d2b0

7 files changed

Lines changed: 168 additions & 44 deletions

File tree

‎src/ServiceControl.Audit.Persistence/Infrastructure/SortInfo.cs‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
namespace ServiceControl.Audit.Infrastructure
22
{
3+
using System.Collections.Frozen;
4+
using System.Collections.Generic;
5+
36
public class SortInfo
47
{
58
public string Direction { get; }
@@ -10,5 +13,22 @@ public SortInfo(string sort, string direction)
1013
Sort = sort;
1114
Direction = direction;
1215
}
16+
17+
// Single source of truth for the audit sort tokens the API accepts.
18+
// Consumed by SortInfoModelBinder so the web allowlist cannot drift
19+
// from the fields the audit message query actually sorts by (anything
20+
// not listed here falls back to TimeSent in the persistence layer).
21+
public static readonly FrozenSet<string> AllowedSortOptions = new HashSet<string>
22+
{
23+
"id",
24+
"message_id",
25+
"message_type",
26+
"status",
27+
"time_sent",
28+
"processed_at",
29+
"critical_time",
30+
"delivery_time",
31+
"processing_time"
32+
}.ToFrozenSet();
1333
}
1434
}

‎src/ServiceControl.Audit.UnitTests/Infrastructure/WebApi/SortInfoModelBinderTests.cs‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
namespace ServiceControl.UnitTests.Infrastructure.WebApi;
2+
3+
using System.Collections.Generic;
4+
using System.Threading.Tasks;
5+
using Microsoft.AspNetCore.Http;
6+
using Microsoft.AspNetCore.Mvc;
7+
using Microsoft.AspNetCore.Mvc.ModelBinding;
8+
using Microsoft.Extensions.Primitives;
9+
using NUnit.Framework;
10+
using ServiceControl.Audit.Infrastructure;
11+
using ServiceControl.Audit.Infrastructure.WebApi;
12+
13+
[TestFixture]
14+
public class SortInfoModelBinderTests
15+
{
16+
[TestCase("processed_at")]
17+
[TestCase("critical_time")]
18+
[TestCase("message_type")]
19+
public async Task Preserves_valid_audit_sort_field(string sort)
20+
{
21+
var sortInfo = await Bind(sort, "asc");
22+
23+
Assert.That(sortInfo.Sort, Is.EqualTo(sort));
24+
}
25+
26+
[Test]
27+
public async Task Falls_back_to_time_sent_for_unknown_sort_field()
28+
{
29+
var sortInfo = await Bind("not_a_real_field", "asc");
30+
31+
Assert.That(sortInfo.Sort, Is.EqualTo("time_sent"));
32+
}
33+
34+
static async Task<SortInfo> Bind(string sort, string direction)
35+
{
36+
var httpContext = new DefaultHttpContext
37+
{
38+
Request =
39+
{
40+
Query = new QueryCollection(new Dictionary<string, StringValues>
41+
{
42+
["sort"] = sort,
43+
["direction"] = direction,
44+
})
45+
}
46+
};
47+
48+
var bindingContext = new DefaultModelBindingContext
49+
{
50+
ActionContext = new ActionContext { HttpContext = httpContext },
51+
ModelName = "sortInfo",
52+
};
53+
54+
await new SortInfoModelBinder().BindModelAsync(bindingContext);
55+
56+
return (SortInfo)bindingContext.Result.Model;
57+
}
58+
}

‎src/ServiceControl.Audit/Infrastructure/WebApi/SortInfoModelBinder.cs‎

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
namespace ServiceControl.Audit.Infrastructure.WebApi;
22

33
using System;
4-
using System.Collections.Frozen;
5-
using System.Collections.Generic;
64
using System.Threading.Tasks;
75
using Microsoft.AspNetCore.Mvc.ModelBinding;
86

@@ -21,25 +19,12 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
2119

2220
bindingContext.HttpContext.Request.Query.TryGetValue("sort", out var sortValue);
2321
var sort = sortValue.ToString();
24-
if (!AllowableSortOptions.Contains(sort))
22+
if (!SortInfo.AllowedSortOptions.Contains(sort))
2523
{
2624
sort = "time_sent";
2725
}
2826

2927
bindingContext.Result = ModelBindingResult.Success(new SortInfo(sort, direction));
3028
return Task.CompletedTask;
3129
}
32-
33-
static readonly FrozenSet<string> AllowableSortOptions = new HashSet<string>
34-
{
35-
"processed_at",
36-
"id",
37-
"message_type",
38-
"time_sent",
39-
"critical_time",
40-
"delivery_time",
41-
"processing_time",
42-
"status",
43-
"message_id"
44-
}.ToFrozenSet();
4530
}

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

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ public static IAsyncDocumentQuery<TSource> Sort<TSource>(this IAsyncDocumentQuer
8282
}
8383

8484
var sort = sortInfo.Sort;
85-
if (!AsyncDocumentQuerySortOptions.Contains(sort))
85+
if (!SortInfo.AllowedSortOptions.Contains(sort))
8686
{
8787
sort = "time_sent";
8888
}
@@ -207,17 +207,6 @@ public static IAsyncDocumentQuery<T> FilterByQueueAddress<T>(this IAsyncDocument
207207
return source;
208208
}
209209

210-
static HashSet<string> AsyncDocumentQuerySortOptions =
211-
[
212-
"id",
213-
"message_id",
214-
"message_type",
215-
"time_sent",
216-
"status",
217-
"modified",
218-
"time_of_failure"
219-
];
220-
221210
static string[] SplitChars =
222211
{
223212
"..."

‎src/ServiceControl.Persistence/Infrastructure/SortInfo.cs‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,36 @@
11
namespace ServiceControl.Persistence.Infrastructure
22
{
3+
using System.Collections.Frozen;
4+
using System.Collections.Generic;
35
using System.Diagnostics;
46

57
[DebuggerDisplay("{Sort} {Direction}")]
68
public class SortInfo(string sort = null, string direction = null)
79
{
810
public string Direction { get; } = string.IsNullOrWhiteSpace(direction) ? "desc" : direction;
911
public string Sort { get; } = string.IsNullOrWhiteSpace(sort) ? "time_sent" : sort;
12+
13+
// Single source of truth for the sort tokens the API accepts. The model
14+
// binder gates incoming requests against this set and the persistence
15+
// layer resolves each accepted token to an index field per endpoint
16+
// (anything it does not recognise falls back to TimeSent). It is the
17+
// union of the fields sortable by the message endpoints (processed_at,
18+
// critical_time, delivery_time, processing_time) and the failed-message
19+
// endpoints (time_of_failure, modified), so the web allowlist and the
20+
// persistence query can no longer drift apart.
21+
public static readonly FrozenSet<string> AllowedSortOptions = new HashSet<string>
22+
{
23+
"id",
24+
"message_id",
25+
"message_type",
26+
"status",
27+
"time_sent",
28+
"modified",
29+
"time_of_failure",
30+
"processed_at",
31+
"critical_time",
32+
"delivery_time",
33+
"processing_time"
34+
}.ToFrozenSet();
1035
}
1136
}

‎src/ServiceControl.UnitTests/Infrastructure/WebApi/SortInfoModelBinderTests.cs‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
namespace ServiceControl.UnitTests.Infrastructure.WebApi
2+
{
3+
using System.Collections.Generic;
4+
using System.Threading.Tasks;
5+
using Microsoft.AspNetCore.Http;
6+
using Microsoft.AspNetCore.Mvc;
7+
using Microsoft.AspNetCore.Mvc.ModelBinding;
8+
using Microsoft.Extensions.Primitives;
9+
using NUnit.Framework;
10+
using Persistence.Infrastructure;
11+
using ServiceControl.Infrastructure.WebApi;
12+
13+
[TestFixture]
14+
public class SortInfoModelBinderTests
15+
{
16+
// Failed-message endpoints (/api/errors) support these sort fields in the
17+
// RavenDB layer. The model binder must not silently rewrite them away.
18+
[TestCase("time_of_failure")]
19+
[TestCase("modified")]
20+
[TestCase("message_type")]
21+
[TestCase("processed_at")]
22+
public async Task Preserves_valid_failed_message_sort_field(string sort)
23+
{
24+
var sortInfo = await Bind(sort, "asc");
25+
26+
Assert.That(sortInfo.Sort, Is.EqualTo(sort));
27+
}
28+
29+
[Test]
30+
public async Task Falls_back_to_time_sent_for_unknown_sort_field()
31+
{
32+
var sortInfo = await Bind("not_a_real_field", "asc");
33+
34+
Assert.That(sortInfo.Sort, Is.EqualTo("time_sent"));
35+
}
36+
37+
static async Task<SortInfo> Bind(string sort, string direction)
38+
{
39+
var httpContext = new DefaultHttpContext
40+
{
41+
Request =
42+
{
43+
Query = new QueryCollection(new Dictionary<string, StringValues>
44+
{
45+
["sort"] = sort,
46+
["direction"] = direction,
47+
})
48+
}
49+
};
50+
51+
var bindingContext = new DefaultModelBindingContext
52+
{
53+
ActionContext = new ActionContext { HttpContext = httpContext },
54+
ModelName = "sortInfo",
55+
};
56+
57+
await new SortInfoModelBinder().BindModelAsync(bindingContext);
58+
59+
return (SortInfo)bindingContext.Result.Model;
60+
}
61+
}
62+
}

‎src/ServiceControl/Infrastructure/WebApi/SortInfoModelBinder.cs‎

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,6 @@
11
namespace ServiceControl.Infrastructure.WebApi;
22

33
using System;
4-
using System.Collections.Frozen;
5-
using System.Collections.Generic;
64
using System.Threading.Tasks;
75
using Microsoft.AspNetCore.Mvc.ModelBinding;
86
using Persistence.Infrastructure;
@@ -22,25 +20,12 @@ public Task BindModelAsync(ModelBindingContext bindingContext)
2220

2321
bindingContext.HttpContext.Request.Query.TryGetValue("sort", out var sortValue);
2422
var sort = sortValue.ToString();
25-
if (!AllowableSortOptions.Contains(sort))
23+
if (!SortInfo.AllowedSortOptions.Contains(sort))
2624
{
2725
sort = "time_sent";
2826
}
2927

3028
bindingContext.Result = ModelBindingResult.Success(new SortInfo(sort, direction));
3129
return Task.CompletedTask;
3230
}
33-
34-
static readonly FrozenSet<string> AllowableSortOptions = new HashSet<string>
35-
{
36-
"processed_at",
37-
"id",
38-
"message_type",
39-
"time_sent",
40-
"critical_time",
41-
"delivery_time",
42-
"processing_time",
43-
"status",
44-
"message_id"
45-
}.ToFrozenSet();
4631
}

0 commit comments

Comments
 (0)