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.

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

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

Merged

Conversation

itboy87
Copy link
Contributor

@itboy87 itboy87 commented Jan 1, 2024

Add Support for subScheme in URLs

This pull request introduces the following changes:

  • New Variable: Added subScheme to support URLs with subScheme.

  • Regex Modification: Adjusted the scheme regex from \\w+: to ^([a-zA-Z0-9+.-]+)(?::([a-zA-Z]+))?: to align with the standard specified in RFC 3986 Section 3.1: ALPHA *( ALPHA / DIGIT / "+" / "-" / "." ) and supporting subScheme.

  • Test Cases: Added new test cases to ensure proper functionality.

Example Use Cases:

  • <scheme>:<subScheme>://<host>:<port>/<path>?<query_parameters>#<fragment>
  • Example: jdbc:mysql://localhost:3306/database

This PR closes #2101

@@ -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

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?

Copy link
Member

@soywiz soywiz left a comment

Choose a reason for hiding this comment

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

LGTM

@soywiz soywiz merged commit 653e56c into korlibs:main Jan 3, 2024
9 checks passed
@soywiz
Copy link
Member

soywiz commented Jan 3, 2024

Thansk!

@itboy87 itboy87 deleted the fix/2101-add-subprotocol-support-in-url branch January 5, 2024 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Support for Subprotocol/subScheme in URL
2 participants