Deal with many TODOs

src/ServiceControl.AcceptanceTests.RavenDB5/Recoverability/MessageFailures/FailedErrorsController.cs

@@ -1,6 +1,5 @@
 namespace ServiceControl.AcceptanceTests.RavenDB.Recoverability.MessageFailures
 {
-    using System;
     using System.Net;
     using System.Net.Http;
     using System.Threading;
@@ -9,7 +8,6 @@
     using Infrastructure.WebApi;
     using Operations;
     using Raven.Client.Documents;
-    using Raven.Client.Documents.Operations;
 
     public class FailedErrorsCountReponse
     {
@@ -52,23 +50,6 @@ public async Task<HttpResponseMessage> ImportFailedErrors(CancellationToken canc
             return Request.CreateResponse(HttpStatusCode.OK);
         }
 
-        [Route("failederrors/forcecleanerrors")]
-        [HttpPost]
-        public Task<HttpResponseMessage> ForceErrorMessageCleanerRun()
-        {
-            // TODO: Is there a way to force the Raven5 expiration to happen? Or does it just happen? Won't be able to tell until we redesign that.
-
-            // May not even need WaitForIndexes given Raven5 implementation isn't index-based
-            WaitForIndexes(store);
-
-            return Task.FromResult(Request.CreateResponse(HttpStatusCode.OK));
-        }
-
-        static void WaitForIndexes(IDocumentStore store)
-        {
-            SpinWait.SpinUntil(() => store.Maintenance.Send(new GetStatisticsOperation()).StaleIndexes.Length == 0, TimeSpan.FromSeconds(10));
-        }
-
         readonly IDocumentStore store;
         readonly ImportFailedErrors importFailedErrors;
     }

src/ServiceControl.AcceptanceTests.RavenDB5/Recoverability/MessageFailures/When_a_failed_message_is_retried.cs

@@ -184,10 +184,11 @@ await Define<Context>()
                 {
                     if (ctx.Retried)
                     {
-                        // trigger cleanup
-                        await this.Post<object>("/api/failederrors/forcecleanerrors");
-
+                        // Note: In RavenDB 3.5 there was a call to /api/failederrors/forcecleanerrors implemented in test-only FailedErrorsController
+                        // that manually ran the Expiration bundle, but RavenDB 5 uses built-in expiration so you can't do that. The test still
+                        // appears to pass, however.
                         failedMessageRetries = await this.TryGet<FailedMessageRetriesCountReponse>("/api/failedmessageretries/count");
+
                         return failedMessageRetries.Count == 0;
                     }
 

src/ServiceControl.AcceptanceTests/Recoverability/MessageFailures/When_a_retry_fails_to_be_sent.cs

@@ -11,7 +11,6 @@
     using NServiceBus.Settings;
     using NServiceBus.Transport;
     using NUnit.Framework;
-    using Operations.BodyStorage;
     using ServiceControl.MessageFailures;
     using ServiceControl.MessageFailures.Api;
     using ServiceControl.Persistence;
@@ -26,7 +25,7 @@ public async Task SubsequentBatchesShouldBeProcessed()
         {
             FailedMessage decomissionedFailure = null, successfullyRetried = null;
 
-            CustomConfiguration = config => config.RegisterComponents(components => components.ConfigureComponent<ReturnToSender>(b => new FakeReturnToSender(b.Build<IBodyStorage>(), b.Build<IErrorMessageDataStore>(), b.Build<MyContext>()), DependencyLifecycle.SingleInstance));
+            CustomConfiguration = config => config.RegisterComponents(components => components.ConfigureComponent<ReturnToSender>(b => new FakeReturnToSender(b.Build<IErrorMessageDataStore>(), b.Build<MyContext>()), DependencyLifecycle.SingleInstance));
 
             await Define<MyContext>()
                 .WithEndpoint<FailureEndpoint>(b => b.DoNotFailOnErrorMessages()
@@ -148,7 +147,7 @@ public class MessageThatWillFail : ICommand
 
         public class FakeReturnToSender : ReturnToSender
         {
-            public FakeReturnToSender(IBodyStorage bodyStorage, IErrorMessageDataStore errorMessageStore, MyContext myContext) : base(bodyStorage, errorMessageStore)
+            public FakeReturnToSender(IErrorMessageDataStore errorMessageStore, MyContext myContext) : base(errorMessageStore)
             {
                 this.myContext = myContext;
             }

src/ServiceControl.AcceptanceTests/TestSupport/ServiceControlComponentRunner.cs

@@ -1,7 +1,6 @@
 namespace ServiceControl.AcceptanceTests.TestSupport
 {
     using System;
-    using System.Configuration;
     using System.IO;
     using System.Net.Http;
     using System.Net.Http.Headers;
@@ -201,7 +200,7 @@ HttpClient HttpClientFactory()
         {
             if (Handler == null)
             {
-                throw new InvalidOperationException("Handler field not yet initialized"); // TODO: This method is invoked before `Initialize` completes which is strange and should be looked into as that seems like a race condition
+                throw new InvalidOperationException("Handler field not yet initialized");
             }
             var httpClient = new HttpClient(Handler);
             httpClient.DefaultRequestHeaders.Accept.Add(new MediaTypeWithQualityHeaderValue("application/json"));

src/ServiceControl.Audit.Persistence.RavenDb5/EmbeddedDatabase.cs

@@ -32,17 +32,21 @@ public EmbeddedDatabase(DatabaseConfiguration configuration)
                 return (localRavenLicense, null);
             }
 
-            //TODO: refactor this to extract the folder name to a constant
-            localRavenLicense = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "Persisters", "RavenDB5", licenseFileName);
+            const string Persisters = "Persisters";
+            const string RavenDB5 = "RavenDB5";
+
+            var persisterDirectory = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, Persisters, RavenDB5);
+
+            localRavenLicense = Path.Combine(persisterDirectory, licenseFileName);
             if (!File.Exists(localRavenLicense))
             {
                 throw new Exception($"RavenDB license not found. Make sure the RavenDB license file, '{licenseFileName}', " +
-                    $"is stored in the '{AppDomain.CurrentDomain.BaseDirectory}' folder or in the 'Persisters/RavenDB5' subfolder.");
+                    $"is stored in the '{AppDomain.CurrentDomain.BaseDirectory}' folder or in the '{Persisters}/{RavenDB5}' subfolder.");
             }
 
             // By default RavenDB 5 searches its binaries in the RavenDBServer right below the BaseDirectory.
             // If we're loading from Persisters/RavenDB5 we also have to signal RavenDB where are binaries
-            var serverDirectory = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "Persisters", "RavenDB5", "RavenDBServer");
+            var serverDirectory = Path.Combine(persisterDirectory, "RavenDBServer");
 
             return (localRavenLicense, serverDirectory);
         }

src/ServiceControl.Persistence.RavenDb5/DatabaseSetup.cs

@@ -1,7 +1,5 @@
 namespace ServiceControl.Persistence.RavenDb5
 {
-    using System;
-    using System.Linq;
     using System.Threading;
     using System.Threading.Tasks;
     using Raven.Client.Documents;
@@ -39,30 +37,7 @@ await documentStore.Maintenance.Server
                 }
             }
 
-            var indexTypes = typeof(DatabaseSetup).Assembly.GetTypes()
-                .Where(t => typeof(IAbstractIndexCreationTask).IsAssignableFrom(t))
-                .ToList();
-
-            //TODO: Handle full text search - if necessary add Where clause to query above to remove the two variants
-            //if (settings.EnableFullTextSearch)
-            //{
-            //    indexList.Add(new MessagesViewIndexWithFullTextSearch());
-            //    await documentStore.Maintenance.SendAsync(new DeleteIndexOperation("MessagesViewIndex"), cancellationToken);
-            //}
-            //else
-            //{
-            //    indexList.Add(new MessagesViewIndex());
-            //    await documentStore.Maintenance
-            //        .SendAsync(new DeleteIndexOperation("MessagesViewIndexWithFullTextSearch"), cancellationToken);
-            //}
-
-            var indexList = indexTypes
-                .Select(t => Activator.CreateInstance(t))
-                .OfType<IAbstractIndexCreationTask>();
-
-            // If no full-text vs not full-text index is required, this can all be simplified using the assembly-based override
-            // await IndexCreation.CreateIndexesAsync(typeof(DatabaseSetup).Assembly, documentStore, null, null, cancellationToken);
-            await IndexCreation.CreateIndexesAsync(indexList, documentStore, null, null, cancellationToken);
+            await IndexCreation.CreateIndexesAsync(typeof(DatabaseSetup).Assembly, documentStore, null, null, cancellationToken);
 
             var expirationConfig = new ExpirationConfiguration
             {

src/ServiceControl.Persistence.RavenDb5/EmbeddedDatabase.cs

@@ -32,12 +32,16 @@ public class EmbeddedDatabase : IDisposable
                 return (localRavenLicense, null);
             }
 
-            //TODO: refactor this to extract the folder name to a constant
-            localRavenLicense = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, "Persisters", "RavenDB5", licenseFileName);
+            const string Persisters = "Persisters";
+            const string RavenDB5 = "RavenDB5";
+
+            var persisterDirectory = Path.Combine(AppDomain.CurrentDomain.BaseDirectory, Persisters, RavenDB5);
+
+            localRavenLicense = Path.Combine(persisterDirectory, licenseFileName);
             if (!File.Exists(localRavenLicense))
             {
                 throw new Exception($"RavenDB license not found. Make sure the RavenDB license file, '{licenseFileName}', " +
-                    $"is stored in the '{AppDomain.CurrentDomain.BaseDirectory}' folder or in the 'Persisters/RavenDB5' subfolder.");
+                    $"is stored in the '{AppDomain.CurrentDomain.BaseDirectory}' folder or in the '{Persisters}/{RavenDB5}' subfolder.");
             }
 
             // By default RavenDB 5 searches its binaries in the RavenDBServer right below the BaseDirectory.
@@ -138,12 +142,6 @@ public async Task<IDocumentStore> Connect(CancellationToken cancellationToken =
                 }
             };
 
-            //TODO: copied from Audit. In Audit FindClrType so I guess this is not needed. Confirm and remove
-            //if (configuration.FindClrType != null)
-            //{
-            //    dbOptions.Conventions.FindClrType += configuration.FindClrType;
-            //}
-
             var store = await EmbeddedServer.Instance.GetDocumentStoreAsync(dbOptions, cancellationToken);
 
             var databaseSetup = new DatabaseSetup(configuration);

src/ServiceControl.Persistence.RavenDb5/ErrorMessagesDataStore.cs

@@ -154,15 +154,6 @@ SortInfo sortInfo
             }
         }
 
-        // TODO: There seem to be several copies of this operation in here
-        public async Task<FailedMessage> FailedMessageFetch(string failedMessageId)
-        {
-            using (var session = documentStore.OpenAsyncSession())
-            {
-                return await session.LoadAsync<FailedMessage>(FailedMessageIdGenerator.MakeDocumentId(failedMessageId));
-            }
-        }
-
         public async Task FailedMessageMarkAsArchived(string failedMessageId)
         {
             using (var session = documentStore.OpenAsyncSession())
@@ -352,20 +343,15 @@ public async Task<IDictionary<string, object>> ErrorsSummary()
             }
         }
 
-        public async Task<FailedMessage> ErrorBy(Guid failedMessageId)
-        {
-            using (var session = documentStore.OpenAsyncSession())
-            {
-                var message = await session.LoadAsync<FailedMessage>(FailedMessageIdGenerator.MakeDocumentId(failedMessageId.ToString()));
-                return message;
-            }
-        }
+        public Task<FailedMessage> ErrorBy(Guid failedMessageId) => ErrorByDocumentId(FailedMessageIdGenerator.MakeDocumentId(failedMessageId.ToString()));
 
-        public async Task<FailedMessage> ErrorBy(string failedMessageId)
+        public Task<FailedMessage> ErrorBy(string failedMessageId) => ErrorByDocumentId(FailedMessageIdGenerator.MakeDocumentId(failedMessageId));
+
+        async Task<FailedMessage> ErrorByDocumentId(string documentId)
         {
             using (var session = documentStore.OpenAsyncSession())
             {
-                var message = await session.LoadAsync<FailedMessage>(FailedMessageIdGenerator.MakeDocumentId(failedMessageId));
+                var message = await session.LoadAsync<FailedMessage>(documentId);
                 return message;
             }
         }
@@ -586,29 +572,33 @@ class DocumentPatchResult
             public string Document { get; set; }
         }
 
-        public async Task<(string[] ids, int count)> UnArchiveMessagesByRange(DateTime from, DateTime to, DateTime cutOff)
+        public async Task<string[]> UnArchiveMessagesByRange(DateTime from, DateTime to)
         {
-            // TODO: Make sure this new implementation actually works, not going to delete the old implementation (commented below) until then
-            var patch = new PatchByQueryOperation(new IndexQuery
+            const int Unresolved = (int)FailedMessageStatus.Unresolved;
+            const int Archived = (int)FailedMessageStatus.Archived;
+
+            var indexName = new FailedMessageViewIndex().IndexName;
+            var query = new IndexQuery
             {
+                // Set based args are treated differently ($name) than other places (args.name)!
+                // https://ravendb.net/docs/article-page/5.4/csharp/client-api/operations/patching/set-based
+                // Removing a property in a patch
                 // https://ravendb.net/docs/article-page/5.4/Csharp/client-api/operations/patching/single-document#remove-property
-
-                Query = $@"from index '{new FailedMessageViewIndex().IndexName} as msg
-                           where msg.LastModified >= args.From and msg.LastModified <= args.To
-                           where msg.Status == args.Archived
+                Query = $@"from index '{indexName}' as msg
+                           where msg.Status == {Archived} and msg.LastModified >= $from and msg.LastModified <= $to
                            update
                            {{
-                                msg.Status = args.Unresolved
-                                {ExpirationManager.DeleteExpirationFieldScript}
+                                msg.Status = {Unresolved};
+                                {ExpirationManager.DeleteExpirationFieldExpression};
                            }}",
-                QueryParameters =
+                QueryParameters = new Parameters
                 {
-                    { "From", from },
-                    { "To", to },
-                    { "Unresolved", (int)FailedMessageStatus.Unresolved },
-                    { "Archived", (int)FailedMessageStatus.Archived },
+                    { "from", from.Ticks },
+                    { "to", to.Ticks }
                 }
-            }, new QueryOperationOptions
+            };
+
+            var patch = new PatchByQueryOperation(query, new QueryOperationOptions
             {
                 AllowStale = true,
                 RetrieveDetails = true
@@ -622,44 +612,10 @@ class DocumentPatchResult
                 .Select(d => d.Id)
                 .ToArray();
 
-            // TODO: Are we *really* returning an array AND the length of the same array?
-            return (ids, ids.Length);
-
-            //            var options = new BulkOperationOptions
-            //            {
-            //                AllowStale = true
-            //            };
-
-            //            var result = await documentStore.AsyncDatabaseCommands.UpdateByIndexAsync(
-            //                new FailedMessageViewIndex().IndexName,
-            //                new IndexQuery
-            //                {
-            //                    Query = string.Format(CultureInfo.InvariantCulture, "LastModified:[{0} TO {1}] AND Status:{2}", from.Ticks, to.Ticks, (int)FailedMessageStatus.Archived),
-            //                    Cutoff = cutOff
-            //                }, new ScriptedPatchRequest
-            //                {
-            //                    Script = @"
-            //if(this.Status === archivedStatus) {
-            //    this.Status = unresolvedStatus;
-            //}
-            //",
-            //                    Values =
-            //                    {
-            //                        {"archivedStatus", (int)FailedMessageStatus.Archived},
-            //                        {"unresolvedStatus", (int)FailedMessageStatus.Unresolved}
-            //                    }
-            //                }, options);
-
-            //            var patchedDocumentIds = (await result.WaitForCompletionAsync())
-            //                .JsonDeserialization<DocumentPatchResult[]>();
-
-            //            return (
-            //                patchedDocumentIds.Select(x => FailedMessageIdGenerator.GetMessageIdFromDocumentId(x.Document)).ToArray(),
-            //                patchedDocumentIds.Length
-            //                );
-        }
-
-        public async Task<(string[] ids, int count)> UnArchiveMessages(IEnumerable<string> failedMessageIds)
+            return ids;
+        }
+
+        public async Task<string[]> UnArchiveMessages(IEnumerable<string> failedMessageIds)
         {
             Dictionary<string, FailedMessage> failedMessages;
 
@@ -683,10 +639,8 @@ class DocumentPatchResult
                 await session.SaveChangesAsync();
             }
 
-            return (
-                failedMessages.Values.Select(x => x.UniqueMessageId).ToArray(), // TODO: (ramon) I don't think we can use Keys here as UniqueMessageId is something different than failedMessageId right?
-                failedMessages.Count
-                );
+            // Return the unique IDs - the dictionary keys are document ids with a prefix
+            return failedMessages.Values.Select(x => x.UniqueMessageId).ToArray();
         }
 
         public async Task RevertRetry(string messageUniqueId)

src/ServiceControl.Persistence.RavenDb5/ExpirationManager.cs

@@ -9,7 +9,7 @@
 
     class ExpirationManager
     {
-        public const string DeleteExpirationFieldScript = "; delete msg['@metadata']['@expires']";
+        public const string DeleteExpirationFieldExpression = "delete msg['@metadata']['@expires']";
 
         readonly TimeSpan errorRetentionPeriod;
         readonly TimeSpan eventsRetentionPeriod;