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

Fail more gracefully from ActiveStorage missing file exceptions #33666

Conversation

@cbothner
Copy link
Contributor

@cbothner cbothner commented Aug 20, 2018

This PR translates service-specific missing object exceptions into one generic ActiveStorage::FileNotFoundError so that the application can fail more gracefully when a missing file is accessed. Specifically, this PR lets DiskController rescue this error and respond with 404 instead of 500. This improves the development experience especially when working, in development, with a database of Blob records that point to files in the production service.

Outstanding question

RepresentationsController does not presently handle ActiveStorage::FileNotFoundError, since @georgeclaghorn argued it truly is exceptional for the service not to have a file that corresponds to a specific blob in the database. I don’t disagree with this, but hope we can nevertheless find a way to reduce the flood of stack traces that fill our development logs when loading an index view that tries to display many thumbnails of images stored in our production service.

Related issue

Resolves #33647

@georgeclaghorn georgeclaghorn self-assigned this Aug 20, 2018
activestorage/lib/active_storage/service/azure_storage_service.rb Outdated
_, io = blobs.get_blob(container, key)
io.force_encoding(Encoding::BINARY)
rescue Azure::Core::Http::HTTPError => e
raise e unless e.type == "BlobNotFound"

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Aug 20, 2018
Member

Let’s use bare raise instead of raise e to avoid munging the backtrace:

if e.type == "BlobNotFound"
  raise ActiveStorage::FileNotFoundError
else
  raise
end
activestorage/lib/active_storage/service/azure_storage_service.rb Outdated
rescue Azure::Core::Http::HTTPError => e
raise e unless e.type == "BlobNotFound"
raise ActiveStorage::FileNotFoundError
end

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Aug 20, 2018
Member

Can we extract a private handle_errors method to avoid duplicating the error handling logic in download and download_chunk?

def download_chunk(key, range)
  instrument :download_chunk, key: key, range: range do
    handle_errors do
      _, io = blobs.get_blob(container, key, start_range: range.begin, end_range: range.exclude_end? ? range.end - 1 : range.end)
      io.force_encoding(Encoding::BINARY)
    end
  end
end

This comment has been minimized.

@cbothner

cbothner Aug 20, 2018
Author Contributor

I was half tempted to put such a private method on the parent class (taking a service specific exception as an argument) but worried it was gonna be too much magic. Is this just needed because of the extra type checking needed for Azure errors? (Related bug fix PR #33667 that should probably do it the same way…)

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Aug 20, 2018
Member

I wouldn’t put it on the parent class. It’s a service-specific method.

activestorage/lib/active_storage/service/azure_storage_service.rb Outdated
raise
end
end
end

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Aug 21, 2018
Member

You don’t need the begin/end block here:

def handle_errors
  yield
rescue Azure::Core::Http::HTTPError => e
  # ...
end
activestorage/lib/active_storage/service/disk_service.rb Outdated
end
rescue Errno::ENOENT
raise ActiveStorage::FileNotFoundError
end

This comment has been minimized.

@georgeclaghorn

georgeclaghorn Aug 21, 2018
Member

Ditto: you don’t need the begin/end block here.

@georgeclaghorn
Copy link
Member

@georgeclaghorn georgeclaghorn commented Aug 21, 2018

Last thing: can you please squash your commits? I think two are appropriate: one for rescuing Errno::ENOENT in DiskController and one for translating service-specific missing object exceptions.

I also think these changes merit separate changelog entries, since they're independent:

  • ActiveStorage::Blob#download and ActiveStorage::Blob#open raise ActiveStorage::FileNotFoundError when the corresponding file is missing from the storage service. Services translate service-specific missing object exceptions (e.g. Google::Cloud::NotFoundError for the GCS service and Errno::ENOENT for the disk service) into ActiveStorage::FileNotFoundError.

  • ActiveStorage::DiskController#show generates a 404 Not Found response when the requested file is missing from the disk service. It previously raised Errno::ENOENT.

cbothner added 2 commits Aug 18, 2018
`ActiveStorage::Blob#download` and `ActiveStorage::Blob#open` raise
`ActiveStorage::FileNotFoundError` when the corresponding file is missing
from the storage service. Services translate service-specific missing
object exceptions (e.g. `Google::Cloud::NotFoundError` for the GCS service
and `Errno::ENOENT` for the disk service) into
`ActiveStorage::FileNotFoundError`.
`ActiveStorage::DiskController#show` generates a 404 Not Found response when
the requested file is missing from the disk service. It previously raised
`Errno::ENOENT`.
@cbothner cbothner force-pushed the cbothner:fail-gracefully-from-activestorage-file-not-found branch to 22efb2e Aug 21, 2018
@cbothner
Copy link
Contributor Author

@cbothner cbothner commented Aug 21, 2018

Done 👍

Is there anything to do about the errors raised in RepresentationsController for the sake of developer experience, or is that inappropriate to handle at the framework level?

@georgeclaghorn
Copy link
Member

@georgeclaghorn georgeclaghorn commented Aug 23, 2018

The DebugExceptions middleware logs the full backtrace if none of the lines in the trace come from the application, which is the case for ActiveStorage::FileNotFoundErrors triggered by requests to RepresentationsController:

trace = wrapper.application_trace
trace = wrapper.framework_trace if trace.empty?
ActiveSupport::Deprecation.silence do
logger.fatal " "
logger.fatal "#{exception.class} (#{exception.message}):"
log_array logger, exception.annoted_source_code if exception.respond_to?(:annoted_source_code)
logger.fatal " "
log_array logger, trace
end

I’m not immediately sure what we can do about that. I’m going to think about it and merge this in the meantime.

@georgeclaghorn georgeclaghorn merged commit dc001db into rails:master Aug 23, 2018
2 checks passed
2 checks passed
codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cbothner cbothner deleted the cbothner:fail-gracefully-from-activestorage-file-not-found branch Aug 23, 2018
suketa added a commit to suketa/rails_sandbox that referenced this pull request May 18, 2019
Fail more gracefully from ActiveStorage missing file exceptions
rails/rails#33666
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.