fix: convert Ecto.Query.CastError on every query path - #864
Merged
Merged
Conversation
Ash does not cast filter values against the attribute type, so a value that cannot be cast reaches Ecto, which raises `Ecto.Query.CastError` while planning the query. `run_query/2` rescues that and converts it to `InvalidFilterValue`, but the conversion had two gaps: - `run_aggregate_query/3`, `run_aggregate_query_with_lateral_join/5` and `run_query_with_lateral_join/4` had no rescue, so `Ash.count`, `Ash.exists`, `Ash.aggregate` and a limited relationship load across several parents raised instead of returning an error tuple, which the `Ash.DataLayer` callbacks declare. - `handle_raised_error/4` did not unwrap `Ecto.SubQueryError`, so a cast error inside a paginated, limited or lateral-join query became `Ash.Error.Unknown` even where a rescue existed. Closes ash-project#855
Contributor
|
🚀 Thank you for your contribution! 🚀 |
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #855.
Ash does not cast filter values against the attribute type (
Eqdeclarestypes: [:any, :same]), so a value that cannot be cast reaches Ecto, which raisesEcto.Query.CastErrorwhile planning the query.run_query/2rescues that and converts it toAsh.Error.Query.InvalidFilterValue, but the conversion had two gaps, all with a UUID primary key andid == "not-a-uuid":run_aggregate_query/3,run_aggregate_query_with_lateral_join/5andrun_query_with_lateral_join/4have no rescue, soAsh.count/2,Ash.exists/2,Ash.aggregate/3and a limited relationship load across several parents raise the exception through the caller. TheAsh.DataLayerbehaviour declares{:ok, _} | {:error, _}for all four callbacks.handle_raised_error/4does not unwrapEcto.SubQueryError, which Ecto raises around a cast error inside a subquery and which keeps the original inexception. So a count under a limit, a paginated read with a relationship filter, andupdate_query/4(AshJsonApi'sPATCH /posts/not-a-uuid) returnAsh.Error.Unknowneven where a rescue exists.Changes, all in
AshPostgres.DataLayer:handle_raised_error/4clause for%Ecto.SubQueryError{exception: inner}that re-dispatches the inner exception, so it converts the same way as at the top level.rescueon the three callbacks, callinghandle_raised_error/4with the query and resource the wayrun_query/2already does.run_aggregate_query/3delegates toAshSql.AggregateQuery; the rescue is placed here rather than in ash_sql because the other rescues and the conversion live here.run_query_with_lateral_join/4previously ignored itsdestination_resourceargument; it is now used for the error.Tests:
test/cast_error_test.exscovers count, exists and aggregate (raisedCastErroronmain), a count under a limit (raisedSubQueryError), a limited relationship load across two parents and a relationship filter on a paginated read (bothAsh.Error.Unknownonmain), plus a control with a valid id on every path. Full suite 1006 passed onmain, where the same threeJoinSubquerySortTest/UniqAggregateSortTestcases from #858 fail with and without this change, and 983 passed with the patch on v2.13.1.mix format --check-formatted,mix credo --strict,mix sobelowandmix dialyzerclean.Overlap with #863. That PR adds a clause to
handle_raised_error/4at the same spot as theSubQueryErrorclause here, so whichever merges second will conflict on that one hunk. The resolution is to keep both clauses (order does not matter, they match different structs). I merged the two branches locally that way, and the full suite plus both new test files pass on the result. Happy to rebase this one once #863 lands, or the other way round.Found by an AI agent working with a human fuzz-testing their own application; the reproduction outside this repo is https://github.com/grempe/ash-fuzz-repros/blob/main/test/ash_postgres/uncastable_filter_value_conversion_test.exs, which also covers the ash_lua, AshGraphql and AshJsonApi surfaces of the same paths.
Contributor checklist
Leave anything that you believe does not apply unchecked.