Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

polymorphic_on_delete option needs refining and potentially moving #189

Open
mindok opened this issue Jan 4, 2024 · 1 comment
Open
Labels
enhancement New feature or request

Comments

@mindok
Copy link

mindok commented Jan 4, 2024

Describe the bug
The references config block containing a polymorphic_on_delete :delete setting is on the "unexpected" side of the relationship between a resource and a polymorphic resource. If it is set on the polymorphic resource side, the generated migration does not implement the delete in the foreign key constraint.

Discussion here
https://elixirforum.com/t/how-to-combine-polymorphic-with-references-on-delete/60756

To Reproduce

defmodule  MyApp.Location do

  use Ash.Resource, data_layer: AshPostgres.DataLayer

  postgres do
    repo MyApp.Repo
    polymorphic? true

    # HAS NO EFFECT
    references do
      polymorphic_on_delete :delete
    end
  end

  attributes do
    uuid_primary_key :id
    attribute :resource_id, :uuid do
      allow_nil? false
      private? true
    end
    attribute :x, :float
    attribute :y, :float
  end
end

defmodule  MyApp.Event do
  use Ash.Resource, data_layer: AshPostgres.DataLayer

  postgres do
    repo MyApp.Repo
    table "events"

    # WORKS IF IT IS HERE
    references do
      polymorphic_on_delete :delete
    end
  end

  attributes do
    uuid_primary_key :id
    attribute :when, :utc_datetime, allow_nil?: false, default: &DateTime.utc_now/0
    attribute :what, :string
    attribute :who, :string
  end

  relationships do
    has_one :location, Location do
      relationship_context %{data_layer: %{table: "event_locations"}}
      destination_attribute :resource_id
    end
  end

Expected behavior
To be agreed. Currently the config location is surprising as for other relationships it is the "owned" resource that defines the on_delete behaviour rather than the owning resource. However, where you have multiple relationships to polymorphic resources (to be expected!) and want to control the delete behaviour for each separately it may make sense to do it this way round. So there's a philosophical question to answer first - should the delete behaviour belong to the polymorphic resource or to the relationship? If it is the relationship it should probably be defined at the specific relationship level.

In the example above, this may be something like:

defmodule  MyApp.Event do
  use Ash.Resource, data_layer: AshPostgres.DataLayer

  postgres do
    repo MyApp.Repo
    table "events"

    references do
      # Either something here that points to the Location resource, but looks different to 
      # the existing config for relationships as it is inverted...  
    end
  end

  attributes do
    uuid_primary_key :id
    attribute :when, :utc_datetime, allow_nil?: false, default: &DateTime.utc_now/0
    attribute :what, :string
    attribute :who, :string
  end

  relationships do
    has_one :location, Location do
       # Or probably better here to collect together all the implementation details of the relationship in one place
      relationship_context %{data_layer: %{table: "event_locations"}, on_delete: :delete}
      destination_attribute :resource_id
    end
  end

**Runtime

  • Elixir version: 1.15.4
  • Erlang version: Erlang/OTP 26 [erts-14.0.2]
  • OS: Mac
  • Ash version: 2.17.17
  • Ash Postgres: 1.3.61
  • any related extension versions
@mindok mindok added bug Something isn't working needs review labels Jan 4, 2024
@zachdaniel
Copy link
Contributor

I like the proposed solution of doing it with relationship_context 👍 It also lends itself to a gradual migration, as we can first support both things but raise an error if they are both configured. Then we can add a warning if the existing config is used, and finally we can remove the existing config entirely. Because of this, we won't necessarily have to wait until 3.0 for this to happen. I won't have a chance to work on this in the near term, but PRs are welcome! 🥳

@zachdaniel zachdaniel added documentation Improvements or additions to documentation enhancement New feature or request and removed bug Something isn't working documentation Improvements or additions to documentation labels Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants