Unclear error or case for better function signature?

[Devon-Olivier]

Environment

• Elixir version (elixir -v):

Erlang/OTP 25 [erts-13.0.2] [source] [64-bit] [smp:12:12] [ds:12:12:10] [async-threads:1] [jit:ns]

Elixir 1.13.2 (compiled with Erlang/OTP 24)

• Phoenix version (mix deps): 1.6.11
• Phoenix LiveView version (mix deps): 0.17.11
• Operating system: Manjaro
• Browsers you attempted to reproduce this bug on (the more the merrier): Firefox Developer Edition, Brave
• Does the problem persist after removing "assets/node_modules" and trying again? Yes/no: Yes

Actual behavior

If Phoenix.LiveView.cancel_upload/3 is called with an invalid ref argument, for example,

cancel_upload(socket, :avatar, "bad_ref")

then a MatchError is raised:

[error] GenServer #PID<0.881.0> terminating
** (MatchError) no match of right hand side value: nil
    (phoenix_live_view 0.17.11) lib/phoenix_live_view/upload.ex:73: Phoenix.LiveView.Upload.cancel_upload/3

From this error type and message, it may not be clear to the developer what the root cause of the error is.

Expected behavior

At least a developer friendly error which reveals the fact that the reference does not refer to an entry in the upload. But we may be able to do better.

Possible solution

I was excited to contribute some code to help improve clarity. I did just that here

The error looks like

[error] GenServer #PID<0.692.0> terminating
** (ArgumentError) no entry in upload ':avatar' with ref 'phx-FwmOHNwo1SSkSwVD'
    (phoenix_live_view 0.18.0-dev) lib/phoenix_live_view/upload.ex:81: Phoenix.LiveView.Upload.cancel_upload/3

Now the developer knows that the error is with some argument related to the ref of some entry.

Then I wrote a test that I am not comfortable with. So I did not make a pull request.

Questions and further improvements

1. Where should such a test be placed? LiveView.cancel_upload/3 delegates to LiveView.Upload but I cannot find where the tests for that module are. I thought that I could add a file test/phoenix_live_view/upload/upload_test.exs. But I realized that the build_client_entry/2 and build_socket/0 helpers are needed from test/phoenix_live_view/upload/config_test.exs so that may require either duplication or extracting into a common helpers module. I was not confident to make that decision so I am here for guidance.

2. From a developer/user perspective, should this error ever occur? (not sure if I asked that properly) It seems as though this error can be avoided if the signature of cancel_upload/3 were

cancel_upload(socket, entry)

or, IMHO, what is clearer

cancel_entry(socket, entry)

It appears that from the UploadEntry struct we have all the information needed to cancel it. Also, the ref of an entry seems to be an implementation detail that can be hidden from developers. Hiding reduces possible error and also reduces the likelihood of code breakage if the implementation has to change in the future.

3. Should such a test make a call to LiveView.Upload.put_entries/4 with nil for the cid parameter or should UploadConfig.put_entries` be called instead then manually assign the upload to the socket?

[mcrumm]

@Devon-Olivier Thanks for the detailed report! We can definitely improve this error message, and a PR would be greatly appreciated :)

Where should such a test be placed?

I think I would place this test in the upload/channel_test.exs module. The channel tests are already configured to drive an UploadLive LiveView under test, invoke upload commands in the LV process scope, and assert errors when the LV process exits.

For instance something like this should work:

@tag allow: [max_entries: 1, accept: :any]
test "cancel_upload with invalid ref", %{lv: lv} do
  avatar =
    file_input(lv, "form", :avatar, [
      %{name: "foo.jpeg", content: String.duplicate("0", 100)}
    ])

  assert UploadLive.exits_with(lv, avatar, ArgumentError, fn ->
           UploadLive.run(lv, fn socket ->
             {:reply, :ok, Phoenix.LiveView.cancel_upload(socket, :avatar, "bad_ref")}
           end)
         end) =~ "no entry in upload :avatar"
end

From a developer/user perspective, should this error ever occur?

I think it would occur rarely and it's more likely if there is more than one upload on a given LiveView.

the UploadEntry struct we have all the information needed to cancel it

This is true, but generally you only have access to an UploadEntry struct in upload callbacks like progress and consume_uploaded_entry, but the ref can be sent back over-the-wire by the client.

Consider the following HEEx that renders a cancel button for each pending entry:

<%= for entry <- @uploads.exhibit.entries do %>
  <div class="upload-entry">
    <!-- other upload stuff -->
    <button phx-click="cancel-upload" phx-value-ref={entry.ref}>cancel</button>
  </div>
<% end %>

...and it's corresponding handle_event callback:

@impl true
def handle_event("cancel-upload", %{"ref" => ref}, socket) do
  {:noreply, cancel_upload(socket, :exhibit, ref)}
end

In any case, a better error is still desirable. :) Would you still like to send a PR?

[Devon-Olivier]

Yes. I would definitely work on that PR.