jpmcgrath / shortener

Shortener makes it easy to create shortened URLs for your rails application.
MIT License
539 stars 223 forks source link

Don't clean_url unless user desires #123

Closed ziaulrehman40 closed 4 years ago

ziaulrehman40 commented 5 years ago

This is good small library, i integrated in my app to solve a problem mentioned in this SO question, but only to find out that this library doesn't for my use case.

To keep it short, i want to shorten urls to app in form of something like: intent://xyz this is common pattern used for mobile apps, and for API based rails apps which are exposing APIs for their mobile apps.

A quick look suggests issue is here in this line: https://github.com/jpmcgrath/shortener/blob/develop/app/models/shortener/shortened_url.rb#L55 Here it is calling clean_url method, which is sort of pre-processing the URL before saving to DB.

What it ends up being is if i send: myapp://some_path to be shortened, it is saved as /myapp://some_path in DB which obviously doesn't redirect properly. And end up something like: https://mydomain.com/myapp://some_path which is incorrect.

Suggestion: Disable url_cleaning by default, and make it configurable through initialize. Or vice versa.

I am happy to submit a patch, let me know how you would like it to be. Thanks. My point

ziaulrehman40 commented 5 years ago

For the time being, i have overridden this method by creating a model as such:

class ShortUrl < Shortener::ShortenedUrl
  def self.clean_url(url)
    url.to_s
  end
end

And using this model for url shortening. What i also liked about this is that it gives me a lot shorter name for the model as well 😄 .

jpmcgrath commented 4 years ago

Closing this now, but I will add that whilst this solution is likely to keep working for the time being, it is relying on internal implementation and we can't guarantee that future releases will not break this workaround.

PRs are welcome to add a toggling url cleaning, or providing alternative cleaning logic.