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

Possible belongs_to improvement #36

Open
PatrikStenmark opened this issue Oct 2, 2024 · 9 comments
Open

Possible belongs_to improvement #36

PatrikStenmark opened this issue Oct 2, 2024 · 9 comments
Assignees

Comments

@PatrikStenmark
Copy link

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor

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
Copy link
Contributor

lurodrigo commented Oct 30, 2024

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
Copy link
Owner

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
Copy link
Author

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants