From 392f7fe93c77ad5ba8821cb59fede5ae4688ae21 Mon Sep 17 00:00:00 2001 From: Antoine Augusti Date: Thu, 14 Sep 2023 14:00:17 +0200 Subject: [PATCH] GBFS : suppression des redirections (#3456) * GBFS : suppression des redirections * Update migration to delete data * Add a catch-all to return 404 responses --- .../lib/gbfs/controllers/index_controller.ex | 6 ++ .../gbfs/controllers/redirect_controller.ex | 10 --- apps/gbfs/lib/gbfs/router.ex | 77 ++----------------- apps/gbfs/lib/page_cache.ex | 2 +- .../controllers/index_controller_test.exs | 6 ++ .../controllers/redirect_controller_test.exs | 54 ------------- apps/gbfs/test/gbfs/page_cache_test.exs | 1 + ...0230913130308_metrics_gbfs_target_name.exs | 2 +- 8 files changed, 22 insertions(+), 136 deletions(-) delete mode 100644 apps/gbfs/lib/gbfs/controllers/redirect_controller.ex delete mode 100644 apps/gbfs/test/gbfs/controllers/redirect_controller_test.exs diff --git a/apps/gbfs/lib/gbfs/controllers/index_controller.ex b/apps/gbfs/lib/gbfs/controllers/index_controller.ex index 3bb77b47f2..f39ace332f 100644 --- a/apps/gbfs/lib/gbfs/controllers/index_controller.ex +++ b/apps/gbfs/lib/gbfs/controllers/index_controller.ex @@ -27,4 +27,10 @@ defmodule GBFS.IndexController do } } end + + def not_found(%Plug.Conn{} = conn, _params) do + conn + |> put_status(:not_found) + |> text("Network not found. See available data: https://transport.data.gouv.fr/datasets?type=bike-scooter-sharing") + end end diff --git a/apps/gbfs/lib/gbfs/controllers/redirect_controller.ex b/apps/gbfs/lib/gbfs/controllers/redirect_controller.ex deleted file mode 100644 index 259585e2b0..0000000000 --- a/apps/gbfs/lib/gbfs/controllers/redirect_controller.ex +++ /dev/null @@ -1,10 +0,0 @@ -defmodule GBFS.RedirectController do - use GBFS, :controller - - def index(%Plug.Conn{assigns: %{redirect_params: %{redirects: redirects}}} = conn, %{"path" => path}) do - case Map.get(redirects, path) do - nil -> conn |> put_status(:not_found) |> text("404 not found") - destination -> conn |> put_status(301) |> redirect(external: destination) - end - end -end diff --git a/apps/gbfs/lib/gbfs/router.ex b/apps/gbfs/lib/gbfs/router.ex index 7582d29856..37f03ceacc 100644 --- a/apps/gbfs/lib/gbfs/router.ex +++ b/apps/gbfs/lib/gbfs/router.ex @@ -12,10 +12,6 @@ defmodule GBFS.Router do plug(:assign_jcdecaux) end - pipeline :redirect_reseau do - plug(:assign_redirect) - end - pipeline :index_pipeline do plug(:assign_index) end @@ -32,66 +28,9 @@ defmodule GBFS.Router do "toulouse" => "Vélô" } - @reseaux_redirects [ - %{ - contract_id: "montpellier", - redirects: %{ - "gbfs.json" => "https://montpellier-fr-smoove.klervi.net/gbfs/gbfs.json", - "system_information.json" => "https://montpellier-fr-smoove.klervi.net/gbfs/en/system_information.json", - "station_information.json" => "https://montpellier-fr-smoove.klervi.net/gbfs/en/station_information.json", - "station_status.json" => "https://montpellier-fr-smoove.klervi.net/gbfs/en/station_status.json" - } - }, - %{ - contract_id: "strasbourg", - redirects: %{ - "gbfs.json" => "https://strasbourg-fr-smoove.klervi.net/gbfs/gbfs.json", - "system_information.json" => "https://strasbourg-fr-smoove.klervi.net/gbfs/en/system_information.json", - "station_information.json" => "https://strasbourg-fr-smoove.klervi.net/gbfs/en/station_information.json", - "station_status.json" => "https://strasbourg-fr-smoove.klervi.net/gbfs/en/station_status.json" - } - }, - %{ - contract_id: "rouen", - redirects: %{ - "gbfs.json" => "https://gbfs.urbansharing.com/lovelolibreservice.fr/gbfs.json", - "system_information.json" => "https://gbfs.urbansharing.com/lovelolibreservice.fr/system_information.json", - "station_information.json" => "https://gbfs.urbansharing.com/lovelolibreservice.fr/station_information.json", - "station_status.json" => "https://gbfs.urbansharing.com/lovelolibreservice.fr/station_status.json" - } - }, - %{ - contract_id: "marseille", - redirects: %{ - "gbfs.json" => - "https://api.omega.fifteen.eu/gbfs/2.2/marseille/en/gbfs.json?key=MjE0ZDNmMGEtNGFkZS00M2FlLWFmMWItZGNhOTZhMWQyYzM2", - "system_information.json" => - "https://api.omega.fifteen.eu/gbfs/2.2/marseille/en/system_information.json?key=MjE0ZDNmMGEtNGFkZS00M2FlLWFmMWItZGNhOTZhMWQyYzM2", - "station_information.json" => - "https://api.omega.fifteen.eu/gbfs/2.2/marseille/en/station_information.json?key=MjE0ZDNmMGEtNGFkZS00M2FlLWFmMWItZGNhOTZhMWQyYzM2", - "station_status.json" => - "https://api.omega.fifteen.eu/gbfs/2.2/marseille/en/station_status.json?key=MjE0ZDNmMGEtNGFkZS00M2FlLWFmMWItZGNhOTZhMWQyYzM2" - } - } - ] - scope "/gbfs", GBFS do pipe_through(:api) - scope "/" do - pipe_through(:index_pipeline) - get("/", IndexController, :index) - end - - @reseaux_redirects - |> Enum.map(fn %{contract_id: contract_id} -> - scope "/" <> contract_id do - pipe_through(:redirect_reseau) - - get("/:path", RedirectController, :index, as: contract_id) - end - end) - scope "/vcub" do get("/gbfs.json", VCubController, :index) get("/system_information.json", VCubController, :system_information) @@ -118,6 +57,12 @@ defmodule GBFS.Router do get("/station_status.json", JCDecauxController, :station_status, as: contract) end end) + + scope "/" do + pipe_through(:index_pipeline) + get("/", IndexController, :index) + get("/*path", IndexController, :not_found) + end end defp assign_jcdecaux(conn, _) do @@ -128,19 +73,11 @@ defmodule GBFS.Router do |> assign(:contract_name, @reseaux_jcdecaux[contract_id]) end - defp assign_redirect(conn, _) do - [_, contract_id, _] = conn.path_info - - conn - |> assign(:redirect_params, Enum.find(@reseaux_redirects, &(&1.contract_id == contract_id))) - end - defp assign_index(conn, _) do conn |> assign( :networks, - ["vcub", "vlille"] ++ - (@reseaux_jcdecaux |> Map.keys()) ++ (@reseaux_redirects |> Enum.map(& &1.contract_id)) + ["vcub", "vlille"] ++ (@reseaux_jcdecaux |> Map.keys()) ) end end diff --git a/apps/gbfs/lib/page_cache.ex b/apps/gbfs/lib/page_cache.ex index 594c7f38a4..560bf693c3 100644 --- a/apps/gbfs/lib/page_cache.ex +++ b/apps/gbfs/lib/page_cache.ex @@ -113,7 +113,7 @@ defmodule PageCache do end def network_name(page_cache_key) do - case Regex.named_captures(~r|/gbfs/(?.+)/|, page_cache_key) do + case Regex.named_captures(~r|/gbfs/(?[a-zA-Z0-9_-]+)/|, page_cache_key) do %{"network" => network} -> network _ -> nil end diff --git a/apps/gbfs/test/gbfs/controllers/index_controller_test.exs b/apps/gbfs/test/gbfs/controllers/index_controller_test.exs index 9b87f36ee2..5f8d264e7d 100644 --- a/apps/gbfs/test/gbfs/controllers/index_controller_test.exs +++ b/apps/gbfs/test/gbfs/controllers/index_controller_test.exs @@ -17,4 +17,10 @@ defmodule GBFS.IndexControllerTest do assert first_href == "http://localhost/gbfs/vcub/gbfs.json" end end + + test "404 pages", %{conn: conn} do + expected_regex = ~r"^Network not found. See available data:" + assert conn |> get("/gbfs/foo") |> text_response(404) =~ expected_regex + assert conn |> get("/gbfs/foo/gbfs.json") |> text_response(404) =~ expected_regex + end end diff --git a/apps/gbfs/test/gbfs/controllers/redirect_controller_test.exs b/apps/gbfs/test/gbfs/controllers/redirect_controller_test.exs deleted file mode 100644 index 8a470322c5..0000000000 --- a/apps/gbfs/test/gbfs/controllers/redirect_controller_test.exs +++ /dev/null @@ -1,54 +0,0 @@ -defmodule GBFS.RedirectControllerTest do - use GBFS.ConnCase, async: true - - describe "Montpellier and Strasbourg" do - test "it redirects", %{conn: conn} do - test_klervi_for_city(conn, "montpellier") - test_klervi_for_city(conn, "strasbourg") - end - - test "unknown path", %{conn: conn} do - assert conn |> get("/gbfs/montpellier/foo") |> text_response(404) == "404 not found" - assert conn |> get("/gbfs/strasbourg/foo") |> text_response(404) == "404 not found" - end - end - - test "redirects for Marseille", %{conn: conn} do - ~w(gbfs system_information station_information station_status)a - |> Enum.each(fn path -> - conn = conn |> get("/gbfs/marseille/#{path}.json") - - expected = - "https://api.omega.fifteen.eu/gbfs/2.2/marseille/en/#{path}.json?key=MjE0ZDNmMGEtNGFkZS00M2FlLWFmMWItZGNhOTZhMWQyYzM2" - - assert redirected_to(conn, 301) == expected - end) - end - - test "redirects for Rouen", %{conn: conn} do - ~w(gbfs system_information station_information station_status)a - |> Enum.each(fn path -> - conn = conn |> get("/gbfs/rouen/#{path}.json") - - expected = "https://gbfs.urbansharing.com/lovelolibreservice.fr/#{path}.json" - - assert redirected_to(conn, 301) == expected - end) - end - - defp test_klervi_for_city(conn, city) do - klervi_base = "https://#{city}-fr-smoove.klervi.net/gbfs/" - - expected = %{ - "gbfs.json" => klervi_base <> "gbfs.json", - "system_information.json" => klervi_base <> "en/system_information.json", - "station_information.json" => klervi_base <> "en/station_information.json", - "station_status.json" => klervi_base <> "en/station_status.json" - } - - for {path, target} <- expected do - conn = conn |> get("/gbfs/#{city}/#{path}") - assert redirected_to(conn, 301) == target - end - end -end diff --git a/apps/gbfs/test/gbfs/page_cache_test.exs b/apps/gbfs/test/gbfs/page_cache_test.exs index 87b129f3af..5e8615c8f6 100644 --- a/apps/gbfs/test/gbfs/page_cache_test.exs +++ b/apps/gbfs/test/gbfs/page_cache_test.exs @@ -61,6 +61,7 @@ defmodule GBFS.PageCacheTest do assert nil == PageCache.network_name("/foo") assert nil == PageCache.network_name("/gbfs") assert "nantes" == PageCache.network_name("/gbfs/nantes/gbfs.json") + assert "nantes" == PageCache.network_name("/gbfs/nantes//gbfs.json") assert "nantes" == PageCache.network_name("/gbfs/nantes/station_information.json") assert "st_helene" == PageCache.network_name("/gbfs/st_helene/station_information.json") assert "cergy-pontoise" == PageCache.network_name("/gbfs/cergy-pontoise/station_information.json") diff --git a/apps/transport/priv/repo/migrations/20230913130308_metrics_gbfs_target_name.exs b/apps/transport/priv/repo/migrations/20230913130308_metrics_gbfs_target_name.exs index 8c3acb4952..fe6b73d02e 100644 --- a/apps/transport/priv/repo/migrations/20230913130308_metrics_gbfs_target_name.exs +++ b/apps/transport/priv/repo/migrations/20230913130308_metrics_gbfs_target_name.exs @@ -3,7 +3,7 @@ defmodule DB.Repo.Migrations.MetricsGBFSTargetName do def up do # See https://github.com/etalab/transport-site/issues/3458 - execute "update metrics set target = replace(target, '/', '') where target like 'gbfs:%/'" + execute "delete from metrics where target like 'gbfs:%/'" end def down do