Skip to content

Conversation

@brys0
Copy link
Contributor

@brys0 brys0 commented Jan 14, 2025

Fix SeekableWriter.kt on file download resumption for non audio/video files

Relevant issue: #2359

Please note:
2. In the case where you are trying to download a video or audio file directly this error will still occur. Fixing this is pretty non-trivial, as such the implementation for more advanced considerations is not currently implemented in the PR.

Possible solutions to also fix audio/video download resuming as well:

  1. Examine Sec-Fetch-Dest header, which will contain "audio" or "video" when a browser is requesting chunked video/audio segments.
    • This is a new header, not all browsers or clients will support it, we cannot guarantee support
  2. Examine Accept header, which will contain specific mime types corresponding to the video/audio formats the browser is expecting.
    • This header varies drastically based on browser capabilities but in general browsers will use "Accept: */*" for download requests and "Accept: <expected format>" I chose not to implement in this PR due to the extensive testing that will need to be done to ensure it works correctly.

@brys0 brys0 marked this pull request as draft January 14, 2025 21:04
Rename isAV() extension to isAudioOrVideo()
Simplify control flow in "when" statements
@brys0
Copy link
Contributor Author

brys0 commented Jan 14, 2025

Still need to create new tests for the new control flow of writeSeekableStream

@brys0 brys0 marked this pull request as ready for review January 14, 2025 21:50
@brys0 brys0 requested a review from tipsy January 14, 2025 22:13
@codecov
Copy link

codecov bot commented Jan 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.12%. Comparing base (5de1d25) to head (a7e9b65).
Report is 1 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #2360      +/-   ##
============================================
+ Coverage     87.09%   87.12%   +0.02%     
- Complexity     1314     1320       +6     
============================================
  Files           148      148              
  Lines          4457     4467      +10     
  Branches        509      512       +3     
============================================
+ Hits           3882     3892      +10     
  Misses          354      354              
  Partials        221      221              

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

@tipsy tipsy merged commit 6192e24 into javalin:master Jan 15, 2025
15 checks passed
@tipsy
Copy link
Member

tipsy commented Jan 15, 2025

Thank you very much !

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