Simplify UrlUtils.equals(), fix percent sign in tests

bitfireAT · Apr 10, 2024 · 93c8350 · 93c8350
1 parent 92f5f3d
commit 93c8350
Show file tree

Hide file tree

Showing 2 changed files with 31 additions and 18 deletions.
diff --git a/src/main/kotlin/at/bitfire/dav4jvm/UrlUtils.kt b/src/main/kotlin/at/bitfire/dav4jvm/UrlUtils.kt
@@ -9,9 +9,14 @@ package at.bitfire.dav4jvm
 import okhttp3.HttpUrl
 import java.net.URI
 import java.net.URISyntaxException
+import java.net.URLDecoder
+import java.util.logging.Level
+import java.util.logging.Logger
 
 object UrlUtils {
 
+    val logger by lazy { Logger.getLogger(javaClass.name) }
+
     /**
      * Compares two URLs in WebDAV context. If two URLs are considered *equal*, both
      * represent the same WebDAV resource (e.g. `http://host:80/folder1` and `http://HOST/folder1#somefragment`).
@@ -35,15 +40,20 @@ object UrlUtils {
         if (url1 == url2)
             return true
 
-        // drop #fragment parts and convert to URI
-        val uri1 = url1.newBuilder().fragment(null).build().toUri()
-        val uri2 = url2.newBuilder().fragment(null).build().toUri()
+        // convert to java.net.URI (also corrects some mistakes)
+        val uri1 = url1.toUri()
+        val uri2 = url2.toUri()
+
+        // if the URIs are the same (ignoring scheme case and fragments), they're equal for us
+        if (uri1.scheme.equals(uri2.scheme, true) && uri1.schemeSpecificPart == uri2.schemeSpecificPart)
+            return true
 
         return try {
-            val decoded1 = URI(uri1.scheme, uri1.schemeSpecificPart, uri1.fragment)
-            val decoded2 = URI(uri2.scheme, uri2.schemeSpecificPart, uri2.fragment)
+            val decoded1 = URI(url1.scheme, uri1.schemeSpecificPart, null)
+            val decoded2 = URI(uri2.scheme, uri2.schemeSpecificPart, null)
             decoded1 == decoded2
         } catch (e: URISyntaxException) {
+            logger.log(Level.WARNING, "Couldn't decode URI for comparison, assuming inequality", e)
             false
         }
     }

diff --git a/src/test/kotlin/at/bitfire/dav4jvm/UrlUtilsTest.kt b/src/test/kotlin/at/bitfire/dav4jvm/UrlUtilsTest.kt
@@ -6,6 +6,8 @@
 
 package at.bitfire.dav4jvm
 
+import okhttp3.HttpUrl
+import okhttp3.HttpUrl.Companion.toHttpUrl
 import okhttp3.HttpUrl.Companion.toHttpUrlOrNull
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertFalse
@@ -17,17 +19,18 @@ class UrlUtilsTest {
 
     @Test
     fun testEquals() {
-        assertTrue(UrlUtils.equals("http://host/resource".toHttpUrlOrNull()!!, "http://host/resource".toHttpUrlOrNull()!!))
-        assertTrue(UrlUtils.equals("http://host:80/resource".toHttpUrlOrNull()!!, "http://host/resource".toHttpUrlOrNull()!!))
-        assertTrue(UrlUtils.equals("https://HOST:443/resource".toHttpUrlOrNull()!!, "https://host/resource".toHttpUrlOrNull()!!))
-        assertTrue(UrlUtils.equals("https://host:443/my@dav/".toHttpUrlOrNull()!!, "https://host/my%40dav/".toHttpUrlOrNull()!!))
-        assertTrue(UrlUtils.equals("http://host/resource".toHttpUrlOrNull()!!, "http://host/resource#frag1".toHttpUrlOrNull()!!))
+        assertTrue(UrlUtils.equals("http://host/resource".toHttpUrl(), "http://host/resource".toHttpUrl()))
+        assertTrue(UrlUtils.equals("http://host:80/resource".toHttpUrl(), "http://host/resource".toHttpUrl()))
+        assertTrue(UrlUtils.equals("https://HOST:443/resource".toHttpUrl(), "https://host/resource".toHttpUrl()))
+        assertTrue(UrlUtils.equals("https://host:443/my@dav/".toHttpUrl(), "https://host/my%40dav/".toHttpUrl()))
+        assertTrue(UrlUtils.equals("http://host/resource".toHttpUrl(), "http://host/resource#frag1".toHttpUrl()))
 
-        // should work, but currently doesn't (see MR #5)
-        // assertTrue(UrlUtils.equals(HttpUrl.parse("https://host/%5bresource%5d/")!!, HttpUrl.parse("https://host/[resource]/")!!))
+        assertFalse(UrlUtils.equals("http://host/resource".toHttpUrl(), "http://host/resource/".toHttpUrl()))
+        assertFalse(UrlUtils.equals("http://host/resource".toHttpUrl(), "http://host:81/resource".toHttpUrl()))
 
-        assertFalse(UrlUtils.equals("http://host/resource".toHttpUrlOrNull()!!, "http://host/resource/".toHttpUrlOrNull()!!))
-        assertFalse(UrlUtils.equals("http://host/resource".toHttpUrlOrNull()!!, "http://host:81/resource".toHttpUrlOrNull()!!))
+        assertTrue(UrlUtils.equals("https://www.example.com/folder/[X]Y!.txt".toHttpUrl(), "https://www.example.com/folder/[X]Y!.txt".toHttpUrl()))
+        assertTrue(UrlUtils.equals("https://www.example.com/folder/%5BX%5DY!.txt".toHttpUrl(), "https://www.example.com/folder/[X]Y!.txt".toHttpUrl()))
+        assertTrue(UrlUtils.equals("https://www.example.com/folder/%5bX%5dY%21.txt".toHttpUrl(), "https://www.example.com/folder/[X]Y!.txt".toHttpUrl()))
     }
 
     @Test
@@ -48,14 +51,14 @@ class UrlUtilsTest {
 
     @Test
     fun testOmitTrailingSlash() {
-        assertEquals("http://host/resource".toHttpUrlOrNull()!!, UrlUtils.omitTrailingSlash("http://host/resource".toHttpUrlOrNull()!!))
-        assertEquals("http://host/resource".toHttpUrlOrNull()!!, UrlUtils.omitTrailingSlash("http://host/resource/".toHttpUrlOrNull()!!))
+        assertEquals("http://host/resource".toHttpUrl(), UrlUtils.omitTrailingSlash("http://host/resource".toHttpUrl()))
+        assertEquals("http://host/resource".toHttpUrl(), UrlUtils.omitTrailingSlash("http://host/resource/".toHttpUrl()))
     }
 
     @Test
     fun testWithTrailingSlash() {
-        assertEquals("http://host/resource/".toHttpUrlOrNull()!!, UrlUtils.withTrailingSlash("http://host/resource".toHttpUrlOrNull()!!))
-        assertEquals("http://host/resource/".toHttpUrlOrNull()!!, UrlUtils.withTrailingSlash("http://host/resource/".toHttpUrlOrNull()!!))
+        assertEquals("http://host/resource/".toHttpUrl(), UrlUtils.withTrailingSlash("http://host/resource".toHttpUrl()))
+        assertEquals("http://host/resource/".toHttpUrl(), UrlUtils.withTrailingSlash("http://host/resource/".toHttpUrl()))
     }
 
 }