From 2e2c0e2a742184047edc8f320b897d344f13e98a Mon Sep 17 00:00:00 2001 From: George Pollard Date: Wed, 31 Aug 2022 22:20:30 +0000 Subject: [PATCH] Add test and [Required] attributes --- .../ApiService/OneFuzzTypes/Requests.cs | 135 +++++++++--------- src/ApiService/Tests/RequestsTests.cs | 45 +++++- 2 files changed, 113 insertions(+), 67 deletions(-) diff --git a/src/ApiService/ApiService/OneFuzzTypes/Requests.cs b/src/ApiService/ApiService/OneFuzzTypes/Requests.cs index 945678d6c3d..405ca1463d1 100644 --- a/src/ApiService/ApiService/OneFuzzTypes/Requests.cs +++ b/src/ApiService/ApiService/OneFuzzTypes/Requests.cs @@ -10,25 +10,25 @@ public record BaseRequest { }; public record CanScheduleRequest( - Guid MachineId, - Guid TaskId + [property: Required] Guid MachineId, + [property: Required] Guid TaskId ) : BaseRequest; public record NodeCommandGet( - Guid MachineId + [property: Required] Guid MachineId ) : BaseRequest; public record NodeCommandDelete( - Guid MachineId, - string MessageId + [property: Required] Guid MachineId, + [property: Required] string MessageId ) : BaseRequest; public record NodeGet( - Guid MachineId + [property: Required] Guid MachineId ) : BaseRequest; public record NodeUpdate( - Guid MachineId, + [property: Required] Guid MachineId, bool? DebugKeepNode ) : BaseRequest; @@ -40,8 +40,8 @@ public record NodeSearch( ) : BaseRequest; public record NodeStateEnvelope( - NodeEventBase Event, - Guid MachineId + [property: Required] NodeEventBase Event, + [property: Required] Guid MachineId ) : BaseRequest; // either NodeEvent or WorkerEvent @@ -63,16 +63,16 @@ public record WorkerEvent( ) : NodeEventBase; public record WorkerRunningEvent( - Guid TaskId); + [property: Required] Guid TaskId); public record WorkerDoneEvent( - Guid TaskId, - ExitStatus ExitStatus, - string Stderr, - string Stdout); + [property: Required] Guid TaskId, + [property: Required] ExitStatus ExitStatus, + [property: Required] string Stderr, + [property: Required] string Stdout); public record NodeStateUpdate( - NodeState State, + [property: Required] NodeState State, [property: JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] NodeStateData? Data = null ) : NodeEventBase; @@ -82,7 +82,7 @@ public record NodeStateUpdate( public abstract record NodeStateData; public record NodeSettingUpEventData( - List Tasks + [property: Required] List Tasks ) : NodeStateData; public record NodeDoneEventData( @@ -105,23 +105,23 @@ public record ExitStatus( bool Success); public record ContainerGet( - Container Name + [property: Required] Container Name ) : BaseRequest; public record ContainerCreate( - Container Name, + [property: Required] Container Name, IDictionary? Metadata = null ) : BaseRequest; public record ContainerDelete( - Container Name, + [property: Required] Container Name, IDictionary? Metadata = null ) : BaseRequest; public record NotificationCreate( - Container Container, - bool ReplaceExisting, - NotificationTemplate Config + [property: Required] Container Container, + [property: Required] bool ReplaceExisting, + [property: Required] NotificationTemplate Config ) : BaseRequest; public record NotificationSearch( @@ -129,18 +129,18 @@ public record NotificationSearch( ) : BaseRequest; public record NotificationGet( - Guid NotificationId + [property: Required] Guid NotificationId ) : BaseRequest; public record JobGet( - Guid JobId + [property: Required] Guid JobId ) : BaseRequest; public record JobCreate( - string Project, - string Name, - string Build, - long Duration, + [property: Required] string Project, + [property: Required] string Name, + [property: Required] string Build, + [property: Required] long Duration, string? Logs ) : BaseRequest; @@ -151,14 +151,17 @@ public record JobSearch( bool? WithTasks = null ) : BaseRequest; -public record NodeAddSshKeyPost(Guid MachineId, string PublicKey) : BaseRequest; +public record NodeAddSshKeyPost( + [property: Required] Guid MachineId, + [property: Required] string PublicKey +) : BaseRequest; public record ReproGet(Guid? VmId) : BaseRequest; public record ReproCreate( - Container Container, - string Path, - long Duration + [property: Required] Container Container, + [property: Required] string Path, + [property: Required] long Duration ) : BaseRequest; public record ProxyGet( @@ -168,31 +171,30 @@ public record ProxyGet( ) : BaseRequest; public record ProxyCreate( - Guid ScalesetId, - Guid MachineId, - int DstPort, - int Duration + [property: Required] Guid ScalesetId, + [property: Required] Guid MachineId, + [property: Required] int DstPort, + [property: Required] int Duration ) : BaseRequest; public record ProxyDelete( - Guid ScalesetId, - Guid MachineId, + [property: Required] Guid ScalesetId, + [property: Required] Guid MachineId, int? DstPort ) : BaseRequest; public record ProxyReset( - string Region + [property: Required] string Region ) : BaseRequest; public record ScalesetCreate( - PoolName PoolName, - string VmSku, - string Image, + [property: Required] PoolName PoolName, + [property: Required] string VmSku, + [property: Required] string Image, string? Region, - [property: Range(1, long.MaxValue)] - long Size, - bool SpotInstances, - Dictionary Tags, + [property: Range(1, long.MaxValue), Required] long Size, + [property: Required] bool SpotInstances, + [property: Required] Dictionary Tags, bool EphemeralOsDisks = false, AutoScaleOptions? AutoScale = null ) : BaseRequest; @@ -214,24 +216,25 @@ public record ScalesetSearch( ) : BaseRequest; public record ScalesetStop( - Guid ScalesetId, - bool Now + [property: Required] Guid ScalesetId, + [property: Required] bool Now ) : BaseRequest; public record ScalesetUpdate( - Guid ScalesetId, + [property: Required] Guid ScalesetId, [property: Range(1, long.MaxValue)] long? Size ) : BaseRequest; -public record TaskGet(Guid TaskId) : BaseRequest; +public record TaskGet( + [property: Required] Guid TaskId +) : BaseRequest; public record TaskCreate( - Guid JobId, + [property: Required] Guid JobId, List? PrereqTasks, - TaskDetails Task, - [property: Required] - TaskPool Pool, + [property: Required] TaskDetails Task, + [property: Required] TaskPool Pool, List? Containers = null, Dictionary? Tags = null, List? Debug = null, @@ -241,7 +244,7 @@ public record TaskCreate( public record TaskSearch( Guid? JobId, Guid? TaskId, - List State) : BaseRequest; + [property: Required] List State) : BaseRequest; public record PoolSearch( Guid? PoolId = null, @@ -250,32 +253,32 @@ public record PoolSearch( ) : BaseRequest; public record PoolStop( - PoolName Name, - bool Now + [property: Required] PoolName Name, + [property: Required] bool Now ) : BaseRequest; public record PoolCreate( - PoolName Name, - Os Os, - Architecture Arch, - bool Managed, + [property: Required] PoolName Name, + [property: Required] Os Os, + [property: Required] Architecture Arch, + [property: Required] bool Managed, Guid? ClientId = null ) : BaseRequest; public record WebhookCreate( - string Name, - Uri Url, - List EventTypes, + [property: Required] string Name, + [property: Required] Uri Url, + [property: Required] List EventTypes, string? SecretToken, WebhookMessageFormat? MessageFormat ) : BaseRequest; public record WebhookSearch(Guid? WebhookId) : BaseRequest; -public record WebhookGet(Guid WebhookId) : BaseRequest; +public record WebhookGet([property: Required] Guid WebhookId) : BaseRequest; public record WebhookUpdate( - Guid WebhookId, + [property: Required] Guid WebhookId, string? Name, Uri? Url, List? EventTypes, diff --git a/src/ApiService/Tests/RequestsTests.cs b/src/ApiService/Tests/RequestsTests.cs index b79c44e9863..dc7637e5c48 100644 --- a/src/ApiService/Tests/RequestsTests.cs +++ b/src/ApiService/Tests/RequestsTests.cs @@ -1,8 +1,14 @@ -using System.IO; +using System; +using System.Collections.Generic; +using System.ComponentModel.DataAnnotations; +using System.IO; +using System.Linq; +using System.Reflection; using System.Text; using System.Text.Json; using System.Threading; using Azure.Core.Serialization; +using FluentAssertions; using Microsoft.OneFuzz.Service; using Microsoft.OneFuzz.Service.OneFuzzLib.Orm; using Xunit; @@ -40,6 +46,43 @@ private void AssertRoundtrips(string json) { Assert.Equal(json, result); } + // Finds all non-nullable properties exposed on request objects (inheriting from BaseRequest). + // Note that at the moment we do not validate inner types since we are reusing some model types + // as request objects/DTOs, which we should stop doing. + public static IEnumerable NonNullableRequestProperties() { + var baseType = typeof(BaseRequest); + var asm = baseType.Assembly; + foreach (var requestType in asm.GetTypes().Where(t => t.IsAssignableTo(baseType))) { + if (requestType == baseType) { + continue; + } + + foreach (var property in requestType.GetProperties()) { + var nullabilityContext = new NullabilityInfoContext(); + var nullability = nullabilityContext.Create(property); + if (nullability.ReadState == NullabilityState.NotNull) { + yield return new object[] { requestType, property }; + } + } + } + } + + [Theory] + [MemberData(nameof(NonNullableRequestProperties))] + public void EnsureRequiredAttributesExistsOnNonNullableRequestProperties(Type requestType, PropertyInfo property) { + if (!property.IsDefined(typeof(RequiredAttribute))) { + // if not required it must have a default + + // find appropriate parameter + var param = requestType.GetConstructors().Single().GetParameters().Single(p => p.Name == property.Name); + Assert.True(param.HasDefaultValue, + "For request types, all non-nullable properties should either have a default value, or the [Required] attribute." + ); + } else { + // it is required, okay + } + } + [Fact] public void NodeEvent_WorkerEvent_Done() { // generated with: onefuzz-agent debug node_event worker_event done