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

Validateur NeTEx : quelques metadata #4160

Merged
merged 4 commits into from
Sep 5, 2024
+106 −78
Merged

Validateur NeTEx : quelques metadata #4160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
24adea1
Validateur NeTEx: quelques metadata
ptitfred Sep 3, 2024
b74f2ea
Validateur NeTEx: temps passés dans les metadata
ptitfred Sep 3, 2024
d07b07b
More idiomatic
ptitfred Sep 5, 2024
645d7b7
Validateur NeTEx : fix timeout et elapsed_seconds
ptitfred Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 26 additions & 19 deletions apps/transport/lib/validators/enroute_chouette_valid_client.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,11 @@ defmodule Transport.EnRouteChouetteValidClient.Wrapper do

@callback create_a_validation(Path.t()) :: binary()
@callback get_a_validation(binary()) ::
{:pending, integer()}
| {:successful, binary()}
| :warning
| :failed
:pending
| {:successful, binary(), integer()}
| {:warning, integer()}
| {:failed, integer()}
Copy link
Contributor

Choose a reason for hiding this comment

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

Je me suis demandé initialement si ce tuple où on ajoute un entier en bout de chaîne ne commençait pas à devenir trop long, ne risquait pas de poser de problème quand on fait évoluer cette structure etc.

Mais au final ma compréhension du code actuel est que c'est une structure éphémère non persistée en base et qui pourra donc être revue sans souci (ex: besoin de passer à une map avec des clés plus explicites).

Je consigne juste cette note pour ma compréhension !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

En effet, c'est bien ça.

Disons que c'est une manifestation de mon habitude de manipuler des types algébriques en entrée ou sortie de toute fonction et d'apprécier d'avoir une documentation implicite via les types des embranchements possibles.

Le fait que l'entier que je rajoute dans cette PR ne soit pas identifié comme une durée en seconde est une limite de cette syntaxe, et on pourrait arguer que le spec ainsi obtenu devient dangereusement verbeux.

(I miss my algebraic types.)

| :unexpected_validation_status
| :unexpected_datetime_format
@callback get_messages(binary()) :: {binary(), map()}

def impl, do: Application.get_env(:transport, :enroute_validator_client)
Expand Down Expand Up @@ -48,28 +47,40 @@ defmodule Transport.EnRouteChouetteValidClient do

case response |> Map.fetch!("user_status") do
"pending" ->
case {get_datetime(response, "created_at"), get_datetime(response, "updated_at")} do
{{:ok, created_at, _}, {:ok, updated_at, _}} ->
{:pending, DateTime.diff(updated_at, created_at)}

_ ->
:unexpected_datetime_format
end
:pending

"successful" ->
{:successful, url}
{:successful, url, get_elapsed!(response)}

"warning" ->
:warning
{:warning, get_elapsed!(response)}

"failed" ->
:failed
{:failed, get_elapsed!(response)}

_ ->
:unexpected_validation_status
end
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Pour ci-dessous : j'ai tendance à préférer des appels en bang "!" qui lèvent une erreur, pour réduire le code, et du coup réduire les potentialités de bug dans le code qui analyse les retours.

Par exemple ici j'aurais opté pour quelque chose comme:

defp get_elapsed!(response) do
  created_at = get_datetime!(response, "created_at")
  updated_at = get_datetime!(response, "updated_at")
  DateTime.diff(updated_at, created_at)
end

Et (voir https://hexdocs.pm/elixir/1.17.2/Date.html#from_iso8601!/2)

defp get_datetime!(map, key) do
  map 
  |> Map.fetch!(key)
  |> DateTime.from_iso8601!()
end

C'est d'ailleurs en phase avec le fait que Map.fetch! va de toute façon lever une exception en cas de clé manquante, et adopte donc plutôt un style "fail fast" aussi.

(Et du coup, gérer l'erreur dans l'appelant de get_elapsed!)

Après ça reste aussi une opinion, mais perso je trouve que ça fait au final beaucoup moins de code sur une codebase quand c'est applicable (il faut que l'appelant puisse intercepter l'erreur, par contre !)

Copy link
Contributor Author

@ptitfred ptitfred Sep 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

il faut que l'appelant puisse intercepter l'erreur, par contre !

Oui, et tu peux avoir des cas où la clef est manquante/invalide sans que ça soit grave, pas forcément la responsabilité du code appelant.

En effet si j'étais cohérent j'aurais pas utilisé Map.fetch!. En pratique je peux faire confiance au fait que le datetime est bien formatté. Ce cas d'erreur spécifique m'ennuie.

Je dois revoir cette partie de toute façon, j'ai identifié un problème en retestant avec la validation OnDemand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Et (voir https://hexdocs.pm/elixir/1.17.2/Date.html#from_iso8601!/2)

Ce helper est fourni pour Date mais pas pour DateTime ; je vais l'émuler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'imagine que c'est lié au fait que DateTime.from_iso8601 renvoie et le datetime et l'offset, et donc qu'il faudrait l'interpréter également (ce que je ne fais pas, je fais l'hypothèse que l'offset est constant).

defp get_elapsed!(response) do
created_at = get_datetime!(response, "started_at")
updated_at = get_datetime!(response, "ended_at")
DateTime.diff(updated_at, created_at)
end

defp get_datetime!(map, key) do
raw_date = Map.fetch!(map, key)

case DateTime.from_iso8601(raw_date) do
{:ok, value, _offset} ->
value

{:error, reason} ->
raise ArgumentError, "cannot parse #{inspect(raw_date)} as datetime, reasaon: #{inspect(reason)}"
end
end

@impl Transport.EnRouteChouetteValidClient.Wrapper
def get_messages(validation_id) do
url = Path.join([validation_url(validation_id), "messages"])
Expand All @@ -78,10 +89,6 @@ defmodule Transport.EnRouteChouetteValidClient do
{url, body |> Jason.decode!()}
end

defp get_datetime(map, key) do
map |> Map.fetch!(key) |> DateTime.from_iso8601()
end

defp make_file_part(field_name, filepath) do
{:file, filepath, {"form-data", [{:name, field_name}, {:filename, Path.basename(filepath)}]}, []}
end
Expand Down
62 changes: 36 additions & 26 deletions apps/transport/lib/validators/netex_validator.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,7 @@ defmodule Transport.Validators.NeTEx do

@no_error "NoError"

# 15 minutes for the validation to complete should be enough.
@timeout 15 * 60
@max_retries 100

@behaviour Transport.Validators.Validator

Expand All @@ -27,19 +26,26 @@ defmodule Transport.Validators.NeTEx do

def validate_resource_history(resource_history, filepath) do
case validate_with_enroute(filepath) do
{:ok, result_url} ->
insert_validation_results(resource_history.id, result_url)
{:ok, %{url: result_url, elapsed_seconds: elapsed_seconds, retries: retries}} ->
insert_validation_results(resource_history.id, result_url, %{elapsed_seconds: elapsed_seconds, retries: retries})

:ok

{:error, {result_url, errors}} ->
insert_validation_results(resource_history.id, result_url, errors)
{:error, %{details: {result_url, errors}, elapsed_seconds: elapsed_seconds, retries: retries}} ->
insert_validation_results(
resource_history.id,
result_url,
%{elapsed_seconds: elapsed_seconds, retries: retries},
errors
)

:ok

{:error, :unexpected_validation_status} ->
Logger.error("Invalid API call to enRoute Chouette Valid")
{:error, "enRoute Chouette Valid: Unexpected validation status"}

{:error, :timeout} ->
{:error, %{message: :timeout, retries: _retries}} ->
Logger.error("Timeout while fetching result on enRoute Chouette Valid")
{:error, "enRoute Chouette Valid: Timeout while fetching results"}
end
Expand All @@ -58,23 +64,29 @@ defmodule Transport.Validators.NeTEx do
def validate(url, opts \\ []) do
with_url(url, fn filepath ->
case validate_with_enroute(filepath, opts) do
{:ok, result_url} ->
{:ok, %{url: result_url, elapsed_seconds: elapsed_seconds, retries: retries}} ->
# result_url in metadata?
Logger.info("Result URL: #{result_url}")
{:ok, %{"validations" => index_messages([]), "metadata" => %{}}}

{:error, {result_url, errors}} ->
{:ok,
%{"validations" => index_messages([]), "metadata" => %{elapsed_seconds: elapsed_seconds, retries: retries}}}

{:error, %{details: {result_url, errors}, elapsed_seconds: elapsed_seconds, retries: retries}} ->
Logger.info("Result URL: #{result_url}")
# result_url in metadata?
{:ok, %{"validations" => index_messages(errors), "metadata" => %{}}}
{:ok,
%{
"validations" => index_messages(errors),
"metadata" => %{elapsed_seconds: elapsed_seconds, retries: retries}
}}

{:error, :unexpected_validation_status} ->
Logger.error("Invalid API call to enRoute Chouette Valid")
{:error, "enRoute Chouette Valid: Unexpected validation status"}
{:error, %{message: "enRoute Chouette Valid: Unexpected validation status"}}

{:error, :timeout} ->
{:error, %{message: :timeout, retries: retries}} ->
Logger.error("Timeout while fetching result on enRoute Chouette Valid")
{:error, "enRoute Chouette Valid: Timeout while fetching results"}
{:error, %{message: "enRoute Chouette Valid: Timeout while fetching results", retries: retries}}
end
end)
end
Expand Down Expand Up @@ -110,7 +122,7 @@ defmodule Transport.Validators.NeTEx do
System.tmp_dir!() |> Path.join("enroute_validation_netex_#{Ecto.UUID.generate()}")
end

def insert_validation_results(resource_history_id, result_url, errors \\ []) do
def insert_validation_results(resource_history_id, result_url, metadata, errors \\ []) do
result = index_messages(errors)

%DB.MultiValidation{
Expand All @@ -120,7 +132,8 @@ defmodule Transport.Validators.NeTEx do
resource_history_id: resource_history_id,
validator_version: validator_version(),
command: result_url,
max_error: get_max_severity_error(result)
max_error: get_max_severity_error(result),
metadata: %DB.ResourceMetadata{metadata: metadata}
}
|> DB.Repo.insert!()
end
Expand Down Expand Up @@ -200,27 +213,24 @@ defmodule Transport.Validators.NeTEx do

defp fetch_validation_results(validation_id, retries, opts) do
case client().get_a_validation(validation_id) do
{:pending, elapsed_seconds} when elapsed_seconds > @timeout ->
{:error, :timeout}
:pending when retries >= @max_retries ->
{:error, %{message: :timeout, retries: retries}}

{:pending, _elapsed_seconds} ->
:pending ->
if Keyword.get(opts, :graceful_retry, true) do
retries |> poll_interval() |> :timer.sleep()
end

fetch_validation_results(validation_id, retries + 1, opts)

{:successful, url} ->
{:ok, url}
{:successful, url, elapsed_seconds} ->
{:ok, %{url: url, elapsed_seconds: elapsed_seconds, retries: retries}}

value when value in [:warning, :failed] ->
{:error, client().get_messages(validation_id)}
{value, elapsed_seconds} when value in [:warning, :failed] ->
{:error, %{details: client().get_messages(validation_id), elapsed_seconds: elapsed_seconds, retries: retries}}

:unexpected_validation_status ->
{:error, :unexpected_validation_status}

:unexpected_datetime_format ->
{:error, :unexpected_datetime_format}
end
end

Expand Down
22 changes: 11 additions & 11 deletions apps/transport/test/transport/validators/enroute_chouette_valid_client_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ defmodule Transport.EnRouteChouetteValidClientTest do
"user_status": "pending",
"started_at": "2024-07-05T14:41:20.680Z",
"created_at": "2024-07-05T14:41:19.933Z",
"updated_at": "2024-07-05T14:45:20.933Z"
"updated_at": "2024-07-05T14:41:20.933Z"
}
"""

Expand All @@ -63,7 +63,7 @@ defmodule Transport.EnRouteChouetteValidClientTest do
%HTTPoison.Response{status_code: 200, body: response_body}
end)

assert {:pending, 4 * 60 + 1} == EnRouteChouetteValidClient.get_a_validation(validation_id)
assert :pending == EnRouteChouetteValidClient.get_a_validation(validation_id)
end

test "successful" do
Expand All @@ -76,9 +76,9 @@ defmodule Transport.EnRouteChouetteValidClientTest do
"rule_set": "french",
"user_status": "successful",
"started_at": "2024-07-05T14:41:20.680Z",
"ended_at": "2024-07-05T14:41:20.685Z",
"ended_at": "2024-07-05T14:41:25.685Z",
"created_at": "2024-07-05T14:41:19.933Z",
"updated_at": "2024-07-05T14:41:19.933Z"
"updated_at": "2024-07-05T14:41:20.933Z"
}
"""

Expand All @@ -89,7 +89,7 @@ defmodule Transport.EnRouteChouetteValidClientTest do
%HTTPoison.Response{status_code: 200, body: response_body}
end)

assert {:successful, url} == EnRouteChouetteValidClient.get_a_validation(validation_id)
assert {:successful, url, 5} == EnRouteChouetteValidClient.get_a_validation(validation_id)
end

test "warning" do
Expand All @@ -102,9 +102,9 @@ defmodule Transport.EnRouteChouetteValidClientTest do
"rule_set": "french",
"user_status": "warning",
"started_at": "2024-07-05T14:41:20.680Z",
"ended_at": "2024-07-05T14:41:20.685Z",
"ended_at": "2024-07-05T14:41:24.685Z",
"created_at": "2024-07-05T14:41:19.933Z",
"updated_at": "2024-07-05T14:41:19.933Z"
"updated_at": "2024-07-05T14:41:20.933Z"
}
"""

Expand All @@ -115,7 +115,7 @@ defmodule Transport.EnRouteChouetteValidClientTest do
%HTTPoison.Response{status_code: 200, body: response_body}
end)

assert :warning == EnRouteChouetteValidClient.get_a_validation(validation_id)
assert {:warning, 4} == EnRouteChouetteValidClient.get_a_validation(validation_id)
end

test "failed" do
Expand All @@ -128,9 +128,9 @@ defmodule Transport.EnRouteChouetteValidClientTest do
"rule_set": "french",
"user_status": "failed",
"started_at": "2024-07-05T14:41:20.680Z",
"ended_at": "2024-07-05T14:41:20.685Z",
"ended_at": "2024-07-05T14:41:28.685Z",
"created_at": "2024-07-05T14:41:19.933Z",
"updated_at": "2024-07-05T14:41:19.933Z"
"updated_at": "2024-07-05T14:41:20.933Z"
}
"""

Expand All @@ -141,7 +141,7 @@ defmodule Transport.EnRouteChouetteValidClientTest do
%HTTPoison.Response{status_code: 200, body: response_body}
end)

assert :failed == EnRouteChouetteValidClient.get_a_validation(validation_id)
assert {:failed, 8} == EnRouteChouetteValidClient.get_a_validation(validation_id)
end
end

Expand Down
Loading