feat: 🏹 Implement Shuttle Route KML Save to S3 #984

nlwstein · 2024-06-11T19:10:28Z

Summary of changes

Asana Ticket: 🏹 Implement Shuttle Route KML Save to S3

Uploaded shape files are now stored in S3.
Uploaded shape files can be downloaded securely from their record in the dashboard.
Uploaded shape files are deleted from S3 upon deletion of the record.
Uploaded shape files cannot squash existing files. This deviates slightly from the ticket description - I feel that it is simpler to just tell the user that the file already exists, since I would imagine these are only being managed from this UI going forward. If we need the ability to squash as opposed to the user simply renaming the file, I think we should automatically detect the existing record and prompt them to remove that as well to keep a 1:1 relationship between file in s3 and record in arrow.

Reviewer Checklist

Meets ticket's acceptance criteria
Any new or changed functions have typespecs
Tests were added for any new functionality (don't just rely on Codecov)
This branch was deployed to the staging environment and is currently running with no unexpected increase in warnings, and no errors or crashes.

github-actions · 2024-06-12T12:30:43Z

Coverage of commit `332d995`

Summary coverage rate:
  lines......: 79.1% (782 of 989 lines)
  functions..: 61.6% (568 of 922 functions)
  branches...: no data found

Files changed coverage rate:
                                                                     |Lines       |Functions  |Branches    
  Filename                                                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================================================
  lib/arrow/shuttle.ex                                               |58.3%     36|84.6%    13|    -      0
  lib/arrow/shuttle/shape.ex                                         | 100%      2|83.3%     6|    -      0
  lib/arrow_web/controllers/shape_controller.ex                      |76.7%     30|85.7%    14|    -      0
  lib/arrow_web/router.ex                                            |90.5%     21|40.7%    81|    -      0
  test/support/fixtures/shuttle_fixtures.ex                          | 100%      4| 100%     3|    -      0

Download coverage report

github-actions · 2024-06-12T12:35:17Z

Coverage of commit `ad0f4d7`

Summary coverage rate:
  lines......: 79.1% (785 of 992 lines)
  functions..: 61.6% (568 of 922 functions)
  branches...: no data found

Files changed coverage rate:
                                                                     |Lines       |Functions  |Branches    
  Filename                                                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================================================
  lib/arrow/shuttle.ex                                               |61.5%     39|84.6%    13|    -      0
  lib/arrow/shuttle/shape.ex                                         | 100%      2|83.3%     6|    -      0
  lib/arrow_web/controllers/shape_controller.ex                      |76.7%     30|85.7%    14|    -      0
  lib/arrow_web/router.ex                                            |90.5%     21|40.7%    81|    -      0
  test/support/fixtures/shuttle_fixtures.ex                          | 100%      4| 100%     3|    -      0

Download coverage report

github-actions · 2024-06-12T12:36:35Z

Coverage of commit `ad0f4d7`

Summary coverage rate:
  lines......: 79.1% (785 of 992 lines)
  functions..: 61.6% (568 of 922 functions)
  branches...: no data found

Files changed coverage rate:
                                                                     |Lines       |Functions  |Branches    
  Filename                                                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================================================
  lib/arrow/shuttle.ex                                               |61.5%     39|84.6%    13|    -      0
  lib/arrow/shuttle/shape.ex                                         | 100%      2|83.3%     6|    -      0
  lib/arrow_web/controllers/shape_controller.ex                      |76.7%     30|85.7%    14|    -      0
  lib/arrow_web/router.ex                                            |90.5%     21|40.7%    81|    -      0
  test/support/fixtures/shuttle_fixtures.ex                          | 100%      4| 100%     3|    -      0

Download coverage report

github-actions · 2024-06-12T12:37:52Z

Coverage of commit `ad0f4d7`

Summary coverage rate:
  lines......: 79.1% (785 of 992 lines)
  functions..: 61.6% (568 of 922 functions)
  branches...: no data found

Files changed coverage rate:
                                                                     |Lines       |Functions  |Branches    
  Filename                                                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================================================
  lib/arrow/shuttle.ex                                               |61.5%     39|84.6%    13|    -      0
  lib/arrow/shuttle/shape.ex                                         | 100%      2|83.3%     6|    -      0
  lib/arrow_web/controllers/shape_controller.ex                      |76.7%     30|85.7%    14|    -      0
  lib/arrow_web/router.ex                                            |90.5%     21|40.7%    81|    -      0
  test/support/fixtures/shuttle_fixtures.ex                          | 100%      4| 100%     3|    -      0

Download coverage report

nlwstein · 2024-06-12T13:15:58Z

config/config.exs

+  http_client: HTTPoison,
+  shape_storage_enabled?: false,
+  shape_storage_prefix_env: "arrow-dev-local",
+  shape_storage_bucket: "mbta-gtfs-s3-sandbox",


This should be mbta-arrow, but I do not have write access at least with my credentials.

nlwstein · 2024-06-12T13:20:32Z

config/config.exs

@@ -34,7 +34,11 @@ config :arrow,
  },
  time_zone: "America/New_York",
  ex_aws_requester: {Fake.ExAws, :admin_group_request},
-  http_client: HTTPoison
+  http_client: HTTPoison,
+  shape_storage_enabled?: false,


Disabled by default, and just stores disabled as the value for all of the relevant s3 columns.

nlwstein · 2024-06-12T13:21:30Z

config/config.exs

-  http_client: HTTPoison
+  http_client: HTTPoison,
+  shape_storage_enabled?: false,
+  shape_storage_prefix_env: "arrow-dev-local",


The storage path is like this: $PREFIX/$PREFIX_ENV/the_file.kml. In prod, the prefix_env is set via the existing AWS env var.

nlwstein · 2024-06-12T13:22:27Z

lib/arrow_web/controllers/layout_html/_flash.html.heex

@@ -2,6 +2,10 @@
  <aside role="alert" class="alert alert-info"><%= message %></aside>
 <% end %>

+<%= with message when is_binary(message) <- Phoenix.Flash.get(@flash, :error) do %>


Added this to display a simple error flash - it looks like the existing error flash was designed to show a whole collection of errors (form validation).

nlwstein · 2024-06-12T13:23:19Z

lib/arrow_web/controllers/shape_controller.ex

+    |> Plug.Conn.resp(:found, "")
+    |> Plug.Conn.put_resp_header(
+      "location",
+      "https://#{shape.bucket}.s3.amazonaws.com/#{shape.path}"


This is why I store the bucket and path - it may be a pre-optimization but should help if we ever have to rename these things vs. string replacing a URL column.

nlwstein · 2024-06-12T13:23:55Z

mix.exs

@@ -51,6 +51,8 @@ defmodule Arrow.MixProject do
      {:ex_aws_rds, "~> 2.0"},
      {:ex_aws_secretsmanager, "~> 2.0"},
      {:ex_aws, "~> 2.1"},
+      {:ex_aws_s3, "~> 2.1"},
+      {:sweet_xml, "~> 0.7.4"},


sweet_xml is soft-required by ex_aws_s3.

nlwstein · 2024-06-12T13:25:26Z

test/arrow/shuttle_test.exs

@@ -20,11 +20,24 @@ defmodule Arrow.ShuttleTest do
      assert Shuttle.get_shape!(shape.id) == shape
    end

-    test "create_shape/1 with valid data creates a shape" do
-      valid_attrs = %{name: "some name"}
+    test "create_shape/1 with valid data creates a shape and uploads to s3" do


Updated our "good-path" test to also actually upload to S3.

My one concern with this is storage usage over time without a cleanup routine. I think the cleanest implementation would be to update the policy to whack anything with arrow/test-runner/* older than n days, vs cleaning it up in the test, because runs that fail may leave detritus on the bucket.

nlwstein · 2024-06-12T13:47:22Z

@meagharty There are a few outstanding issues here that aren't really related to the core feature but may block it's acceptance into the repo until they are complete:

S3 bucket mbta-arrow's permissions don't seem to be right. (Also, my tester S3 bucket mbta-gtfs-s3-sandbox's permissions are fine for writing but it does not have public read and thus the /shape/{id}/download route isn't working quite right, but we shouldn't fix this...and just update the correct bucket!)
The tests fail on GitHub because the AWS user has less permissions than I do to interact with S3.
We should also add a policy for truncating the test files uploaded (and possibly the dev env files uploaded) to avoid a ton of useless storage accumulation on s3.

Otherwise, I think the feature as-is can be reviewed while we deal with these issues 😄 If you disagree I can take it back as a draft while I resolve these things.

meagharty · 2024-06-12T15:37:52Z

test/arrow/shuttle_test.exs

+        "path" => "some/path/to/sample.kml",
+        "prefix" => prefix,
+        "bucket" => Application.get_env(:arrow, :shape_storage_bucket),
+        "filename" => %Plug.Upload{filename: "sample.kml", path: "test_files/sample.kml"}


not blocking: we could resolve this in a future PR since the full featureset is expected to extract individual shapes from a KML file (not part of this PR)

But: I was talking with @bfauble and we might want this filename to be saved as #{name}.kml. To make the names easier to reconcile between gtfs_creator and arrow.

I am pro-keeping the KML format around since it's industry common.

@meagharty This is a great idea and would also mean that we could drop the columns eventually - since the file path / name / etc. would be inherent.

I guess an alternative would be a "smart" connection between the two systems where it would pull some metadata JSON payload, which might be more extensible long term BUT I think starting with the names makes a lot of sense.

It would be an easy change here, too!

I also feel like this could be a follow up ticket, just to avoid this getting stuck in review as long as you're cool with that (re: non-blocking).

github-actions · 2024-06-21T15:45:21Z

Coverage of commit `49bf9d2`

Summary coverage rate:
  lines......: 80.4% (813 of 1011 lines)
  functions..: 61.7% (586 of 950 functions)
  branches...: no data found

Files changed coverage rate:
                                                                     |Lines       |Functions  |Branches    
  Filename                                                           |Rate     Num|Rate    Num|Rate     Num
  =========================================================================================================
  lib/arrow/mock/exaws/request.ex                                    | 100%      1| 100%     1|    -      0
  lib/arrow/shuttle.ex                                               |86.0%     43|92.3%    13|    -      0
  lib/arrow/shuttle/shape.ex                                         | 100%      2|83.3%     6|    -      0
  lib/arrow_web/controllers/shape_controller.ex                      |85.3%     34|92.9%    14|    -      0
  lib/arrow_web/router.ex                                            |95.2%     21|41.4%    87|    -      0
  test/support/fixtures/shuttle_fixtures.ex                          | 100%      4| 100%     3|    -      0

Download coverage report

bfauble · 2024-06-28T16:47:58Z

config/prod.exs

+
+config :arrow,
+  shape_storage_enabled?: true,
+  shape_storage_prefix_env: System.get_env("AWS_SECRET_PREFIX") || "arrow-prod-unknown"


instead of having this default, should it just be nil and included as part of the check with enabled?

bfauble · 2024-06-28T16:54:50Z

lib/arrow/shuttle.ex

+      case upload_shape(attrs["filename"]) do
+        {:ok, new_attrs} ->
+          # Delete old file:
+          {:ok, _} = delete_shape_file(shape)


is there not a way to just overwrite when uploading?

nlwstein changed the base branch from master to meag/add-phx7-core-components June 11, 2024 19:11

nlwstein changed the base branch from meag/add-phx7-core-components to meag/add-shuttle-shape-page June 11, 2024 19:11

nlwstein commented Jun 12, 2024

View reviewed changes

nlwstein changed the title ~~Nlws s3 upload~~ feat: 🏹 Implement Shuttle Route KML Save to S3 Jun 12, 2024

nlwstein requested a review from meagharty June 12, 2024 13:43

nlwstein marked this pull request as ready for review June 12, 2024 13:47

meagharty reviewed Jun 12, 2024

View reviewed changes

meagharty mentioned this pull request Jun 13, 2024

feat: add shuttle shape page #978

Merged

4 tasks

Base automatically changed from meag/add-shuttle-shape-page to master June 13, 2024 15:59

meagharty self-requested a review June 14, 2024 18:56

feat: add s3 upload functionality

303a9e9

nlwstein force-pushed the nlws-s3-upload branch from bfaf658 to 303a9e9 Compare June 21, 2024 15:27

nlwstein added 3 commits June 21, 2024 11:31

chore: update structure.sql

9eb54d5

fix: run other migrations

1f3309a

test: just validate that we get the redirect action

49bf9d2

nlwstein requested review from a team and bfauble and removed request for a team June 21, 2024 15:46

bfauble reviewed Jun 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: 🏹 Implement Shuttle Route KML Save to S3 #984

feat: 🏹 Implement Shuttle Route KML Save to S3 #984

nlwstein commented Jun 11, 2024 •

edited

Loading

github-actions bot commented Jun 12, 2024

github-actions bot commented Jun 12, 2024

github-actions bot commented Jun 12, 2024

github-actions bot commented Jun 12, 2024

nlwstein Jun 12, 2024 •

edited

Loading

nlwstein Jun 12, 2024

nlwstein Jun 12, 2024

nlwstein Jun 12, 2024

nlwstein Jun 12, 2024

nlwstein Jun 12, 2024

nlwstein Jun 12, 2024

nlwstein commented Jun 12, 2024

meagharty Jun 12, 2024 •

edited

Loading

nlwstein Jun 13, 2024

nlwstein Jun 13, 2024 •

edited

Loading

github-actions bot commented Jun 21, 2024

bfauble Jun 28, 2024

bfauble Jun 28, 2024

feat: 🏹 Implement Shuttle Route KML Save to S3 #984

Are you sure you want to change the base?

feat: 🏹 Implement Shuttle Route KML Save to S3 #984

Conversation

nlwstein commented Jun 11, 2024 • edited Loading

Summary of changes

Reviewer Checklist

github-actions bot commented Jun 12, 2024

Coverage of commit 332d995

github-actions bot commented Jun 12, 2024

Coverage of commit ad0f4d7

github-actions bot commented Jun 12, 2024

Coverage of commit ad0f4d7

github-actions bot commented Jun 12, 2024

Coverage of commit ad0f4d7

nlwstein Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlwstein commented Jun 12, 2024

meagharty Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlwstein Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jun 21, 2024

Coverage of commit 49bf9d2

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nlwstein commented Jun 11, 2024 •

edited

Loading

Coverage of commit `332d995`

Coverage of commit `ad0f4d7`

Coverage of commit `ad0f4d7`

Coverage of commit `ad0f4d7`

nlwstein Jun 12, 2024 •

edited

Loading

meagharty Jun 12, 2024 •

edited

Loading

nlwstein Jun 13, 2024 •

edited

Loading

Coverage of commit `49bf9d2`