update the ado logic to consume the list of existing items once (#3014)

* update the ado logic to consume the list of existing items once * format * Update src/ApiService/ApiService/onefuzzlib/notifications/Ado.cs Co-authored-by: Teo Voinea <[email protected]> * Adding a notification testing endpoint * fix tests * format * regen docs * update logic * format * fix dummy name * mypy fix * make mypy happy * bandit fix * renaming * address PR Comment --------- Co-authored-by: Teo Voinea <[email protected]>
microsoft · Apr 19, 2023 · aa28550 · aa28550
1 parent 6f06b8f
commit aa28550
Show file tree

Hide file tree

Showing 17 changed files with 240 additions and 99 deletions.
diff --git a/docs/webhook_events.md b/docs/webhook_events.md
@@ -238,6 +238,10 @@ If webhook is set to have Event Grid message format then the payload will look a
                     "title": "Onefuzz Version",
                     "type": "string"
                 },
+                "report_url": {
+                    "title": "Report Url",
+                    "type": "string"
+                },
                 "scariness_description": {
                     "title": "Scariness Description",
                     "type": "string"
@@ -2158,6 +2162,10 @@ If webhook is set to have Event Grid message format then the payload will look a
                     "title": "Onefuzz Version",
                     "type": "string"
                 },
+                "report_url": {
+                    "title": "Report Url",
+                    "type": "string"
+                },
                 "scariness_description": {
                     "title": "Scariness Description",
                     "type": "string"
@@ -6578,6 +6586,10 @@ If webhook is set to have Event Grid message format then the payload will look a
                     "title": "Onefuzz Version",
                     "type": "string"
                 },
+                "report_url": {
+                    "title": "Report Url",
+                    "type": "string"
+                },
                 "scariness_description": {
                     "title": "Scariness Description",
                     "type": "string"

diff --git a/src/ApiService/ApiService/Functions/Notifications.cs b/src/ApiService/ApiService/Functions/Notifications.cs
@@ -22,7 +22,7 @@ private async Async.Task<HttpResponseData> Get(HttpRequestData req) {
             return await _context.RequestHandling.NotOk(req, request.ErrorV, "notification search");
         }
 
-        var entries = request.OkV switch { { Container: null, NotificationId: null } => _context.NotificationOperations.SearchAll(), { Container: var c, NotificationId: null } => _context.NotificationOperations.SearchByRowKeys(c.Select(x => x.String)), { Container: var _, NotificationId: var n } => new[] { await _context.NotificationOperations.GetNotification(n.Value) }.ToAsyncEnumerable(),
+        var entries = request.OkV switch { { Container: null, NotificationId: null } => _context.NotificationOperations.SearchAll(), { Container: var c, NotificationId: null } => _context.NotificationOperations.SearchByRowKeys(c.Select(x => x.String)), { Container: var _, NotificationId: var n } => new[] { await _context.NotificationOperations.GetNotification(n.Value) }.ToAsyncEnumerable()
         };
 
         var response = req.CreateResponse(HttpStatusCode.OK);

diff --git a/src/ApiService/ApiService/Functions/NotificationsTest.cs b/src/ApiService/ApiService/Functions/NotificationsTest.cs
@@ -0,0 +1,41 @@
+using System.Net;
+using Microsoft.Azure.Functions.Worker;
+using Microsoft.Azure.Functions.Worker.Http;
+
+namespace Microsoft.OneFuzz.Service.Functions;
+
+public class NotificationsTest {
+    private readonly ILogTracer _log;
+    private readonly IEndpointAuthorization _auth;
+    private readonly IOnefuzzContext _context;
+
+    public NotificationsTest(ILogTracer log, IEndpointAuthorization auth, IOnefuzzContext context) {
+        _log = log;
+        _auth = auth;
+        _context = context;
+    }
+
+    private async Async.Task<HttpResponseData> Post(HttpRequestData req) {
+        _log.WithTag("HttpRequest", "GET").Info($"Notification test");
+        var request = await RequestHandling.ParseRequest<NotificationTest>(req);
+        if (!request.IsOk) {
+            return await _context.RequestHandling.NotOk(req, request.ErrorV, "notification search");
+        }
+
+        var notificationTest = request.OkV;
+        var result = await _context.NotificationOperations.TriggerNotification(notificationTest.Notification.Container, notificationTest.Notification,
+            notificationTest.Report, isLastRetryAttempt: true);
+        var response = req.CreateResponse(HttpStatusCode.OK);
+        await response.WriteAsJsonAsync(new NotificationTestResponse(result.IsOk, result.ErrorV?.ToString()));
+        return response;
+    }
+
+
+    [Function("NotificationsTest")]
+    public Async.Task<HttpResponseData> Run([HttpTrigger(AuthorizationLevel.Anonymous, "POST", Route = "notifications/test")] HttpRequestData req) {
+        return _auth.CallIfUser(req, r => r.Method switch {
+            "POST" => Post(r),
+            _ => throw new InvalidOperationException("Unsupported HTTP method"),
+        });
+    }
+}
diff --git a/src/ApiService/ApiService/OneFuzzTypes/Model.cs b/src/ApiService/ApiService/OneFuzzTypes/Model.cs
@@ -527,6 +527,10 @@ public RegressionReport Truncate(int maxLength) {
     }
 }
 
+public record UnknownReportType(
+    Uri? ReportUrl
+) : IReport;
+
 [JsonConverter(typeof(NotificationTemplateConverter))]
 #pragma warning disable CA1715
 public interface NotificationTemplate {

diff --git a/src/ApiService/ApiService/OneFuzzTypes/Requests.cs b/src/ApiService/ApiService/OneFuzzTypes/Requests.cs
@@ -129,6 +129,12 @@ public record NotificationSearch(
     Guid? NotificationId
 ) : BaseRequest;
 
+
+public record NotificationTest(
+    [property: Required] Report Report,
+    [property: Required] Notification Notification
+) : BaseRequest;
+
 public record NotificationGet(
     [property: Required] Guid NotificationId
 ) : BaseRequest;

diff --git a/src/ApiService/ApiService/OneFuzzTypes/Responses.cs b/src/ApiService/ApiService/OneFuzzTypes/Responses.cs
@@ -205,3 +205,8 @@ List<Guid> FailedNotificationIds
 public record JinjaToScribanMigrationDryRunResponse(
     List<Guid> NotificationIdsToUpdate
 ) : BaseResponse();
+
+public record NotificationTestResponse(
+    bool Success,
+    string? Error = null
+) : BaseResponse();
diff --git a/src/ApiService/ApiService/onefuzzlib/NotificationOperations.cs b/src/ApiService/ApiService/onefuzzlib/NotificationOperations.cs
@@ -10,6 +10,9 @@ public interface INotificationOperations : IOrm<Notification> {
     IAsyncEnumerable<(Task, IEnumerable<Container>)> GetQueueTasks();
     Async.Task<OneFuzzResult<Notification>> Create(Container container, NotificationTemplate config, bool replaceExisting);
     Async.Task<Notification?> GetNotification(Guid notifificationId);
+
+    System.Threading.Tasks.Task<OneFuzzResultVoid> TriggerNotification(Container container,
+        Notification notification, IReport? reportOrRegression, bool isLastRetryAttempt = false);
 }
 
 public class NotificationOperations : Orm<Notification>, INotificationOperations {
@@ -30,22 +33,7 @@ public async Async.Task NewFiles(Container container, string filename, bool isLa
                 }
 
                 done.Add(notification.Config);
-
-                if (notification.Config is TeamsTemplate teamsTemplate) {
-                    await _context.Teams.NotifyTeams(teamsTemplate, container, filename, reportOrRegression!, notification.NotificationId);
-                }
-
-                if (reportOrRegression == null) {
-                    continue;
-                }
-
-                if (notification.Config is AdoTemplate adoTemplate) {
-                    await _context.Ado.NotifyAdo(adoTemplate, container, filename, reportOrRegression, isLastRetryAttempt, notification.NotificationId);
-                }
-
-                if (notification.Config is GithubIssuesTemplate githubIssuesTemplate) {
-                    await _context.GithubIssues.GithubIssue(githubIssuesTemplate, container, filename, reportOrRegression, notification.NotificationId);
-                }
+                _ = await TriggerNotification(container, notification, reportOrRegression, isLastRetryAttempt);
             }
         }
 
@@ -74,6 +62,25 @@ public async Async.Task NewFiles(Container container, string filename, bool isLa
         }
     }
 
+    public async System.Threading.Tasks.Task<OneFuzzResultVoid> TriggerNotification(Container container,
+        Notification notification, IReport? reportOrRegression, bool isLastRetryAttempt = false) {
+        switch (notification.Config) {
+            case TeamsTemplate teamsTemplate:
+                await _context.Teams.NotifyTeams(teamsTemplate, container, reportOrRegression!,
+                    notification.NotificationId);
+                break;
+            case AdoTemplate adoTemplate when reportOrRegression is not null:
+                return await _context.Ado.NotifyAdo(adoTemplate, container, reportOrRegression, isLastRetryAttempt,
+                    notification.NotificationId);
+            case GithubIssuesTemplate githubIssuesTemplate when reportOrRegression is not null:
+                await _context.GithubIssues.GithubIssue(githubIssuesTemplate, container, reportOrRegression,
+                    notification.NotificationId);
+                break;
+        }
+
+        return OneFuzzResultVoid.Ok;
+    }
+
     public IAsyncEnumerable<Notification> GetNotifications(Container container) {
         return SearchByRowKeys(new[] { container.String });
     }

diff --git a/src/ApiService/ApiService/onefuzzlib/Reports.cs b/src/ApiService/ApiService/onefuzzlib/Reports.cs
@@ -35,7 +35,17 @@ public Reports(ILogTracer log, IContainers containers) {
             return null;
         }
 
-        var blob = await _containers.GetBlob(container, fileName, StorageType.Corpus);
+        var containerClient = await _containers.FindContainer(container, StorageType.Corpus);
+        if (containerClient == null) {
+            if (expectReports) {
+                _log.Error($"get_report invalid container: {filePath:Tag:FilePath}");
+            }
+            return null;
+        }
+
+        Uri reportUrl = containerClient.GetBlobClient(fileName).Uri;
+
+        var blob = (await containerClient.GetBlobClient(fileName).DownloadContentAsync()).Value.Content;
 
         if (blob == null) {
             if (expectReports) {
@@ -44,11 +54,9 @@ public Reports(ILogTracer log, IContainers containers) {
             return null;
         }
 
-        var reportUrl = await _containers.GetFileUrl(container, fileName, StorageType.Corpus);
-
         var reportOrRegression = ParseReportOrRegression(blob.ToString(), reportUrl);
 
-        if (reportOrRegression == null && expectReports) {
+        if (reportOrRegression is UnknownReportType && expectReports) {
             _log.Error($"unable to parse report ({filePath:Tag:FilePath}) as a report or regression");
         }
 
@@ -64,7 +72,7 @@ public Reports(ILogTracer log, IContainers containers) {
         }
     }
 
-    public static IReport? ParseReportOrRegression(string content, Uri? reportUrl) {
+    public static IReport ParseReportOrRegression(string content, Uri reportUrl) {
         var regressionReport = TryDeserialize<RegressionReport>(content);
         if (regressionReport is { CrashTestResult: { } }) {
             return regressionReport with { ReportUrl = reportUrl };
@@ -73,12 +81,17 @@ public Reports(ILogTracer log, IContainers containers) {
         if (report is { CrashType: { } }) {
             return report with { ReportUrl = reportUrl };
         }
-        return null;
+        return new UnknownReportType(reportUrl);
     }
 }
 
 public interface IReport {
     Uri? ReportUrl {
         init;
+        get;
+    }
+    public string FileName() {
+        var segments = (this.ReportUrl ?? throw new ArgumentException()).Segments.Skip(2);
+        return string.Concat(segments);
     }
 };
diff --git a/src/ApiService/ApiService/onefuzzlib/Secrets.cs b/src/ApiService/ApiService/onefuzzlib/Secrets.cs
@@ -56,13 +56,11 @@ public virtual async Task<SecretData<T>> SaveToKeyvault<T>(SecretData<T> secretD
     }
 
     public async Task<string?> GetSecretStringValue<T>(SecretData<T> data) {
-
-        if (data.Secret is SecretAddress<T> secretAddress) {
-            var secret = await GetSecret(secretAddress.Url);
-            return secret.Value;
-        } else {
-            return data.Secret.ToString();
-        }
+        return (data.Secret) switch {
+            SecretAddress<T> secretAddress => (await GetSecret(secretAddress.Url)).Value,
+            SecretValue<T> sValue => sValue.Value?.ToString(),
+            _ => data.Secret.ToString(),
+        };
     }
 
     public Uri GetKeyvaultAddress() {

diff --git a/src/ApiService/ApiService/onefuzzlib/notifications/Ado.cs b/src/ApiService/ApiService/onefuzzlib/notifications/Ado.cs
@@ -8,17 +8,18 @@
 namespace Microsoft.OneFuzz.Service;
 
 public interface IAdo {
-    public Async.Task NotifyAdo(AdoTemplate config, Container container, string filename, IReport reportable, bool isLastRetryAttempt, Guid notificationId);
+    public Async.Task<OneFuzzResultVoid> NotifyAdo(AdoTemplate config, Container container, IReport reportable, bool isLastRetryAttempt, Guid notificationId);
 }
 
 public class Ado : NotificationsBase, IAdo {
     public Ado(ILogTracer logTracer, IOnefuzzContext context) : base(logTracer, context) {
     }
 
-    public async Async.Task NotifyAdo(AdoTemplate config, Container container, string filename, IReport reportable, bool isLastRetryAttempt, Guid notificationId) {
+    public async Async.Task<OneFuzzResultVoid> NotifyAdo(AdoTemplate config, Container container, IReport reportable, bool isLastRetryAttempt, Guid notificationId) {
+        var filename = reportable.FileName();
         if (reportable is RegressionReport) {
             _logTracer.Info($"ado integration does not support regression report. container:{container:Tag:Container} filename:{filename:Tag:Filename}");
-            return;
+            return OneFuzzResultVoid.Ok;
         }
 
         var report = (Report)reportable;
@@ -44,8 +45,11 @@ public async Async.Task NotifyAdo(AdoTemplate config, Container container, strin
             } else {
                 _logTracer.WithTags(notificationInfo).Exception(e, $"Failed to process ado notification");
                 await LogFailedNotification(report, e, notificationId);
+                return OneFuzzResultVoid.Error(ErrorCode.NOTIFICATION_FAILURE,
+                    $"Failed to process ado notification : exception: {e}");
             }
         }
+        return OneFuzzResultVoid.Ok;
     }
 
     private static bool IsTransient(Exception e) {
@@ -205,7 +209,7 @@ public async IAsyncEnumerable<WorkItem> ExistingWorkItems((string, string)[] not
                 }
             }
 
-            var query = "select [System.Id] from WorkItems";
+            var query = "select [System.Id] from WorkItems order by [System.Id]";
             if (parts != null && parts.Any()) {
                 query += " where " + string.Join(" AND ", parts);
             }
@@ -331,47 +335,42 @@ private async Async.Task<WorkItem> CreateNew() {
         }
 
         public async Async.Task Process((string, string)[] notificationInfo) {
-            var matchingWorkItems = await ExistingWorkItems(notificationInfo).ToListAsync();
-
-            var nonDuplicateWorkItems = matchingWorkItems
-                .Where(wi => !IsADODuplicateWorkItem(wi))
-                .ToList();
-
-            if (nonDuplicateWorkItems.Count > 1) {
-                var nonDuplicateWorkItemIds = nonDuplicateWorkItems.Select(wi => wi.Id);
-                var matchingWorkItemIds = matchingWorkItems.Select(wi => wi.Id);
-
-                var extraTags = new List<(string, string)> {
-                    ("NonDuplicateWorkItemIds", JsonSerializer.Serialize(nonDuplicateWorkItemIds)),
-                    ("MatchingWorkItemIds", JsonSerializer.Serialize(matchingWorkItemIds))
-                };
-                extraTags.AddRange(notificationInfo);
-
-                _logTracer.WithTags(extraTags).Info($"Found more than 1 matching, non-duplicate work item");
-                foreach (var workItem in nonDuplicateWorkItems) {
-                    _ = await UpdateExisting(workItem, notificationInfo);
+            var updated = false;
+            WorkItem? oldestWorkItem = null;
+            await foreach (var workItem in ExistingWorkItems(notificationInfo)) {
+                // work items are ordered by id, so the oldest one is the first one
+                oldestWorkItem ??= workItem;
+                _logTracer.WithTags(new List<(string, string)> { ("MatchingWorkItemIds", $"{workItem.Id}") }).Info($"Found matching work item");
+                if (IsADODuplicateWorkItem(workItem)) {
+                    continue;
                 }
-            } else if (nonDuplicateWorkItems.Count == 1) {
-                _ = await UpdateExisting(nonDuplicateWorkItems.Single(), notificationInfo);
-            } else if (matchingWorkItems.Any()) {
-                // We have matching work items but all are duplicates
-                _logTracer.WithTags(notificationInfo).Info($"All matching work items were duplicates, re-opening the oldest one");
-                var oldestWorkItem = matchingWorkItems.OrderBy(wi => wi.Id).First();
-                var stateChanged = await UpdateExisting(oldestWorkItem, notificationInfo);
-                if (stateChanged) {
-                    // add a comment if we re-opened the bug
-                    _ = await _client.AddCommentAsync(
-                        new CommentCreate() {
-                            Text = "This work item was re-opened because OneFuzz could only find related work items that are marked as duplicate."
-                        },
-                        _project,
-                        (int)oldestWorkItem.Id!);
+                _logTracer.WithTags(new List<(string, string)> { ("NonDuplicateWorkItemId", $"{workItem.Id}") }).Info($"Found matching non-duplicate work item");
+                _ = await UpdateExisting(workItem, notificationInfo);
+                updated = true;
+            }
+
+            if (!updated) {
+                if (oldestWorkItem != null) {
+                    // We have matching work items but all are duplicates
+                    _logTracer.WithTags(notificationInfo)
+                        .Info($"All matching work items were duplicates, re-opening the oldest one");
+                    var stateChanged = await UpdateExisting(oldestWorkItem, notificationInfo);
+                    if (stateChanged) {
+                        // add a comment if we re-opened the bug
+                        _ = await _client.AddCommentAsync(
+                            new CommentCreate() {
+                                Text =
+                                    "This work item was re-opened because OneFuzz could only find related work items that are marked as duplicate."
+                            },
+                            _project,
+                            (int)oldestWorkItem.Id!);
+                    }
+                } else {
+                    // We never saw a work item like this before, it must be new
+                    var entry = await CreateNew();
+                    var adoEventType = "AdoNewItem";
+                    _logTracer.WithTags(notificationInfo).Event($"{adoEventType} {entry.Id:Tag:WorkItemId}");
                 }
-            } else {
-                // We never saw a work item like this before, it must be new
-                var entry = await CreateNew();
-                var adoEventType = "AdoNewItem";
-                _logTracer.WithTags(notificationInfo).Event($"{adoEventType} {entry.Id:Tag:WorkItemId}");
             }
         }