jpmcgrath / shortener

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

Expirarion Update #149

Open OrH-Simplee opened 2 years ago

OrH-Simplee commented 2 years ago

when generating a shortened URL for the same link, and sending a new expiration date, it doesn't update the expiration. example: generate a link today, with 6.month.from_now expiration date with : shortened_url = Shortener::ShortenedUrl.generate(url, expires_at: 6.month.from_now) then in 3 months, generate the same link, with the exact same code: shortened_url = Shortener::ShortenedUrl.generate(url, expires_at: 6.month.from_now)

expectation - expiration would be updated.

important to note - I don't know that the link already exists, but I do want to get the same shortened link. is it on purpose? any way around it?

fschwahn commented 1 year ago

Confusingly, this even returns an already expired ShortenedUrl in case one exists. Not sure the default behavior can change, but it possibly could be an option passed to generate to optionally update the expires_at-column.

jpmcgrath commented 1 year ago

I believe the history here is the generate method was introduced in the original release. If someone asks to generate a shortened url for an existing address, the logic was to return the original record. We provided the fresh option for when you wanted to create a new ShortenedUrl instead of retrieving an existing record. See:

https://github.com/jpmcgrath/shortener#label-Fresh+Links

Then the expires_at option was introduced without taking into account the existing behaviour that returns previously created ShortenedUrls.

Right now you can, with one call:

  1. get the existing record if it exists, without updating, and create a new one if it does not exist
  2. force the generation of a new record with the new expiry date

But you cannot get link if it exists, and update date the expiry if it exists, all in a single call.

If someone provides a new expires_at for an URL that already has a ShortenedUrl, does it mean they want to get the existing record, and update it? Or does it mean they want a completely new ShortenedUrl? Or does it mean they want the existing record, and to keep it as is? I can people expecting any of these as a the "correct and intuitive result". Probably the "least inituitive" is supplying a new expiry, and the resulting expiry is sometimes ignored, but it still makes some sense. Perhaps you don't want to destroy that info?

A workaround here is to do a second step to update the expiry date of your shortened url. ActiveRecord should prevent DB activity when it is not required (e.g. if you provide the same expiry date as was set on a newly generated record).

As for a "fix", I am reluctant to change the existing behaviour now, as people may have come to rely on the current behaviour as a feature. @fschwahn suggestion of a new option could work, or perhaps a configuration option.

OrH-Simplee commented 1 year ago

@jpmcgrath what about adding a new feature flag on the generate function, that will be false by default, and it will refresh existing exires_at if exists