Respond with error if asset requested from S3, but S3 not configured #84
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Jul 25, 2017
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.
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
To make the code more readable.
To better reflect its purpose.
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.
|
Rebasing against master and force-pushing. |
floehopper
force-pushed
the
respond-with-error-if-asset-requested-from-s3-but-s3-not-configured
branch
from
735e650
to
dc1b56b
Compare
floehopper
added a commit
that referenced
this pull request
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
|
This looks good to me! 👍 I'm not fond of the double negative in |
chrislo
approved these changes
Jul 25, 2017
|
Thanks for reviewing.
In fact it was introduced in this PR. I understand where you're coming from, but the problem I had was that I want the stubbing behaviour to be the default. Can you think of a better way of doing it? I think I'm going to get this merged anyway and we can address this separately as you suggest. |
floehopper
deleted the
respond-with-error-if-asset-requested-from-s3-but-s3-not-configured
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
If the AWS S3 bucket is not configured (i.e. the bucket name is not set) 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've also done a bunch of tidying up in this pull request. The actual change is in the last commit, "Error if asset requested from S3, but S3 not configured". See that commit note for further details of the choices I made.
Note that I've had to disable a Rubocop rule for one line in the "Extract AWS_S3_BUCKET_NAME env var into app config" commit. See the commit note for details.