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

feat(server): add random suffix mode #69

Merged
merged 15 commits into from
Jun 30, 2023

Conversation

tessus
Copy link
Collaborator

@tessus tessus commented Jun 20, 2023

Unfortunately the current implementation does not retain multiple extensions. So if you upload foo.tar.gz, the resulting file will be eu7f92x1.gz.

This PR includes 2 changes:

  • multiple extensions are retained (e.g. eu7f92x1.tar.gz)
  • added an optional option suffix_mode (bool) to random_url (result: foo.eu7f92x1.tar.gz)

closes #41

@tessus tessus requested a review from orhun as a code owner June 20, 2023 20:14
@codecov-commenter
Copy link

codecov-commenter commented Jun 20, 2023

Codecov Report

Patch coverage: 96.29% and project coverage change: -1.50 ⚠️

Comparison is base (5b01c98) 68.94% compared to head (aaed658) 67.45%.

❗ Current head aaed658 differs from pull request most recent head e5691db. Consider uploading reports for the commit e5691db to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #69      +/-   ##
==========================================
- Coverage   68.94%   67.45%   -1.50%     
==========================================
  Files          11       11              
  Lines         483      510      +27     
==========================================
+ Hits          333      344      +11     
- Misses        150      166      +16     
Flag Coverage Δ
unit-tests 67.45% <96.29%> (-1.50%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/random.rs 83.33% <ø> (ø)
src/paste.rs 84.95% <96.29%> (+2.30%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@orhun
Copy link
Owner

orhun commented Jun 23, 2023

Can you please add tests to cover these changes?

@tessus
Copy link
Collaborator Author

tessus commented Jun 24, 2023

I have to look into it. I did of course my own testing: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=fbd05ecc4a7979366febb168fafd43b5

src/paste.rs Outdated Show resolved Hide resolved
@tessus
Copy link
Collaborator Author

tessus commented Jun 28, 2023

Please let me know, whether I missed anything. Otherwise I'd hope for a 0.11.0 release soon. Haha. ;-)

@orhun
Copy link
Owner

orhun commented Jun 29, 2023

I cleaned up the implementation a little bit for better readability and added fixture tests. Can you also confirm that it works as expected?

@tessus
Copy link
Collaborator Author

tessus commented Jun 29, 2023

I'll test within the hour. What I could see from the code at a first glance, it should be the same behavior.

Although I chose suffix_mode for a reason. It is an option to random_url (and is only relevant when random_url is enabled), so naming the option random_ is superfluous. e.g. you don't call the other option random_type either. The term suffix alone is too less specific IMO, but would still be better than random_suffix.
But it's your choice of course.

I'll comment on the result of my tests in a bit.

@orhun
Copy link
Owner

orhun commented Jun 29, 2023

Yeah, it makes sense to use suffix_mode in that context. Feel free to revert it!

@tessus
Copy link
Collaborator Author

tessus commented Jun 29, 2023

Yep, the code works the same, but I found a bug in my code (I missed it in my earlier test cases), which of course propagated to the refactored one:

.foo           -> https://rptest.local/paste/.foo.wtNk.
.foo.bar       -> https://rptest.local/paste/.foo.SL2N.bar
.foo.bar.txt   -> https://rptest.local/paste/.foo.vTGL.bar.txt
foo            -> https://rptest.local/paste/foo.0OPe.txt
foo.bar        -> https://rptest.local/paste/foo.g7yj.bar
foo.bar.txt    -> https://rptest.local/paste/foo.kEaw.bar.txt

For the first file .foo, the resulting URL should be: https://rptest.local/paste/.foo.wtNk.txt

I can look into this later today. For now I just reverted the option name.

@tessus
Copy link
Collaborator Author

tessus commented Jun 29, 2023

All fixed. Good to go. ;-)

Not sure why the test suite fails. If you run cargo test and the fixtures locally, everythig is fine. I guess gh is still recovering.

P.S.:

.foo           -> https://rptest.local/paste/.foo.yG31.txt
.foo.bar       -> https://rptest.local/paste/.foo.qx74.bar
.foo.bar.txt   -> https://rptest.local/paste/.foo.4pMU.bar.txt
foo            -> https://rptest.local/paste/foo.daes.txt
foo.bar        -> https://rptest.local/paste/foo.1Mzu.bar
foo.bar.txt    -> https://rptest.local/paste/foo.RFA2.bar.txt

@tessus tessus changed the title add random suffix mode feat(server): add random suffix mode Jun 30, 2023
@orhun
Copy link
Owner

orhun commented Jun 30, 2023

Looks good, thank you!

I'd hope for a 0.11.0 release soon.

I will work on it tomorrow 🐻

Copy link
Owner

@orhun orhun left a comment

Choose a reason for hiding this comment

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

🥳🥳🥳

@orhun orhun merged commit 62bbfef into orhun:master Jun 30, 2023
8 checks passed
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.

Random suffix mode
3 participants