sloanelybutsurely / typeid-elixir

Elixir implementation of TypeIDs: type-safe, K-sortable, and globally unique identifiers inspired by Stripe IDs
MIT License
59 stars 9 forks source link

Possible belongs_to improvement #36

Open PatrikStenmark opened 1 month ago

PatrikStenmark commented 1 month ago

I just started to work with adding TypeIDs to my app and immediately ran into issues with belongs_to. I've looked at the other issues and I did get it to work by using define_field: false and a separate field :thing_id. It did however feel like it should work to just do

schema "posts" do
  has_many :comments, Comment
end

schema "comments" do
  belongs_to :post, Post, type: TypeID 
end

but when I tried that I got an error about type was only allowed to be :uuid or :string. Without using type: TypeID I got an error about value #TypeID<"aaa_01j95zhepsf75rfymj6egjqg9m"> in where cannot be cast to type :integer in query.

Digging further I realized that when using type param to belongs_to, the value for type is included in the call to TypeID.init. This means when using type: TypeID, dump will try to see what database type it should convert itself to, but the type is TypeID, which is the Ecto custom type, not the database type.

I have a solution to this which changes the name of the type parameter to column_type, so the primary key definitions become

@primary_key {:id, TypeID, autogenerate: true, prefix: "post", column_type: :uuid}

and the belongs_to definitions becomes

belongs_to :wallet, Wallet, type: TypeID, column_type: :uuid

With a default of :uuid, the column_type can also be dropped.

I'm guessing this is something around the differences of how Ecto handles a ParameterizedType when used as a normal field and an association. Would you be interested in a PR for this if I clean it up? Or am I missing something here that makes it a bad idea?

sloanelybutsurely commented 1 month ago

I'm open to a PR. I'm fine with what you've laid out and my only real reservation is that i just don't love column_type here. But if Ecto is getting in the way of using just type perhaps that's just what has to be done.

sloanelybutsurely commented 1 month ago

what happens if the comments schema does not specify type?

schema "comments" do
  belongs_to :post, Post 
end

the existing code should infer all of the TypeID information using the Post schema. what is Post's @primary_key value?

lurodrigo commented 4 weeks ago

what happens if the comments schema does not specify type?

In that case Ecto assumes type: :integer as per docs. So we do need to specify the type with belongs_to and thus we run into the conflict between TypeID's use of type vs Ecto use of type.

What would an acceptable solution look like? I've just ran into this issue an can write a PR

lurodrigo commented 4 weeks ago

I started drafting a solution here https://github.com/sloanelybutsurely/typeid-elixir/pull/40/files and the following approach seems to work:

To get the type, we first read column_type from opts, and only if it's not available we read type from opts. This way, we can use column_type everywhere if we want to, but we can also keep using type and not break existing code.

For belongs_to, the user can use belongs_to(:post, Post, type: TypeID) to use the default_type for the column_type or specify it: belongs_to(:post, Post, type: TypeID, column_type: :string)

If you think the solution makes sense I can polish the PR

sloanelybutsurely commented 4 weeks ago

Are either of you able to provide a minimal reproducing project to share? My guess is the issues you're running into have something to do with how the project is configured (the values of @primary_key and @foreign_key_type in particular)

PatrikStenmark commented 4 weeks ago

I haven't been able to try without specifying type yet, I got pulled into something else. I will try to do it soon and report back.

lurodrigo commented 4 weeks ago

I've built one in this repo: https://github.com/lurodrigo/typeid_blog

main branch: Vanilla binary_ids

The relevant bits are /lib/blog/post.ex and /lib/blog/comment.ex

defmodule Blog.Post do
  use Ecto.Schema
  import Ecto.Changeset

  @primary_key {:id, :binary_id, autogenerate: true}
  @foreign_key_type :binary_id
  schema "posts" do
    field :title, :string
    field :body, :string

    has_many :comments, Blog.Comment
    timestamps(type: :utc_datetime)
  end
# ...
defmodule Blog.Comment do
  use Ecto.Schema
  import Ecto.Changeset

  @primary_key {:id, :binary_id, autogenerate: true}
  @foreign_key_type :binary_id
  schema "comments" do
    field :body, :string
    belongs_to :post, Blog.Post

    timestamps(type: :utc_datetime)
  end

# ...

In priv/repo/seeds.exs:


Repo.insert!(%Post{
  title: "My First Post",
  body: "This is the body of my first post",
  comments: [%{body: "This is the body of my first comment"}]
})

If you run mix ecto.reset and then iex -S mix phx.server and run Repo.all(Post) |> Repo.preload([:comments]) you get:

[
  %Blog.Post{
    __meta__: #Ecto.Schema.Metadata<:loaded, "posts">,
    id: "791cc4cb-2feb-4eed-9d6e-3e4ef447eff0",
    title: "My First Post",
    body: "This is the body of my first post",
    comments: [
      %Blog.Comment{
        __meta__: #Ecto.Schema.Metadata<:loaded, "comments">,
        id: "e99d9d1f-e72a-4352-9b84-b17a817a40e5",
        body: "This is the body of my first comment",
        post_id: "791cc4cb-2feb-4eed-9d6e-3e4ef447eff0",
        post: #Ecto.Association.NotLoaded<association :post is not loaded>,
        inserted_at: ~U[2024-10-31 17:18:24Z],
        updated_at: ~U[2024-10-31 17:18:24Z]
      }
    ],
    inserted_at: ~U[2024-10-31 17:18:24Z],
    updated_at: ~U[2024-10-31 17:18:24Z]
  }
]

type_id_failing branch

Here I'm using @primary_key and @foreign_key_type as per docs:

  @primary_key {:id, TypeID, autogenerate: true, prefix: "post", type: :uuid}
  @foreign_key_type TypeID

  schema "posts" do
    field :title, :string
    field :body, :string

    has_many :comments, Blog.Comment
    timestamps(type: :utc_datetime)
  end
  @primary_key {:id, TypeID, autogenerate: true, prefix: "comment", type: :uuid}
  @foreign_key_type TypeID

  schema "comments" do
    field :body, :string
    belongs_to :post, Blog.Post

    timestamps(type: :utc_datetime)
  end

When running mix ecto.reset, we get

** (DBConnection.EncodeError) Postgrex expected a binary of 16 bytes, got "post_01jbhseaexeh6vp5zfym3tcf5h". Please make sure the value you are passing matches the definition in your table or in your query or convert the value accordingly.
    (postgrex 0.19.2) lib/postgrex/type_module.ex:1084: Postgrex.DefaultTypes.encode_params/3
    (postgrex 0.19.2) lib/postgrex/query.ex:75: DBConnection.Query.Postgrex.Query.encode/3
    (db_connection 2.7.0) lib/db_connection.ex:1449: DBConnection.encode/5
    (db_connection 2.7.0) lib/db_connection.ex:1549: DBConnection.run_prepare_execute/5
    (db_connection 2.7.0) lib/db_connection.ex:772: DBConnection.parsed_prepare_execute/5
    (db_connection 2.7.0) lib/db_connection.ex:764: DBConnection.prepare_execute/4
    (postgrex 0.19.2) lib/postgrex.ex:295: Postgrex.query/4
    (ecto_sql 3.12.1) lib/ecto/adapters/sql.ex:1150: Ecto.Adapters.SQL.struct/10
    (ecto 3.12.4) lib/ecto/repo/schema.ex:834: Ecto.Repo.Schema.apply/4
    (ecto 3.12.4) lib/ecto/repo/schema.ex:415: anonymous fn/15 in Ecto.Repo.Schema.do_insert/4
    (ecto 3.12.4) lib/ecto/association.ex:948: Ecto.Association.Has.on_repo_change/5
    (ecto 3.12.4) lib/ecto/association.ex:648: anonymous fn/8 in Ecto.Association.on_repo_change/7
    (elixir 1.17.2) lib/enum.ex:2531: Enum."-reduce/3-lists^foldl/2-0-"/3
    (ecto 3.12.4) lib/ecto/association.ex:644: Ecto.Association.on_repo_change/7
    (elixir 1.17.2) lib/enum.ex:2531: Enum."-reduce/3-lists^foldl/2-0-"/3
    (ecto 3.12.4) lib/ecto/association.ex:589: Ecto.Association.on_repo_change/4
    (ecto 3.12.4) lib/ecto/repo/schema.ex:1018: Ecto.Repo.Schema.process_children/5
    (ecto 3.12.4) lib/ecto/repo/schema.ex:1096: anonymous fn/3 in Ecto.Repo.Schema.wrap_in_transaction/6
    (ecto_sql 3.12.1) lib/ecto/adapters/sql.ex:1400: anonymous fn/3 in Ecto.Adapters.SQL.checkout_or_transaction/4
    (db_connection 2.7.0) lib/db_connection.ex:1756: DBConnection.run_transaction/4

type_id_fixed branch:

Here I'm pointing to my fix using column_type as described below:

  @primary_key {:id, TypeID, autogenerate: true, prefix: "post", type: :uuid}
  schema "posts" do
    field :title, :string
    field :body, :string

    has_many :comments, Blog.Comment
    timestamps(type: :utc_datetime)
  end
  @primary_key {:id, TypeID, autogenerate: true, prefix: "comment", type: :uuid}
  schema "comments" do
    field :body, :string
    belongs_to :post, Blog.Post, type: TypeID, column_type: :uuid

    timestamps(type: :utc_datetime)
  end

If you run mix ecto.reset and then iex -S mix phx.server and run Repo.all(Post) |> Repo.preload([:comments]) you get:

[
  %Blog.Post{
    __meta__: #Ecto.Schema.Metadata<:loaded, "posts">,
    id: #TypeID<"post_01jbhsr6mpe74a9tjmtryhwzxw">,
    title: "My First Post",
    body: "This is the body of my first post",
    comments: [
      %Blog.Comment{
        __meta__: #Ecto.Schema.Metadata<:loaded, "comments">,
        id: #TypeID<"comment_01jbhsr6mtewsb2b5e20va8ymj">,
        body: "This is the body of my first comment",
        post_id: #TypeID<"post_01jbhsr6mpe74a9tjmtryhwzxw">,
        post: #Ecto.Association.NotLoaded<association :post is not loaded>,
        inserted_at: ~U[2024-10-31 17:25:11Z],
        updated_at: ~U[2024-10-31 17:25:11Z]
      }
    ],
    inserted_at: ~U[2024-10-31 17:25:11Z],
    updated_at: ~U[2024-10-31 17:25:11Z]
  }
]

So my fix seems to work

PatrikStenmark commented 2 weeks ago

I can confirm that I also run into this issue when only using belongs_to :post, Post. This is an old app which means there are a mix of serial integer, uuid and TypeID ids, so I haven't set @foreign_key to anything. @primary_key is set to @primary_key {:id, TypeID, autogenerate: true, prefix: "post", column_type: :uuid}. Maybe this is related?

sloanelybutsurely commented 2 weeks ago

thanks for the input here. i'm investigating this issue and coming up with some potential solutions.