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

Non-standard id via primary_key option on Belongs To field not respected #3413

Open
3 of 9 tasks
therabidbanana opened this issue Nov 12, 2024 · 4 comments · May be fixed by #3422
Open
3 of 9 tasks

Non-standard id via primary_key option on Belongs To field not respected #3413

therabidbanana opened this issue Nov 12, 2024 · 4 comments · May be fixed by #3422
Labels
Bug Something isn't working

Comments

@therabidbanana
Copy link

Describe the bug

We have a table that has an integer id (deprecated) and a uuid used for all new tables. This is easy to define in the activerecord relationship via primary_key:

belongs_to :user, primary_key: :uuid

But the Avo belongs to field for this relationship still tries to save using the #id method. Depending on if the id field exists on the table this will either generate a NoMethodError or, in our case since we actually have both, using an invalid integer type id, silently failing.

Steps to Reproduce

Steps to reproduce the behavior:

  1. Add uuid identifier column to existing table with integer ids. Define relationship using this new column.
  2. Configure a field :user, as: :belongs_to
  3. Try to edit the newly added field via Avo

Expected behavior & Actual behavior

Observe id param is sent rather than uuid

Models and resource files

# == Schema Information
#
# Table name: posts
#
#  id                :uuid             not null, primary key
#  title             :string
# user_id :uuid
#
# Indexes
#
#  index_posts_on_user_id  (user_id)
#
class Post
  belongs_to :user, primary_key: :uuid, optional: true
end
# == Schema Information
#
# Table name: users
#
#  id                     :bigint(8)        not null, primary key
#  name                   :string           not null
#  uuid                   :uuid             not null
#
# Indexes
#
#  index_users_on_uuid                  (uuid) UNIQUE
class User
end
class Avo::Resources::Post
  def fields
    field :title, as: :text
    field :user, as: :belongs_to
  end
end

System configuration

Avo version:
= 3.13.7

Rails version:
= 7.2.1.2

Ruby version:

License type:

  • Community
  • Pro
  • Advanced

Additional context

My best guess following code path is that it looks like id is assumed to the the primary key here, but reflections should be used like they are for the foreign key option:

https://github.com/avo-hq/avo/blob/main/lib/avo/fields/belongs_to_field.rb#L219

Impact

  • High impact (It makes my app un-usable.)
  • Medium impact (I'm annoyed, but I'll live.)
  • Low impact (It's really a tiny thing that I could live with.)

Urgency

  • High urgency (I can't continue development without it.)
  • Medium urgency (I found a workaround, but I'd love to have it fixed.)
  • Low urgency (It can wait. I just wanted you to know about it.)
@therabidbanana
Copy link
Author

I have worked around this for now with a method like this on my Post object:

  # Hack for https://github.com/avo-hq/avo/issues/3413
  def user_id=(user_id)
    case user_id
    when Integer
      super(User.find(user_id).uuid)
    else
      super
    end
  end

@Paul-Bob
Copy link
Contributor

Hi @therabidbanana thanks for opening the issue.

Could you please create a minimal reproduction repository?

Here's a quick command that will help you generate a new Avo app in no-time.
https://docs.avohq.io/3.0/technical-support.html#reproduction-repository

Thank you!

@Paul-Bob Paul-Bob added the Waiting on Reproduction Pending reproduction repository or detailed reproduction steps to proceed with issue resolution. label Nov 13, 2024
@therabidbanana
Copy link
Author

Sure! Here's a repository reproducing the issue. I went with SQLite which doesn't have native uuid types, but that's kind of a side concern so strings are fine here. The point is that primary_key is not used as expected so the user_id is being set to the integer rather than the string column.

https://github.com/therabidbanana/avo-repro-3413

In this screenshot, we'd expect proper functioning to either be:

  1. we find the User record via the resource, then get the primary_key specified by reflection
  2. we include the primary_key specified by reflection and set that instead

Instead what happens is it sends the integer id and doesn't look up the primary_key desired, resulting in a "user must be specified" error because user_id mismatches here.

Image

@Paul-Bob Paul-Bob linked a pull request Nov 14, 2024 that will close this issue
4 tasks
@Paul-Bob
Copy link
Contributor

Thanks for the repo, @therabidbanana! It was helpful and made reproducing the issue quick and easy.

While working through it, I put together this PR which solves the issue.

However, tests are currently failing on polymorphic associations. The PR will stay in draft for a while, so feel free to refine it if you'd like to contribute.

@Paul-Bob Paul-Bob added Bug Something isn't working and removed Waiting on Reproduction Pending reproduction repository or detailed reproduction steps to proceed with issue resolution. labels Nov 14, 2024
@Paul-Bob Paul-Bob moved this to To Do in Issues Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
Status: To Do
Development

Successfully merging a pull request may close this issue.

2 participants