Another change to the way add_expression_part/2 handles relations

[adonig]

I had to change the way add_expression_part/2 handles relations. Instead of using the source attribute and a recursive call it now uses the destination attribute and needs no recursive call anymore.

Changes

• Old Method: [author: [eq: 1]] becomes [author_id: [eq: 1]]
• New Method: [author: [eq: 1]] becomes [author: [id: [eq: 1]]]

Benefits

• The source attribute can now be private.
• Eliminates recursion by modifying the nested_statement when all conditions are met.

How it works

  defp add_expression_part({field, nested_statement}, context, expression)
       when is_atom(field) or is_binary(field) do
    cond do
      rel = relationship(context, field) ->

        # Instead of trying to recursively call add_expression_part/2 with {source_attr.name, nested_expression}
        # we now change the nested_statement to [{pk, nested_statement}] where pk is the destination attribute
        # of the relationship. We can then use this new nested_statement with the old code. We make sure that
        # the nested_statement only gets changed when its first key isn't an attribute, because otherwise we
        # would turn [author: [name: [eq: "Zach"]]] into [author: [id: [name: [eq: "Zach"]]]].
        nested_statement =
          with relation_type when relation_type != :many_to_many <- rel.type,
               destination <- rel.destination,
               dest_attr <- rel.destination_attribute,
               [pk] when pk == dest_attr <- Ash.Resource.Info.primary_key(destination),
               attr <- attribute(%{public?: true, resource: destination}, pk),
               %Ash.Resource.Attribute{} = attr,
               true <- is_list(nested_statement) || is_map(nested_statement),
               {kw, _} <- Enum.at(nested_statement, 0),
               nil <- attribute(%{public?: true, resource: destination}, kw) do
            if is_list(nested_statement) do
              [{dest_attr, nested_statement}]
            else
              Map.put(%{}, dest_attr, nested_statement)
            end
          else
            _ -> nested_statement
          end

          # here comes the old code

Contributor checklist

• [x] Bug fixes include regression tests
• [x] Chores
• [x] Documentation changes
• [x] Features include unit/acceptance tests
• [x] Refactoring
• [x] Update dependencies

[zachdaniel]

I'm thinking that there are issues both with the original code and this code. You can do things like reference calculations and relationships in a nested filter, so the "is attribute" check is insufficient.

We need to add some tests for this specific case with filter parsing. Can you add some tests for this implementation? I'll then suggest some example tests that would break this behavior. I think what we may want to do is then add a test for the original issue you saw, revert the code back to its original state(before the previous PR), and explore how to fix that original issue.

[adonig]

This is the original implementation:

      rel = relationship(context, field) ->
        context =
          context
          |> Map.update!(:relationship_path, fn path -> path ++ [rel.name] end)
          |> Map.put(:resource, rel.destination)

        if is_list(nested_statement) || is_map(nested_statement) do
          case parse_expression(nested_statement, context) do
            {:ok, nested_expression} ->
              {:ok, BooleanExpression.optimized_new(:and, expression, nested_expression)}

            {:error, error} ->
              {:error, error}
          end
        else
          with [field] <- Ash.Resource.Info.primary_key(context.resource),
               attribute <- attribute(context, field),
               {:ok, casted} <-
                 Ash.Type.cast_input(attribute.type, nested_statement, attribute.constraints) do
            add_expression_part({field, casted}, context, expression)
          else
            _other ->
              {:error,
               InvalidFilterValue.exception(
                 value: inspect(nested_statement),
                 message:
                   "a single value must be castable to the primary key of the resource: #{inspect(context.resource)}"
               )}
          end
        end

I believe it suffers from at least two issues:

1. It crashes when the destination primary key is private because we access attribute.type despite attribute is nil.
2. It supports filter statements like [author: uuid] but not filter statements like [author: [in: [uuid1, uuid2]]]. Instead it turns the latter into [in: [uuid1, uuid2]].

I believe the implementation I suggested does better because it turns [author: uuid] into [author: [id: uuid]] and [author: [in: [uuid1, uuid2]]] into [author: [id: [in: [uuid1, uuid2]]]].

In the current version it turns author into author_id. That works but it starts to fail when the source attribute is private. I realized this when I made my source attributes private because I didn't want them to show up in the JSON.

I believe those two tests already check that it works:

    test "understands relationship filter subsets when a value coincides with the join field" do
      id1 = Ash.UUID.generate()
      id2 = Ash.UUID.generate()
      filter = Filter.parse!(Post, author1: [id: [in: [id1, id2]]])

      candidate = Filter.parse!(Post, author1_id: id1)

      assert Filter.strict_subset_of?(filter, candidate)
    end

    test "allows to omit the join field for belongs_to relationships" do
      id1 = Ash.UUID.generate()
      id2 = Ash.UUID.generate()
      filter = Filter.parse!(Post, author1: [in: [id1, id2]])

      candidate = Filter.parse!(Post, author1: id1)

      assert Filter.strict_subset_of?(filter, candidate)
    end

I made author1_id private to make sure this case also gets checked.

[zachdaniel]

An example of something that would fail this is that you can use nested relationships or calculations in the expression syntax.

[author: [org: [name: "fred"]]]

In that case it would be expanded by the current code to be [author_id: [org: [name: "fred"]]]

Or if author has a calculation on it called has_friend

[author: [has_friend: true]]

And keep in mind you can mix and match these, i.e

[author: [eq: 1, has_friend: true]]

So we need to test those cases and confirm that this transformation has the desired effects and does not break those formats.

[adonig]

[author: [eq: 1, has_friend: true]]

This one is a bit harder. I believe I have to split the nested statement into multiple statements and then use recursion to handle them separately and combine the result using a boolean and.

I also realized that there is an issue with embedded fields but I have to check whether it already existed in the original implementation.

[adonig]

I believe I found a bug. The code either doesn't get executed or op_module becomes true:

https://github.com/ash-project/ash/blob/e1dffc0c0c4fe386dcbffc2038e9f1ad90fcb3aa/lib/ash/filter/filter.ex#L2857-L2872

I think it should be something like this instead:

 (op_module = get_operator(field)) && match?([_, _ | _], nested_statement) -> 

[adonig]

I implemented something terrible that seems to work but I believe the whole filter module needs some love. Maybe we can add lots of tests to make sure everything works as expected and then start to clean it up piece by piece.

[adonig]

Not sure why we get those dialyzer errors. Locally my dialyzer run succeeds:

Starting Dialyzer
[
  check_plt: false,
  init_plt: ~c"/home/asd/Code/github.com/adonig/ash/_build/dev/dialyxir_erlang-26.0.2_elixir-1.17.0_deps-dev.plt",
  files: [~c"/home/asd/Code/github.com/adonig/ash/_build/dev/lib/ash/ebin/Elixir.Ash.ProcessHelpers.beam",
   ~c"/home/asd/Code/github.com/adonig/ash/_build/dev/lib/ash/ebin/Elixir.Ash.Resource.Actions.Update.beam",
   ~c"/home/asd/Code/github.com/adonig/ash/_build/dev/lib/ash/ebin/Elixir.Ash.Resource.Change.GetAndLockForUpdate.beam",
   ~c"/home/asd/Code/github.com/adonig/ash/_build/dev/lib/ash/ebin/Elixir.Inspect.Ash.Vector.beam",
   ~c"/home/asd/Code/github.com/adonig/ash/_build/dev/lib/ash/ebin/Elixir.Ash.Resource.Dsl.Aggregates.Sum.beam",
   ...],
  warnings: [:unknown]
]
Total errors: 0, Skipped: 0, Unnecessary Skips: 0
done in 0m18.48s
done (passed successfully)

[zachdaniel]

🚀 Thank you for your contribution! 🚀