zoonect-oss / ash_uuid

AshUUID: Extension for using UUID v4 and v7, with supports encoding and prefixing
https://hexdocs.pm/ash_uuid
MIT License
13 stars 6 forks source link

AshUUID causes compilation deadlock between mutually related resources #10

Closed spacebat closed 3 months ago

spacebat commented 3 months ago

Given two resources that belong_to each other, such as the below, compilation of the project fails in a deadlock:

defmodule Pineapple do
  use Ash.Resource, data_layer: AshPostgres.DataLayer, extensions: [AshUUID]

  code_interface do
    define_for Area
  end

  postgres do
    table "pineapples"
    repo MyApp.Repo
  end

  attributes do
    uuid_attribute :id, prefix: "pnp"
    create_timestamp :inserted_at
  end

  relationships do
    belongs_to :ham, Ham
  end

  actions do
    defaults [:create, :read, :update]
  end
end

defmodule Ham do
  use Ash.Resource, data_layer: AshPostgres.DataLayer, extensions: [AshUUID]

  code_interface do
    define_for Area
  end

  postgres do
    table "hams"
    repo MyApp.Repo
  end

  attributes do
    uuid_attribute :id, prefix: "ham"
    create_timestamp :inserted_at
  end

  relationships do
    belongs_to :pineapple, Pineapple
  end

  actions do
    defaults [:create, :read, :update]
  end
end

The error is:

== Compilation error in file lib/my_app/my_domain/resources/pineapple.ex ==
** (CompileError) deadlocked waiting on module Ham
    :erlang.garbage_collect/1
    (ash_uuid 1.0.0) lib/ash_uuid/transformers/belongs_to_attribute.ex:79: anonymous fn/2 in AshUUID.Transformers.BelongsToAttribute.reject_yet_existent/1
    (elixir 1.17.1) lib/enum.ex:4429: Enum.reject_list/2
    (elixir 1.17.1) lib/enum.ex:4432: Enum.reject_list/2
    (ash_uuid 1.0.0) lib/ash_uuid/transformers/belongs_to_attribute.ex:15: AshUUID.Transformers.BelongsToAttribute.transform/1
    (spark 2.2.3) lib/spark/dsl/extension.ex:590: anonymous fn/4 in Spark.Dsl.Extension.run_transformers/4

== Compilation error in file lib/my_app/my_domain/resources/ham.ex ==
** (CompileError) deadlocked waiting on module Pineapple
    (ash_uuid 1.0.0) lib/ash_uuid/transformers/belongs_to_attribute.ex:79: anonymous fn/2 in AshUUID.Transformers.BelongsToAttribute.reject_yet_existent/1
    (elixir 1.17.1) lib/enum.ex:4429: Enum.reject_list/2
    (ash_uuid 1.0.0) lib/ash_uuid/transformers/belongs_to_attribute.ex:15: AshUUID.Transformers.BelongsToAttribute.transform/1
    (spark 2.2.3) lib/spark/dsl/extension.ex:590: anonymous fn/4 in Spark.Dsl.Extension.run_transformers/4
    (elixir 1.17.1) lib/enum.ex:4858: Enumerable.List.reduce/3
    (elixir 1.17.1) lib/enum.ex:2585: Enum.reduce_while/3

Compilation failed because of a deadlock between files.
The following files depended on the following modules:

    lib/my_app/my_domain/resources/pineapple.ex => Pineapple
  lib/my_app/my_domain/resources/ham.ex => Ham

Ensure there are no compile-time dependencies between those files and that the modules they reference exist and are correctly named

Though they refer to each other this did not cause a compilation error until AshUUID was introduced.

moissela commented 3 months ago

Interesting, thank you for reporting this issue!

I've been able to reproduce that but I actually not know well how we can solve this, maybe @zachdaniel has had similar problems before and can give an hint here?

zachdaniel commented 3 months ago

So I'm not sure what belongs_to_attribute.ex is meant to do, but it is unfortunately not possible to use information from a related resource to modify the current resource, for this exact reason. I don't really have any advice fro you aside from "you'll have to figure something else out" 😦 . This typically means that whatever you were trying to derive from the destination must instead be explicitly configured by users.

zachdaniel commented 3 months ago

Specifically, this call:

 relationship.destination.spark_dsl_config()

will always cause deadlocks between mutually related resources (and slow down compile times in general)

moissela commented 3 months ago

Thanks, @zachdaniel , I suspected that this wasn't quite right.

Since AshUUID is configurable for each of its uses, within belongs_to_attribute the specific settings used on the foreign_key target of the relationship are retrieved to automatically replicate them on the attribute generated by the belongs_to macro. It has always worked well, but in this specific case, it breaks.

A workaround is to have the user disable the auto-generation of the xxx_id field via the belongs_to macro so that they can manually define the field with the same settings as the target.

This is one of the reasons why I was already considering making a much simpler and non-configurable version of this module, replaced by the idea we discussed today of directly incorporating uuidv7 into the core of Ash.

Tomorrow I will try to comment here with a temporary solution.

zachdaniel commented 3 months ago

Hmm...but a belongs_to attribute should never be auto generated, right? Can we just omit the autogeneration constraints for the belongs_to attribute?

spacebat commented 3 months ago

A simpler mode or package would be nice, it's not clear from the docs what the prefixing, encoding and other settings are for, nor whether AshUUID will cause uuid_primary_key to give v7 ids, or if I have to change uuid_primary_key :id to uuid_attribute :id for every resource. The compilation deadlock means I haven't got that far in my experimentation.

I'd guess most users just want the uuids to be v7 either globally or per reource/attribute, no other changes or options, and maybe a function for timestamp extraction.

BTW I'm very grateful that you've gone ahead and provided this package to the community!

zachdaniel commented 3 months ago

Yeah, if we add it in core it would probably look like this uuid_v7_primary_key :id

moissela commented 3 months ago

Okay, I found a solution to the problem and I will try to address everything.

Regarding this issue reported by @spacebat, as I imagined, the only solution I found is to manually define the _id attributes when there are cross-relations between 2 resources. I have added tests for the suggested workaround and these are the interesting parts:

Ham resource:

defmodule AshUUID.Test.Ham do
  @moduledoc false

  use Ash.Resource, domain: AshUUID.Test, data_layer: AshPostgres.DataLayer, extensions: [AshUUID]

  postgres do
    table "hams"
    repo AshUUID.Test.Repo
  end

  attributes do
    uuid_attribute :id, prefix: "ham"

    uuid_attribute :burger_id,
      prefix: "bur",
      version: 4,
      primary_key?: false,
      allow_nil?: true,
      default: nil,
      writable?: true,
      public?: true

    create_timestamp :inserted_at
  end

  relationships do
    belongs_to :burger, AshUUID.Test.Burger, define_attribute?: false
  end

  actions do
    defaults [:read, create: :*, update: :*]
    default_accept :*
  end
end

Burger resource:

defmodule AshUUID.Test.Burger do
  @moduledoc false

  use Ash.Resource, domain: AshUUID.Test, data_layer: AshPostgres.DataLayer, extensions: [AshUUID]

  postgres do
    table "burgers"
    repo AshUUID.Test.Repo
  end

  attributes do
    uuid_attribute :id, prefix: "bur", version: 4

    uuid_attribute :ham_id,
      prefix: "ham",
      primary_key?: false,
      default: nil,
      writable?: true,
      public?: true

    create_timestamp :inserted_at
  end

  relationships do
    belongs_to :ham, AshUUID.Test.Ham, define_attribute?: false
  end

  actions do
    defaults [:read, create: :*, update: :*]
    default_accept :*
  end
end

Usage test:

test "testing hams and burgers" do
  ham =
    AshUUID.Test.Ham
    |> Ash.Changeset.for_create(:create, %{})
    |> Ash.create!()

  assert %AshUUID.Test.Ham{} = ham
  assert :prefixed = AshUUID.identify_format(ham.id)
  assert ["ham", b62_string_uuid] = String.split(ham.id, "_")
  assert {:ok, string_uuid} = AshUUID.Encoder.decode(b62_string_uuid)
  assert {:ok, %{version: 7}} = Uniq.UUID.info(string_uuid)

  burger =
    AshUUID.Test.Burger
    |> Ash.Changeset.for_create(:create, %{ham_id: ham.id})
    |> Ash.create!()

  assert %AshUUID.Test.Burger{} = burger
  assert :prefixed = AshUUID.identify_format(burger.id)
  assert ["bur", b62_string_uuid] = String.split(burger.id, "_")
  assert {:ok, string_uuid} = AshUUID.Encoder.decode(b62_string_uuid)
  assert {:ok, %{version: 4}} = Uniq.UUID.info(string_uuid)
  assert :prefixed = AshUUID.identify_format(burger.ham_id)
  assert ["ham", b62_string_uuid] = String.split(burger.ham_id, "_")
  assert {:ok, string_uuid} = AshUUID.Encoder.decode(b62_string_uuid)
  assert {:ok, %{version: 7}} = Uniq.UUID.info(string_uuid)

  ham =
    ham
    |> Ash.Changeset.for_update(:update, %{burger_id: burger.id})
    |> Ash.update!()

  assert :prefixed = AshUUID.identify_format(ham.burger_id)
  assert ["bur", b62_string_uuid] = String.split(ham.burger_id, "_")
  assert {:ok, string_uuid} = AshUUID.Encoder.decode(b62_string_uuid)
  assert {:ok, %{version: 4}} = Uniq.UUID.info(string_uuid)

  burger = Ash.load!(burger, :ham)
  ham_id = ham.id

  assert %AshUUID.Test.Ham{id: ^ham_id} = loaded_ham = burger.ham
  assert :prefixed = AshUUID.identify_format(loaded_ham.id)

  assert burger.id == ham.burger_id
end

In response to @zachdaniel, the need for the belongs_to_attribute transformer arises from the fact that AshUUID allows for a configurable id definition among various versions of UUID, in encoded or non-encoded mode, with a configurable prefix or without, and when a belongs_to relationship is defined, the generated _id attribute should match to the same configurations for everything to work well.

What might not be clear regarding this extension is that its goal was not only to allow the use of UUID version 7 within Ash but also to enable replication of what is well described in this article.

In any case, with the upcoming addition of support for UUID version 7 into the core of Ash, those who are only interested in this part will be able to do it completely without the AshUUID extension, and alternative solutions like the one proposed here will no longer be necessary.

But I ask @zachdaniel if he has any suggestions on how to achieve in Ash what is described in the article I mentioned regarding base62 encoded ids and/or prefixed when they end up in routes or APIs: do you think it can be done automatically with an extension or is it something that will be better managed manually project by project? An affirmative answer to this question could help me and make AshUUID definitively and completely obsolete.

zachdaniel commented 2 months ago

I think for those trying to replicate that kind of id behavior, it would make sense to have an extension like this one. At minimum, a custom type like the one provided here makes sense, because you're separating the application-representation from the data-layer representation, and custom types are the way to go there.

zachdaniel commented 2 months ago

We can potentially enrich our UUID types in core to support these kinds of options, but we'd need to do that one at a time, and see how we go. Something like constraints: [encode?: :base64, prefix: "some-text"]

moissela commented 2 months ago

Perfect @zachdaniel , thank you!

That's what I was hoping for and I agree: let's start by adding uuid v7 to Ash and then later I will try to prepare other PRs to gradually add the rest like encoding or prefixing and we'll see how it goes 🤓