1380 function clause error changeset json error

[benedictus-dev]

Notes for the reviewer

Update the CompleteRun handler in Lightning.Attempts.Handlers to handle
cases where a run is not found by returning a detailed error changeset.

Related issue

Fixes #1380

Review checklist

• I have performed a self-review of my code
• I have verified that all appropriate authorization policies have been implemented and tested
• If needed, I have updated the changelog
• Product has QA'd this feature

[codecov]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (448a320) 85.82% compared to head (e4de1a3) 85.81%.
Report is 9 commits behind head on main.

Files	Patch %	Lines
lib/lightning/attempts/handlers.ex	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1393      +/-   ##
==========================================
- Coverage   85.82%   85.81%   -0.02%     
==========================================
  Files         216      216              
  Lines        6393     6393              
==========================================
- Hits         5487     5486       -1     
- Misses        906      907       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[stuartc]

I'm not sure this is the way to go about it. In the case of an error we don't return a Run changeset, we return a CompleteRun changeset, it's purpose is to stand in between the worker sending a bunch of data that represents data that is for a run and a dataclip. It can be thought of as a 'command'.

My suggestion is to add a run: Run field to the schema, we don't cast it - but set the field and then validate it.

  # ... ^ new/1
  |> then(&assign_run/1)
end

defp assign_run(changeset) do
  with %{valid?: true} <- changeset,
        %Run{} = run <-
          get_field(changeset, :run_id) |> get_run() || :not_found do
    put_change(changeset, :run, run)
  else
    :not_found ->
      add_error(changeset, :run_id, "not found")

    changeset ->
      changeset
  end
end

And then further down the update/1 function can assume the changeset is valid, and get the run off the changeset instead of finding it itself.

[benedictus-dev]

The proposal is to be very granular with the fix changing only the flow of the complete run.
The change consists of just adding the changeset to the returned value.

[jyeshe]

Just to keep the previous order of validations and I think this change as 9ef9359 is safer, going directly to the point by adding the validation only to the flow reported on the error (on new/insertion the validation won't be needed)

[jyeshe]

Sorry Ben, just noticed after the pairing that the get_run makes more sense before the dataclip insertion as it was previously.

[stuartc]

Please don't return a Run changeset. As mentioned in my first review:

In the case of an error we don't return a Run changeset, we return a CompleteRun changeset

The Run model does not have a :run_id key. You have access to the original CompleteRun changeset. Please change the nil case to:

{:error, complete_run |> add_error(:run_id, "not found")}