Requests to stream assets from S3 can result in an empty 200 response #78
Assignees
Comments
floehopper
added a commit
that referenced
this issue
Jul 25, 2017
If the AWS S3 bucket is not configured and a request is made to stream an asset from S3, then the server now responds with a "500 Internal server error" instead of a "200 OK" and an empty file. * I decided to introduce a `CloudStorage` class as a namespace for the base class of the `NotConfiguredError` exception to keep the `ApplicationController` agnostic of the underlying cloud storage implementation. * The "500 Internal server error" status seemed the most appropriate. I also considered the following: * "501 Not implemented", but the functionality *is* implemented, it's just not correctly configured. * "503 Service unavailable", but this error supposed to only be temporary and that doesn't seem quite right in this case. * "404 Not found", but the problem seems more serious than that. * I decided to capitalise "Internal" in the error status, because that seems like a more common approach. I wanted to capitalise the "not found" status for the 404 error, but I was concerned there might be clients depending on it, so I left it unchanged. * I decided to append the exception message to the error status string, because I think it's useful to make as much information available as possible. I tried to see whether there was any standard way of doing this for other GOV.UK APIs, but couldn't find any consistent approach. * I introduced the `disable_cloud_storage_stub: true` option for specs so that I could write an example where the exception was raised. Fixes #78.
floehopper
added a commit
that referenced
this issue
Jul 25, 2017
If the AWS S3 bucket is not configured and a request is made to stream an asset from S3, then the server now responds with a "500 Internal server error" instead of a "200 OK" and an empty file. * I decided to introduce a `CloudStorage` class as a namespace for the base class of the `NotConfiguredError` exception to keep the `ApplicationController` agnostic of the underlying cloud storage implementation. * The "500 Internal server error" status seemed the most appropriate. I also considered the following: * "501 Not implemented", but the functionality *is* implemented, it's just not correctly configured. * "503 Service unavailable", but this error supposed to only be temporary and that doesn't seem quite right in this case. * "404 Not found", but the problem seems more serious than that. * I decided to capitalise "Internal" in the error status, because that seems like a more common approach. I wanted to capitalise the "not found" status for the 404 error, but I was concerned there might be clients depending on it, so I left it unchanged. * I decided to append the exception message to the error status string, because I think it's useful to make as much information available as possible. I tried to see whether there was any standard way of doing this for other GOV.UK APIs, but couldn't find any consistent approach. * I introduced the `disable_cloud_storage_stub: true` option for specs so that I could write an example where the exception was raised. Fixes #78.
|
This has been addressed in #84. |
floehopper
added a commit
that referenced
this issue
Jul 25, 2017
Squashed commits from the following branch: respond-with-error-if-asset-requested-from-s3-but-s3-not-configured See PR #84. Squashed commit of the following: commit 735e650 Author: James Mead <james@floehopper.org> Date: Tue Jul 25 13:07:23 2017 +0100 Error if asset requested from S3, but S3 not configured If the AWS S3 bucket is not configured and a request is made to stream an asset from S3, then the server now responds with a "500 Internal server error" instead of a "200 OK" and an empty file. * I decided to introduce a `CloudStorage` class as a namespace for the base class of the `NotConfiguredError` exception to keep the `ApplicationController` agnostic of the underlying cloud storage implementation. * The "500 Internal server error" status seemed the most appropriate. I also considered the following: * "501 Not implemented", but the functionality *is* implemented, it's just not correctly configured. * "503 Service unavailable", but this error supposed to only be temporary and that doesn't seem quite right in this case. * "404 Not found", but the problem seems more serious than that. * I decided to capitalise "Internal" in the error status, because that seems like a more common approach. I wanted to capitalise the "not found" status for the 404 error, but I was concerned there might be clients depending on it, so I left it unchanged. * I decided to append the exception message to the error status string, because I think it's useful to make as much information available as possible. I tried to see whether there was any standard way of doing this for other GOV.UK APIs, but couldn't find any consistent approach. * I introduced the `disable_cloud_storage_stub: true` option for specs so that I could write an example where the exception was raised. Fixes #78. commit 86ba9d2 Author: James Mead <james@floehopper.org> Date: Tue Jul 25 12:30:23 2017 +0100 Rename spec/support/s3_storage.rb -> cloud_storage.rb To better reflect its purpose. commit fa9d9f5 Author: James Mead <james@floehopper.org> Date: Tue Jul 25 11:33:30 2017 +0100 Extract MediaController#stream_from_s3? method To make the code more readable. commit 13ba282 Author: James Mead <james@floehopper.org> Date: Tue Jul 25 11:31:34 2017 +0100 Extract AWS_S3_BUCKET_NAME env var into app config So it's easier to stub. Unfortunately I've had to disable a Rubocop rule around one line. This violation is due to the digit `3` in the variable name. It looks as if the constraints for this rule have been relaxed [1] in Rubocop v0.44.0, but we're currently using v0.43.0 and upgrading means first upgrading the constraints on the Rubocop version in the specification for the `govuk-lint` gem and then upgrading that gem in this project. I'm going to do that separately and I'll make a note to remove this disabling of the rule when I do that. [1]: rubocop/rubocop@833aad6 commit fd0a75b Author: James Mead <james@floehopper.org> Date: Mon Jul 24 18:00:13 2017 +0100 Use have_http_status matcher & status code symbols To make controller & request specs more expressive. Also: * Reworded some example names to make them more consistent. * `be_successful` -> `be_success` for consistency. * In one example: `be_success` -> `have_http_status(:success)` to contrast with example immediately preceding it. commit 985ce25 Author: James Mead <james@floehopper.org> Date: Mon Jul 24 17:43:48 2017 +0100 Add blank lines to some specs to improve readability
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I found it confusing that a request to serve an asset from S3 results in an empty 200 response if the
AWS_S3_BUCKET_NAMEenvironment variable isn't set. I think I'd prefer it to fail fast if I'm requesting a file from S3 when the app doesn't have enough information to be able to satisfy that request.The text was updated successfully, but these errors were encountered: