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 Url capability: parsing/validating url strings #1903

Open
edenman opened this issue May 28, 2020 · 9 comments
Open

Add Url capability: parsing/validating url strings #1903

edenman opened this issue May 28, 2020 · 9 comments
Assignees
Labels
feature triaged up for grabs Involve simple fixes and/or implementations which are great for folks wanting to contribute

Comments

@edenman
Copy link

edenman commented May 28, 2020

Subsystem
ktor-http

Is your feature request related to a problem? Please describe.
My app gives me unexpected results on Url(String)

Describe the solution you'd like
Url should have a factory method that returns an optional Url, something like:

class Url {
  companion object {
    fun parse(url: String): Url? { ... }
  }
}

This method should return null if the url is not valid. It should not throw exceptions.

Motivation to include to ktor
User-submitted content often has urls in it and it would be useful to validate them and extract the various url parts.

Specific urls that are currently broken:

Url("bogus").toString() returns http://localhost/bogus and I think it should return null from this new method.
Url("http://localhost:7000Pragma").toString() throws an exception and I think it should return null from this new method.

@edenman
Copy link
Author

edenman commented May 28, 2020

For now I'm just working around this on my end by doing:

fun Url.Companion.parse(str: String): Url? {
  try {
    return Url(str)
  } catch (e: Throwable) {
    return null
  }
}

but obviously this doesn't address the "bogus" case.

@e5l e5l self-assigned this Jun 15, 2020
@e5l e5l added triaged up for grabs Involve simple fixes and/or implementations which are great for folks wanting to contribute labels Jun 15, 2020
@oleg-larshin
Copy link

Please check the following ticket on YouTrack for follow-ups to this issue. GitHub issues will be closed in the coming weeks.

@victorcwai
Copy link

Url("bogus").toString() returns http://localhost/bogus and I think it should return null from this new method.

Currently the takeFrom(String) function called by Url(urlString) allow parsing relative URLs (see #849).
However if no base URL is provided in Url(urlString), protocol and host will default to http and localhost respectively.

I am not sure if the Url(urlString) function should return a default protocol and host. Maybe it should just set protocol and host to null or throw an exception if no protocol is provided?

@chetankokil
Copy link

can i work on this @edenman , @victorcwai ?

@chetankokil
Copy link

@e5l team, please review the PR when you get a chance.

chetankokil pushed a commit to chetankokil/ktor that referenced this issue Aug 28, 2022
chetankokil pushed a commit to chetankokil/ktor that referenced this issue Aug 28, 2022
chetankokil pushed a commit to chetankokil/ktor that referenced this issue Aug 28, 2022
chetankokil pushed a commit to chetankokil/ktor that referenced this issue Aug 28, 2022
chetankokil pushed a commit to chetankokil/ktor that referenced this issue Aug 28, 2022
@J-Swift
Copy link

J-Swift commented Nov 2, 2022

This also got me. Its extremely unexpected to have a relative url get parsed as localhost relative.

@jsoberg
Copy link

jsoberg commented Jan 15, 2023

Would it make sense to return a kotlin.Result here? The consumer could then fold it to handle success vs failure, or call getOrNull() on the result if they want it to have the functionality described here (null for invalid).

@ablx
Copy link

ablx commented Oct 17, 2023

@e5l if this is still needed, I'll give it a try this week.

@e5l
Copy link
Member

e5l commented Oct 25, 2023

@ablx, thanks! it would be great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature triaged up for grabs Involve simple fixes and/or implementations which are great for folks wanting to contribute
Projects
None yet
Development

No branches or pull requests

8 participants