Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add subScheme Support and Align with RFC 3986 Section 3.1 for Scheme Extraction (Fixes #2101) #2110

Merged
merged 5 commits into from
Jan 3, 2024
Merged

Add subScheme Support and Align with RFC 3986 Section 3.1 for Scheme Extraction (Fixes #2101) #2110

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3369d0e
add subScheme support in URL.kt
itboy87 Jan 1, 2024
d1b4385
support for scheme standard according to rfc3986#section-3.1
itboy87 Jan 1, 2024
0a95008
Merge branch 'main' into fix/2101-add-subprotocol-support-in-url
itboy87 Jan 2, 2024
ed6f635
URL.invoke overload function for backward compatibility.
itboy87 Jan 3, 2024
4bafcd7
Deprecate URL.invoke with parameters and introduce replacement using …
itboy87 Jan 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions korge-core/src/korlibs/io/net/URL.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import korlibs.io.lang.*
data class URL private constructor(
val isOpaque: Boolean,
val scheme: String?,
val subScheme: String?,
val userInfo: String?,
val host: String?,
val path: String,
Expand Down Expand Up @@ -43,6 +44,7 @@ data class URL private constructor(
fun toUrlString(includeScheme: Boolean = true, out: StringBuilder = StringBuilder()): StringBuilder {
if (includeScheme && scheme != null) {
out.append("$scheme:")
if (subScheme != null) out.append("$subScheme:")
if (!isOpaque) out.append("//")
}
if (userInfo != null) out.append("$userInfo@")
Expand All @@ -58,7 +60,7 @@ data class URL private constructor(

override fun toString(): String = fullUrl
fun toComponentString(): String {
return "URL(" + listOf(::scheme, ::userInfo, ::host, ::path, ::query, ::fragment)
return "URL(" + listOf(::scheme, ::subScheme, ::userInfo, ::host, ::path, ::query, ::fragment)
.map { it.name to it.get() }
.filter { it.second != null }
.joinToString(", ") { "${it.first}=${it.second}" } + ")"
Expand All @@ -79,16 +81,27 @@ data class URL private constructor(

operator fun invoke(
scheme: String?,
subScheme: String?,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should either create a new overload keeping the old (for compatibility), or placing the subScheme at the end to avoid people not using named arguments to have broken code

Copy link
Member

@soywiz soywiz Jan 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively maybe we could just support subschemes by using scheme = "jdbc:mysql" and validating it somewhere else and providing subScheme and mainScheme or something like that as read-only properties?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should create an overload function and keep the subScheme variable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we create a fun fromComponents, keep the invoke as it is and mark it as deprecated? The main problem is that since we have several strings in a row and named arguments are not mandatory, this might be a compatibility problem

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per my suggestion, URL(..) offers a much better and developer-friendly syntax compared to URL.fromComponents.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, we should at least deprecate the other signature:

But let me insist:

The problem with the invoke is probably disambiguation in some circumstances.

Also, let's consider this example:

val url2 = URL(scheme = "HTTPs", userInfo = null, host = "Google.com", path = "", query = null, fragment = null)

I would say that forcing to provide nulls for all the arguments is also not a good experience. It would be better to provide all the values as null by default, and since we have a typical URL(fullUrl: String) signature, that might be a problem.

I would say it could be better to have:

val url2 = URL.fromComponents(scheme = "HTTPs", host = "Google.com")
//
val url2 = URL.fromComponents(scheme = "jdbc",  subScheme ="mysql", host = "localhost")

No disambiguation required.

Also you can keep the current invoke, without adding the extra subScheme to the end nulled by default, so current code still works like this even if used without named arguments:

fun fromComponents(
  scheme: String? = null,
  subScheme: String? = null,
  userInfo: String? = null,
  host: String? = null,
  path: String = "",
  query: String? = null,
  fragment: String? = null,
  opaque: Boolean = false,
  port: Int = DEFAULT_PORT,
) = ...

@Deprecated(replaceWith = ReplaceWith("URL.fromComponents(scheme, subScheme, userInfo, host, path, query, fragment, opaque)"))
operator fun invoke(
  scheme: String?,
  userInfo: String?,
  host: String?,
  path: String,
  query: String?,
  fragment: String?,
  opaque: Boolean = false,
  port: Int = DEFAULT_PORT,
  subScheme: String? = null,
): URL = URL.fromComponents(scheme, subScheme, userInfo, host, path, query, fragment, opaque)

with the Deprecated, it gives people visibility to the new function. It allows providing all the arguments by default so not having to provide every argument not used as null, as there is no signature conflict with URL(String).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, that sounds better.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made changes. Could you please take a look?

userInfo: String?,
host: String?,
path: String,
query: String?,
fragment: String?,
opaque: Boolean = false,
port: Int = DEFAULT_PORT
): URL = URL(opaque, scheme?.lowercase(), userInfo, host, path, query, fragment, port)

private val schemeRegex = Regex("\\w+:")
): URL = URL(
isOpaque = opaque,
scheme = scheme?.lowercase(),
subScheme = subScheme?.lowercase(),
userInfo = userInfo,
host = host,
path = path,
query = query,
fragment = fragment,
defaultPort = port
)

private val schemeRegex = Regex("^([a-zA-Z0-9+.-]+)(?::([a-zA-Z]+))?:")

operator fun invoke(url: String): URL {
val r = StrReader(url)
Expand All @@ -97,7 +110,9 @@ data class URL private constructor(
schemeColon != null -> {
val isHierarchical = r.tryLit("//") != null
val nonScheme = r.readRemaining()
val scheme = schemeColon.dropLast(1)
val schemeParts = schemeColon.dropLast(1).split(":")
val scheme = schemeParts[0]
val subScheme = schemeParts.getOrNull(1)

val nonFragment = nonScheme.substringBefore('#')
val fragment = nonScheme.substringAfterOrNull('#')
Expand All @@ -117,6 +132,7 @@ data class URL private constructor(
URL(
opaque = !isHierarchical,
scheme = scheme,
subScheme = subScheme,
userInfo = userInfo,
host = host.takeIf { it.isNotEmpty() },
path = if (path != null) "/$path" else "",
Expand All @@ -133,6 +149,7 @@ data class URL private constructor(
URL(
opaque = false,
scheme = null,
subScheme = null,
userInfo = null,
host = null,
path = path,
Expand Down
57 changes: 56 additions & 1 deletion korge-core/test/korlibs/io/net/URLTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,65 @@ class URLTest {
assertEquals("q=1", url.query)
assertEquals("https://Google.com:442/test?q=1", url.fullUrl)

val url2 = URL(scheme = "HTTPs", userInfo = null, host = "Google.com", path = "", query = null, fragment = null)
val url2 = URL(scheme = "HTTPs", subScheme = null, userInfo = null, host = "Google.com", path = "", query = null, fragment = null)
assertEquals("https", url2.scheme)
assertEquals(443, url2.port)
assertEquals("https://Google.com", url2.fullUrl)

val url3 = URL("https://username:[email protected]:8443/path/to/resource?q=1")
assertEquals("https", url3.scheme)
assertNull(url3.subScheme)
assertEquals("username:password", url3.userInfo)
assertEquals("username", url3.user)
assertEquals("password", url3.password)
assertEquals("example.com", url3.host)
assertEquals(8443, url3.port)
assertEquals("/path/to/resource", url3.path)
assertEquals("/path/to/resource?q=1", url3.pathWithQuery)
assertEquals("q=1", url3.query)
assertEquals("https://username:[email protected]:8443/path/to/resource?q=1", url3.fullUrl)
}

@Test
fun testSubSchemeUrls(){
// <scheme>:<subScheme>://<host>:<port>/<path>?<query_parameters>#<fragment>
val url = URL("jdbc:mysql://localhost:3306/database?useSSL=false")
assertEquals("jdbc", url.scheme) // always lowercase issue #2092
assertEquals("mysql", url.subScheme)
assertEquals("localhost", url.host)
assertEquals(3306, url.port)
assertEquals("/database", url.path)
assertEquals("/database?useSSL=false", url.pathWithQuery)
assertEquals("useSSL=false", url.query)
assertEquals("jdbc:mysql://localhost:3306/database?useSSL=false", url.fullUrl)

val url2 = URL(scheme = "jdbc", subScheme = "mysql", userInfo = null, host = "localhost", path = "/database", query = null, fragment = null, port = 3306)
assertEquals("jdbc:mysql://localhost:3306/database", url2.fullUrl)


val url3 = URL("jdbc:mysql://localhost:3306/database?useSSL=false")
assertEquals("jdbc", url3.scheme) // always lowercase issue #2092
assertEquals("mysql", url3.subScheme)
assertEquals("localhost", url3.host)
assertEquals(3306, url3.port)
assertEquals("/database", url3.path)
assertEquals("/database?useSSL=false", url3.pathWithQuery)
assertEquals("useSSL=false", url3.query)
assertEquals("jdbc:mysql://localhost:3306/database?useSSL=false", url3.fullUrl)
}

@Test
fun testUrlScheme() {
// test scheme standard (rfc3986#section-3.1): ALPHA *( ALPHA / DIGIT / "+" / "-" / "." )
val url = URL("custom+scheme123://example.com/path/to/resource")
assertEquals("custom+scheme123", url.scheme)
assertEquals("example.com", url.host)
assertEquals("custom+scheme123://example.com/path/to/resource", url.fullUrl)

val url2 = URL("alpha-numeric+scheme.123://example.com/path/to/resource")
assertEquals("alpha-numeric+scheme.123", url2.scheme)
assertEquals("example.com", url2.host)
assertEquals("alpha-numeric+scheme.123://example.com/path/to/resource", url2.fullUrl)
}

@Test
Expand Down