Fix VersionChecker logging, timeout, URL and tag-name parsing (#33)

ECoreNetto.Tools.Tests/Services/VersionCheckerTestFixture.cs

@@ -93,6 +93,72 @@ public async Task Verify_that_when_cancelled_exception_is_thrown()
             await Assert.ThatAsync(() => checker.ExecuteAsync(cts.Token), Throws.TypeOf<OperationCanceledException>());
         }
 
+        [Test]
+        public void Verify_that_default_url_and_timeout_are_used_when_not_configured()
+        {
+            Assert.Multiple(() =>
+            {
+                Assert.That(this.versionChecker.ReleasesUrl, Is.EqualTo(VersionChecker.DefaultReleasesUrl));
+                Assert.That(this.versionChecker.Timeout, Is.EqualTo(TimeSpan.FromSeconds(2)));
+            });
+        }
+
+        [Test]
+        public void Verify_that_url_and_timeout_are_configurable()
+        {
+            var checker = new VersionChecker(this.httpClientFactory, this.loggerFactory,
+                "https://example.test/releases/latest", TimeSpan.FromSeconds(5));
+
+            Assert.Multiple(() =>
+            {
+                Assert.That(checker.ReleasesUrl, Is.EqualTo("https://example.test/releases/latest"));
+                Assert.That(checker.Timeout, Is.EqualTo(TimeSpan.FromSeconds(5)));
+            });
+        }
+
+        [Test]
+        public async Task Verify_that_QueryLatestReleaseAsync_uses_the_configured_url()
+        {
+            var recordingHandler = new RecordingHandler();
+            var factory = new StubHttpClientFactory(new HttpClient(recordingHandler));
+
+            var checker = new VersionChecker(factory, this.loggerFactory, "https://example.test/releases/latest");
+
+            await checker.QueryLatestReleaseAsync(new CancellationTokenSource().Token);
+
+            Assert.That(recordingHandler.LastRequestUri?.ToString(), Is.EqualTo("https://example.test/releases/latest"));
+        }
+
+        [Test]
+        public async Task Verify_that_ExecuteAsync_handles_a_malformed_tag_name_gracefully()
+        {
+            var factory = new StubHttpClientFactory(new HttpClient(new TagNameHandler("not-a-version")));
+
+            var checker = new VersionChecker(factory, this.loggerFactory);
+
+            await Assert.ThatAsync(() => checker.ExecuteAsync(new CancellationTokenSource().Token), Throws.Nothing);
+        }
+
+        [Test]
+        public async Task Verify_that_ExecuteAsync_handles_an_empty_tag_name()
+        {
+            var factory = new StubHttpClientFactory(new HttpClient(new TagNameHandler("")));
+
+            var checker = new VersionChecker(factory, this.loggerFactory);
+
+            await Assert.ThatAsync(() => checker.ExecuteAsync(new CancellationTokenSource().Token), Throws.Nothing);
+        }
+
+        [Test]
+        public async Task Verify_that_ExecuteAsync_handles_a_v_prefixed_tag_name()
+        {
+            var factory = new StubHttpClientFactory(new HttpClient(new TagNameHandler("v9.9.9")));
+
+            var checker = new VersionChecker(factory, this.loggerFactory);
+
+            await Assert.ThatAsync(() => checker.ExecuteAsync(new CancellationTokenSource().Token), Throws.Nothing);
+        }
+
         /// <summary>
         /// Very simple IHttpClientFactory used just for tests.
         /// It always returns the HttpClient passed in the constructor.
@@ -153,5 +219,66 @@ protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage reques
                 throw new TaskCanceledException();
             }
         }
+
+        /// <summary>
+        /// An <see cref="IHttpClientFactory"/> that always returns the provided <see cref="HttpClient"/>.
+        /// </summary>
+        private sealed class StubHttpClientFactory : IHttpClientFactory
+        {
+            private readonly HttpClient client;
+
+            public StubHttpClientFactory(HttpClient client)
+            {
+                this.client = client;
+            }
+
+            public HttpClient CreateClient(string name)
+            {
+                return this.client;
+            }
+        }
+
+        /// <summary>
+        /// A handler that records the requested <see cref="Uri"/> and returns a successful release payload.
+        /// </summary>
+        private sealed class RecordingHandler : HttpMessageHandler
+        {
+            public Uri? LastRequestUri { get; private set; }
+
+            protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
+            {
+                this.LastRequestUri = request.RequestUri;
+
+                const string json = "{\"tag_name\":\"1.2.3\",\"body\":\"notes\",\"html_url\":\"https://example.com\"}";
+
+                return Task.FromResult(new HttpResponseMessage(System.Net.HttpStatusCode.OK)
+                {
+                    Content = new StringContent(json)
+                });
+            }
+        }
+
+        /// <summary>
+        /// A handler that returns a release payload with a configurable tag name.
+        /// </summary>
+        private sealed class TagNameHandler : HttpMessageHandler
+        {
+            private readonly string tagName;
+
+            public TagNameHandler(string tagName)
+            {
+                this.tagName = tagName;
+            }
+
+            protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
+            {
+                var json = $"{{\"tag_name\":\"{this.tagName}\",\"body\":\"notes\",\"html_url\":\"https://example.com\"}}";
+
+                return Task.FromResult(new HttpResponseMessage(System.Net.HttpStatusCode.OK)
+                {
+                    Content = new StringContent(json)
+                });
+            }
+        }
     }
 }

ECoreNetto.Tools/Services/VersionChecker.cs

@@ -21,6 +21,7 @@
 namespace ECoreNetto.Tools.Services
 {
     using System;
+    using System.Buffers;
     using System.Net.Http;
     using System.Reflection;
     using System.Text.Json;
@@ -47,6 +48,17 @@ public class VersionChecker : IVersionChecker
         /// </summary>
         private readonly IHttpClientFactory httpClientFactory;
 
+        /// <summary>
+        /// The default GitHub API URL used to query the latest release
+        /// </summary>
+        public const string DefaultReleasesUrl = "https://api.github.com/repos/STARIONGROUP/EcoreNetto/releases/latest";
+
+        /// <summary>
+        /// The cached <see cref="SearchValues{T}"/> of the SemVer pre-release / build-metadata separators
+        /// used when trimming a tag name before version parsing.
+        /// </summary>
+        private static readonly SearchValues<char> SemVerSuffixSeparators = SearchValues.Create("-+");
+
         /// <summary>
         /// Initializes a new instance of the <see cref="VersionChecker"/>
         /// </summary>
@@ -56,12 +68,30 @@ public class VersionChecker : IVersionChecker
         /// <param name="loggerFactory">
         /// The (injected) <see cref="ILoggerFactory"/> used to set up logging
         /// </param>
-        public VersionChecker(IHttpClientFactory httpClientFactory, ILoggerFactory? loggerFactory = null)
+        /// <param name="releasesUrl">
+        /// The GitHub API URL used to query the latest release. When null, <see cref="DefaultReleasesUrl"/> is used.
+        /// </param>
+        /// <param name="timeout">
+        /// The timeout applied to the HTTP request. When null, a default of 2 seconds is used.
+        /// </param>
+        public VersionChecker(IHttpClientFactory httpClientFactory, ILoggerFactory? loggerFactory = null, string? releasesUrl = null, TimeSpan? timeout = null)
         {
             this.httpClientFactory = httpClientFactory;
             this.logger = loggerFactory == null ? NullLogger<VersionChecker>.Instance : loggerFactory.CreateLogger<VersionChecker>();
+            this.ReleasesUrl = releasesUrl ?? DefaultReleasesUrl;
+            this.Timeout = timeout ?? TimeSpan.FromSeconds(2);
         }
 
+        /// <summary>
+        /// Gets the GitHub API URL used to query the latest release
+        /// </summary>
+        public string ReleasesUrl { get; }
+
+        /// <summary>
+        /// Gets the timeout applied to the HTTP request
+        /// </summary>
+        public TimeSpan Timeout { get; }
+
         /// <summary>
         /// Checks for the lastest release
         /// </summary>
@@ -82,7 +112,12 @@ public async Task ExecuteAsync(CancellationToken cancellationToken)
                 if (payload != null)
                 {
                     var currentVersion = Assembly.GetExecutingAssembly().GetName().Version;
-                    var publishedVersion = new Version(payload.TagName);
+
+                    if (!TryParseVersion(payload.TagName, out var publishedVersion))
+                    {
+                        this.logger.LogWarning("Unable to parse the published version '{TagName}' returned by the GitHub API", payload.TagName);
+                        return;
+                    }
 
                     if (currentVersion < publishedVersion)
                     {
@@ -120,15 +155,13 @@ public async Task ExecuteAsync(CancellationToken cancellationToken)
         public async Task<GitHubRelease?> QueryLatestReleaseAsync(CancellationToken cancellationToken)
         {
             var httpClient = this.httpClientFactory.CreateClient();
-            httpClient.Timeout = TimeSpan.FromSeconds(2);
-
-            const string requestUrl = "https://api.github.com/repos/STARIONGROUP/EcoreNetto/releases/latest";
+            httpClient.Timeout = this.Timeout;
 
             try
             {
                 httpClient.DefaultRequestHeaders.UserAgent.ParseAdd("ECoreNetto.Tools");
 
-                var response = await httpClient.GetAsync(requestUrl, cancellationToken);
+                var response = await httpClient.GetAsync(this.ReleasesUrl, cancellationToken);
 
                 if (response.IsSuccessStatusCode)
                 {
@@ -140,14 +173,47 @@ public async Task ExecuteAsync(CancellationToken cancellationToken)
             }
             catch (TaskCanceledException taskCanceledException)
             {
-                this.logger.LogWarning(taskCanceledException, "Contacting the GitGub API at {Url} timed out", requestUrl);
+                this.logger.LogWarning(taskCanceledException, "Contacting the GitHub API at {Url} timed out", this.ReleasesUrl);
             }
             catch (Exception ex)
             {
-                this.logger.LogError(ex, "");
+                this.logger.LogError(ex, "An error occurred while querying the latest release from the GitHub API at {Url}", this.ReleasesUrl);
             }
 
             return null;
         }
+
+        /// <summary>
+        /// Attempts to parse the version from a GitHub release tag name, tolerating a leading
+        /// 'v'/'V' prefix and any SemVer pre-release or build-metadata suffix (e.g. '-beta', '+build').
+        /// </summary>
+        /// <param name="tagName">
+        /// The raw tag name returned by the GitHub API
+        /// </param>
+        /// <param name="version">
+        /// The parsed <see cref="Version"/> when parsing succeeds; otherwise null.
+        /// </param>
+        /// <returns>
+        /// true when the tag name could be parsed into a <see cref="Version"/>, false otherwise.
+        /// </returns>
+        private static bool TryParseVersion(string? tagName, out Version? version)
+        {
+            version = null;
+
+            if (string.IsNullOrWhiteSpace(tagName))
+            {
+                return false;
+            }
+
+            var candidate = tagName!.Trim().TrimStart('v', 'V');
+
+            var suffixIndex = candidate.AsSpan().IndexOfAny(SemVerSuffixSeparators);
+            if (suffixIndex >= 0)
+            {
+                candidate = candidate.Substring(0, suffixIndex);
+            }
+
+            return Version.TryParse(candidate, out version);
+        }
     }
 }

ECoreNetto/ModelElement/EObject.cs

@@ -85,7 +85,7 @@
         /// <returns>
         /// The meta class.
         /// </returns>
         public EClass EClass()
         {
             throw new NotImplementedException();
         }
@@ -110,7 +110,7 @@
         /// <returns>
         /// The <see cref="EStructuralFeature"/> that actually contains the object.
         /// </returns>
         public EStructuralFeature EContainingFeature()
         {
             throw new NotImplementedException();
         }
@@ -123,7 +123,7 @@
         /// <returns>
         /// The feature that properly contains the object.
         /// </returns>
         public EReference EContainmentFeature()
         {
             throw new NotImplementedException();
         }
@@ -138,7 +138,7 @@
         /// <returns>
         /// A list view of the content objects.
         /// </returns>
         public IEnumerable<EObject> EContents()
         {
             throw new NotImplementedException();
         }
@@ -149,7 +149,7 @@
         /// <returns>
         /// A tree iterator that iterates over all contents.
         /// </returns>
         public IEnumerable<EObject> EAllContents()
         {
             throw new NotImplementedException();
         }
@@ -165,7 +165,7 @@
         /// <returns>
         /// true if this object is a proxy or false, otherwise.
         /// </returns>
         public bool EIsProxy()
         {
             throw new NotImplementedException();
         }
@@ -181,7 +181,7 @@
         /// <returns>
         /// A list view of the cross referenced objects. 
         /// </returns>
         public IEnumerable<EObject> ECrossReferences()
         {
             throw new NotImplementedException();
         }
@@ -220,7 +220,7 @@
         /// <exception cref="ArgumentException">
         /// If the <see cref="EStructuralFeature"/> is not one the meta class's features and is also not affiliated with one of the meta class's features. 
         /// </exception>        
         public object EGet(EStructuralFeature feature, bool resolve)
         {
             throw new NotImplementedException();
         }
@@ -248,7 +248,7 @@
         /// <exception cref="InvalidCastException">
         /// If there is a type conflict.
         /// </exception>
         public void ESet(EStructuralFeature feature, object newValue)
         {
             throw new NotImplementedException();
         }
@@ -272,7 +272,7 @@
         /// <exception cref="ArgumentException">
         /// If the feature is not one the meta class's <see cref="EStructuralFeature"/>s.
         /// </exception>
         public bool EIsSet(EStructuralFeature feature)
         {
             throw new NotImplementedException();
         }
@@ -377,6 +377,12 @@
             }
         }
 
+        /// <summary>
+        /// The names of the reference attributes whose values may contain implicit references
+        /// that need to be rewritten to point at the current top package.
+        /// </summary>
+        private static readonly string[] ReferenceAttributes = { "eType", "eSuperTypes", "eOpposite" };
+
         /// <summary>
         /// Process the attribute value and rewrite if required.
         /// </summary>
@@ -391,9 +397,7 @@
         /// </returns>
         private string ProcessAttributeValue(string attributeName, string attributeValue)
         {
-            var referenceAttributes = new[] { "eType", "eSuperTypes", "eOpposite" };
-
-            if (!referenceAttributes.Contains(attributeName))
+            if (!ReferenceAttributes.Contains(attributeName))
             {
                 // nothing to do
                 return attributeValue;