Non-standard record IDs can cause a record not found error

[davidlormor]

Describe the bug

Avo can't handle non-standard primary keys/IDs that don't follow Rails' out-of-the-box conventions (e.g. periods in a string-based ID). Rails documents how to work around this via constraints in their guides. Unfortunately, it appears that segment-level constraints cannot be passed down to routes in a mounted engine.

Steps to Reproduce

Steps to reproduce the behavior:

1. Create a new model with a string-based ID (use a generator to create the Avo resource, too):

bin/rails g model book code:string:uniq title:string

2. Edit the migration to use a custom primary key

class CreateBooks < ActiveRecord::Migration[7.0]
  def change
    create_table :books, id: false, primary_key: :code do |t|
      t.string :code, null: false, index: { unique: true }
      t.string :title

      t.timestamps
    end
  end
end

3. Run the migration to set up the books table

bin/rails db:migrate

4. Tell the model about the primary key:

class Book < ApplicationRecord
  self.primary_key = "code"
end

5. Create a book with an unsupported ID param via Rails console (or Avo):

Book.create(code: '11-2003.34', title: 'Some book')

6. Navigate to the book record in Avo

Expected behavior & Actual behavior

I would expect the book to be rendered via Avo, but instead I get a 500 page. Inspecting the Rails server output, it appears that the book cannot be found, due to an incorrectly parsed ID param. The request params are logged as params: { id: "11-2003" } (note the missing content starting at the . character).

According to the Rails guides, this issue can be solved via a constraint:

Rails.application.routes.draw do
  mount Avo::Engine, at: Avo.configuration.root_path, constraints: { id: /[*\/]+/ }
end

Unfortunately, it appears that because the constraint applies at the route segment level, it is not passed to the Rails engine (I've confirmed this on a separate example engine). I've logged a question on the Rails discussion board to see if anything can/should be done at the Rails level, since this affects all engines, not just Avo.

Models and resource files

See above for setup.

System configuration

Avo version: 3.13.7

Rails version: 8.1.0.alpha

Ruby version: 3.3.5

License type:

• Community
• Pro
• Advanced

Are you using Avo monkey patches, overriding views or view components?

• Yes. If so, please post code samples.
• No

Screenshots or screen recordings

Additional context

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.)

[Paul-Bob]

Hey @davidlormor thank you for such a detailed issue.

I think we could insert that constrain somewhere around here https://github.com/avo-hq/avo/blob/main/lib/avo/dynamic_router.rb#L7

Based on your research, are there any potential consequences of applying this constraint, and why might it not be set as the default for rails?

[davidlormor]

Thanks for the quick reply @Paul-Bob!

I think that would make sense as a quick workaround, although you'd probably need to apply it around the whole route file, due to the fact that there are several routes at that reference the :id param.

As far as consequences, it seems like the main concern, is that it opens up some issues around route parsing and possibly security (e.g. if you had a key like example.json it could throw off the default response formatting, or possibly edge cases where you execute the :id via method missing and the key was set to something like Book.destroy_all)

In the Avo use case, I would imagine those concerns are probably pretty negligible.

A few thoughts I had:

• Set up some config like allow_string_ids or allow_dot_ids that would conditionally apply the constraint.
• Or, allow the specific constraint to be defined by the user via config, i.e. config.routing_constraints = { ... }

One thing I noticed is the current dynamic routing introduces a bug that prevents a constraint from being passed down to the resource routes even when manually adding it to the Avo engine routes config. I'm going to open a PR shortly with a fix for that, as the current setup appears to go against some of the recommendations in the Rails routing guide.

[davidlormor]

Added #3369 as a preemptive fix to the resource route loading that would be required for any solution to applying external constraints to the routes.

[Paul-Bob]

@davidlormor I just committed this 41ee230 to fix the routes on that PR accordingly to rails docs and what you suggested on the PR. This change makes sense, thanks for exposing it.

However, from my tests, the constraints are still not passed down to the resource routes.

I've tried with:

mount Avo::Engine, at: Avo.configuration.root_path, constraints: { id: /[^\/]+/ }

And

constraints id: /[^\/]+/ do
  mount Avo::Engine, at: Avo.configuration.root_path
end

On the other side applying those constraints around the draw (here https://github.com/avo-hq/avo/pull/3369/files#diff-959bc9abc46a55332bb64d5155a79323afa75a50ec1a2137ddd22d926f62c6c5R35) works:

   constraints id: /[^\/]+/ do
     draw(:dynamic_routes)
   end

[davidlormor]

@Paul-Bob thanks for taking a look! Yeah, I didn't expect #3369 to fix the issue entirely, but from my testing locally, that fix will ensure that wrapping the entire routes block with a constraint will properly pass the constraint to all routes. Without that fix, a constraint around the entire routes block wouldn't get passed to the dynamic routes because of the extra call to Avo::Engine.routes.draw in the dynamic routes file.

As mentioned in the PR comment after it was merged, I've discovered the scope of our issue is actually a bit bigger because we also have some tables that have composite primary keys. I'll circle back soon with additional details and thoughts on how Avo might be able to handle it gracefully.

Thanks for the help and the quick turnaround on the PR!

[github-actions]

This issue has been marked as stale because there was no activity for the past 15 days.

[Paul-Bob]

@davidlormor have you found any workaround?

[Paul-Bob]

Related discussion: #3451