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

Write additional metadata back into the source sheet #4

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

micrictor
Copy link

There's three changes in this PR - let me know if you would prefer I break it up into three separate PRs.

  1. Make redirect matching case-insensitive. Since the map keys were already normalized to lowercase, this fixes an issue where mixed case redirects wouldn't work.
  2. Upon redirect, increment the third column ('Hit Count') and write the current time to a fourth column ('Last Visited') for the row of the redirect.
  3. Permit users to specify their desired redirect code via environment variable. Primarily, I'd expect users to use this to change between the default permanent redirect and a temporary redirect.

While technically path components are case sensitive, I have no
personal need for case-sensitive redirection.
Upon redirect, record the current time into the correct row.
Give users the ability to specify the HTTP response code to use.
Default is 301 - Moved Permanantly.
Make sheet writes in one goroutine. Also, make sure that the
writes are thread-safe using mutexes.
@ahmetb
Copy link
Owner

ahmetb commented Jan 22, 2023

I don't think I'll accept the write requests back into the sheet. They will be buggy on concurrent instances. Resp codes do not sound very useful either.
Feel free to keep these in your fork.

@micrictor
Copy link
Author

micrictor commented Jan 22, 2023 via email

@ahmetb
Copy link
Owner

ahmetb commented Jan 22, 2023

Even then, I don't wanna break anyone's workflow without them noticing. I personally don't use uppercase at all and it does not seem like I'd need it.

@micrictor
Copy link
Author

micrictor commented Jan 22, 2023

I don't think it would be a breaking change for lookups that currently work - the lookup key is already lowercased here:

https://github.com/ahmetb/sheets-url-shortener/blob/master/main.go#L189-L200

I believe this means is that if I have a shortcut ShortCut, it is impossible for it to match a URL path.

It may possibly impact users if they depend on mixed-case shortcuts not ever redirecting - e.g. if you have a shortcut for gh and need http://example.local/GH to not redirect.

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.

2 participants