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.

Sign up for GitHub

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

Jump to bottom

Respond with error if asset requested from S3, but S3 not configured #84

Merged
merged 6 commits into from
Jul 25, 2017
Merged

Respond with error if asset requested from S3, but S3 not configured #84

merged 6 commits into from
Jul 25, 2017

Commits on Jul 25, 2017

  1. Add blank lines to some specs to improve readability

    @floehopper
    floehopper committed Jul 25, 2017
    Configuration menu
    Copy the full SHA
    750c4d8 View commit details
    Browse the repository at this point in the history

  2. 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.
    @floehopper
    floehopper committed Jul 25, 2017
    Configuration menu
    Copy the full SHA
    b907140 View commit details
    Browse the repository at this point in the history

  3. 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
    @floehopper
    floehopper committed Jul 25, 2017
    Configuration menu
    Copy the full SHA
    7e061d7 View commit details
    Browse the repository at this point in the history

  4. Extract MediaController#stream_from_s3? method 

    To make the code more readable.
    @floehopper
    floehopper committed Jul 25, 2017
    Configuration menu
    Copy the full SHA
    b8b79f5 View commit details
    Browse the repository at this point in the history

  5. Rename spec/support/s3_storage.rb -> cloud_storage.rb 

    To better reflect its purpose.
    @floehopper
    floehopper committed Jul 25, 2017
    Configuration menu
    Copy the full SHA
    8cceb8e View commit details
    Browse the repository at this point in the history

  6. 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.
    @floehopper
    floehopper committed Jul 25, 2017
    Configuration menu
    Copy the full SHA
    dc1b56b View commit details
    Browse the repository at this point in the history