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

Avo with ActiveModel #3680

Open
rkh opened this issue Feb 21, 2025 · 6 comments
Open

Avo with ActiveModel #3680

rkh opened this issue Feb 21, 2025 · 6 comments

Comments

@rkh
Copy link

rkh commented Feb 21, 2025

Was evaluating Avo with plans to get a paid license, initial attempt seemed good, but ran into issues with using it for models that aren't backed by ActiveRecord.

I thought Array Resources would be the way to go, but they aren't.

  • They can only handle hashes and ActiveRecord instances, not ActiveModel objects or anything else.
  • When converting ActiveModel objects to hashes, I ran into the following issues:
    • For a full list of resources, all data needs to be injected into the hashes inside resources, for large amounts of data, this can be slow, lazy-loading needs to be done on field level. I got around this by only having an id field in the hash and then using
    • There still are code paths that are very easy to hit that lead Avo to treat objects like as if they are AR models. Why would ArrayResource call where on a class it defined itself, that doesn't implement it?
  • It defines a top level, global ActiveModel class based on the name of the resource, in the process overriding the class that already exists (or rather, not triggering Zeitwerk to load it). What the actual f**k? This breaks our main app. There is no documentation for this. I spent hours hunting down what's going wrong, only figuring it out because of the next issue…
  • The code for generating the constant name is broken, it tried to define a global Display name class for a resource called DisplayName

Are there any plans to support models not backed by the database?
Is there any documentation on how to avoid Avo defining classes outside the Avo namespace?

@rkh
Copy link
Author

rkh commented Feb 21, 2025

Apologies for the harsh language. I will keep an eye on the ticket, but will be giving ActiveAdmin another try in the mean time.

@adrianthedev
Copy link
Collaborator

Hey @rkh.

Thanks for looking into Avo.
Your initial assessment was correct. We don't appreciate the language, but still I'd like to address your comments.

  1. The Array driver and field are still in a beta stage.
    Given how each use-case is different and there could be literally an infinite ways to set up an app, they won't cover them all.

We are open to add support to more edge-cases. We did in the past for many developers.

If you send in a better description of your particular case we would know better how to support it. A reproduction repository is the best way.

  1. That's correct, with large amounts of data this won't be performant. It's not built for that. Again, if you'll share how you were thinking to set everything up, we'd love to give it a try.
    Maybe the array driver is not what you need.

  2. I see what you mean. Like if you already had a Movie model and you define a Movie array resource, Avo will tell Rails to use this newly defined object instead of your.
    I see how that could break your app and I'm sorry you got frustrated by that. We'll better document that process and maybe we could add a guard for it (@Paul-Bob can you please create an issue for it?).

  3. Yup. another bug we can fix as part of the beta status.

I'm actually curious about how you tried to use the Array driver.
Were you querying records from the database?
What did you mean when you said "ActiveModel objects"? Can you share some of that code?

Are there any plans to support models not backed by the database?
Yes. What's your use-case? Where are you getting those records from?

Is there any documentation on how to avoid Avo defining classes outside the Avo namespace?
Avo is plenty documented. There isn't one page where it talks about that. I haven't seen too many projects doing that.

@rkh
Copy link
Author

rkh commented Feb 21, 2025

I do apologize for the outburst, that was absolutely not appropriate. Avo looks much more modern than the other popular alternatives. And from a maintenance point with the pitch of us not having to put too much work into an admin UI, I do actually quite like that there's a paid option. Though that also makes me hesitant to contribute to the code base itself.

I will look into a repository to reproduce things.

I've been trying to use it in two ways:

  1. We do have some models that are not backed by a database. They are called Country and Timezone (we do have many more, that also behave differently, like SearchResult, but these are the two I was trying to set up). Loading the entire data can be a bit slow, as they do fetch entries from the CLDR. This is a development-only issue for us, as all this is cached in production. The are ActiveModel instances (implementing all the methods that aren't database relates – to_param, model_name, attributes, valid?, errors, etc, and the class implements find and find_by).
  2. We do have some models, like Country, Location, etc, that have a display_names attribute, which is a jsonb field.

Example display names:

{
  "de": {
    "self": { "long": "Vila", "short": "Vila" },
    "admin1": { "long": "Encamp", "short": "Encamp" }
  },
  "en": {
    "self": { "long": "Vila", "short": "Vila" },
    "admin1": { "long": "Encamp", "short": "Encamp" }
  }
}

What I tried

Location

Here's a "normal" resource (shortened the file):

class Avo::Resources::Location < Avo::BaseResource
  self.title = -> {
    name = record.display_names.dig("en", "self", "long") || record.geoname_name
    name
  }

  def fields
    field :id, as: :id, link_to_resource: true, sortable: true
    field :type, as: :text, sortable: true

    field :name, only_on: [:index], &self.title

    # TODO broken:
    #   field :country, as: :belongs_to
    # Alternative:
    field :country_code, as: :text, sortable: true, format_using: -> {
      next unless value and country = ISO3166::Country[value]
      [country.emoji_flag, value, "(#{country.translation("en")})"].join(' ')
    }

    # TODO broken:
    #   field :timezone, as: :belongs_to
    # Alternative also broken:
    #   field(:timezone, as: :array) { [value] }
    field(:timezone, as: :string) { value.iana }

    # FIXME: broken
    field :display_names, as: :array do
      Avo::Resources::DisplayName.generate(tz.display_names)
    end

    # ... more fields here ...
  end
end

Timezone

# This does not work as it *sometimes* overrides ::Timezone
class Avo::Resources::Timezone < Avo::Resources::ArrayResource
  self.record_selector = false

  # only load ids for performance reasons
  def records = ::Timezone.all.map {{ id: _1.id }}

  def fields
    tz = ::Timezone.find(record.id)
    field :id, as: :id

    # Note: sortable has no effect
    %i[iana bcp47 base_utc_offset meta_zone].each do |attribute|
      field(attribute, as: :text, sortable: true) { tz.public_send(attribute) }
    end

    field :display_names, as: :array do
      # this does not work, because loading DisplayName gives an error
      Avo::Resources::DisplayName.generate(tz.display_names)
    end
  end
end

Display names

# Does not work at all, as it fails to define a constant named `::Display name`
class Avo::Resources::DisplayName < Avo::Resources::ArrayResource
  def self.generate(value)
    value.display_names.flat_map do |language, display_names|
      display_names.flat_map do |type, formats|
        formats.flat_map do |style, name|
          {
            id: "#{value.id}/#{language}/#{type}/#{style}",
            language:, type:, style:, name:
          }
        end
      end
    end
  end

  def records
    # empty array does not work, will still trigger a db request
    [{ id: nil }]
  end

  def fields
    field :id, as: :id
    field :language, as: :text
    field :type, as: :text
    field :name: as: :text
  end
end

@adrianthedev
Copy link
Collaborator

All good @rkh.

Let us have a look next week and see how we can improve some things.

@Paul-Bob
Copy link
Contributor

Hi @rkh,

Two of the issues that you mentioned have already been resolved and released in 3.17.9. The DisplayName resource should now work as expected, and the Timezone should never be overridden.

Regarding the performance concerns and the sortable notes, we'll update the documentation to clarify that the array resource currently does not support sorting, and performance on large datasets might be suboptimal. Caching is one of the possibles solutions.

I noticed your comment about looking into a reproduction repository. If you could provide a minimal reproduction repository along with clear steps to replicate each of the remaining issues you're encountering, that would be great.

Thank you!

@rkh
Copy link
Author

rkh commented Feb 25, 2025

Thanks. I'll give it another go, probably next week, and see how well things work. Then, I'll decide if a repro repo is worth your time and mine and whether we'll move forward with Avo. Very happy with how responsive you've been!

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

No branches or pull requests

3 participants