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

fix(server): return the correct file on multiple files with same name #234

Merged
merged 2 commits into from Feb 9, 2024

Conversation

tessus
Copy link
Contributor

@tessus tessus commented Jan 30, 2024

Description

The glob_match_file function returns the wrong entry. glob yields paths in alphabetical order. We want the file with the longest expiry date/time.

Motivation and Context

fixes #218

How Has This Been Tested?

  • manually
  • cargo test
  • fixtures

Changelog Entry

### Changed

- Fixed an erroneous `file not found` occurrence

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (no code change)
  • Refactor (refactoring production code)
  • Other

Checklist:

  • My code follows the code style of this project.
  • I have updated the documentation accordingly.
  • I have formatted the code with rustfmt.
  • I checked the lints with clippy.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@tessus tessus requested a review from orhun as a code owner January 30, 2024 23:14
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8de2450) 70.11% compared to head (5056916) 70.11%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #234   +/-   ##
=======================================
  Coverage   70.11%   70.11%           
=======================================
  Files          11       11           
  Lines         609      609           
=======================================
  Hits          427      427           
  Misses        182      182           
Flag Coverage Δ
unit-tests 70.11% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Such an effective fix! Can you think of any edge cases that this would break backwards compatibility?

fixtures/test-fix-218/config.toml Outdated Show resolved Hide resolved
@tessus
Copy link
Contributor Author

tessus commented Jan 31, 2024

I thought about it for a while and it won't break anything.

But I see an issue that has nothing to do with this or the previous PRs: One can upload multiple files with the same name, but that was already possible with the curl -F "file=@x.txt;filename=wow.nice" command when random_url was not set. This case was never taken into consideration.
We can look into this in a separate discussion or issue.

@tessus
Copy link
Contributor Author

tessus commented Feb 1, 2024

I created a separate discussion #241

Can you think of any edge cases that this would break backwards compatibility?

A bit more info:

If multiple files with the same name are on the server, the file with the longest expiry timestamp will be returned when accessing the file via curl <server>/file.txt

Previously a different file was returned. Which one is not entirely clear depending on expiration but not yet cleaned from disk, and so on.

But this was never a defined behavior anyway, thus no breaking change.

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.

Thanks for looking into this and creating a discussion! I will get to that soon - I think this PR is in good shape to merge now.

@orhun orhun merged commit 8e6393c into orhun:master Feb 9, 2024
8 of 9 checks passed
@orhun orhun changed the title fix(server): file not found, even though on server and not expired fix(server): return the correct file on multiple files with same name Feb 9, 2024
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.

file not found, even though on server and not expired
3 participants