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

[PR] Addressing edge cases #86

Merged
merged 29 commits into from
Jul 17, 2023
Merged

[PR] Addressing edge cases #86

merged 29 commits into from
Jul 17, 2023

Conversation

LuchoTurtle
Copy link
Member

closes #69

This adds pattern matching and returns the person meaningful error messages according to whether there was an error:

  • reading the path from the image.
  • creating the CID from the file contents.
  • getting the file extension.
  • uploading the file to S3.

I had to use mock to force the request to S3 to fail in tests and to emulate invalid binary data so CID would also fail, as detailed in #69.

@LuchoTurtle LuchoTurtle added enhancement New feature or enhancement of existing functionality chore a tedious but necessary task often paying technical debt in-progress An issue or pull request that is being worked on by the assigned person elixir Pull requests that update Elixir code labels Jun 28, 2023
@LuchoTurtle LuchoTurtle self-assigned this Jun 28, 2023
@LuchoTurtle LuchoTurtle added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person labels Jun 28, 2023
@LuchoTurtle LuchoTurtle assigned nelsonic and unassigned LuchoTurtle Jun 28, 2023
@LuchoTurtle
Copy link
Member Author

This should be reviewable now 🆗

@nelsonic nelsonic added this to More ToDo ThanCanEver Be Done in Nelson's List via automation Jul 1, 2023
@nelsonic nelsonic moved this from More ToDo ThanCanEver Be Done to Awaiting Feedback/Review in Nelson's List Jul 1, 2023
nelsonic
nelsonic previously approved these changes Jul 4, 2023
Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

Going to make a couple of adjustments. 🧑‍💻
Thanks for proactively opening this PR. 👍

@nelsonic
Copy link
Member

nelsonic commented Jul 4, 2023

On my localhost, ran the following:

git pull 
git checkout 'edge-cases-#69'
mix deps.get
source .env
mix c

Got:

  1) test upload/1 happy path REAL Upload (App.UploadTest)
     test/app/upload_test.exs:4
     Assertion with == failed
     code:  assert App.Upload.upload(image) == {:ok, expected_response}
     left:  {:error, :upload_fail}
     right: {:ok, %{compressed_url: "https://s3.eu-west-3.amazonaws.com//zb2rhXACvyoVCaV1GF5ozeoNCXYdxcKAEWvBTpsnabo3moYwB.png", url: "https://s3.eu-west-3.amazonaws.com//zb2rhXACvyoVCaV1GF5ozeoNCXYdxcKAEWvBTpsnabo3moYwB.png"}}
     stacktrace:
       test/app/upload_test.exs:18: (test)



  2) test upload with image keyword (AppWeb.APITest)
     test/app_web/api_test.exs:64
     ** (RuntimeError) expected response with status 200, got: 400, with body:
     "{\"errors\":{\"detail\":\"Error uploading file. There was an error uploading the file to S3.\"}}"
     code: assert Jason.decode!(response(conn, 200)) == expected
     stacktrace:
       (phoenix 1.7.6) lib/phoenix/test/conn_test.ex:373: Phoenix.ConnTest.response/2
       test/app_web/api_test.exs:74: (test)

..

  3) test upload succeeds (happy path) (AppWeb.APITest)
     test/app_web/api_test.exs:51
     ** (RuntimeError) expected response with status 200, got: 400, with body:
     "{\"errors\":{\"detail\":\"Error uploading file. There was an error uploading the file to S3.\"}}"
     code: assert Jason.decode!(response(conn, 200)) == expected
     stacktrace:
       (phoenix 1.7.6) lib/phoenix/test/conn_test.ex:373: Phoenix.ConnTest.response/2
       test/app_web/api_test.exs:61: (test)

..........
Finished in 0.2 seconds (0.1s async, 0.1s sync)
15 tests, 3 failures

Randomized with seed 654535
----------------
COV    FILE                                        LINES RELEVANT   MISSED
100.0% lib/app.ex                                      9        0        0
100.0% lib/app/repo.ex                                 5        0        0
 90.0% lib/app/upload.ex                             108       20        2
100.0% lib/app_web/components/layouts.ex               5        0        0
 90.0% lib/app_web/controllers/api_controller.e       46       10        1
 66.7% lib/app_web/controllers/api_json.ex            20        3        1
100.0% lib/app_web/controllers/page_controller.       25        5        0
100.0% lib/app_web/controllers/page_html.ex            5        0        0
100.0% lib/app_web/endpoint.ex                        48        0        0
100.0% lib/app_web/live/imgup_live.ex                 93       26        0
100.0% lib/app_web/router.ex                          30        4        0
100.0% lib/app_web/s3_upload.ex                      127       24        0
[TOTAL]  95.7%
----------------
Generating report...
Saved to: cover/
FAILED: Expected minimum coverage of 100%, got 95.7%.

@LuchoTurtle
Copy link
Member Author

Does your .env file have the following format?

export AWS_ACCESS_KEY_ID='XXXX'
export AWS_SECRET_ACCESS_KEY='XXX'
export AWS_REGION='eu-XX-3'
export AWS_ORIGINAL_BUCKET='imgup-XX'
export AWS_COMPRESSED_BUCKET='imgup-XX'

@LuchoTurtle
Copy link
Member Author

Nevermind.
The API is not working correctly...

image

The old problem where we need to update the secrets remotely is happening again :/

@LuchoTurtle
Copy link
Member Author

LuchoTurtle commented Jul 4, 2023

It seems the bucket env variables are not being correctly loaded when deploying, according to the logs on fly.io.

 [error] ** (RuntimeError) Elixir.ExAws.Operation.ExAws.Operation.S3.perform/2 cannot perform operation on `nil` bucket

https://fly.io/apps/imgup/monitoring

@nelsonic
Copy link
Member

nelsonic commented Jul 4, 2023

The buckets need to be set on Fly.io: https://fly.io/apps/imgup/secrets
image

The values of the Bucket Environment Variables aren't secret; they appear in the URL ... 🔗 😉
So we don't need to obfuscate/hide them.
Just advise people to replace them with their own values.

flyctl secrets set AWS_ORIGINAL_BUCKET=imgup-original -a imgup
flyctl secrets set AWS_COMPRESSED_BUCKET=imgup-compressed -a imgup

Updated:
image

But still getting the error:

Couldn't upload files to S3. Open an issue on Github and contact the repo owner.
image

as noted in: #69 (comment)

@nelsonic
Copy link
Member

nelsonic commented Jul 4, 2023

With the env vars as required:

export AWS_ACCESS_KEY_ID=AKIAetc.
export AWS_SECRET_ACCESS_KEY=aietc.
export AWS_REGION='eu-west-3'
export AWS_ORIGINAL_BUCKET='imgup-original'
export AWS_COMPRESSED_BUCKET='imgup-compressed'

Still get:

mix c


  1) test upload with image keyword (AppWeb.APITest)
     test/app_web/api_test.exs:64
     Assertion with == failed
     code:  assert Jason.decode!(response(conn, 200)) == expected
     left:  %{"compressed_url" => "https://s3.eu-west-3.amazonaws.com//zb2rhXACvyoVCaV1GF5ozeoNCXYdxcKAEWvBTpsnabo3moYwB.png", "url" => "https://s3.eu-west-3.amazonaws.com/imgup-original/zb2rhXACvyoVCaV1GF5ozeoNCXYdxcKAEWvBTpsnabo3moYwB.png"}
     right: %{
              "compressed_url" => "https://s3.eu-west-3.amazonaws.com/imgup-compressed/zb2rhXACvyoVCaV1GF5ozeoNCXYdxcKAEWvBTpsnabo3moYwB.png",
              "url" => "https://s3.eu-west-3.amazonaws.com/imgup-original/zb2rhXACvyoVCaV1GF5ozeoNCXYdxcKAEWvBTpsnabo3moYwB.png"
            }
     stacktrace:
       test/app_web/api_test.exs:74: (test)

....

  2) test upload/1 happy path REAL Upload (App.UploadTest)
     test/app/upload_test.exs:4
     Assertion with == failed
     code:  assert App.Upload.upload(image) == {:ok, expected_response}
     left:  {:ok, %{compressed_url: "https://s3.eu-west-3.amazonaws.com//zb2rhXACvyoVCaV1GF5ozeoNCXYdxcKAEWvBTpsnabo3moYwB.png", url: "https://s3.eu-west-3.amazonaws.com/imgup-original/zb2rhXACvyoVCaV1GF5ozeoNCXYdxcKAEWvBTpsnabo3moYwB.png"}}
     right: {
              :ok,
              %{
                compressed_url: "https://s3.eu-west-3.amazonaws.com/imgup-compressed/zb2rhXACvyoVCaV1GF5ozeoNCXYdxcKAEWvBTpsnabo3moYwB.png",
                url: "https://s3.eu-west-3.amazonaws.com/imgup-original/zb2rhXACvyoVCaV1GF5ozeoNCXYdxcKAEWvBTpsnabo3moYwB.png"
              }
            }
     stacktrace:
       test/app/upload_test.exs:18: (test)



  3) test upload succeeds (happy path) (AppWeb.APITest)
     test/app_web/api_test.exs:51
     Assertion with == failed
     code:  assert Jason.decode!(response(conn, 200)) == expected
     left:  %{"compressed_url" => "https://s3.eu-west-3.amazonaws.com//zb2rhXACvyoVCaV1GF5ozeoNCXYdxcKAEWvBTpsnabo3moYwB.png", "url" => "https://s3.eu-west-3.amazonaws.com/imgup-original/zb2rhXACvyoVCaV1GF5ozeoNCXYdxcKAEWvBTpsnabo3moYwB.png"}
     right: %{
              "compressed_url" => "https://s3.eu-west-3.amazonaws.com/imgup-compressed/zb2rhXACvyoVCaV1GF5ozeoNCXYdxcKAEWvBTpsnabo3moYwB.png",
              "url" => "https://s3.eu-west-3.amazonaws.com/imgup-original/zb2rhXACvyoVCaV1GF5ozeoNCXYdxcKAEWvBTpsnabo3moYwB.png"
            }
     stacktrace:
       test/app_web/api_test.exs:61: (test)

........
Finished in 1.2 seconds (1.1s async, 0.1s sync)
15 tests, 3 failures

Randomized with seed 88516
----------------
COV    FILE                                        LINES RELEVANT   MISSED
100.0% lib/app.ex                                      9        0        0
100.0% lib/app/repo.ex                                 5        0        0
100.0% lib/app/upload.ex                             108       20        0
100.0% lib/app_web/components/layouts.ex               5        0        0
100.0% lib/app_web/controllers/api_controller.e       46       10        0
100.0% lib/app_web/controllers/api_json.ex            20        3        0
100.0% lib/app_web/controllers/page_controller.       25        5        0
100.0% lib/app_web/controllers/page_html.ex            5        0        0
100.0% lib/app_web/endpoint.ex                        48        0        0
100.0% lib/app_web/live/imgup_live.ex                 93       26        0
100.0% lib/app_web/router.ex                          30        4        0
100.0% lib/app_web/s3_upload.ex                      127       24        0
[TOTAL] 100.0%
----------------
Generating report...
Saved to: cover/

The URL in the tests is:

https://s3.eu-west-3.amazonaws.com//zb2rhXACvyoVCaV1GF5ozeoNCXYdxcKAEWvBTpsnabo3moYwB.png

The bucket is being omitted ... 😕

https://github.com/dwyl/imgup/blob/0366a9184f404115fb3c18c17cd90eebc04aaafa/test/app_web/api_test.exs#L43C13-L48

@LuchoTurtle happy to show you this at my desk.

@LuchoTurtle
Copy link
Member Author

image

This is most definitely an issue regarding env variables, which are not being correctly loaded. Perhaps restarting/redeploying might help?

@nelsonic
Copy link
Member

nelsonic commented Jul 5, 2023

Ok. We are looking at two separate (but apparently related) issues here.

  1. The tests don't pass on my localhost.
  2. S3 Bucket environment variables are not set on Fly.io.

I'm going to figure out localhost first.
I have the environment variables defined correctly.
When I run printenv I see the following:

...
HOMEBREW_PREFIX=/opt/homebrew
HOMEBREW_CELLAR=/opt/homebrew/Cellar
HOMEBREW_REPOSITORY=/opt/homebrew
MANPATH=/opt/homebrew/share/man::
INFOPATH=/opt/homebrew/share/info:
ANDROID_HOME=/Users/n/Library/Android/sdk
AWS_ACCESS_KEY_ID=AWS
AWS_SECRET_ACCESS_KEY=aitookourjobs
AWS_REGION=eu-west-3
AWS_ORIGINAL_BUCKET=imgup-original
AWS_COMPRESSED_BUCKET=imgup-compressed
_=/usr/bin/printenv

Checked the spelling on the env vars AWS_ORIGINAL_BUCKET
image

and AWS_COMPRESSED_BUCKET:
image

Ran: MIX_ENV=test mix compile --force

Got the following warning:

warning: Application.get_env/2 is discouraged in the module body, use Application.compile_env/3 instead
  lib/app/upload.ex:7: App.Upload

This relates to the following lines:
https://github.com/dwyl/imgup/blob/0366a9184f404115fb3c18c17cd90eebc04aaafa/test/app_web/api_test.exs#L43C13-L48

But the Good News is that tests work on localhost:

mix c
...............
Finished in 1.5 seconds (1.4s async, 0.1s sync)
15 tests, 0 failures

Randomized with seed 591967
----------------
COV    FILE                                        LINES RELEVANT   MISSED
100.0% lib/app.ex                                      9        0        0
100.0% lib/app/repo.ex                                 5        0        0
100.0% lib/app/upload.ex                             108       20        0
100.0% lib/app_web/components/layouts.ex               5        0        0
100.0% lib/app_web/controllers/api_controller.e       46       10        0
100.0% lib/app_web/controllers/api_json.ex            20        3        0
100.0% lib/app_web/controllers/page_controller.       25        5        0
100.0% lib/app_web/controllers/page_html.ex            5        0        0
100.0% lib/app_web/endpoint.ex                        48        0        0
100.0% lib/app_web/live/imgup_live.ex                 93       26        0
100.0% lib/app_web/router.ex                          30        4        0
100.0% lib/app_web/s3_upload.ex                      127       24        0
[TOTAL] 100.0%
----------------
Generating report...
Saved to: cover/

image

Now looking into "Part 2" on Fly.io.

@nelsonic
Copy link
Member

nelsonic commented Jul 5, 2023

FYI: I've renamed the two S3 Bucket environment variables:

AWS_COMPRESSED_BUCKET-> AWS_S3_BUCKET_ORIGINAL
AWS_COMPRESSED_BUCKET -> AWS_S3_BUCKET_COMPRESSED

The reason is:

  1. Including the S3 keyword in the env var makes it clear what kind of bucket we are using. Believe it or not, I've had several people ask me this question in the past when using S3 ... 🙄 So having it clear avoids those questions. 👌
  2. Putting the "variable" at the end of the variable name helps with scanning.

I've updated the variables in the project, CI and Fly.io.

flyctl secrets set AWS_S3_BUCKET_ORIGINAL=imgup-original -a imgup
flyctl secrets set AWS_S3_BUCKET_COMPRESSED=imgup-compressed -a imgup

Removed the "old" ones to avoid confusion:

fly secrets unset AWS_ORIGINAL_BUCKET -a imgup
fly secrets unset AWS_COMPRESSED_BUCKET -a imgup

https://fly.io/apps/imgup/secrets
image

FYI: the two bucket names were not in the README.md Env Var section:
https://github.com/dwyl/imgup/tree/edge-cases-%2369#2-get-your-aws-keys-and-export-as-environment-variables
image

So added them. ✅

```elixir
def upload(image) do
# Create `CID` from file contents so filenames are unique
case File.read(image.path) do
Copy link
Member

Choose a reason for hiding this comment

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

@LuchoTurtle you've shifted the logic without simplifying it.
This is worse than inlining it.
And without any logging this code is basically a black box when debugging.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added comment on this in #86 (comment).

Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

I'm not happy with writing a mock or code for an unspecified AWS S3 failure event that we cannot test for.
It's totally fine to have a generic failure event in this exceptionally unlikely event of there being an error with S3.

Generally speaking, a Software product/app that calls many external APIs in the "backend"
does not expose these details to the client/consumer.
Unless the client already knows those services are being used, e.g. Google Auth
In the case of S3 the image URL currently has S3 in the address but it definitely won't for ever ...

The last S3 outage was in 2017: https://aws.amazon.com/message/41926/
They are exceedingly rare and if we cannot test the scenario, to all intents and purposes, it doesn't exist.

In the "Mars Rover" analogy: if the uplink back to NASA HQ is down
the lander isn't going to send a messages back to NASA HQ saying the channel is broken because they already know and what channel would they use ... 🤷‍♂️

If an error occurs with S3 we just return a generic:
"Sorry uploads are not working right now, please try again later."
Or if we want to get fancy, we return a "trace ID" that allows the engineers to immediately find that request in the logs.
See: https://www.w3.org/TR/trace-context/

We can (should) log the error in our infra so that we know why it happened.
And when we eventually have a "status" page we can write a quick post explaining what the root cause is so people how how we fixed it.
But adding mocks to the project just so that we can pretend to test for this black swan event is not useful.

We don't yet have a "style guide" for our Elixir code.
But if we did the with keyword and <- (reverse arrow) notation would definitely be on the "No" list.
They make the code terse without any real benefit other than masking complexity of implementation.

    with {:ok, {file_cid, file_extension}} <- check_file_binary_and_extension(image),
         {:ok, {file_name, upload_response_body}} <-
           upload_file_to_s3(file_cid, file_extension, image) do

This is just shifting business logic into one line and requires significant time for the (human) reader to parse.
"Go do all these things and if anything fails return a bunch of errors..."
Then putting a bunch of matches for those errors:

{:error, :failure_read} -> {:error, :failure_read}
{:error, :invalid_extension_and_cid} -> {:error, :invalid_extension_and_cid}
{:error, :invalid_cid} -> {:error, :invalid_cid}
{:error, :invalid_extension} -> {:error, :invalid_extension}
{:error, :upload_fail} -> {:error, :upload_fail}

If you aren't actually doing anything with the error message/type,
then just pass it straight through with a generic "reason":

{:error, reason} -> {:error, reason}

Otherwise you are duplicating logic unnecessarily.

I don't like this code at all. Because all you have done is move the complexity elsewhere.
One of the biggest problems smart people have is they write complex code.
Code that will confuse and overwhelm a beginner.
This is fine in a close source 2-person project.
But not desirable in an Open Source project where we want others to read and contribute to the code.

@nelsonic
Copy link
Member

I don't want to just close this PR and waste both my own time and Lucho's time. ⏳ 💸 🔥
But I don't feel like any of this code addresses a problem that a person using the API has identified. 🤷‍♂️
Yes, we want to address potential edge cases. ✅
We want to make the API helpful to the people using it. 🙏
But this PR doesn't do that. 😕
It has zero Logging which means the engineers cannot debug errors
and returns unhelpful errors that mask the underlying issue.

@nelsonic nelsonic added in-progress An issue or pull request that is being worked on by the assigned person and removed awaiting-review An issue or pull request that needs to be reviewed labels Jul 15, 2023
@nelsonic nelsonic moved this from Delegated / External to In progress in Nelson's List Jul 15, 2023
@nelsonic
Copy link
Member

For future reference, whenever you are tempted to put the word "and" in a function name, don't.
check_file_binary_and_extension is an instant "code smell".

api.md Outdated Show resolved Hide resolved
@LuchoTurtle LuchoTurtle assigned LuchoTurtle and unassigned nelsonic Jul 17, 2023
@nelsonic
Copy link
Member

@LuchoTurtle I've already removed (commented out) the offending mock and added a valid test.
The remaining tests are all passing with 100% coverage.
This PR is mergeable. Assign back to me once you've had a chance to tidy the section in the README.md
Thanks. 👍

@LuchoTurtle
Copy link
Member Author

LuchoTurtle commented Jul 17, 2023

I've made some changes, namely:

  • added logging.
  • added tests for invalid content types.
  • changed API error messages that are relevant to the person, and defaulted unlikely scenarios (such as S3 being down) to display a generic message.

I didn't change "code smells" like check_file_binary_and_extension since you want to get this merged ASAP and that would entail more time spend reviewing.
Granted, although I understand your statements about not wanting to use with statements and avoiding nested case statements that capture these edge cases, I don't know how to "simplify" the code that will, simultaneously:

  • log each scenario so we know what exactly went wrong.
  • not use nested cases without containing logic in functions

Perhaps my Elixir syntax skills are lacking but I don't know how to make the code more readable and simpler without doing these.

Assigning back to you, I've made the changes according to the feedback and commented on those that were affected.

@LuchoTurtle LuchoTurtle assigned nelsonic and unassigned LuchoTurtle Jul 17, 2023
@LuchoTurtle LuchoTurtle added awaiting-review An issue or pull request that needs to be reviewed and removed in-progress An issue or pull request that is being worked on by the assigned person labels Jul 17, 2023
Copy link
Member

@nelsonic nelsonic left a comment

Choose a reason for hiding this comment

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

Thanks @LuchoTurtle 👌
All good enhancements. ✅
Hopefully we can focus on features requested by people using the product. 🤞

@nelsonic nelsonic merged commit 66a8e0f into main Jul 17, 2023
3 checks passed
Nelson's List automation moved this from In progress to Done Jul 17, 2023
@nelsonic nelsonic deleted the edge-cases-#69 branch July 17, 2023 13:10
@LuchoTurtle
Copy link
Member Author

@nelsonic working on #87 because of merge conflicts and trying to remove Mock, fyi

@nelsonic
Copy link
Member

Cool. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting-review An issue or pull request that needs to be reviewed chore a tedious but necessary task often paying technical debt elixir Pull requests that update Elixir code enhancement New feature or enhancement of existing functionality
Projects
Nelson's List
  
Done
Status: Done
Development

Successfully merging this pull request may close these issues.

Edge cases in Upload
2 participants