Calcul et sauvegarde de métadonnées pour les ressources GBFS #1905

AntoineAugusti · 2021-11-05T13:24:14Z

Implémentation de #1852, utilise #1902

Cette PR ajoute une tâche quotidienne en charge de calculer et de stocker des métadonnées pour les ressources GBFS. Elle identifie les ressources GBFS, calcule des métadonnées (dont la validité du flux) et enregisre ceci dans les métadonnées de chaque ressource.

Un exemple de métadonnées

%{
  languages: ["fr"],
  system_details: %{name: "velhop", timezone: "Europe/Paris"},
  ttl: 3600,
  types: ["stations"],
  versions: ["1.1"],
  feeds: ["system_information", "station_information", "station_status"],
  validation: %GBFSValidationSummary{
    errors_count: 0,
    has_errors: false,
    version_detected: "1.1",
    version_validated: "1.1"
  },
  has_cors: true,
  is_cors_allowed: true
}

AntoineAugusti · 2021-11-08T13:35:35Z

apps/shared/test/validation/gbfs_validator_test.exs

+  setup do
+    :verify_on_exit!
+    # Do not use a mock for the GBFS Validator as we'll mock HTTP calls
+    old_value = Application.fetch_env!(:transport, :gbfs_validator_impl)
+    on_exit(fn -> Application.put_env(:transport, :gbfs_validator_impl, old_value) end)
+    Application.put_env(:transport, :gbfs_validator_impl, Shared.Validation.GBFSValidator.HTTPClient)
+  end


Similaire aux tests utilisant le "page cache" dans l'app GBFS

apps/transport/lib/transport/gbfs_metadata.ex

AntoineAugusti · 2021-11-09T07:48:43Z

apps/transport/lib/transport/gbfs_metadata.ex

+  end
+
+  @spec compute_feed_metadata(Resource.t()) :: map()
+  def compute_feed_metadata(resource) do


C'est cette fonction qui est testée avec attention

AntoineAugusti · 2021-11-09T07:48:57Z

config/prod.exs

@@ -20,6 +20,8 @@ config :transport, Transport.Scheduler,
    {"@daily", {Transport.DataChecker, :outdated_data, []}},
    # Set inactive data
    {"@daily", {Transport.DataChecker, :inactive_data, []}},
+    # Compute metadata for GBFS resources
+    {"@daily", {Transport.GBFSMetadata, :set_gbfs_feeds_metadata, []}},


Un avis sur la fréquence et l'enchainement par rapport aux autres jobs ?

Actuellement ça finit en moins de 30 secondes car ça analyse 30 feeds

Pour commencer une fois par nuit ça me paraît bien comme tu as mis là !

Après une fois qu'on sera sur du job récurrent, on pourra peut-être accélérer et voir comment ça gère.

AntoineAugusti · 2021-11-10T09:16:28Z

apps/transport/lib/transport/gbfs_metadata.ex

+      Resource
+      |> join(:inner, [r], d in Dataset, on: r.dataset_id == d.id)
+      |> where([_r, d], d.type == "bike-scooter-sharing" and d.is_active)
+      |> where([r, _d], like(r.url, "%gbfs.json") or r.format == "gbfs")
+      |> where([r, _d], not fragment("? ~ ?", r.url, "station|free_bike"))
+      |> Repo.all()


Reprend la requête SQL ici #1891 (comment)

thbar

Je vais proposer de petits refactorings sur les tests, et ajouter un peu de doc, mais ça semble good !

thbar · 2021-11-15T08:51:39Z

apps/shared/lib/validation/gbfs_validator.ex

@@ -8,6 +8,7 @@ defmodule Shared.Validation.GBFSValidator do
    A structure holding validation results for a GBFS feed
    """
    @enforce_keys [:has_errors, :errors_count, :version_detected, :version_validated]
+    @derive Jason.Encoder


Lectures recommandées:

https://elixir-lang.org/getting-started/protocols.html

https://hexdocs.pm/jason/Jason.Encoder.html#module-example

apps/shared/test/validation/gbfs_validator_test.exs

apps/transport/lib/transport/gbfs_metadata.ex

thbar · 2021-11-15T10:56:23Z

apps/transport/lib/transport/gbfs_metadata.ex

+        ttl: ttl(json)
+      }
+    else
+      e ->


Dans le futur, peut-être qu'on gagnera à mettre de la télémétrie ici (et/ou à retourner une erreur à l'appelant)

apps/transport/lib/transport/gbfs_metadata.ex

thbar · 2021-11-15T10:59:02Z

apps/transport/lib/transport/gbfs_metadata.ex

+  iex> Transport.GBFSMetadata.has_cors?(%HTTPoison.Response{headers: [{"Access-Control-Allow-Origin", "*"}]})
+  true
+  """
+  def has_cors?(%HTTPoison.Response{} = response) do


Toute cette partie pourra peut-être être bougée dans le futur vers un module non spécifique au GBFS, si on retrouve l'usage ailleurs.

apps/shared/test/validation/gbfs_validator_test.exs

The previous use was just an atom expression, evaluated but not leading to ExUnit callback call.

… gbfs-metadata

thbar · 2021-11-15T16:36:41Z

apps/transport/lib/transport/gbfs_metadata.ex

+    end
+  end
+
+  defp types(%{"data" => _data} = payload) do


Dans un style un peu plus fonctionnel, je suggère :

Suggested change

defp types(%{"data" => _data} = payload) do

defp types(%{"data" => _data} = payload) do

result =

[]

|> maybe_add_type("free_floating", has_feed?(payload, "free_bike_status"))

|> maybe_add_type("stations", has_feed?(payload, "station_information"))

if Enum.empty?(result) do

Logger.error("Cannot detect GBFS types for feed #{inspect(payload)}")

nil

else

result

end

end

def maybe_add_type(result, tag, condition) do

if condition, do: result ++ [tag], else: result

end

(à vérifier!)

thbar

Beau boulot !

J'ai fait des modifications liées à des éléments simples (simplification d'un test, correction d'un bug sur l'usage de Mox, etc), et fait une proposition de refactoring.

Par ailleurs plusieurs remarques de "haut niveau" (qui n'appellent pas forcément à modification, je te laisse juge, ou à dispo pour en discuter comme tu veux) :

Sérialisation / désérialisation

Il faudra faire attention au fait qu'une séquence sérialisation + désérialisation comme suit (dont je me suis servi pour regarder un peu le résultat dans IEx sans aller dans la base de données) ne redonne pas les mêmes éléments:

Transport.GBFSMetadata.gbfs_feeds_query()
|> DB.Repo.all()
|> Enum.take(1)
|> Enum.each(fn(resource) ->
  Transport.GBFSMetadata.compute_feed_metadata(resource)
  |> IO.inspect(IEx.inspect_opts())
  |> Jason.encode!()
  |> Jason.decode!()
  |> IO.inspect(IEx.inspect_opts())
end)

%{
  feeds: ["system_information", "station_information", "station_status"],
  has_cors: false,
  is_cors_allowed: false,
  languages: ["en"],
  system_details: %{name: "Vélocéo Vannes", timezone: "Europe/Paris"},
  ttl: 60,
  types: ["stations"],
  validation: %Shared.Validation.GBFSValidator.Summary{
    errors_count: 0,
    has_errors: false,
    version_detected: "1.0",
    version_validated: "1.0"
  },
  versions: ["1.0"]
}
%{
  "feeds" => ["system_information", "station_information", "station_status"],
  "has_cors" => false,
  "is_cors_allowed" => false,
  "languages" => ["en"],
  "system_details" => %{
    "name" => "Vélocéo Vannes",
    "timezone" => "Europe/Paris"
  },
  "ttl" => 60,
  "types" => ["stations"],
  "validation" => %{
    "errors_count" => 0,
    "has_errors" => false,
    "version_detected" => "1.0",
    "version_validated" => "1.0"
  },
  "versions" => ["1.0"]
}

En particulier le typage sur %Shared.Validation.GBFSValidator.Summary disparaît.

C'est sans impact a priori là, mais à garder en tête si on utilise @derive Jason.Encoder davantage.

Impact des méthodes privées `defp` sur la complexité du test

Je pense qu'on pourrait avoir un test un peu "intégration" avec les stubs, et pour le reste tester plus unitairement chaque méthode (en la passant de defp à def), et que ça pourrait donner (à vérifier, c'est juste une intuition) des tests plus simples à appréhender.

thbar · 2021-11-15T17:00:53Z

(J'ai aussi adapté le code en provenance de master, il y avait un peu de travail après le merge de #1875).

AntoineAugusti · 2021-11-16T07:47:27Z

@thbar Merci de ta relecture attentive et des tests localement ! Et des améliorations effectuées :)

J'avais remarqué la différence de désérialisation/sérialisation où l'on perd la structure quand on récupère un objet depuis la BD. C'est facheux, mais je pense que c'est partout comme ça dans le projet étant donné qu'on indique juste que metadata est du JSON, mais pas moyen de faire un chargement plus intelligent. Les tests mettent en évidence ce comportement, il sera toujours temps d'adapter ça, sûrement en faisant du cast quand on hydrate l'objet.

C'est vrai que je trouve que j'ai plus souvent à des defp que le reste du code, je vais faire attention à l'avenir, et ne pas avoir peur de tester ces différentes fonctions, avec des doctests ou des tests.

AntoineAugusti force-pushed the gbfs-metadata branch from 36b5b3d to a8dfb43 Compare November 8, 2021 07:47

AntoineAugusti added 2 commits November 8, 2021 09:54

WIP

c01b452

Add simple test

1553a0e

AntoineAugusti force-pushed the gbfs-metadata branch from a8dfb43 to 1553a0e Compare November 8, 2021 08:54

AntoineAugusti added 2 commits November 8, 2021 11:32

More tests

b814395

Not async

32aeb07

AntoineAugusti force-pushed the gbfs-metadata branch from 7132187 to 32aeb07 Compare November 8, 2021 11:01

AntoineAugusti added 2 commits November 8, 2021 14:25

Store metadata for resources

e623cf4

Clean code

a9bbc9a

AntoineAugusti commented Nov 8, 2021

View reviewed changes

AntoineAugusti marked this pull request as ready for review November 8, 2021 13:38

AntoineAugusti requested review from thbar and fchabouis November 8, 2021 13:38

AntoineAugusti changed the title ~~Gbfs metadata~~ Calcul et sauvegarde de métadonnées pour les ressources GBFS Nov 8, 2021

AntoineAugusti added 2 commits November 9, 2021 08:35

Merge branch 'master' into gbfs-metadata

6d0e421

Move to dedicated file

dc853fd

AntoineAugusti commented Nov 9, 2021

View reviewed changes

AntoineAugusti added 3 commits November 9, 2021 08:54

Merge branch 'master' into gbfs-metadata

2464339

Add details about CORS

e2dc605

Merge branch 'master' into gbfs-metadata

8749f62

AntoineAugusti commented Nov 10, 2021

View reviewed changes

AntoineAugusti added 2 commits November 10, 2021 11:29

Add tests for set_gbfs_feeds_metadata

8841a99

remove aom from dataset

d252108

AntoineAugusti mentioned this pull request Nov 10, 2021

Visualisation de métadonnées GBFS #1915

Merged

thbar reviewed Nov 15, 2021

View reviewed changes

thbar added 5 commits November 15, 2021 12:22

Make sure verify_on_exit! is actually called

2503f49

The previous use was just an atom expression, evaluated but not leading to ExUnit callback call.

Bypass the wrapper to simplify the tests & make them async

b3cc5cd

Rename HTTPClient to HTTPValidatorClient (+ remove as: for clarity)

d362712

Merge branch 'master' into gbfs-metadata

670b351

Fix credo error

3098490

thbar added 6 commits November 15, 2021 12:33

Merge branch 'gbfs-metadata' of github.com:etalab/transport-site into…

0c20ff0

… gbfs-metadata

Extract query to separate method to make it easier to invoke it from IEx

30cc1da

Merge branch 'master' into gbfs-metadata

2bc3228

Merge branch 'master' into gbfs-metadata

181cc8f

Update test port to reflect currently in use value & fix tests

0dcbe72

Add a bit of documentation

80b68d7

thbar reviewed Nov 15, 2021

View reviewed changes

thbar approved these changes Nov 15, 2021

View reviewed changes

AntoineAugusti merged commit 069d93b into master Nov 16, 2021

AntoineAugusti deleted the gbfs-metadata branch November 16, 2021 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calcul et sauvegarde de métadonnées pour les ressources GBFS #1905

Calcul et sauvegarde de métadonnées pour les ressources GBFS #1905

AntoineAugusti commented Nov 5, 2021 •

edited

Loading

AntoineAugusti Nov 8, 2021

AntoineAugusti Nov 9, 2021

AntoineAugusti Nov 9, 2021

thbar Nov 9, 2021

AntoineAugusti Nov 10, 2021

thbar left a comment

thbar Nov 15, 2021

thbar Nov 15, 2021

thbar Nov 15, 2021

thbar Nov 15, 2021

thbar Nov 15, 2021

thbar left a comment

thbar commented Nov 15, 2021

AntoineAugusti commented Nov 16, 2021

-  defp types(%{"data" => _data} = payload) do
+  defp types(%{"data" => _data} = payload) do
+    result =
+      []
+      |> maybe_add_type("free_floating", has_feed?(payload, "free_bike_status"))
+      |> maybe_add_type("stations", has_feed?(payload, "station_information"))
+    if Enum.empty?(result) do
+      Logger.error("Cannot detect GBFS types for feed #{inspect(payload)}")
+      nil
+    else
+      result
+    end
+  end
+  def maybe_add_type(result, tag, condition) do
+    if condition, do: result ++ [tag], else: result
+  end

Calcul et sauvegarde de métadonnées pour les ressources GBFS #1905

Calcul et sauvegarde de métadonnées pour les ressources GBFS #1905

Conversation

AntoineAugusti commented Nov 5, 2021 • 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

thbar left a 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

thbar left a comment

Choose a reason for hiding this comment

Sérialisation / désérialisation

Impact des méthodes privées defp sur la complexité du test

thbar commented Nov 15, 2021

AntoineAugusti commented Nov 16, 2021

AntoineAugusti commented Nov 5, 2021 •

edited

Loading

Impact des méthodes privées `defp` sur la complexité du test