Removing automatic integer parsing for find() method

rubyonrails-talk
1

Hello everyone,

The problem we have is this: We have many places where we call the find method with the id that we obtain from the params, however, we realized that calling .find("123a123") actually finds the record with id=123 instead of returning an error, and this is bothering us.

We want to stop this automatic parsing and instead return an exception, saying that id is invalid. Since our codebase is huge, adding a validation to every controller that uses id’s makes the solution pretty messy. So e.g. doing validate_id(params[:id]) before each find(params[:id]) call requires a lot of line changes.

The id’s in the params are also not always the same, so sometimes they exist in params[:id], sometimes they exist as params[:book_id], so adding a before_action method to our ApplicationController for each request that checks params[:id] each time is also not sufficient, and it also causes unnecessary checking for each request, looking if params[:id] is nil or not.

Overriding the Model’s find method also doesn’t work, because it only applies to find from ActiveRecord, but not other type of objects where we can also call find.

Is there a way of overcoming this problem without too many changes?

Thanks

Atalay

2

Hi Atalay.

Interesting question.

Short answer: I agree that those potential solutions don’t work, and I can’t think of an application-level implementation that would.

For one thing, Rails allows non-integer keys for ActiveRecord objects, see: ActiveRecord: Can not assign any text value to non-primary attribute id of type text · Issue #34402 · rails/rails · GitHub and Composite primary keys in Rails - Honeybadger Developer Blog. So any logic that validates the id attribute would have to account for those cases.

There are other ways to retrieve records by id values as well, e.g.: Person.where(id: [param1, param2, param3]). So any solution would also involve adding logic in the database query generator. And as you point out, any such solution would need to work with foreign keys such as Employee#person_id.

You didn’t say how invalid ID values are passed to the find method in the first place. Generally speaking, it’s best to validate your inputs as far upstream as possible to simplify error handling and troubleshooting.

I hope this helps, at least a little.

Regards, DJ

3

Could you add constraints to your routes so the param must be an integer?

Also I don’t follow this bit:

If you had the following in your application_record.rb wouldn’t that do it?

class ApplicationRecord < ActiveRecord::Base
  class < self
    def find(...)
      valid_ids = Array.wrap(...).all? { valid_id? id }
      raise ActiveRecord::RecordNotFound, "invalid id" unless valid_ids

      super
    end

    def valid_id?(id) = id =~ /^\d+$/
  end
end

The above is off the top of my head so untested but that should be the general idea. You might prefer to just filter the invalid ids if an array is passed in.