ash icon indicating copy to clipboard operation
ash copied to clipboard

This should not be possible, please report a detailed bug at error when error is very clear

Open jeroen11dijk opened this issue 1 year ago • 8 comments

Bug Report

Describe the Bug

When attempting to use Ash.Sort.expr_sort with a resource or relationship that does not exist, the error message suggests reporting a bug instead of providing a clear and direct error message about the invalid resource/relationship.


To Reproduce

Add the following code to a resource:

prepare build(
  sort: [
    Ash.Sort.expr_sort(not_existing_resource_or_relationship.attribute, :string)
  ]
)

And you get the error

** (Ash.Error.Framework.AssumptionFailed) Assumption failed: Listing refs in not_existing_resource_or_relationship.attribute:

Found reference :attribute at path [:not_existing_resource_or_relationship]

No related resource at path [:not_existing_resource_or_relationship] from Cvs.MatchingTable.Entry

This should not be possible, please report a detailed bug at:

https://github.com/ash-project/ash/issues/new?assignees=&labels=bug%2C+needs+review&template=bug_report.md&title=

Expected Behavior

A more specific error message pointing directly to the Ash.Sort.expr_sort call and that it is a human error. Not asying that you should report a bug here.

Runtime

[ - Elixir version](elixir: "> 1.16",) {:ash, "> 3.4.37"}, {:ash_postgres, "> 2.4.12"}, {:ash_phoenix, "> 2.0"}, {:ash_admin, "> 0.11.9"}, {:ash_authentication, "> 4.2.7"}, {:ash_authentication_phoenix, "~> 2.1.2"},

jeroen11dijk avatar Nov 21 '24 09:11 jeroen11dijk

@zachdaniel Is this fixed? Seems like it is. When I try this out compile doesn't pass:

    error: undefined variable "not_existing_resource_or_relationship"
    │
 29 │                   Ash.Sort.expr_sort(not_existing_resource_or_relationship.attribute, :string)
    │                                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    │

== Compilation error in file lib/nutrea/food.ex ==
** (CompileError) cannot compile module (errors have been logged)
    (ash 3.4.64) expanding macro: Ash.Resource.Dsl.Actions.Read.Actions.Preparations.Prepare.prepare/1

ken-kost avatar Feb 25 '25 12:02 ken-kost

You have to first require Ash.Sort

zachdaniel avatar Feb 25 '25 13:02 zachdaniel

I didn't reproduce the same error but something similar:

Unknown Error

* ** (MatchError) no match of right hand side value: {:error, "Invalid reference not_existing_resource_or_relationship.attribute"}
  (ash 3.4.64) lib/ash/actions/read/read.ex:1833: anonymous fn/8 in Ash.Actions.Read.add_calc_context_to_sort/8

which happens on:

            {:ok, expr} =            # <- L1833
              Ash.Filter.hydrate_refs(
                expr,
                %{
                  resource: resource,
                  public?: false,
                  parent_stack: opts[:parent_stack]
                }
              )

This happens in add_calc_context_to_sort which is called in do_read

         query <- %{
           query
           | ...,
             sort:
               add_calc_context_to_sort(
                 query,
                 opts[:actor],
                 opts[:authorize?],
                 query.tenant,
                 opts[:tracer],
                 query.resource,
                 query.domain,
                 parent_stack: parent_stack_from_context(query.context)
               )
         }

My idea is adding some kind of validation in the with chain before: :ok <- validate_expr_path(query). Not sure if right approach but I can't figure out where else I could plug in the error.

ken-kost avatar Feb 25 '25 19:02 ken-kost

Yeah, that would make sense to me 😄

zachdaniel avatar Feb 25 '25 20:02 zachdaniel

Okay cool. One more technical question: What's in expression is: type(not_existing_resource_or_relationship.attribute, Ash.Type.String, []) And I'm not sure how to break that down. 🤔 I was thinking of using Info module for checks but i need to get from the above resource NotExistingResourceOrRelationship and attribute :attribute right? What would be the clean way of getting that (if there is a clean way 😬)?

ken-kost avatar Feb 25 '25 20:02 ken-kost

Oh, I would not try to an analyze the expression yourself. just make sure that the error from hydrate_refs gets returned instead of a pattern match error.

zachdaniel avatar Feb 25 '25 20:02 zachdaniel

Yea, that does make more sense. ⭐ Which would mean that instead of

         query <- %{
           query
           | filter:
               add_calc_context_to_filter(
                 query.filter,
                 opts[:actor],
                 opts[:authorize?],
                 query.tenant,
                 opts[:tracer],
                 query.domain,
                 query.resource,
                 parent_stack: parent_stack_from_context(query.context)
               ),
             sort:
               add_calc_context_to_sort(
                 query,
                 opts[:actor],
                 opts[:authorize?],
                 query.tenant,
                 opts[:tracer],
                 query.resource,
                 query.domain,
                 parent_stack: parent_stack_from_context(query.context)
               )

I will put add_calc_context_to_filter and add_calc_context_to_sort into the with chain above and they should return {:ok filter} and {:ok, sort} and will also have their with chains. Yea, that's juicy. 🐛 Would that be the right approach? I'll assume it is.

ken-kost avatar Feb 25 '25 20:02 ken-kost

Yeah, that code is pretty convoluted as well, so make sure that any other usages of those functions are updated. Are they public? I forget.

zachdaniel avatar Feb 25 '25 23:02 zachdaniel