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 processing of google drive files #6128

Merged
merged 1 commit into from
Nov 19, 2024
Merged

Fix processing of google drive files #6128

merged 1 commit into from
Nov 19, 2024

Conversation

masaball
Copy link
Contributor

When we made the switch from mediainfo to ffprobe, we created a new class for processing the file. Because Google Drive utilizes headers/params for its authorization flow, this change resulted in the file information becoming divorced from the auth token. By explicitly passing the auth token through to the classes downloading and processing the file, we are able to fix this oversight.

raw_output = `#{ffprobe} -i "#{@media_file.location}" -v quiet -show_format -show_streams -of json`
# Include authorization headers for cases like Google Drive
header = "-headers 'Authorization: #{@media_file.auth_header.fetch('Authorization')}'" if @media_file.auth_header.present?
raw_output = `#{ffprobe} #{header} -i "#{@media_file.location}" -v quiet -show_format -show_streams -of json`
Copy link
Contributor Author

@masaball masaball Nov 18, 2024

Choose a reason for hiding this comment

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

This is downloading the file for processing, but we have already downloaded the file as part of the valid_content_type? check. And possibly there is third download for actually feeding the file into ActiveEncode?

It would probably be best as a separate ticket, but should we rework this so that when we are trying to access an internet hosted file we download to a tempfile and use the tempfile for all the processing instead of downloading it multiple times? (Stand-up 11/19/24: Yes, we should make this change)

When we made the switch from mediainfo to ffprobe, we created a new
class for processing the file. Because Google Drive utilizes
headers/params for its authorization flow, this change resulted in the
file information becoming divorced from the auth token. By explicitly
passing the auth token through to the classes downloading and processing
the file, we are able to fix this oversight.
Copy link
Member

@cjcolvar cjcolvar left a comment

Choose a reason for hiding this comment

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

This looks great!
Do you think it would make sense to add some tests? I'm not sure because it seems like the best that could be done is checking that mocks are called with the auth_header.

@masaball
Copy link
Contributor Author

masaball commented Nov 19, 2024

I'm not sure that any tests we tried to add would actually be looking at anything substantial.

@cjcolvar cjcolvar merged commit a60af64 into develop Nov 19, 2024
2 checks passed
@cjcolvar cjcolvar deleted the fix_google_drive branch November 19, 2024 21:01
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