ash-project / spark

Tooling for building DSLs in Elixir
MIT License
105 stars 23 forks source link

Merging Fragments overwrites set values with defaults. #29

Closed barnabasJ closed 3 months ago

barnabasJ commented 1 year ago

Describe the bug

When using fragments, default values should not overwrite properties set on the resource while merging.

To Reproduce A minimal set of resource definitions and calls that can reproduce the bug.

defmodule CodeInterfaceFragment do
  use Spark.Dsl.Fragment, of: Ash.Resource

  code_interface do
    define get_by_id, action: :read, get_by: [:id]
  end
end

defmodule Resource do
  use Ash.Resource, fragments: [CodeInterfaceFragment]

  code_interface do
    define_for Api
  end

  actions do
     defaults [:read]
  end
end

This leads to the following compile warning:

warning: code_interface.define_for is being overwritten from Api to false by fragment: Elixir.CodeInterfaceFragment
  (spark 1.0.5) lib/spark/dsl.ex:492: anonymous fn/5 in Spark.Dsl.merge_with_warning/4
  (elixir 1.14.3) lib/keyword.ex:1048: Keyword.do_merge/6
  (spark 1.0.5) lib/spark/dsl.ex:476: anonymous fn/4 in Spark.Dsl.handle_fragments/2
  (elixir 1.14.3) lib/map.ex:687: Map.update/4
  (stdlib 4.2) maps.erl:411: :maps.fold_1/3
  (elixir 1.14.3) lib/enum.ex:2468: Enum."-reduce/3-lists^foldl/2-0-"/3

Expected behavior A clear and concise description of what you expected to happen.

code_interface.define_for should not be overwritten.

Runtime

Additional context Add any other context about the problem here.

zachdaniel commented 1 year ago

So we probably need to not set default values in fragments. Might actually be pretty difficult to accomplish.

zachdaniel commented 1 year ago

Actually...we could set some process context to say that we're in a fragment, and doing that could cause the DSL macros to remove the default key on any fragments? Would also need to remove the required key, which is a bit problematic. Perhaps we should instead store wether or not a given DSL key was set, and then have code_interface in Ash not do what it currently does (using define_for false as a sentinal "disabled" value).

zachdaniel commented 3 months ago

Unfortunately there is no good way for us to do this.