Add missing cond for frames #787
Conversation
lib/sentry/client.ex
Outdated
| |> update_if_present(:mechanism, &Map.from_struct/1) | ||
| |> update_if_present(:stacktrace, fn %Interfaces.Stacktrace{frames: frames} -> | ||
| %{frames: Enum.map(frames, &Map.from_struct/1)} | ||
| %{frames: frames && Enum.map(frames, &Map.from_struct/1)} |
There was a problem hiding this comment.
Should frames be present at all if it's null, according to Sentry docs?
There was a problem hiding this comment.
@savhappy need to figure this ☝️ out before merging 🙃
There was a problem hiding this comment.
Yep yep did some digging in the docs and it says the frames attribute is required if stacktrace is present so we should remove the property entirely if frames: nil.
|
We also need a test for this 🙃 |
lib/sentry/client.ex
Outdated
| defp update_if_present(map, :stacktrace, fun) do | ||
| case Map.pop(map, :stacktrace) do | ||
| {nil, _} -> | ||
| map | ||
|
|
||
| {value, map} -> | ||
| if is_nil(value.frames) || value.frames == [] do | ||
| Map.delete(map, :stacktrace) | ||
| else | ||
| Map.put(map, :stacktrace, fun.(value)) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Let's not special-case update_if_present/3 to handle :stacktrace. In general, that leads to somewhat-confusing code because then function behaves differently based on the key, so it's effectively two functions.
You can do this instead:
case Map.from_struct(thread) do
%{stacktrace: %{frames: [_ | _]} = stacktrace} ->
%{stacktrace | frames: Enum.map(frames, &Map.from_struct/1)}
thread_as_map ->
Map.delete(thread_as_map, :stacktrace)
endThis is special-cased to handle :stacktrace and the :frames logic specifically, reads pretty clear IMO.
lib/sentry/client.ex
Outdated
| defp render_exception(%Interfaces.Exception{} = exception) do | ||
| exception | ||
| |> Map.from_struct() | ||
| case Map.from_struct(exception) do |
There was a problem hiding this comment.
Ok I missed that this was done in two places. Let's get this case code and extract it into a render_stacktrace function then!
savhappy
force-pushed
the
sav/frames-missing-condition
branch
from
a55605e to
ffb8f3e
Compare
|
Ah I see that! Thank you for the correction and the updated tests! |
-render_exception was missing a conditional and nil was being passed to Enum.map causing a
Protocol.Undefinederror-closes #786