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

Add downloading file attachment handler #793

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

Conversation

skyhirider
Copy link

@skyhirider skyhirider commented May 18, 2024

First draft of an attachment handler that mimics how browsers handle attachments, specifically

  • download file into a default folder when attachment response is detected
  • infer filename from octet stream response and use that when saving file
  • if a file already exists, append number to it. keep incrementing number until you find a slot that is free (thats how Chrome handles duplicate filenames)

The implementation allows defining the download folder. If empty constructor is used, temporary folder is used by default.

Open questions for Ronald

  • how should I handle exceptions?
    • When the download folder can't be used, throw an exception, and if so which one? A silent error would not be useful, but don't want to crash the whole driver
    • When the handle attachment method fails, either due to i/o exception or something else, which exceptions, if any, should be thrown? or should the method eat up the exception and only report and error in the logs?
  • What about the test cases, how do you recommend mocking the features?
    • I've added todo notes in my "test" that is really just a method I used to run the thing. The test scenarios I would like to cover are mentioned there, but I have experience with Junit 5 only. Would use a temp dir to test, but if you want to remove any i/o in tests then another option would be to set up mocks.

In general, treat this as a draft as the code is not very clean, but looking for your opinion before refactoring.

@skyhirider
Copy link
Author

@rbri how would you recommend I handle exceptions in the handleAttachments method?

I can promote them upwards, or eat them, log error and move on.

Normal browsers don't crash if a file fails to download, so I would choose that option.

For the constructor, if the destination folder is not readable, throwing an IO exception seems accurate, what are your thoughts?

@rbri
Copy link
Member

rbri commented May 26, 2024

Sounds all reasonable for me.

@skyhirider
Copy link
Author

@rbri updated attachment handler so it should be good to go.

For tests, how would you recommend creating or mocking a webresponse that is an attachment?

And when are you planning to release 4.2.0? Would like to finish this and have it in a mergeable state before a new version is released.

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.

None yet

2 participants