Skip to content

Commit a041d34

Browse files
WolfyUKWolfyUK
committed
feat: Refactored ValidateArgumentAttribute to consume new AbstractValidator instead of Regex
1 parent d6df591 commit a041d34

11 files changed

+433
-98
lines changed

‎web/src/Web.App/Attributes/ValidateArgumentAttribute.cs

Lines changed: 70 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1,47 +1,94 @@
1-
using System.Net;
2-
using System.Text.RegularExpressions;
1+
using FluentValidation;
32
using Microsoft.AspNetCore.Mvc;
43
using Microsoft.AspNetCore.Mvc.Filters;
4+
using Web.App.Validators;
55

66
namespace Web.App.Attributes;
77

88
[AttributeUsage(AttributeTargets.Class | AttributeTargets.Method)]
9-
public class ValidateArgumentAttribute(string argumentName, Regex regex) : ActionFilterAttribute
9+
public abstract class ValidateArgumentAttribute : TypeFilterAttribute
10+
{
11+
protected ValidateArgumentAttribute(string argumentName, string type) : this(argumentName, type, string.Empty)
12+
{
13+
}
14+
15+
protected ValidateArgumentAttribute(string argumentName, string type, string typeName) : base(typeof(ValidateArgumentFilter))
16+
{
17+
ArgumentName = argumentName;
18+
Type = type;
19+
TypeName = typeName;
20+
21+
// arguments list must match the `ValidateArgument` constructor arguments
22+
Arguments =
23+
[
24+
ArgumentName,
25+
Type,
26+
TypeName
27+
];
28+
}
29+
30+
internal string ArgumentName { get; }
31+
internal string Type { get; }
32+
internal string TypeName { get; }
33+
}
34+
35+
internal class ValidateArgumentFilter(
36+
ILogger<ValidateArgumentFilter> logger,
37+
IValidator<OrganisationIdentifier> validator,
38+
string argumentName,
39+
string type,
40+
string typeName) : ActionFilterAttribute
1041
{
1142
public override void OnActionExecuting(ActionExecutingContext context)
1243
{
13-
if (context.ActionArguments.TryGetValue(argumentName, out var value))
44+
if (context.ActionArguments.TryGetValue(argumentName, out var identifierValue))
1445
{
15-
var parsed = value?.ToString();
46+
var parsed = identifierValue?.ToString();
1647

1748
// Only attempt to validate if non-null or empty value. This _should_ always be the case,
1849
// otherwise ASP.NET action binding would fail and a `404` would be served by ASP.NET anyway.
19-
if (!string.IsNullOrWhiteSpace(parsed) && !regex.IsMatch(parsed))
50+
if (!string.IsNullOrWhiteSpace(parsed))
2051
{
21-
context.Result = new ViewResult
52+
var parsedType = type;
53+
if (string.IsNullOrWhiteSpace(parsedType) && !string.IsNullOrWhiteSpace(typeName))
54+
{
55+
if (context.ActionArguments.TryGetValue(typeName, out var typeValue))
56+
{
57+
parsedType = typeValue?.ToString();
58+
}
59+
}
60+
61+
// no type provided, so unable to validate identifier
62+
if (string.IsNullOrWhiteSpace(parsedType))
2263
{
23-
ViewName = "../Error/NotFound",
24-
StatusCode = (int)HttpStatusCode.BadRequest
25-
};
64+
logger.LogDebug("Unable to validate identifier {Identifier} because organisation type could not be resolved", parsed);
65+
context.Result = new NotFoundResult();
66+
}
67+
else
68+
{
69+
var identifier = new OrganisationIdentifier
70+
{
71+
Value = parsed,
72+
Type = parsedType
73+
};
74+
75+
var result = validator.Validate(identifier);
76+
77+
logger.LogDebug("Validation of identifier {Identifier} of type {Type} returned {ErrorCount} error(s)", identifier.Value, identifier.Type, result.Errors.Count);
78+
if (!result.IsValid)
79+
{
80+
context.Result = new NotFoundResult();
81+
}
82+
}
2683
}
2784
}
2885

2986
base.OnActionExecuting(context);
3087
}
3188
}
3289

33-
public static partial class ValidateArgumentAttributeRegex
34-
{
35-
[GeneratedRegex(@"^\d{6}$")]
36-
public static partial Regex UrnRegex();
37-
38-
[GeneratedRegex(@"^\d{8}$")]
39-
public static partial Regex CompanyNumberRegex();
90+
public class ValidateUrnAttribute() : ValidateArgumentAttribute("urn", OrganisationTypes.School);
4091

41-
[GeneratedRegex(@"^\d{3}$")]
42-
public static partial Regex LaCodeRegex();
43-
}
92+
public class ValidateCompanyNumberAttribute() : ValidateArgumentAttribute("companyNumber", OrganisationTypes.Trust);
4493

45-
public class ValidateUrnAttribute() : ValidateArgumentAttribute("urn", ValidateArgumentAttributeRegex.UrnRegex());
46-
public class ValidateCompanyNumberAttribute() : ValidateArgumentAttribute("companyNumber", ValidateArgumentAttributeRegex.CompanyNumberRegex());
47-
public class ValidateLaCodeAttribute() : ValidateArgumentAttribute("code", ValidateArgumentAttributeRegex.LaCodeRegex());
94+
public class ValidateLaCodeAttribute() : ValidateArgumentAttribute("code", OrganisationTypes.LocalAuthority);

‎web/src/Web.App/Extensions/ServiceCollectionExtensions.cs

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using System.Security.Claims;
2+
using FluentValidation;
23
using IdentityModel.Client;
34
using Microsoft.AspNetCore.Authentication;
45
using Microsoft.AspNetCore.Authentication.Cookies;
@@ -17,6 +18,7 @@
1718
using Web.App.Infrastructure.Apis.NonFinancial;
1819
using Web.App.Infrastructure.Storage;
1920
using Web.App.Services;
21+
using Web.App.Validators;
2022

2123
namespace Web.App.Extensions;
2224

@@ -291,8 +293,18 @@ private static AuthenticationBuilder AddDfeSignInOpenIdConnect(this Authenticati
291293
});
292294
}
293295

294-
public static IServiceCollection AddActionResults(this IServiceCollection services) => services
295-
.AddSingleton<IActionResultExecutor<CsvResult>, CsvResultActionResultExecutor>()
296-
.AddSingleton<IActionResultExecutor<CsvResults>, CsvResultsActionResultExecutor>()
297-
.AddSingleton<ICsvService, CsvService>();
296+
public static IServiceCollection AddActionResults(this IServiceCollection services)
297+
{
298+
return services
299+
.AddSingleton<IActionResultExecutor<CsvResult>, CsvResultActionResultExecutor>()
300+
.AddSingleton<IActionResultExecutor<CsvResults>, CsvResultsActionResultExecutor>()
301+
.AddSingleton<ICsvService, CsvService>();
302+
}
303+
304+
public static IServiceCollection AddValidation(this IServiceCollection services)
305+
{
306+
return services
307+
.AddScoped<IFinancialPlanStageValidator, FinancialPlanStageValidator>()
308+
.AddScoped<IValidator<OrganisationIdentifier>, OrganisationIdentifierValidator>();
309+
}
298310
}

‎web/src/Web.App/Program.cs

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616
using Web.App.Middleware;
1717
using Web.App.Services;
1818
using Web.App.Telemetry;
19-
using Web.App.Validators;
2019

2120
[assembly: InternalsVisibleTo("Web.Tests")]
2221

@@ -40,12 +39,12 @@
4039
.AddScoped<ISchoolComparatorSetService, SchoolComparatorSetService>()
4140
.AddScoped<ITrustComparatorSetService, TrustComparatorSetService>()
4241
.AddScoped<ISuggestService, SuggestService>()
43-
.AddScoped<IFinancialPlanStageValidator, FinancialPlanStageValidator>()
4442
.AddScoped<ICustomDataService, CustomDataService>()
4543
.AddScoped<IUserDataService, UserDataService>()
4644
.AddScoped<IClaimsIdentifierService, ClaimsIdentifierService>()
4745
.AddScoped<ILocalAuthorityComparatorSetService, LocalAuthorityComparatorSetService>()
4846
.AddScoped<ISearchService, SearchService>()
47+
.AddValidation()
4948
.AddActionResults();
5049

5150
builder.Services.AddHealthChecks()

‎web/src/Web.App/Validators/OrganisationIdentifierValidator.cs

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
using FluentValidation;
2+
3+
namespace Web.App.Validators;
4+
5+
public class OrganisationIdentifierValidator : AbstractValidator<OrganisationIdentifier>
6+
{
7+
private readonly string[] _organisationTypes = [OrganisationTypes.School, OrganisationTypes.Trust, OrganisationTypes.LocalAuthority];
8+
9+
public OrganisationIdentifierValidator()
10+
{
11+
RuleFor(p => p.Value)
12+
.NotEmpty()
13+
.WithMessage("Organisation identifier must be provided");
14+
15+
RuleFor(p => p.Type)
16+
.NotEmpty()
17+
.WithMessage("Organisation type must be provided");
18+
19+
RuleFor(p => p.Type)
20+
.Must(type => _organisationTypes.Contains(type))
21+
.When(p => p.Type != null)
22+
.WithMessage("Organisation type must be valid");
23+
24+
RuleFor(p => p.Value)
25+
.Length(6)
26+
.When(p => p.Type == OrganisationTypes.School)
27+
.When(p => p.ValueAsInt != null)
28+
.WithMessage("School URN must be 6 characters");
29+
30+
RuleFor(p => p.Value)
31+
.Length(8)
32+
.When(p => p.Type == OrganisationTypes.Trust)
33+
.When(p => p.ValueAsInt != null)
34+
.WithMessage("Trust company number must be 8 characters");
35+
36+
RuleFor(p => p.Value)
37+
.Length(3)
38+
.When(p => p.Type == OrganisationTypes.LocalAuthority)
39+
.When(p => p.ValueAsInt != null)
40+
.WithMessage("Local authority code must be 3 characters");
41+
42+
RuleFor(p => p.ValueAsInt)
43+
.NotEmpty()
44+
.When(p => p.Value != null)
45+
.WithMessage("Organisation identifier must be a number");
46+
}
47+
}
48+
49+
public class OrganisationIdentifier
50+
{
51+
public string? Value { get; init; }
52+
public string? Type { get; init; }
53+
internal int? ValueAsInt
54+
{
55+
get
56+
{
57+
if (int.TryParse(Value!, out var value))
58+
{
59+
return value;
60+
}
61+
62+
return null;
63+
}
64+
}
65+
}

‎web/tests/Web.Integration.Tests/Pages/LocalAuthorities/WhenViewingHome.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,14 +117,14 @@ public async Task CanDisplayNotFound()
117117
}
118118

119119
[Fact]
120-
public async Task CanDisplayBadRequest()
120+
public async Task CanDisplayNotFoundForBadIdentifier()
121121
{
122122
const string code = "1234";
123123
var page = await Client
124124
.Navigate(Paths.LocalAuthorityHome(code));
125125

126126
PageAssert.IsNotFoundPage(page);
127-
DocumentAssert.AssertPageUrl(page, Paths.LocalAuthorityHome(code).ToAbsolute(), HttpStatusCode.BadRequest);
127+
DocumentAssert.AssertPageUrl(page, Paths.LocalAuthorityHome(code).ToAbsolute(), HttpStatusCode.NotFound);
128128
}
129129

130130
[Fact]

‎web/tests/Web.Integration.Tests/Pages/Schools/WhenViewingDetails.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,14 +42,14 @@ public async Task CanDisplayNotFound()
4242
}
4343

4444
[Fact]
45-
public async Task CanDisplayBadRequest()
45+
public async Task CanDisplayNotFoundForBadIdentifier()
4646
{
4747
const string urn = nameof(urn);
4848
var page = await Client
4949
.Navigate(Paths.SchoolDetails(urn));
5050

5151
PageAssert.IsNotFoundPage(page);
52-
DocumentAssert.AssertPageUrl(page, Paths.SchoolDetails(urn).ToAbsolute(), HttpStatusCode.BadRequest);
52+
DocumentAssert.AssertPageUrl(page, Paths.SchoolDetails(urn).ToAbsolute(), HttpStatusCode.NotFound);
5353
}
5454

5555
[Fact]

‎web/tests/Web.Integration.Tests/Pages/Trusts/WhenViewingDetails.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,14 +40,14 @@ public async Task CanDisplayNotFound()
4040
}
4141

4242
[Fact]
43-
public async Task CanDisplayBadRequest()
43+
public async Task CanDisplayNotFoundForBadIdentifier()
4444
{
4545
const string companyName = nameof(companyName);
4646
var page = await Client
4747
.Navigate(Paths.TrustDetails(companyName));
4848

4949
PageAssert.IsNotFoundPage(page);
50-
DocumentAssert.AssertPageUrl(page, Paths.TrustDetails(companyName).ToAbsolute(), HttpStatusCode.BadRequest);
50+
DocumentAssert.AssertPageUrl(page, Paths.TrustDetails(companyName).ToAbsolute(), HttpStatusCode.NotFound);
5151
}
5252

5353
[Fact]

0 commit comments

Comments
 (0)