fix: don’t expose bulk indexes in error source pointers#430
Conversation
| end | ||
|
|
||
| defp source_pointer(resource, field, path, :action) do | ||
| clean_path = path |> List.wrap() |> Enum.reject(&is_integer/1) |
There was a problem hiding this comment.
We may have errors pointing at nested paths for list attributes or relationships etc. Instead what we should do is modify the callsite that uses Ash.bulk_update to handle the error response and remove the first item in it if its 0 exactly IMHO.
There was a problem hiding this comment.
Indeed, it feels cleaner to handle it just after the response from Ash.bulk_update/3. I edited my commit with a new proposal.
The new test still passes, and all the tests from my project also pass with the :ash_json_api deps pointing to the local fork.
Also, I think I should modify the :update for :destroy as the operation argument passed to add_error/4 line 638 of AshJsonApi.Controllers.Helpers module (that I already modified). Do you agree?
There was a problem hiding this comment.
I just updated my commit to include this little fix. Do you mind the comment before the private function strip_bulk_index_from_errors/1 or is it good for you?
67d5393 to
107a562
Compare
107a562 to
d17267c
Compare
|
🚀 Thank you for your contribution! 🚀 |
Contributor checklist
Leave anything that you believe does not apply unchecked.
Context
After updating all my deps, there were errors in my tests.
Potentially involved Ash updated deps are:
The error was that returned
JsonApiErrordidn’t fitted my helpers anymore to extract the error source (pointer) and assert on received errors, by fields.The source pointers looked like that in some API responses:
rather than expected:
Then I asked AI (Claude Code) to investigate whether it was due to a change in AshJsonApi (I've read the changeset and haven’t found it) or maybe AshAuthentication (the error came from validations provided by this package).
Finally, I think that without AI I wouldn’t have found the error cause easily. So here are AI explanations for this PR, after I brainstormed with it:
AI explanations
NOTE: a bit long but precise on the problem (I've read and validated everything).
Problem
Single-record update operations via JSON:API return error source pointers with an incorrect
/0/bulk index prefix.Expected:
{ "errors": [{ "source": { "pointer": "/data/attributes/password" } }] }Actual:
{ "errors": [{ "source": { "pointer": "/data/attributes/0/password" } }] }Root Cause
Ash.bulk_updateinternally for all update operations (since v1.2.0)Ash.Actions.Helpers.Bulk.set_index_path/2-> NOTE: I just updated from Ash v3.19.1 to v3.23.1, so it coincidesAshJsonApi.Error.source_pointer/4includes the full path (including the integer index) when building source pointersThe index is set on errors during bulk processing to identify which record in a batch caused the error. However, for single-record operations exposed via JSON:API, this index (
0) should not appear in the response.When It Manifests
Affected (includes
/0/):validatein actions)Not affected (no
/0/):allow_nil?: false→Required)Proposed Fix
Filter out integer indices from the path in
source_pointer/4:This strips integer indices (like
0) from the path while preserving any string-based nested path segments.Considerations
/0/prefix would need to update, but such reliance would itself be a bug.My added notes
:ash_json_apipath to my local fork, and it solves the errors I encountered after deps update:integerin theresultand:stringin the expected block, but I haven’t dig on which type is the right one)