Don't use EscapeDataString for full URLs, delete dead tests

Cmdline/Action/Show.cs

@@ -258,43 +258,53 @@ private int ShowMod(CkanModule module, ShowOptions opts)
                 user.RaiseMessage(Properties.Resources.ShowResourcesHeader);
                 if (module.resources.homepage != null)
                 {
-                    user.RaiseMessage(Properties.Resources.ShowHomePage, Uri.EscapeDataString(module.resources.homepage.ToString()));
+                    user.RaiseMessage(Properties.Resources.ShowHomePage,
+                                      Net.NormalizeUri(module.resources.homepage.ToString()));
                 }
                 if (module.resources.manual != null)
                 {
-                    user.RaiseMessage(Properties.Resources.ShowManual, Uri.EscapeDataString(module.resources.manual.ToString()));
+                    user.RaiseMessage(Properties.Resources.ShowManual,
+                                      Net.NormalizeUri(module.resources.manual.ToString()));
                 }
                 if (module.resources.spacedock != null)
                 {
-                    user.RaiseMessage(Properties.Resources.ShowSpaceDock, Uri.EscapeDataString(module.resources.spacedock.ToString()));
+                    user.RaiseMessage(Properties.Resources.ShowSpaceDock,
+                                      Net.NormalizeUri(module.resources.spacedock.ToString()));
                 }
                 if (module.resources.repository != null)
                 {
-                    user.RaiseMessage(Properties.Resources.ShowRepository, Uri.EscapeDataString(module.resources.repository.ToString()));
+                    user.RaiseMessage(Properties.Resources.ShowRepository,
+                                      Net.NormalizeUri(module.resources.repository.ToString()));
                 }
                 if (module.resources.bugtracker != null)
                 {
-                    user.RaiseMessage(Properties.Resources.ShowBugTracker, Uri.EscapeDataString(module.resources.bugtracker.ToString()));
+                    user.RaiseMessage(Properties.Resources.ShowBugTracker,
+                                      Net.NormalizeUri(module.resources.bugtracker.ToString()));
                 }
                 if (module.resources.curse != null)
                 {
-                    user.RaiseMessage(Properties.Resources.ShowCurse, Uri.EscapeDataString(module.resources.curse.ToString()));
+                    user.RaiseMessage(Properties.Resources.ShowCurse,
+                                      Net.NormalizeUri(module.resources.curse.ToString()));
                 }
                 if (module.resources.store != null)
                 {
-                    user.RaiseMessage(Properties.Resources.ShowStore, Uri.EscapeDataString(module.resources.store.ToString()));
+                    user.RaiseMessage(Properties.Resources.ShowStore,
+                                      Net.NormalizeUri(module.resources.store.ToString()));
                 }
                 if (module.resources.steamstore != null)
                 {
-                    user.RaiseMessage(Properties.Resources.ShowSteamStore, Uri.EscapeDataString(module.resources.steamstore.ToString()));
+                    user.RaiseMessage(Properties.Resources.ShowSteamStore,
+                                      Net.NormalizeUri(module.resources.steamstore.ToString()));
                 }
                 if (module.resources.remoteAvc != null)
                 {
-                    user.RaiseMessage(Properties.Resources.ShowVersionFile, Uri.EscapeDataString(module.resources.remoteAvc.ToString()));
+                    user.RaiseMessage(Properties.Resources.ShowVersionFile,
+                                      Net.NormalizeUri(module.resources.remoteAvc.ToString()));
                 }
                 if (module.resources.remoteSWInfo != null)
                 {
-                    user.RaiseMessage(Properties.Resources.ShowSpaceWarpInfo, Uri.EscapeDataString(module.resources.remoteSWInfo.ToString()));
+                    user.RaiseMessage(Properties.Resources.ShowSpaceWarpInfo,
+                                      Net.NormalizeUri(module.resources.remoteSWInfo.ToString()));
                 }
             }
 

Core/Net/Net.cs

@@ -260,7 +260,7 @@ public static Uri ResolveRedirect(Uri url)
             {
                 var req = new HttpRequestMessage(HttpMethod.Head, url);
                 req.Headers.UserAgent.Clear();
-                req.Headers.UserAgent.Add(new ProductInfoHeaderValue(UserAgentString));
+                req.Headers.UserAgent.ParseAdd(UserAgentString);
                 var response = nonRedirectingHttpClient.SendAsync(
                     req, HttpCompletionOption.ResponseHeadersRead).Result;
                 if (response.Headers.Location == null)
@@ -286,6 +286,52 @@ public static Uri ResolveRedirect(Uri url)
             return null;
         }
 
+        /// <summary>
+        /// Provide an escaped version of the given Uri string, including converting
+        /// square brackets to their escaped forms.
+        /// </summary>
+        /// <returns>
+        /// <c>null</c> if the string is not a valid <see cref="Uri"/>, otherwise its normalized form.
+        /// </returns>
+        public static string NormalizeUri(string uri)
+        {
+            // Uri.EscapeUriString has been deprecated because its purpose was ambiguous.
+            // Is it supposed to turn a "&" into part of the content of a form field,
+            // or is it supposed to assume that it separates different form fields?
+            // https://github.com/dotnet/runtime/issues/31387
+            // So now we have to just substitude certain characters ourselves one by one.
+
+            // Square brackets are "reserved characters" that should not appear
+            // in strings to begin with, so C# doesn't try to escape them in case
+            // they're being used in a special way. They're not; some mod authors
+            // just have crazy ideas as to what should be in a URL, and SD doesn't
+            // escape them in its API. There's probably more in RFC 3986.
+            var escaped = UriEscapeAll(uri.Replace(" ", "+"),
+                                       '"', '<', '>', '^', '`',
+                                       '{', '|', '}', '[', ']');
+
+            // Make sure we have a "http://" or "https://" start.
+            if (!Regex.IsMatch(escaped, "(?i)^(http|https)://"))
+            {
+                // Prepend "http://", as we do not know if the site supports https.
+                escaped = "http://" + escaped;
+            }
+
+            if (Uri.IsWellFormedUriString(escaped, UriKind.Absolute))
+            {
+                return escaped;
+            }
+            else
+            {
+                return null;
+            }
+        }
+
+        private static string UriEscapeAll(string orig, params char[] characters)
+            => characters.Aggregate(orig,
+                                    (s, c) => s.Replace(c.ToString(),
+                                                        Uri.HexEscape(c)));
+
         /// <summary>
         /// Translate a URL into a form that returns the raw contents of a file
         /// Only changes GitHub URLs, others are left as-is

Core/Relationships/RelationshipResolver.cs

@@ -878,7 +878,6 @@ public override CkanModule Parent
             {
                 get
                 {
-                    Debug.Assert(false);
                     throw new Exception("Should never be called on Installed");
                 }
             }
@@ -893,7 +892,6 @@ public override CkanModule Parent
             {
                 get
                 {
-                    Debug.Assert(false);
                     throw new Exception("Should never be called on UserRequested");
                 }
             }

Netkan/Sources/Curse/CurseApi.cs

@@ -38,7 +38,7 @@ public CurseMod GetMod(string nameOrId)
             }
             // Check if the mod has been removed from Curse and if it corresponds to a KSP mod.
             var error = JsonConvert.DeserializeObject<CurseError>(json);
-            if (!string.IsNullOrWhiteSpace(error.error))
+            if (!string.IsNullOrWhiteSpace(error?.error))
             {
                 throw new Kraken($"Could not get the mod from Curse, reason: {error.message}.");
             }

Netkan/Sources/Curse/CurseMod.cs

@@ -91,9 +91,12 @@ public override string ToString()
         public static CurseMod FromJson(string json)
         {
             CurseMod mod = JsonConvert.DeserializeObject<CurseMod>(json);
-            foreach (CurseFile file in mod.files)
+            if (mod != null)
             {
-                file.ModPageUrl = mod.GetPageUrl();
+                foreach (CurseFile file in mod.files)
+                {
+                    file.ModPageUrl = mod.GetPageUrl();
+                }
             }
             return mod;
         }

Netkan/Transformers/CurseTransformer.cs

@@ -263,54 +263,6 @@ private Metadata TransformOne(JObject json, CurseMod curseMod, CurseFile latestV
         }
 
         private static string Normalize(Uri uri)
-        {
-            return Normalize(uri.ToString());
-        }
-
-        /// <summary>
-        /// Provide an escaped version of the given Uri string, including converting
-        /// square brackets to their escaped forms.
-        /// </summary>
-        /// <returns>
-        /// <c>null</c> if the string is not a valid <see cref="Uri"/>, otherwise its normlized form.
-        /// </returns>
-        private static string Normalize(string uri)
-        {
-            if (uri == null)
-            {
-                return null;
-            }
-
-            Log.DebugFormat("Escaping {0}", uri);
-
-            var escaped = Uri.EscapeDataString(uri);
-
-            // Square brackets are "reserved characters" that should not appear
-            // in strings to begin with, so C# doesn't try to escape them in case
-            // they're being used in a special way. They're not; some mod authors
-            // just have crazy ideas as to what should be in a URL, and Curse doesn't
-            // escape them in its API. There's probably more in RFC 3986.
-
-            escaped = escaped.Replace("[", Uri.HexEscape('['));
-            escaped = escaped.Replace("]", Uri.HexEscape(']'));
-
-            // Make sure we have a "http://" or "https://" start.
-            if (!Regex.IsMatch(escaped, "(?i)^(http|https)://"))
-            {
-                // Prepend "http://", as we do not know if the site supports https.
-                escaped = "http://" + escaped;
-            }
-
-            if (Uri.IsWellFormedUriString(escaped, UriKind.Absolute))
-            {
-                Log.DebugFormat("Escaped to {0}", escaped);
-                return escaped;
-            }
-            else
-            {
-                Log.WarnFormat("Could not normalize URL: {0}", uri);
-                return null;
-            }
-        }
+            => Net.NormalizeUri(uri.ToString());
     }
 }

Netkan/Transformers/JenkinsTransformer.cs

@@ -72,9 +72,7 @@ private Metadata TransformOne(Metadata metadata, JObject json, JenkinsBuild buil
                 case 1:
                     JenkinsArtifact artifact = artifacts.Single();
 
-                    string download = Uri.EscapeDataString(
-                        $"{build.Url}artifact/{artifact.RelativePath}"
-                    );
+                    string download = $"{build.Url}artifact/{artifact.RelativePath}";
                     Log.DebugFormat("Using download URL: {0}", download);
                     json.Remove("$kref");
                     json.SafeAdd("download", download);
@@ -93,7 +91,7 @@ private Metadata TransformOne(Metadata metadata, JObject json, JenkinsBuild buil
                     }
 
                     var resourcesJson = (JObject)json["resources"];
-                    resourcesJson.SafeAdd("ci", Uri.EscapeDataString(metadata.Kref.Id));
+                    resourcesJson.SafeAdd("ci", metadata.Kref.Id);
 
                     Log.DebugFormat("Transformed metadata:{0}{1}", Environment.NewLine, json);
 

Netkan/Transformers/SpacedockTransformer.cs

@@ -186,7 +186,7 @@ private void TryAddResourceURL(string identifier, JObject resources, string key,
         {
             if (!string.IsNullOrEmpty(rawURL))
             {
-                string normalized = Normalize(rawURL);
+                string normalized = Net.NormalizeUri(rawURL);
                 if (!string.IsNullOrEmpty(normalized))
                 {
                     resources.SafeAdd(key, normalized);
@@ -198,46 +198,6 @@ private void TryAddResourceURL(string identifier, JObject resources, string key,
             }
         }
 
-        /// <summary>
-        /// Provide an escaped version of the given Uri string, including converting
-        /// square brackets to their escaped forms.
-        /// </summary>
-        /// <returns>
-        /// <c>null</c> if the string is not a valid <see cref="Uri"/>, otherwise its normlized form.
-        /// </returns>
-        private static string Normalize(string uri)
-        {
-            Log.DebugFormat("Escaping {0}", uri);
-
-            var escaped = Uri.EscapeDataString(uri);
-
-            // Square brackets are "reserved characters" that should not appear
-            // in strings to begin with, so C# doesn't try to escape them in case
-            // they're being used in a special way. They're not; some mod authors
-            // just have crazy ideas as to what should be in a URL, and SD doesn't
-            // escape them in its API. There's probably more in RFC 3986.
-
-            escaped = escaped.Replace("[", Uri.HexEscape('['));
-            escaped = escaped.Replace("]", Uri.HexEscape(']'));
-
-            // Make sure we have a "http://" or "https://" start.
-            if (!Regex.IsMatch(escaped, "(?i)^(http|https)://"))
-            {
-                // Prepend "http://", as we do not know if the site supports https.
-                escaped = "http://" + escaped;
-            }
-
-            if (Uri.IsWellFormedUriString(escaped, UriKind.Absolute))
-            {
-                Log.DebugFormat("Escaped to {0}", escaped);
-                return escaped;
-            }
-            else
-            {
-                return null;
-            }
-        }
-
         private static List<string> GetAuthors(SpacedockMod mod)
         {
             var result = new List<string> { mod.author };

Tests/Core/Net/NetAsyncModulesDownloaderTests.cs

@@ -140,75 +140,5 @@ public void TargetFromModuleGroup_WithModules_ExpectedTarget(string[] moduleJson
                 Assert.AreEqual(correctURLs, urls);
             }
         }
-
-        [Test]
-        [Category("Online")]
-        [Category("FlakyNetwork")]
-        public void SingleDownload()
-        {
-            log.Info("Performing single download test.");
-
-            // We know kOS is in the TestKAN data, and hosted in KS. Let's get it.
-
-            var modules = new List<CkanModule>();
-
-            CkanModule kOS = registry.LatestAvailable("kOS", null);
-            Assert.IsNotNull(kOS);
-
-            modules.Add(kOS);
-
-            // Make sure we don't alread have kOS somehow.
-            Assert.IsFalse(cache.IsCached(kOS));
-
-            //
-            log.InfoFormat("Downloading kOS from {0}", kOS.download);
-
-            // Download our module.
-            async.DownloadModules(modules);
-
-            // Assert that we have it (which meansit passed zip validation)
-            Assert.IsTrue(cache.IsCached(kOS));
-        }
-
-        [Test]
-        [Category("Online")]
-        [Category("FlakyNetwork")]
-        public void MultiDownload()
-        {
-            var modules = new List<CkanModule>();
-
-            CkanModule kOS = registry.LatestAvailable("kOS", null);
-            CkanModule quick_revert = registry.LatestAvailable("QuickRevert", null);
-
-            modules.Add(kOS);
-            modules.Add(quick_revert);
-
-            Assert.IsFalse(cache.IsCached(kOS));
-            Assert.IsFalse(cache.IsCached(quick_revert));
-
-            async.DownloadModules(modules);
-
-            Assert.IsTrue(cache.IsCached(kOS));
-            Assert.IsTrue(cache.IsCached(quick_revert));
-        }
-
-        [Test]
-        [Category("Online")]
-        [Category("FlakyNetwork")]
-        public void RandSdownload()
-        {
-            var modules = new List<CkanModule>();
-
-            var rAndS = TestData.RandSCapsuleDyneModule();
-
-            modules.Add(rAndS);
-
-            Assert.IsFalse(cache.IsCached(rAndS), "Module not yet downloaded");
-
-            async.DownloadModules(modules);
-
-            Assert.IsTrue(cache.IsCached(rAndS), "Module download successful");
-        }
-
     }
 }